Fix/merge commit verification #801
Conversation
* feat(terminal): respect preferred terminal setting for Windows PTY shell Adds Windows shell selection in the embedded PTY terminal based on the user's preferredTerminal setting from onboarding/settings. On Windows, the terminal preference (PowerShell, Windows Terminal, CMD) now maps to the appropriate shell executable when spawning PTY processes. This ensures the embedded terminal matches user expectations when they select their preferred terminal during setup. - Adds WINDOWS_SHELL_PATHS mapping for powershell, windowsterminal, cmd - Implements getWindowsShell() to find first available shell executable - Falls back to COMSPEC/cmd.exe for 'system' or unknown terminals - Reads preferredTerminal from user settings on each spawn * fix(ci): cache pip wheels to speed up Intel Mac builds The real_ladybug package has no pre-built wheel for macOS x86_64 (Intel), requiring Rust compilation from source on every build. This caused builds to take 5-10+ minutes. Changes: - Remove --no-cache-dir from pip install so wheels get cached - Add pip wheel cache to GitHub Actions cache for all platforms - Include requirements.txt hash in cache keys for proper invalidation - Fix restore-keys to avoid falling back to incompatible old caches After this fix, subsequent Intel Mac builds will use the cached compiled wheel instead of rebuilding from source each time. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * # 🔥 hotfix(electron): restore app functionality on Windows broken by GPU cache errors (AndyMik90#569) ## 📋 Critical Issue | Severity | Impact | Affected Users | |----------|--------|----------------| | 🔴 **CRITICAL** | 🚫 **Non-functional** | 🪟 **Windows users** | On Windows systems, the Electron app failed to create GPU shader and program caches due to filesystem permission errors (**Error 0x5: Access Denied**). This prevented users from initiating the autonomous coding phase, rendering the application **non-functional** for its primary purpose. --- ## 🔍 Root Cause Analysis ### The Problem Chromium's GPU process attempts to create persistent shader caches in the following locations: %LOCALAPPDATA%\auto-claude-ui\GPUCache\ %LOCALAPPDATA%\auto-claude-ui\ShaderCache\ ### Why It Fails | Factor | Description | |--------|-------------| | 🦠 **Antivirus** | Real-time scanning blocks cache directory creation | | 🛡️ **Windows Defender** | Protection policies deny write access | | ☁️ **Sync Software** | OneDrive/Dropbox interferes with AppData folders | | 🔐 **Permissions** | Insufficient rights in default Electron cache paths | ### Error Console Output ❌ ERROR:net\disk_cache\cache_util_win.cc:25] Unable to move the cache: Zugriff verweigert (0x5) ❌ ERROR:gpu\ipc\host\gpu_disk_cache.cc:724] Gpu Cache Creation failed: -2 ❌ ERROR:net\disk_cache\disk_cache.cc:236] Unable to create cache --- ## ✅ Solution Implemented ### 1️⃣ GPU Shader Disk Cache Disabled app.commandLine.appendSwitch('disable-gpu-shader-disk-cache'); - ⚡ Prevents Chromium from writing shader caches to disk - ✅ GPU acceleration remains fully functional - 🎯 Zero performance impact on typical usage ### 2️⃣ GPU Program Disk Cache Disabled app.commandLine.appendSwitch('disable-gpu-program-cache'); - 🚫 Prevents compiled GPU program caching issues - 🔒 Eliminates permission-related failures ### 3️⃣ Startup Cache Clearing session.defaultSession.clearCache() .then(() => console.log('[main] Cleared cache on startup')) .catch((err) => console.warn('[main] Failed to clear cache:', err)); - 🧹 Clears stale session cache on initialization - 🔧 Prevents errors from corrupted cache artifacts -⚠️ Includes error handling for robustness --- ## 📝 Technical Changes ### Files Modified | File | Changes | |------|---------| | apps/frontend/src/main/index.ts | +13 lines (cache fixes) | ### Platform Gating ✅ **Windows Only** (process.platform === 'win32') ✅ macOS & Linux behavior unchanged --- ## 🎯 Impact Assessment | Aspect | Status | Details | |--------|--------|---------| | 🎮 **GPU Acceleration** | ✅ **PRESERVED** | Hardware rendering fully functional | | 🤖 **Agent Functionality** | ✅ **RESTORED** | Coding phase now works on Windows | | 🖥️ **Console Errors** | ✅ **ELIMINATED** | Clean startup on all Windows systems | | ⚡ **Performance** | ✅ **NO IMPACT** | Typical usage unaffected | | 🔙 **Compatibility** | ✅ **MAINTAINED** | No breaking changes | --- ## 🧪 Testing ### Test Environments | Platform | Antivirus | Result | |----------|-----------|--------| | Windows 10 | Windows Defender | ✅ Pass | | Windows 11 | Real-time scanning | ✅ Pass | ### Test Scenarios ✅ Application starts without cache errors ✅ Agent initialization completes successfully ✅ Coding phase executes without GPU failures ✅ GPU acceleration functional (hardware rendering active) --- ## 📦 Meta Information | Field | Value | |-------|-------| | 📍 **Component** | apps/frontend/src/main/index.ts | | 🪟 **Platform** | Windows (win32) - platform-gated | | 🔥 **Type** | Hotfix (critical functionality restoration) | --- ## 🔄 Backwards Compatibility | Check | Status | |-------|--------| | Breaking Changes | ❌ None | | User Data Migration | ❌ Not required | | Settings Impact | ❌ Unaffected | | Workflow Changes | ❌ None required | --- *This hotfix restores critical functionality for Windows users while maintaining full compatibility with macOS and Linux platforms. GPU acceleration remains fully functional — only disk-based caching is disabled.* Co-authored-by: sniggl <snigg1337@gmail.com> * ci(release): add CHANGELOG.md validation and fix release workflow The release workflow was failing with "GitHub Releases requires a tag" when triggered via workflow_dispatch because no tag existed. Changes: - prepare-release.yml: Validates CHANGELOG.md has entry for version BEFORE creating tag (fails early with clear error message) - release.yml: Uses CHANGELOG.md content instead of release-drafter for release notes; fixes workflow_dispatch to be dry-run only - bump-version.js: Warns if CHANGELOG.md missing entry for new version - RELEASE.md: Updated documentation for new changelog-first workflow This ensures releases are only created when CHANGELOG.md is properly updated, preventing incomplete releases and giving better release notes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(merge): handle Windows CRLF line endings in regex fallback The merge conflict layer was failing on Windows when tree-sitter was unavailable. The regex-based fallback used split("\n") which doesn't handle CRLF line endings, and findall() returned tuples for JS/TS patterns breaking function detection. Changes: - Normalize line endings (CRLF → LF) before parsing in regex_analyzer.py - Use splitlines() instead of split("\n") in file_merger.py - Fix tuple extraction from findall() for JS/TS function patterns - Normalize line endings before tree-sitter parsing for consistent byte positions All 111 merge tests pass. These changes are cross-platform safe and maintain compatibility with macOS and Linux. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * 2.7.2 release --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: sniggl <sniggl1337@gmail.com> Co-authored-by: sniggl <snigg1337@gmail.com>
* Fix Windows UTF-8 encoding errors in spec file reading Fixes UnicodeDecodeError on Windows when reading spec files with non-ASCII characters. Python's read_text() defaults to cp1252 on Windows instead of UTF-8, causing crashes when spec.md contains Unicode characters. Changes: - spec_document_validator.py: Add encoding='utf-8' to spec.md reading - compaction.py: Add encoding='utf-8' to phase output file reading - validation_strategy.py: Add encoding='utf-8' to requirements/pyproject reading - agent_runner.py: Add encoding='utf-8' to prompt file reading Impact: - Fixes spec generation crashes on Windows with Unicode content - No impact on Unix/Mac (already default to UTF-8) - Prevents charmap codec errors during spec validation Tested on Windows 10 with Auto-Claude v2.7.3 * Add error handling for file read operations Implements Gemini Code Assist review suggestions to improve robustness: 1. spec_document_validator.py: - Wrap read_text() in try-except to handle corrupted UTF-8 files - Provide clear error message when spec.md is unreadable 2. validation_strategy.py: - Silently skip unreadable requirements.txt/pyproject.toml files - Prevents crashes during project type detection This defensive programming prevents crashes from: - Corrupted or invalid UTF-8 files - Permission errors (OSError) - Other file system issues Addresses code review feedback from @gemini-code-assist * Fix Windows UTF-8 encoding errors in project analyzer - Add encoding='utf-8' to file reads in analyzer.py (load/save profile) - Add encoding='utf-8' to file reads in config_parser.py (read_json, read_text) - Add encoding='utf-8' to file reads in stack_detector.py (YAML files) This fixes the 'charmap' codec error that was blocking roadmap generation on Windows: Warning: Could not load security profile: 'charmap' codec can't decode byte 0x8d Same root cause as spec file reading - Windows defaults to cp1252 instead of UTF-8. * Revert "Fix Windows UTF-8 encoding errors in project analyzer" This reverts commit 41deed0. --------- Co-authored-by: TamerineSky <TamerineSky@users.noreply.github.com>
…ik90#744)" (AndyMik90#770) This reverts commit a01634d.
- Add merge commit verification in worktree. py before reporting success - Add pre-delete verification in worktree-handlers.ts to prevent data loss - Verify branch is fully merged before deleting worktree - Keep worktree if merge verification fails (safe-by-default) Fixes AndyMik90#797
|
|
📝 WalkthroughWalkthroughThis PR adds changelog validation/extraction to release flows, introduces per-platform pip wheel caching in CI, normalizes line endings and improves semantic analysis, hardens post-merge cleanup with merge verification, adds Windows GPU/cache and terminal selection fixes, updates release/docs/assets for v2.7.2, and adds changelog extraction tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant PrepareWF as prepare-release.yml
participant Validator as changelog validator (awk / script)
participant Artifacts as Artifact Store
participant GitHub as Git/GitHub
Dev->>PrepareWF: Trigger release workflow (PR merge / tag)
PrepareWF->>Validator: Extract new_version and check CHANGELOG.md
alt changelog entry exists
Validator-->>PrepareWF: extracted notes (stdout + file)
PrepareWF->>Artifacts: upload changelog-extract.md
PrepareWF->>GitHub: create & push tag
GitHub-->>PrepareWF: tag pushed → release proceeds
else missing entry
Validator-->>PrepareWF: NOT_FOUND
PrepareWF->>PrepareWF: add release-blocked message
PrepareWF-->>Dev: fail workflow (missing changelog)
end
sequenceDiagram
autonumber
participant UI as Frontend UI (user)
participant IPC as worktree IPC handler
participant Git as local git
participant Verifier as merge verification logic
participant Cleaner as cleanup logic
UI->>IPC: Request "Merge to main" for task
IPC->>Git: perform merge action
Git-->>IPC: merge result (success)
IPC->>Verifier: check branch `auto-claude/{taskId}` merged into HEAD
alt branch merged
Verifier-->>IPC: verified
IPC->>Cleaner: git worktree remove --force
Cleaner->>Git: git branch -D `auto-claude/{taskId}`
Cleaner-->>IPC: cleanup successful
IPC-->>UI: Merge complete
else not merged
Verifier-->>IPC: verification failed
IPC->>IPC: preserve worktree, set status human_review
IPC-->>UI: Manual review required
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @sakshyasinha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical data loss vulnerability in the worktree merging workflow. It introduces comprehensive verification steps in both the backend and frontend to guarantee that a merge commit is successfully recorded in Git before any worktree cleanup occurs. Should these checks fail, the system now intelligently preserves the worktree and flags the task for human review, thereby safeguarding user data and providing clear guidance for resolution. Additionally, the release process has been fortified with mandatory changelog validation, and several Windows-specific compatibility issues have been addressed. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a critical fix to prevent data loss during worktree merges by adding verification steps on both the backend and frontend. The frontend changes add a pre-delete safety check that is robust and safe-by-default. The backend changes introduce commit verification logic, which is a great addition, but contains a few typos and a logic error in the commit message comparison that need to be addressed. I've also pointed out some minor inconsistencies in logging messages. The other changes, including documentation updates and process improvements for releases, are well-implemented.
apps/backend/core/worktree.py
Outdated
| # --no-commit stages the merge but doesn't create the commit | ||
| merge_args.append("--no-commit") | ||
| else: | ||
| merge_args.extend(["-m", f"auto-claude: Merge {info. branch}"]) |
There was a problem hiding this comment.
There appears to be a typo here: info. branch should be info.branch.
| merge_args.extend(["-m", f"auto-claude: Merge {info. branch}"]) | |
| merge_args.extend(["-m", f"auto-claude: Merge {info.branch}"]) |
apps/backend/core/worktree.py
Outdated
|
|
||
| # Check if branch is already merged (acceptable edge case) | ||
| check_merged = self._run_git( | ||
| ["branch", "--merged", self.base_branch, info. branch] |
There was a problem hiding this comment.
There appears to be a typo here: info. branch should be info.branch.
| ["branch", "--merged", self.base_branch, info. branch] | |
| ["branch", "--merged", self.base_branch, info.branch] |
apps/backend/core/worktree.py
Outdated
| # Switch to base branch in main project | ||
| result = self._run_git(["checkout", self.base_branch]) | ||
| if result.returncode != 0: | ||
| print(f"Error: Could not checkout base branch: {result.stderr}") |
There was a problem hiding this comment.
There are inconsistent double spaces in this print statement. For consistency with other error messages in this file, please use a single space after the colon.
| print(f"Error: Could not checkout base branch: {result.stderr}") | |
| print(f"Error: Could not checkout base branch: {result.stderr}") |
apps/backend/core/worktree.py
Outdated
| self._run_git(["merge", "--abort"]) | ||
| return False | ||
| if result.returncode != 0: | ||
| print("Merge conflict! Aborting merge...") |
There was a problem hiding this comment.
There's an inconsistent double space here. For consistency, please use a single space.
| print("Merge conflict! Aborting merge...") | |
| print("Merge conflict! Aborting merge...") |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/terminal/pty-manager.ts (1)
71-87: Remove the breaking change concern; keep the performance optimization suggestion.The
spawnPtyProcesssignature change is not a breaking change—the only caller in the codebase (terminal-lifecycle.ts:57) has already been updated to pass thecolsparameter correctly.However, the performance consideration is valid:
readSettingsFile()is called on every PTY spawn at line 78. If terminals are created frequently, consider cachingpreferredTerminalwith invalidation only when settings actually change, rather than reading from disk on every spawn.
🤖 Fix all issues with AI agents
In @.github/workflows/beta-release.yml:
- Around line 284-290: The pip cache path used in the GitHub Actions step for
actions/cache@v4 uses backslashes; update the path string in the "Cache pip
wheel cache" step from the backslash form to POSIX-style forward slashes (change
the path value referenced in that step to use ~/AppData/Local/pip/Cache) so the
cache action performs consistent path comparisons on Windows and matches
restore-keys behavior.
In @.github/workflows/prepare-release.yml:
- Around line 89-111: The awk check for the changelog header uses the raw
VERSION in a regex (the test $2 ~ "^"ver"[[:space:]]*-") so dots in versions are
treated as wildcards; escape dots before using VERSION in the regex. Inside the
awk script, copy ver into a new variable (e.g., ver_escaped) and run gsub(/\./,
"\\\\.", ver_escaped) to replace '.' with '\.' and then use ver_escaped in the
regex test (e.g., $2 ~ "^"ver_escaped"[[:space:]]*-") while keeping the existing
equality check ($2 == ver) as-is.
In @.github/workflows/release.yml:
- Around line 509-566: Duplicate awk-based changelog extraction (the block that
sets VERSION, runs awk to fill CHANGELOG_CONTENT, and writes changelog-body.md)
should be moved to a shared script (e.g., scripts/extract-changelog.sh) and
invoked from both this workflow and prepare-release.yml; in the script preserve
the same fallback behavior but ensure the awk regex safely escapes dots in the
VERSION variable (e.g., in awk create ver_esc = ver; gsub(/\./,"\\.",ver_esc)
and match against ver_esc instead of raw ver) so the version matching like $2 ~
("^" ver_esc "[[:space:]]*-") is correct, then replace the inline block with a
simple call that sets VERSION and captures the multiline output into
CHANGELOG_CONTENT (writing to changelog-body.md and the same file-based
$GITHUB_OUTPUT pattern).
In @apps/backend/core/worktree.py:
- Line 427: Several attribute accesses have stray spaces before the dot (e.g.,
"info. branch", "verify_result. stdout", "self. remove_worktree") causing
syntax/AttributeError; update all occurrences to use proper attribute access
(info.branch, verify_result.stdout, self.remove_worktree) and review nearby
similar lines (merge_args construction using info.branch, any other references
to info. or verify_result.) to ensure no other space-before-dot typos remain.
- Line 395: The function signature contains an extra space in the parameter
annotation: change "delete_after: bool = False" to "delete_after: bool = False"
in the signature that currently reads "spec_name: str, delete_after: bool =
False, no_commit: bool = False" so the type annotation spacing is consistent.
- Line 447: The merge-message check is inconsistent: `expected_msg =
f"auto-claude: Merge {info.branch}"` has two spaces after the colon while
elsewhere (e.g., the other use of `f"auto-claude: Merge {info.branch}"`) uses
one; update the `expected_msg` assignment in worktree.py (the `expected_msg`
variable) to use a single space after the colon so it matches the other
occurrence (i.e., change to `f"auto-claude: Merge {info.branch}"`) or otherwise
make both places produce the exact same string.
- Around line 394-396: The merge_worktree method definition (def
merge_worktree(self, spec_name: str, delete_after: bool = False, no_commit:
bool = False) -> bool) is misindented and likely placed outside the class,
causing a syntax error; fix it by indenting the method signature and its entire
body one level into the class (standard 4-space indent), ensuring the parameters
delete_after and no_commit remain in the signature and the return type -> bool
is preserved, and verify the method's internal statements and any nested helper
calls are consistently indented to match the class method scope.
- Around line 441-467: Fix three syntax issues in merge_worktree and the merge
verification block: correct the method indentation for def merge_worktree to use
4 spaces to match other class methods; remove the stray space in verify_result.
stdout.strip() to call verify_result.stdout.strip(); and remove the space in
info. branch to use info.branch wherever referenced (including the branch
--merged check and error messages). After these syntax fixes, ensure the
verification uses the existing _run_git and base_branch symbols correctly and
update any callsites if necessary to comply with the repository's dynamic
command allowlisting policy for _run_git.
In @apps/backend/merge/file_merger.py:
- Around line 48-52: The current merge logic uses splitlines() but always
rejoins with "\n", normalizing CRLF to LF; update the code in file_merger.py
around the splitlines() usage to detect the original line ending (e.g.,
original_ending = "\r\n" if "\r\n" in content else "\n"), use splitlines() as
before, then rejoin with original_ending.join(lines) so CRLF files keep CRLF
(ensure you still insert change.content_after at import_end found by
find_import_end); alternatively, if LF normalization is intentional, add a clear
note to the function docstring stating the function normalizes line endings to
LF and confirm this aligns with the repo git settings (core.autocrlf) before
committing.
In @apps/backend/merge/semantic_analysis/regex_analyzer.py:
- Around line 97-109: The local helper extract_func_names should be moved to
module level, renamed to _extract_func_names_from_matches, and annotated with
type hints (e.g., matches: list[str | tuple[str, ...]] -> set[str]) to improve
clarity and testability; implement the same logic but as a top-level function so
callers in regex_analyzer.py can import/test it, and update any references to
call the new function name.
In @apps/frontend/src/main/terminal/pty-manager.ts:
- Around line 15-42: The WINDOWS_SHELL_PATHS constant currently hardcodes common
Windows shell locations which fails for custom installs; make shell path
resolution configurable and more robust by: 1) exposing a user-configurable
setting (e.g., terminal.shellPaths or similar) that can override or extend
entries in WINDOWS_SHELL_PATHS, 2) implementing a fallback lookup routine used
by the PTY manager (the function that currently reads WINDOWS_SHELL_PATHS when
selecting a shell) that first checks the user setting, then searches the PATH
for the executable name, then falls back to COMSPEC/your existing
WINDOWS_SHELL_PATHS entries, and 3) ensure the selection code merges
user-provided arrays with the default WINDOWS_SHELL_PATHS keys (powershell, cmd,
gitbash, cygwin, msys2) so custom paths are respected without breaking defaults.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/beta-release.yml.github/workflows/prepare-release.yml.github/workflows/release.ymlCHANGELOG.mdREADME.mdRELEASE.mdapps/backend/core/worktree.pyapps/backend/merge/file_merger.pyapps/backend/merge/semantic_analysis/regex_analyzer.pyapps/backend/merge/semantic_analyzer.pyapps/frontend/scripts/download-python.cjsapps/frontend/src/main/index.tsapps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/terminal/pty-manager.tsscripts/bump-version.js
🧰 Additional context used
📓 Path-based instructions (5)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/merge/semantic_analyzer.pyapps/backend/merge/semantic_analysis/regex_analyzer.pyapps/backend/merge/file_merger.pyapps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/merge/semantic_analyzer.pyapps/backend/merge/semantic_analysis/regex_analyzer.pyapps/backend/merge/file_merger.pyapps/backend/core/worktree.py
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/main/index.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/main/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.tsapps/frontend/src/main/terminal/pty-manager.tsapps/frontend/src/main/index.ts
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/worktree.py
🧠 Learnings (1)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: When submitting PRs to the upstream AndyMik90/Auto-Claude repository, always target the `develop` branch, not `main`
Applied to files:
RELEASE.md
🧬 Code graph analysis (1)
apps/backend/merge/file_merger.py (2)
apps/frontend/scripts/download-python.cjs (2)
lines(568-568)content(567-567)scripts/bump-version.js (2)
content(146-146)content(161-161)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~71-~71: Ensure spelling is correct
Context: ... to prefer versioned Python over system python3 - Added support for Bun 1.2.0+ lock file f...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~186-~186: The official name of this software platform is spelled with a capital “H”.
Context: ...inux (#404) by @mitsu in 230de5f - fix(github): pass repo parameter to GHClient for e...
(GITHUB)
[uncategorized] ~194-~194: The official name of this software platform is spelled with a capital “H”.
Context: ...9) by @michael Ludlow in 68548e3 - fix(github): improve PR review with structured out...
(GITHUB)
[uncategorized] ~226-~226: The official name of this software platform is spelled with a capital “H”.
Context: ... path (#308) by @andy in c0a02a4 - fix(github): add augmented PATH env to all gh CLI ...
(GITHUB)
[uncategorized] ~233-~233: The official name of this software platform is spelled with a capital “H”.
Context: ...stage (#293) by @alex in 8416f30 - fix(github): add explicit GET method to gh api com...
(GITHUB)
[uncategorized] ~241-~241: The official name of this software platform is spelled with a capital “H”.
Context: ...) by @dependabot[bot] in 50dd107 - fix(github): resolve follow-up review API issues b...
(GITHUB)
[uncategorized] ~251-~251: The official name of this software platform is spelled with a capital “H”.
Context: ... by @dependabot[bot] in d4cad80 - feat(github): add automated PR review with follow-u...
(GITHUB)
[uncategorized] ~255-~255: The official name of this software platform is spelled with a capital “H”.
Context: ...tsu in f843811 - Revert "Feat/Auto Fix Github issues and do extensive AI PR reviews (...
(GITHUB)
[uncategorized] ~256-~256: The official name of this software platform is spelled with a capital “H”.
Context: ...1) by @andy in 5e8c530 - Feat/Auto Fix Github issues and do extensive AI PR reviews (...
(GITHUB)
🔇 Additional comments (28)
apps/backend/merge/semantic_analyzer.py (3)
214-217: LGTM! Normalization ensures cross-platform consistency.The line-ending normalization correctly handles Windows (CRLF), old Mac (CR), and Unix (LF) formats. This is essential for ensuring tree-sitter byte positions and line counts work consistently across platforms.
219-220: LGTM! Parsing with normalized content.Using normalized content for tree-sitter parsing ensures consistent AST structure regardless of the platform or original line-ending format.
223-225: LGTM! Element extraction aligned with parsed trees.Using normalized content for element extraction ensures that byte positions from tree-sitter nodes correctly index into the content, preventing off-by-one errors or misaligned extractions.
apps/backend/merge/semantic_analysis/regex_analyzer.py (3)
33-36: LGTM! Consistent normalization across modules.The normalization logic matches the implementation in
semantic_analyzer.py, ensuring consistent cross-platform behavior throughout the semantic analysis pipeline.
41-42: LGTM! Diff computation uses normalized content.Using normalized content for the unified diff prevents spurious differences caused by line-ending inconsistencies, ensuring accurate change detection.
111-112: LGTM! Helper correctly applied with normalized content.The helper function is properly used with normalized content, ensuring consistent function name extraction across platforms.
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts (3)
1683-1738: Solid merge verification implementation - good safety-first approach.The verification logic correctly uses
git branch --merged HEADto confirm the task branch is fully merged before cleanup. The early return pattern on verification failure preserves the worktree and escalates tohuman_review, which aligns with the PR objective to prevent data loss.A few observations:
- The verification handles both the "not merged" case (lines 1702-1718) and the "verification error" case (lines 1722-1737) appropriately.
- Both failure paths correctly resolve with
success: truebut with updated messaging and status, avoiding false error reports while still flagging the issue.
1740-1756: Cleanup sequence is correct - worktree removal followed by branch deletion.The cleanup correctly:
- Removes worktree with
--forceflag (line 1741)- Attempts branch deletion with
-D(line 1749)- Gracefully handles the case where the branch is already deleted (lines 1754-1756)
This follows best practices for git worktree cleanup.
1705-1706: Use i18n translation keys for user-facing messages.Per coding guidelines, all user-facing text in the frontend should use i18n translation keys instead of hardcoded strings.
Example refactor for i18n
- message = 'Merge completed but worktree kept for safety. Please verify with: git log --oneline -n 10'; + message = t('worktree:merge.verificationFailed.notMerged');Similar changes needed for:
- Line 1724:
'Merge completed but worktree kept (verification failed). Please check: git log'Also applies to: 1724-1725
⛔ Skipped due to learnings
Learnt from: CR Repo: AndyMik90/Auto-Claude PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-30T16:38:36.314Z Learning: Applies to apps/frontend/src/**/*.{ts,tsx,jsx} : Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded stringsLearnt from: MikeeBuilds Repo: AndyMik90/Auto-Claude PR: 661 File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189 Timestamp: 2026-01-04T23:59:45.209Z Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.Learnt from: CR Repo: AndyMik90/Auto-Claude PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-30T16:38:36.314Z Learning: Applies to apps/frontend/src/shared/i18n/locales/**/*.json : When implementing new frontend features, add translation keys to all language files (minimum: en/*.json and fr/*.json)apps/frontend/scripts/download-python.cjs (1)
613-620: This change is justified and properly supported by existing CI/CD infrastructure.The pip cache enablement is intentional to preserve compiled wheels for packages like
real_ladybugon Intel Mac. This is explicitly supported by the release and beta-release workflows, which configure platform-specific and architecture-specific pip cache management viaactions/cache@v4with cache keys hashed byrequirements.txt. The cache paths and scoping (pip-wheel-${{ runner.os }}-{x64|arm64}) handle per-platform cache lifecycle, and stale cache is prevented through requirements-based key invalidation.README.md (1)
27-31: LGTM! Version bump is consistent.The version update from 2.7.1 to 2.7.2 is applied consistently across all platform download links and aligns with the version badge updates elsewhere in the file.
apps/frontend/src/main/index.ts (2)
113-118: Windows GPU cache workaround looks appropriate.The command line switches to disable GPU shader and program caches address documented Windows permission errors (0x5 Access Denied). This is a valid defensive approach for Windows environments.
125-130: Consider cache clearing frequency impact.Clearing the Electron cache on every Windows startup adds initialization overhead. While this prevents stale cache permission errors, consider whether:
- Cache clearing could be triggered only on first run or after updates
- A more targeted approach (clearing only GPU cache) would suffice
- The performance impact is acceptable for the target user base
The error handling is appropriate, and the async pattern ensures it doesn't block startup.
If cache clearing on every startup is intentional for reliability, this is acceptable. Otherwise, consider adding a flag to clear cache only when necessary.
.github/workflows/beta-release.yml (3)
100-106: LGTM! Pip wheel caching properly implemented.The cache key includes the requirements.txt hash, ensuring cache invalidation when dependencies change. The macOS cache path
~/Library/Caches/pipis correct for the platform.
192-198: LGTM! ARM64-specific cache key properly scoped.The cache key distinguishes between ARM64 and x64 architectures, preventing cache collisions between different build targets.
362-368: LGTM! Linux pip cache properly configured.The cache path
~/.cache/pipis the standard location for pip cache on Linux systems.CHANGELOG.md (1)
1-280: Comprehensive 2.7.2 changelog entry.The changelog is well-structured with clear categorization of features, improvements, and bug fixes. The extensive "What's Changed" section provides detailed commit-level traceability, and contributor attribution is present.
The static analysis hints flag minor issues (GitHub capitalization in git context, "python3" spacing) but these are acceptable in informal changelog prose.
This entry aligns with the changelog-driven release process described in the PR summary, where CHANGELOG.md is validated before tagging.
apps/frontend/src/main/terminal/pty-manager.ts (1)
47-66: Shell resolution logic is sound.The function gracefully handles missing preferences and falls back to COMSPEC. The existsSync checks introduce a minor TOCTOU race condition (file could be deleted between check and spawn), but this is acceptable for user configuration scenarios where the risk is negligible.
scripts/bump-version.js (3)
152-171: LGTM! Well-implemented changelog validation.The function correctly validates changelog entries with proper regex escaping for version dots. The fallback behavior (returning
falsewhen file missing or entry not found) is appropriate for a validation check.
222-248: Clear user guidance for missing changelog entries.The warning block is well-formatted with visual separators and provides an actionable template. This should effectively guide developers to add changelog entries before releasing.
260-281: LGTM! Conditional next steps are appropriately ordered.The step numbering adjusts correctly based on changelog presence, and the warning about updating CHANGELOG.md before pushing is prominently placed.
RELEASE.md (2)
75-101: LGTM! Clear documentation for the changelog requirement.The step-by-step instructions with the template format and
git commit --amendworkflow are practical and easy to follow.
207-221: Good troubleshooting guidance for the new failure mode.The remediation steps are clear. One note: the instruction says to push directly to
main(line 220), but based on project conventions, changes should typically go throughdevelopfirst via PR. Consider whether this emergency fix path aligns with your branching strategy..github/workflows/prepare-release.yml (2)
177-184: LGTM! Artifact upload for changelog is well-configured.The 1-day retention is appropriate for a pipeline artifact that's only needed during the release workflow.
186-187: Good safety gate: tag creation conditioned on changelog validation.This ensures tags are only created when the changelog has been properly updated, preventing releases without documented changes.
.github/workflows/release.yml (3)
50-56: LGTM! Pip wheel caching will improve build times.The cache path and key structure are correct for macOS Intel builds.
220-226: LGTM! Windows pip cache path is correct.The path
~\AppData\Local\pip\Cacheis the appropriate location for pip's cache on Windows runners.
568-584: LGTM! Release body format is clean and informative.The structure with changelog content, VirusTotal results, and a link to the full CHANGELOG.md provides good context for users reviewing releases.
- Add merge commit verification in worktree handlers (fixes AndyMik90#797) - Verify branch is fully merged before worktree cleanup - Prevent data loss by keeping worktree if verification fails - Set status to human_review when verification fails - Fix pip cache path in beta-release workflow - Use POSIX-style forward slashes for consistent Windows path handling - Fix changelog extraction regex escaping - Escape dots in VERSION to prevent wildcard matching in awk regex - Create shared scripts/extract-changelog.sh for reuse across workflows - Preserve line endings in file merger - Detect and preserve original CRLF vs LF line endings - Prevent unwanted line ending normalization - Improve regex analyzer code organization - Extract helper function to module level with type hints - Enhance testability and maintainability - Make terminal shell paths configurable - Support custom shell paths via terminal.shellPaths setting - Add PATH search fallback for shell executables - Improve robustness for custom Windows shell installations
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/merge/file_merger.py (1)
102-109: Inconsistent line ending handling – should preserve original style.This function uses
splitlines()but always rejoins with"\n"(line 109), normalizing all line endings to LF. This is inconsistent withapply_single_task_changes(lines 48-54), which detects and preserves the original line ending style. The inconsistency can cause:
- Unexpected git diffs when the same file is processed by different functions
- CRLF files being silently converted to LF in multi-task scenarios
- Violation of the PR's stated objective to preserve line endings
🔧 Proposed fix to preserve line endings
# Add imports if imports: - # Use splitlines() to handle all line ending styles (LF, CRLF, CR) + # Use splitlines() to handle all line ending styles (LF, CRLF, CR) + # Detect and preserve original line ending style + original_ending = "\r\n" if "\r\n" in content else "\n" lines = content.splitlines() import_end = find_import_end(lines, file_path) for imp in imports: if imp.content_after and imp.content_after not in content: lines.insert(import_end, imp.content_after) import_end += 1 - content = "\n".join(lines) + content = original_ending.join(lines)
🤖 Fix all issues with AI agents
In @.github/workflows/prepare-release.yml:
- Around line 119-162: The echo statements inside the CHANGELOG validation block
use unquoted $VERSION (e.g., the various echo "::error:: Version $VERSION not
found..." and the lines appending to $GITHUB_STEP_SUMMARY), which triggers
shellcheck SC2086; update all occurrences to quote the variable (e.g.,
"$VERSION") so the messages and summary lines use quoted expansions, ensuring
safe handling if the version ever contains spaces or special characters while
keeping the existing message text and semantics unchanged.
In @.github/workflows/release.yml:
- Around line 220-227: The "Cache pip wheel cache" step uses Windows backslashes
in the path which actions/cache expects POSIX-style paths; update the path
string in the release.yml step named "Cache pip wheel cache" from
`~\AppData\Local\pip\Cache` to use forward slashes `~/AppData/Local/pip/Cache`
so it matches the beta-release.yml convention and will restore/cache correctly
on Windows runners.
In @apps/backend/core/worktree.py:
- Around line 441-467: Import validate_command from the security module and run
it on any user-derived git args before calling _run_git: validate info.branch
and self.base_branch (and any other user-controlled values) and use the
validated values when constructing the git argument lists (e.g., the ["branch",
"--merged", self.base_branch, info.branch] call and any other _run_git calls
that include those variables); ensure validate_command is applied consistently
and return/raise on validation failure so unvalidated input is never passed to
_run_git.
- Around line 455-463: The fallback merged-branch verification uses the
subprocess result from self._run_git (stored in check_merged) but never checks
its return code, so a failed git command can produce empty stdout and a false
negative; modify the block that calls self._run_git(["branch", "--merged",
self.base_branch, info.branch]) to first verify check_merged.returncode == 0 (or
use check=True) before inspecting check_merged.stdout, and if the command
failed, log or print an explicit error mentioning the git failure and return
False (or propagate the error) instead of proceeding to string-match info.branch
against possibly empty stdout.
In @apps/frontend/src/main/terminal/pty-manager.ts:
- Around line 67-68: Duplicate reads of the settings file occur when spawning a
PTY; cache the settings once and pass them to functions that need them (or
implement a cached read with invalidation). Specifically, call
readSettingsFile() once in spawnPtyProcess and store its result, pass that
settings object into getWindowsShell (or similar helper) so getWindowsShell can
use settings?.terminal?.shellPaths (customShellPaths) instead of calling
readSettingsFile again; alternatively implement a module-level cachedSettings
with a file watcher to invalidate on change and have readSettingsFile return the
cached value. Ensure you update uses of readSettingsFile in spawnPtyProcess and
the Windows shell resolution to accept and use the provided settings object.
- Around line 65-73: The function getWindowsShell reads settings via
readSettingsFile and blindly casts settings?.terminal?.shellPaths to
customShellPaths; validate that settings?.terminal?.shellPaths is an object
whose values are arrays of strings before using them: check that
customShellPaths is a plain object, iterate its entries and filter each value to
an array, then map/filter to keep only string elements (or ignore malformed
entries), and fall back to an empty record if validation fails; use the
validatedCustomShellPaths when spreading paths and before calling existsSync so
you never pass non-string values to path operations.
- Around line 44-59: The findShellInPath function builds PATH parts manually and
always uses a backslash for joining (inconsistent with detected platform);
replace the manual pathSeparator and string concatenation with Node's path
utilities: use path.delimiter to split process.env.PATH and use path.join(dir,
shellName) to construct shellPath (ensure you import/require 'path'); keep the
existsSync check and return behavior unchanged.
- Around line 127-133: Validate the user setting returned by readSettingsFile
before asserting it as SupportedTerminal: check that settings?.preferredTerminal
is one of the known SupportedTerminal enum/string values (use a whitelist or
type-guard function) and only pass the validated value to getWindowsShell; if
validation fails, fallback to undefined (or a default) and optionally log the
invalid value for debugging to avoid blindly casting and passing bad data into
getWindowsShell.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/beta-release.yml.github/workflows/prepare-release.yml.github/workflows/release.ymlapps/backend/core/worktree.pyapps/backend/merge/file_merger.pyapps/backend/merge/semantic_analysis/regex_analyzer.pyapps/frontend/src/main/terminal/pty-manager.tsscripts/extract-changelog.sh
🧰 Additional context used
📓 Path-based instructions (5)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/merge/semantic_analysis/regex_analyzer.pyapps/backend/merge/file_merger.pyapps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/merge/semantic_analysis/regex_analyzer.pyapps/backend/merge/file_merger.pyapps/backend/core/worktree.py
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/terminal/pty-manager.ts
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/worktree.py
🪛 actionlint (1.7.10)
.github/workflows/release.yml
512-512: shellcheck reported issue in this script: SC2086:info:24:6: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/prepare-release.yml
76-76: shellcheck reported issue in this script: SC2086:info:103:6: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:105:32: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:65:53: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:66:14: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:67:65: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:68:14: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:69:29: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:70:76: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:71:44: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:72:53: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:73:14: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:74:34: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:75:28: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:76:41: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:77:14: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:78:32: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:79:35: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:80:14: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:81:29: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:82:31: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2086:info:83:20: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: shellcheck reported issue in this script: SC2129:style:65:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
🔇 Additional comments (15)
apps/backend/merge/file_merger.py (1)
48-54: LGTM! Line ending preservation implemented correctly.The code now properly detects the original line ending style (CRLF or LF) and preserves it when reconstructing content after inserting imports. This addresses the previous review concern about unintended line ending normalization.
apps/backend/merge/semantic_analysis/regex_analyzer.py (3)
13-37: LGTM! Helper function fully addresses previous review feedback.The extraction of
_extract_func_names_from_matchesto module level with comprehensive type hints (list[str | tuple[str, ...]] -> set[str]) directly addresses the previous review comment. The implementation correctly handles both string matches and tuple matches from regex patterns with alternation, and the detailed docstring clearly explains the behavior.As per coding guidelines, Python best practices include comprehensive type hints.
60-69: Appropriate normalization for analysis purposes.The intentional normalization to LF (lines 60-63) is correct for the analysis use case, as this code is reading and comparing file contents, not writing them back. The normalization ensures consistent diff computation across platforms, and the comment clearly documents the intent.
126-127: Correct usage of the new helper function.The code properly uses
_extract_func_names_from_matcheswith the normalized content to extract function names, replacing the previous directfindall()usage. This ensures that regex patterns with alternation (which return tuples) are handled correctly.apps/frontend/src/main/terminal/pty-manager.ts (2)
8-13: LGTM! Imports support the new configurable shell feature.The new imports (existsSync, readSettingsFile, SupportedTerminal) are appropriate for implementing the enhanced shell resolution logic.
15-42: Past review feedback addressed: shell paths are now configurable.This implementation successfully addresses the previous review's suggestion by:
- Providing sensible defaults in WINDOWS_SHELL_PATHS
- Allowing user overrides via
terminal.shellPathssetting (read at line 68)- Adding PATH-based discovery as fallback (lines 96-112)
- Falling back to COMSPEC (line 115)
The configurable approach resolves the limitation of hardcoded paths for users with custom installations.
scripts/extract-changelog.sh (1)
1-63: Well-structured shared extraction script.This script properly addresses the code duplication issue flagged in previous reviews by centralizing changelog extraction logic. The implementation correctly:
- Escapes dots in version for regex matching (lines 30-31)
- Uses strict shell options (
set -euo pipefail)- Provides clear error output and usage guidance
One minor note: the
exit 1inside the awkENDblock (line 48) sets awk's exit status, but due toset -e, if awk exits non-zero, the script will terminate before reaching the outer check on line 57. This is acceptable behavior since the double-check at line 57 handles edge cases where awk might succeed but outputNOT_FOUND..github/workflows/beta-release.yml (4)
100-114: LGTM - Well-structured pip wheel caching for macOS Intel.The cache configuration correctly:
- Uses the appropriate macOS cache path (
~/Library/Caches/pip)- Includes architecture in the key (
x64) for platform-specific caching- Incorporates
requirements.txthash for proper cache invalidation- Adds the requirements hash to the python-bundle key for consistency
284-298: Windows pip cache path correctly uses forward slashes.The path
~/AppData/Local/pip/Cachenow uses POSIX-style forward slashes as recommended foractions/cache@v4. This addresses the previous review feedback.
362-376: LGTM - Linux pip cache configuration is correct.Uses the standard Linux pip cache path (
~/.cache/pip) with consistent cache key structure.
399-406: LGTM - Array-style needs improves readability.The expanded array syntax for job dependencies is cleaner and easier to maintain when multiple upstream jobs are required.
.github/workflows/prepare-release.yml (1)
183-206: LGTM - Proper gating prevents tag creation without valid changelog.The workflow correctly gates both artifact upload (line 185) and tag creation (line 193) on
changelog_valid == 'true'. Combined with theexit 1on validation failure, this ensures releases cannot proceed without documented changes..github/workflows/release.yml (3)
50-64: LGTM - macOS Intel pip wheel caching correctly configured.Cache path and keys are properly set up with architecture-specific identifiers and requirements hash.
289-303: LGTM - Linux pip wheel caching correctly configured.Uses the standard
~/.cache/pippath with consistent cache key structure.
538-554: LGTM - Release creation with proper gating and body structure.The release is correctly gated to tag pushes only (line 539), and the body includes the extracted changelog, VirusTotal results, and a link to the full CHANGELOG.md for reference.
| if [ "$CHANGELOG_CONTENT" = "NOT_FOUND" ] || [ -z "$CHANGELOG_CONTENT" ]; then | ||
| echo "" | ||
| echo "::error::═══════════════════════════════════════════════════════════════════════" | ||
| echo "::error:: CHANGELOG VALIDATION FAILED" | ||
| echo "::error::═══════════════════════════════════════════════════════════════════════" | ||
| echo "::error::" | ||
| echo "::error:: Version $VERSION not found in CHANGELOG.md!" | ||
| echo "::error::" | ||
| echo "::error:: Before releasing, please update CHANGELOG.md with an entry like:" | ||
| echo "::error::" | ||
| echo "::error:: ## $VERSION - Your Release Title" | ||
| echo "::error::" | ||
| echo "::error:: ### ✨ New Features" | ||
| echo "::error:: - Feature description" | ||
| echo "::error::" | ||
| echo "::error:: ### 🐛 Bug Fixes" | ||
| echo "::error:: - Fix description" | ||
| echo "::error::" | ||
| echo "::error::═══════════════════════════════════════════════════════════════════════" | ||
| echo "" | ||
| # Also add to job summary for visibility | ||
| echo "## ❌ Release Blocked: Missing Changelog" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "Version **$VERSION** was not found in CHANGELOG.md." >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### How to fix:" >> $GITHUB_STEP_SUMMARY | ||
| echo "1. Update CHANGELOG.md with release notes for version $VERSION" >> $GITHUB_STEP_SUMMARY | ||
| echo "2. Commit and push the changes" >> $GITHUB_STEP_SUMMARY | ||
| echo "3. The release will automatically retry" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### Expected format:" >> $GITHUB_STEP_SUMMARY | ||
| echo "\`\`\`markdown" >> $GITHUB_STEP_SUMMARY | ||
| echo "## $VERSION - Release Title" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### ✨ New Features" >> $GITHUB_STEP_SUMMARY | ||
| echo "- Feature description" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### 🐛 Bug Fixes" >> $GITHUB_STEP_SUMMARY | ||
| echo "- Fix description" >> $GITHUB_STEP_SUMMARY | ||
| echo "\`\`\`" >> $GITHUB_STEP_SUMMARY | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider quoting variables in error output for consistency.
Static analysis flags SC2086 for unquoted $VERSION in several echo statements. While the version string from workflow outputs is unlikely to contain problematic characters, quoting variables is a shell best practice.
♻️ Optional: Quote VERSION in echo statements
- echo "::error:: Version $VERSION not found in CHANGELOG.md!"
+ echo "::error:: Version \"$VERSION\" not found in CHANGELOG.md!"Alternatively, since these are just informational error messages and $VERSION is a controlled semver string, the current implementation is acceptable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$CHANGELOG_CONTENT" = "NOT_FOUND" ] || [ -z "$CHANGELOG_CONTENT" ]; then | |
| echo "" | |
| echo "::error::═══════════════════════════════════════════════════════════════════════" | |
| echo "::error:: CHANGELOG VALIDATION FAILED" | |
| echo "::error::═══════════════════════════════════════════════════════════════════════" | |
| echo "::error::" | |
| echo "::error:: Version $VERSION not found in CHANGELOG.md!" | |
| echo "::error::" | |
| echo "::error:: Before releasing, please update CHANGELOG.md with an entry like:" | |
| echo "::error::" | |
| echo "::error:: ## $VERSION - Your Release Title" | |
| echo "::error::" | |
| echo "::error:: ### ✨ New Features" | |
| echo "::error:: - Feature description" | |
| echo "::error::" | |
| echo "::error:: ### 🐛 Bug Fixes" | |
| echo "::error:: - Fix description" | |
| echo "::error::" | |
| echo "::error::═══════════════════════════════════════════════════════════════════════" | |
| echo "" | |
| # Also add to job summary for visibility | |
| echo "## ❌ Release Blocked: Missing Changelog" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "Version **$VERSION** was not found in CHANGELOG.md." >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### How to fix:" >> $GITHUB_STEP_SUMMARY | |
| echo "1. Update CHANGELOG.md with release notes for version $VERSION" >> $GITHUB_STEP_SUMMARY | |
| echo "2. Commit and push the changes" >> $GITHUB_STEP_SUMMARY | |
| echo "3. The release will automatically retry" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### Expected format:" >> $GITHUB_STEP_SUMMARY | |
| echo "\`\`\`markdown" >> $GITHUB_STEP_SUMMARY | |
| echo "## $VERSION - Release Title" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### ✨ New Features" >> $GITHUB_STEP_SUMMARY | |
| echo "- Feature description" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### 🐛 Bug Fixes" >> $GITHUB_STEP_SUMMARY | |
| echo "- Fix description" >> $GITHUB_STEP_SUMMARY | |
| echo "\`\`\`" >> $GITHUB_STEP_SUMMARY | |
| exit 1 | |
| fi | |
| if [ "$CHANGELOG_CONTENT" = "NOT_FOUND" ] || [ -z "$CHANGELOG_CONTENT" ]; then | |
| echo "" | |
| echo "::error::═══════════════════════════════════════════════════════════════════════" | |
| echo "::error:: CHANGELOG VALIDATION FAILED" | |
| echo "::error::═══════════════════════════════════════════════════════════════════════" | |
| echo "::error::" | |
| echo "::error:: Version \"$VERSION\" not found in CHANGELOG.md!" | |
| echo "::error::" | |
| echo "::error:: Before releasing, please update CHANGELOG.md with an entry like:" | |
| echo "::error::" | |
| echo "::error:: ## \"$VERSION\" - Your Release Title" | |
| echo "::error::" | |
| echo "::error:: ### ✨ New Features" | |
| echo "::error:: - Feature description" | |
| echo "::error::" | |
| echo "::error:: ### 🐛 Bug Fixes" | |
| echo "::error:: - Fix description" | |
| echo "::error::" | |
| echo "::error::═══════════════════════════════════════════════════════════════════════" | |
| echo "" | |
| # Also add to job summary for visibility | |
| echo "## ❌ Release Blocked: Missing Changelog" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "Version **\"$VERSION\"** was not found in CHANGELOG.md." >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### How to fix:" >> $GITHUB_STEP_SUMMARY | |
| echo "1. Update CHANGELOG.md with release notes for version \"$VERSION\"" >> $GITHUB_STEP_SUMMARY | |
| echo "2. Commit and push the changes" >> $GITHUB_STEP_SUMMARY | |
| echo "3. The release will automatically retry" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### Expected format:" >> $GITHUB_STEP_SUMMARY | |
| echo "\`\`\`markdown" >> $GITHUB_STEP_SUMMARY | |
| echo "## \"$VERSION\" - Release Title" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### ✨ New Features" >> $GITHUB_STEP_SUMMARY | |
| echo "- Feature description" >> $GITHUB_STEP_SUMMARY | |
| echo "" >> $GITHUB_STEP_SUMMARY | |
| echo "### 🐛 Bug Fixes" >> $GITHUB_STEP_SUMMARY | |
| echo "- Fix description" >> $GITHUB_STEP_SUMMARY | |
| echo "\`\`\`" >> $GITHUB_STEP_SUMMARY | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In @.github/workflows/prepare-release.yml around lines 119 - 162, The echo
statements inside the CHANGELOG validation block use unquoted $VERSION (e.g.,
the various echo "::error:: Version $VERSION not found..." and the lines
appending to $GITHUB_STEP_SUMMARY), which triggers shellcheck SC2086; update all
occurrences to quote the variable (e.g., "$VERSION") so the messages and summary
lines use quoted expansions, ensuring safe handling if the version ever contains
spaces or special characters while keeping the existing message text and
semantics unchanged.
| - name: Cache pip wheel cache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~\AppData\Local\pip\Cache | ||
| key: pip-wheel-${{ runner.os }}-x64-${{ hashFiles('apps/backend/requirements.txt') }} | ||
| restore-keys: | | ||
| pip-wheel-${{ runner.os }}-x64- | ||
There was a problem hiding this comment.
Use forward slashes for the Windows pip cache path.
The path ~\AppData\Local\pip\Cache uses backslashes, but actions/cache@v4 expects POSIX-style forward slashes. The beta-release.yml workflow correctly uses ~/AppData/Local/pip/Cache with forward slashes - this file should be updated for consistency.
🔧 Suggested fix
- name: Cache pip wheel cache
uses: actions/cache@v4
with:
- path: ~\AppData\Local\pip\Cache
+ path: ~/AppData/Local/pip/Cache
key: pip-wheel-${{ runner.os }}-x64-${{ hashFiles('apps/backend/requirements.txt') }}
restore-keys: |
pip-wheel-${{ runner.os }}-x64-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Cache pip wheel cache | |
| uses: actions/cache@v4 | |
| with: | |
| path: ~\AppData\Local\pip\Cache | |
| key: pip-wheel-${{ runner.os }}-x64-${{ hashFiles('apps/backend/requirements.txt') }} | |
| restore-keys: | | |
| pip-wheel-${{ runner.os }}-x64- | |
| - name: Cache pip wheel cache | |
| uses: actions/cache@v4 | |
| with: | |
| path: ~/AppData/Local/pip/Cache | |
| key: pip-wheel-${{ runner.os }}-x64-${{ hashFiles('apps/backend/requirements.txt') }} | |
| restore-keys: | | |
| pip-wheel-${{ runner.os }}-x64- |
🤖 Prompt for AI Agents
In @.github/workflows/release.yml around lines 220 - 227, The "Cache pip wheel
cache" step uses Windows backslashes in the path which actions/cache expects
POSIX-style paths; update the path string in the release.yml step named "Cache
pip wheel cache" from `~\AppData\Local\pip\Cache` to use forward slashes
`~/AppData/Local/pip/Cache` so it matches the beta-release.yml convention and
will restore/cache correctly on Windows runners.
| # NEW: Verify merge commit was created (for non-staged merges) | ||
| if not no_commit: | ||
| # Get the latest commit message to verify merge succeeded | ||
| verify_result = self._run_git(["log", "-1", "--format=%s"]) | ||
| if verify_result.returncode == 0: | ||
| latest_commit_msg = verify_result.stdout.strip() | ||
| expected_msg = f"auto-claude: Merge {info.branch}" | ||
|
|
||
| if latest_commit_msg != expected_msg: | ||
| print(f"Warning: Expected merge commit message not found.") | ||
| print(f"Expected: '{expected_msg}'") | ||
| print(f"Got: '{latest_commit_msg}'") | ||
|
|
||
| # Check if branch is already merged (acceptable edge case) | ||
| check_merged = self._run_git( | ||
| ["branch", "--merged", self.base_branch, info.branch] | ||
| ) | ||
|
|
||
| if info.branch not in check_merged.stdout: | ||
| print(f"Error: Branch {info.branch} is not fully merged into {self.base_branch}") | ||
| print("This may indicate a fast-forward merge or other issue.") | ||
| print("Please verify with: git log --oneline -n 10") | ||
| return False | ||
| else: | ||
| print(f"Branch {info.branch} appears to be already merged. Continuing...") | ||
| else: | ||
| print(f"✓ Merge commit created successfully: {latest_commit_msg}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and read the relevant lines
echo "=== Reading worktree.py lines 441-467 ==="
sed -n '441,467p' apps/backend/core/worktree.py
echo -e "\n=== File length check ==="
wc -l apps/backend/core/worktree.pyRepository: AndyMik90/Auto-Claude
Length of output: 1725
🏁 Script executed:
#!/bin/bash
# Check for security-related code: validate if git commands are allowlisted
echo "=== Checking for security.py usage in worktree.py ==="
rg -n "security|allowlist|validate.*command" apps/backend/core/worktree.py || echo "No security validation found"
echo -e "\n=== Checking git command patterns in the file ==="
rg -n "_run_git\|subprocess|shell=True" apps/backend/core/worktree.py | head -20
echo -e "\n=== Checking imports in worktree.py ==="
head -30 apps/backend/core/worktree.pyRepository: AndyMik90/Auto-Claude
Length of output: 883
🏁 Script executed:
#!/bin/bash
# Verify past issues mentioned in scratchpad
echo "=== Checking line 395 (delete_after parameter) ==="
sed -n '393,397p' apps/backend/core/worktree.py
echo -e "\n=== Checking space-before-dot issues around mentioned lines ==="
sed -n '425,460p' apps/backend/core/worktree.py | rg '\. \w' || echo "No space-before-dot issues in this section"
echo -e "\n=== Checking for type hints ==="
rg -n "def.*->|: bool|: str|: int" apps/backend/core/worktree.py | grep -E "(441|442|443|445|446|450|455|460|465|467)" || echo "Checking broader context..."
head -50 apps/backend/core/worktree.py | grep "def\|import"Repository: AndyMik90/Auto-Claude
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Check security.py to understand the required validation pattern
echo "=== Reading security.py to understand command allowlisting ==="
cat apps/backend/core/security.py | head -80
echo -e "\n=== Checking all _run_git calls in worktree.py ==="
rg -n "_run_git" apps/backend/core/worktree.py
echo -e "\n=== Checking how other backend files implement security validation ==="
find apps/backend -name "*.py" -type f | head -5 | xargs grep -l "security\|allowlist" 2>/dev/null || echo "Checking for security imports..."
rg -n "from.*security import|import.*security" apps/backend/ --type pyRepository: AndyMik90/Auto-Claude
Length of output: 2595
🏁 Script executed:
#!/bin/bash
# Check the type of info.branch and where it comes from
echo "=== Checking info type and branch source ==="
rg -B 5 -A 2 "def merge_worktree" apps/backend/core/worktree.py | head -30
echo -e "\n=== Checking class definition and info structure ==="
rg -n "class.*Info|@dataclass" apps/backend/core/worktree.py | head -10
sed -n '1,100p' apps/backend/core/worktree.py | grep -A 10 "@dataclass\|class"Repository: AndyMik90/Auto-Claude
Length of output: 1262
🏁 Script executed:
#!/bin/bash
# Verify the security module API and how it should be used
echo "=== Checking security module validate_command function ==="
rg -A 10 "def validate_command" apps/backend/security/
echo -e "\n=== Checking how client.py uses bash_security_hook ==="
rg -B 3 -A 5 "bash_security_hook" apps/backend/core/client.py | head -20
echo -e "\n=== Checking _run_git implementation to understand if it supports validation ==="
sed -n '126,145p' apps/backend/core/worktree.pyRepository: AndyMik90/Auto-Claude
Length of output: 2513
Security validation required for git command arguments.
This code violates the mandatory security guideline for apps/backend/core/**/*.py files. User-controlled branch names (info.branch) are passed directly to git commands without validation via the security module. The guideline requires implementing command allowlisting using the security validation framework.
Required changes:
- Import and use
validate_command()from the security module to validate git command arguments before execution - Ensure
info.branchand other user-derived values passed to git commands are validated - Apply validation consistently across all
_run_git()calls that incorporate user-controlled input
The merge verification logic itself is sound, but it cannot proceed to approval until security validation is properly implemented.
🤖 Prompt for AI Agents
In @apps/backend/core/worktree.py around lines 441 - 467, Import
validate_command from the security module and run it on any user-derived git
args before calling _run_git: validate info.branch and self.base_branch (and any
other user-controlled values) and use the validated values when constructing the
git argument lists (e.g., the ["branch", "--merged", self.base_branch,
info.branch] call and any other _run_git calls that include those variables);
ensure validate_command is applied consistently and return/raise on validation
failure so unvalidated input is never passed to _run_git.
| verify_result = self._run_git(["log", "-1", "--format=%s"]) | ||
| if verify_result.returncode == 0: | ||
| latest_commit_msg = verify_result.stdout.strip() | ||
| expected_msg = f"auto-claude: Merge {info.branch}" | ||
|
|
||
| if latest_commit_msg != expected_msg: | ||
| print(f"Warning: Expected merge commit message not found.") | ||
| print(f"Expected: '{expected_msg}'") | ||
| print(f"Got: '{latest_commit_msg}'") | ||
|
|
||
| # Check if branch is already merged (acceptable edge case) | ||
| check_merged = self._run_git( | ||
| ["branch", "--merged", self.base_branch, info.branch] | ||
| ) | ||
|
|
||
| if info.branch not in check_merged.stdout: | ||
| print(f"Error: Branch {info.branch} is not fully merged into {self.base_branch}") | ||
| print("This may indicate a fast-forward merge or other issue.") | ||
| print("Please verify with: git log --oneline -n 10") | ||
| return False | ||
| else: | ||
| print(f"Branch {info.branch} appears to be already merged. Continuing...") | ||
| else: | ||
| print(f"✓ Merge commit created successfully: {latest_commit_msg}") |
There was a problem hiding this comment.
Missing error handling when verification command fails.
If the git log command fails (returncode != 0), the verification block is silently skipped, and the merge proceeds as if successful. This defeats the PR's purpose of preventing data loss by ensuring a merge commit exists before cleanup.
🐛 Proposed fix: Add else branch to handle verification failure
verify_result = self._run_git(["log", "-1", "--format=%s"])
if verify_result.returncode == 0:
latest_commit_msg = verify_result.stdout.strip()
expected_msg = f"auto-claude: Merge {info.branch}"
if latest_commit_msg != expected_msg:
print(f"Warning: Expected merge commit message not found.")
print(f"Expected: '{expected_msg}'")
print(f"Got: '{latest_commit_msg}'")
# Check if branch is already merged (acceptable edge case)
check_merged = self._run_git(
["branch", "--merged", self.base_branch, info.branch]
)
if info.branch not in check_merged.stdout:
print(f"Error: Branch {info.branch} is not fully merged into {self.base_branch}")
print("This may indicate a fast-forward merge or other issue.")
print("Please verify with: git log --oneline -n 10")
return False
else:
print(f"Branch {info.branch} appears to be already merged. Continuing...")
else:
print(f"✓ Merge commit created successfully: {latest_commit_msg}")
+ else:
+ print(f"Error: Could not verify merge commit: {verify_result.stderr}")
+ print("Aborting to prevent potential data loss.")
+ return False| check_merged = self._run_git( | ||
| ["branch", "--merged", self.base_branch, info.branch] | ||
| ) | ||
|
|
||
| if info.branch not in check_merged.stdout: | ||
| print(f"Error: Branch {info.branch} is not fully merged into {self.base_branch}") | ||
| print("This may indicate a fast-forward merge or other issue.") | ||
| print("Please verify with: git log --oneline -n 10") | ||
| return False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add returncode check for the fallback merged-branch verification.
The fallback check at lines 455-457 doesn't verify that the git branch --merged command succeeded before checking its output. If the command fails, check_merged.stdout may be empty, causing a false negative and unnecessary failure.
♻️ Proposed improvement
# Check if branch is already merged (acceptable edge case)
check_merged = self._run_git(
["branch", "--merged", self.base_branch, info.branch]
)
- if info.branch not in check_merged.stdout:
+ if check_merged.returncode != 0:
+ print(f"Error: Could not verify branch merge status: {check_merged.stderr}")
+ return False
+ elif info.branch not in check_merged.stdout:
print(f"Error: Branch {info.branch} is not fully merged into {self.base_branch}")🤖 Prompt for AI Agents
In @apps/backend/core/worktree.py around lines 455 - 463, The fallback
merged-branch verification uses the subprocess result from self._run_git (stored
in check_merged) but never checks its return code, so a failed git command can
produce empty stdout and a false negative; modify the block that calls
self._run_git(["branch", "--merged", self.base_branch, info.branch]) to first
verify check_merged.returncode == 0 (or use check=True) before inspecting
check_merged.stdout, and if the command failed, log or print an explicit error
mentioning the git failure and return False (or propagate the error) instead of
proceeding to string-match info.branch against possibly empty stdout.
| /** | ||
| * Search for a shell executable in PATH | ||
| */ | ||
| function findShellInPath(shellName: string): string | null { | ||
| const pathEnv = process.env.PATH || ''; | ||
| const pathSeparator = process.platform === 'win32' ? ';' : ':'; | ||
| const paths = pathEnv.split(pathSeparator); | ||
|
|
||
| for (const dir of paths) { | ||
| const shellPath = `${dir}\\${shellName}`; | ||
| if (existsSync(shellPath)) { | ||
| return shellPath; | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Fix cross-platform path construction.
The function determines the PATH separator based on platform (line 49) but always uses backslash for path construction (line 53). While this works today since the function is only called on Windows, the inconsistency could cause bugs if the code is reused.
🔧 Proposed fix using path.join
+import * as path from 'path';
function findShellInPath(shellName: string): string | null {
const pathEnv = process.env.PATH || '';
const pathSeparator = process.platform === 'win32' ? ';' : ':';
const paths = pathEnv.split(pathSeparator);
for (const dir of paths) {
- const shellPath = `${dir}\\${shellName}`;
+ const shellPath = path.join(dir, shellName);
if (existsSync(shellPath)) {
return shellPath;
}
}
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Search for a shell executable in PATH | |
| */ | |
| function findShellInPath(shellName: string): string | null { | |
| const pathEnv = process.env.PATH || ''; | |
| const pathSeparator = process.platform === 'win32' ? ';' : ':'; | |
| const paths = pathEnv.split(pathSeparator); | |
| for (const dir of paths) { | |
| const shellPath = `${dir}\\${shellName}`; | |
| if (existsSync(shellPath)) { | |
| return shellPath; | |
| } | |
| } | |
| return null; | |
| } | |
| import * as path from 'path'; | |
| /** | |
| * Search for a shell executable in PATH | |
| */ | |
| function findShellInPath(shellName: string): string | null { | |
| const pathEnv = process.env.PATH || ''; | |
| const pathSeparator = process.platform === 'win32' ? ';' : ':'; | |
| const paths = pathEnv.split(pathSeparator); | |
| for (const dir of paths) { | |
| const shellPath = path.join(dir, shellName); | |
| if (existsSync(shellPath)) { | |
| return shellPath; | |
| } | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/pty-manager.ts around lines 44 - 59, The
findShellInPath function builds PATH parts manually and always uses a backslash
for joining (inconsistent with detected platform); replace the manual
pathSeparator and string concatenation with Node's path utilities: use
path.delimiter to split process.env.PATH and use path.join(dir, shellName) to
construct shellPath (ensure you import/require 'path'); keep the existsSync
check and return behavior unchanged.
| function getWindowsShell(preferredTerminal: SupportedTerminal | undefined): string { | ||
| // Read user settings for custom shell paths | ||
| const settings = readSettingsFile(); | ||
| const customShellPaths = settings?.terminal?.shellPaths as Record<string, string[]> | undefined; | ||
|
|
||
| // If no preference or 'system', use COMSPEC (usually cmd.exe) | ||
| if (!preferredTerminal || preferredTerminal === 'system') { | ||
| return process.env.COMSPEC || 'cmd.exe'; | ||
| } |
There was a problem hiding this comment.
Validate user settings to prevent runtime errors.
The code reads terminal.shellPaths from user settings and type-asserts it to Record<string, string[]> without validation (line 68). If the settings file is malformed or contains non-string values, the code could crash when spreading custom paths (line 80) or calling existsSync (line 91).
🛡️ Proposed validation
function getWindowsShell(preferredTerminal: SupportedTerminal | undefined): string {
// Read user settings for custom shell paths
const settings = readSettingsFile();
- const customShellPaths = settings?.terminal?.shellPaths as Record<string, string[]> | undefined;
+ let customShellPaths: Record<string, string[]> | undefined;
+
+ // Validate custom shell paths structure
+ if (settings?.terminal?.shellPaths && typeof settings.terminal.shellPaths === 'object') {
+ const paths = settings.terminal.shellPaths as Record<string, unknown>;
+ const validated: Record<string, string[]> = {};
+
+ for (const [key, value] of Object.entries(paths)) {
+ if (Array.isArray(value) && value.every(v => typeof v === 'string')) {
+ validated[key] = value;
+ }
+ }
+
+ if (Object.keys(validated).length > 0) {
+ customShellPaths = validated;
+ }
+ }
// If no preference or 'system', use COMSPEC (usually cmd.exe)
if (!preferredTerminal || preferredTerminal === 'system') {
return process.env.COMSPEC || 'cmd.exe';
}🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/pty-manager.ts around lines 65 - 73, The
function getWindowsShell reads settings via readSettingsFile and blindly casts
settings?.terminal?.shellPaths to customShellPaths; validate that
settings?.terminal?.shellPaths is an object whose values are arrays of strings
before using them: check that customShellPaths is a plain object, iterate its
entries and filter each value to an array, then map/filter to keep only string
elements (or ignore malformed entries), and fall back to an empty record if
validation fails; use the validatedCustomShellPaths when spreading paths and
before calling existsSync so you never pass non-string values to path
operations.
| const settings = readSettingsFile(); | ||
| const customShellPaths = settings?.terminal?.shellPaths as Record<string, string[]> | undefined; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching settings to reduce file I/O.
The settings file is read twice per PTY spawn (here and again at line 128). While not critical, this duplicated file I/O could impact performance when creating multiple terminals.
💡 Optimization approaches
Option 1: Read settings once and pass as parameter:
function getWindowsShell(
preferredTerminal: SupportedTerminal | undefined,
settings?: ReturnType<typeof readSettingsFile>
): string {
const customShellPaths = settings?.terminal?.shellPaths as Record<string, string[]> | undefined;
// ... rest of function
}
// In spawnPtyProcess:
const settings = readSettingsFile();
const preferredTerminal = settings?.preferredTerminal as SupportedTerminal | undefined;
const shell = process.platform === 'win32'
? getWindowsShell(preferredTerminal, settings)
: process.env.SHELL || '/bin/zsh';Option 2: Cache settings with a file watcher to invalidate on changes.
🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/pty-manager.ts around lines 67 - 68,
Duplicate reads of the settings file occur when spawning a PTY; cache the
settings once and pass them to functions that need them (or implement a cached
read with invalidation). Specifically, call readSettingsFile() once in
spawnPtyProcess and store its result, pass that settings object into
getWindowsShell (or similar helper) so getWindowsShell can use
settings?.terminal?.shellPaths (customShellPaths) instead of calling
readSettingsFile again; alternatively implement a module-level cachedSettings
with a file watcher to invalidate on change and have readSettingsFile return the
cached value. Ensure you update uses of readSettingsFile in spawnPtyProcess and
the Windows shell resolution to accept and use the provided settings object.
| // Read user's preferred terminal setting | ||
| const settings = readSettingsFile(); | ||
| const preferredTerminal = settings?.preferredTerminal as SupportedTerminal | undefined; | ||
|
|
||
| const shell = process.platform === 'win32' | ||
| ? process.env.COMSPEC || 'cmd.exe' | ||
| ? getWindowsShell(preferredTerminal) | ||
| : process.env.SHELL || '/bin/zsh'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating preferredTerminal setting.
The code type-asserts settings?.preferredTerminal to SupportedTerminal | undefined without validation (line 129). While getWindowsShell has fallback logic that handles invalid values safely, explicit validation would make the code more robust and easier to debug.
🔍 Proposed validation
// Read user's preferred terminal setting
const settings = readSettingsFile();
- const preferredTerminal = settings?.preferredTerminal as SupportedTerminal | undefined;
+
+ const validTerminals = new Set<SupportedTerminal>([
+ 'system', 'powershell', 'cmd', 'gitbash', 'cygwin', 'msys2', 'windowsterminal'
+ ]);
+
+ const preferredTerminal = settings?.preferredTerminal &&
+ validTerminals.has(settings.preferredTerminal as SupportedTerminal)
+ ? (settings.preferredTerminal as SupportedTerminal)
+ : undefined;This ensures only valid terminal types are passed to getWindowsShell, making debugging easier if users provide invalid configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Read user's preferred terminal setting | |
| const settings = readSettingsFile(); | |
| const preferredTerminal = settings?.preferredTerminal as SupportedTerminal | undefined; | |
| const shell = process.platform === 'win32' | |
| ? process.env.COMSPEC || 'cmd.exe' | |
| ? getWindowsShell(preferredTerminal) | |
| : process.env.SHELL || '/bin/zsh'; | |
| // Read user's preferred terminal setting | |
| const settings = readSettingsFile(); | |
| const validTerminals = new Set<SupportedTerminal>([ | |
| 'system', 'powershell', 'cmd', 'gitbash', 'cygwin', 'msys2', 'windowsterminal' | |
| ]); | |
| const preferredTerminal = settings?.preferredTerminal && | |
| validTerminals.has(settings.preferredTerminal as SupportedTerminal) | |
| ? (settings.preferredTerminal as SupportedTerminal) | |
| : undefined; | |
| const shell = process.platform === 'win32' | |
| ? getWindowsShell(preferredTerminal) | |
| : process.env.SHELL || '/bin/zsh'; |
🤖 Prompt for AI Agents
In @apps/frontend/src/main/terminal/pty-manager.ts around lines 127 - 133,
Validate the user setting returned by readSettingsFile before asserting it as
SupportedTerminal: check that settings?.preferredTerminal is one of the known
SupportedTerminal enum/string values (use a whitelist or type-guard function)
and only pass the validated value to getWindowsShell; if validation fails,
fallback to undefined (or a default) and optionally log the invalid value for
debugging to avoid blindly casting and passing bad data into getWindowsShell.
|
Can you check if this is fixed? |
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - Merge conflicts must be resolved before merge.
Blocked: PR has merge conflicts with base branch. Resolve conflicts before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | High | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Merge Conflicts: PR has conflicts with base branch that must be resolved
Findings Summary
- High: 3 issue(s)
- Medium: 3 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (7 selected of 7 total)
🟠 [b3020c430ec3] [HIGH] Incorrect git branch --merged syntax will cause verification failures
📁 apps/backend/core/worktree.py:455
The command git branch --merged {self.base_branch} {info.branch} passes the branch name as a second positional argument, which is not valid git syntax. git branch --merged <commit> lists branches merged into but does not accept a second branch argument. This will likely fail or produce unexpected output, causing the verification check at line 459 to behave incorrectly.
Suggested fix:
Use `['branch', '--merged', self.base_branch]` (without the second argument) and then check if info.branch appears in the output. Alternatively, use `git merge-base --is-ancestor {info.branch} {self.base_branch}` which returns exit code 0 if the branch is merged.
🟠 [6938279a0ff5] [HIGH] Inconsistent git executable usage - hardcoded 'git' vs getToolPath
📁 apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts:1627
On line 1627-1630, the code uses execFileSync('git', ...) directly instead of execFileSync(getToolPath('git'), ...) which is used consistently throughout the rest of the file (lines 1690, 1741, 1749, etc.). This inconsistency could cause the merge-base check to fail on systems where git is not in the default PATH but is configured via getToolPath.
Suggested fix:
Change line 1627 from execFileSync('git', ...) to execFileSync(getToolPath('git'), ...) to maintain consistency with the rest of the file.
🟠 [db854b218e24] [HIGH] Line ending preservation inconsistency causes mixed endings in merged files
📁 apps/backend/merge/file_merger.py:109
In apply_single_task_changes() at lines 50-54, the code correctly detects and preserves original line endings. However, in combine_non_conflicting_changes() at line 109, it simply uses '\n'.join(lines) without preserving the original line ending style. This inconsistency could cause mixed line endings in files when multiple tasks modify the same file.
Suggested fix:
Apply the same line ending detection and preservation pattern from apply_single_task_changes() to combine_non_conflicting_changes(): detect the original ending at the start and use 'content = original_ending.join(lines)'
🟡 [6bd45aa8b7b5] [MEDIUM] Branch name substring matching may cause false positives
📁 apps/backend/core/worktree.py:459
The check if info.branch not in check_merged.stdout uses substring matching. If you have branches like 'auto-claude/spec1' and 'auto-claude/spec10', checking for 'auto-claude/spec1' would match 'auto-claude/spec10' as well. This could lead to incorrectly concluding a branch is merged when it is not.
Suggested fix:
Parse the output line by line and strip whitespace before comparison: `if info.branch not in [line.strip().lstrip('* ') for line in check_merged.stdout.splitlines()]`
🟡 [6ff97bc26132] [MEDIUM] Inconsistent line splitting may break on Windows CRLF
📁 apps/backend/core/worktree.py:153
Multiple locations in worktree.py use split('\n') instead of splitlines() for git command output: line 153 (staged_files), line 171 (gitignored files loop), line 527 (list_all_spec_branches). On Windows with CRLF line endings, this would leave trailing \r characters in file/branch names, causing subsequent git commands to fail.
Suggested fix:
Change all instances of split('\n') to splitlines() for cross-platform consistency: staged_files = result.stdout.strip().splitlines()
🟡 [5c2cb0db46c0] [MEDIUM] Inline changelog extraction duplicates scripts/extract-changelog.sh
📁 .github/workflows/prepare-release.yml:89
Lines 89-117 contain a 50+ line awk script for changelog extraction, but scripts/extract-changelog.sh already provides this functionality and is used by release.yml at line 520. This creates maintenance burden - if the changelog format changes, two places need updating. It also makes the workflow file harder to read and debug.
Suggested fix:
Replace the inline awk script with: CHANGELOG_CONTENT=$(bash scripts/extract-changelog.sh "$VERSION" "$CHANGELOG_FILE") as done in release.yml
🔵 [2aad9363d558] [LOW] Fast-forward merges may be incorrectly treated as failures
📁 apps/backend/core/worktree.py:444
When git performs a fast-forward merge (even with --no-ff flag in certain edge cases), no new commit is created, so the expected merge commit message at line 447 won't be found. The code then falls back to the git branch --merged check which has its own issues (finding-1). A successful fast-forward or already-merged scenario could trigger the error path.
Suggested fix:
Before checking commit message, use `git merge-base --is-ancestor` to check if the branch is already fully contained in the base, which handles both fast-forward and already-merged scenarios correctly.
This review was generated by Auto Claude.
5 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Fixes a critical bug where clicking "Merge to main" deletes the worktree but doesn't actually commit changes to git. This PR adds verification that the merge commit is created before cleanup, preventing data loss. If verification fails, the worktree is preserved for manual investigation.
Related Issue
Closes #797
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
fix: verify merge commit creation before worktree cleanupChanges Made
Backend (
apps/backend/core/worktree.py)git mergecommand"auto-claude: Merge {branch}"Frontend (
apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts)git branch --mergedhuman_reviewgit logRoot Cause Analysis
The original code deleted the worktree immediately after
git mergereturned exit code 0. However, git can return success even when no merge commit is created:This caused commits to disappear from
git log, leading users to believe their work was lost.Checklist
developbranchCI/Testing Requirements
Test Scenarios Verified
git log, worktree deletedhuman_reviewScreenshots
N/A - Backend/Git workflow fix
Feature Toggle
use_feature_nameNote: This is a critical bug fix that prevents data loss. The verification logic is safe-by-default (preserves worktree on errors) and should be enabled for all users immediately.
Breaking Changes
Breaking: No
This PR maintains backward compatibility:
Additional Notes
For Reviewers
Please pay special attention to:
worktree.py(lines 518-548)worktree-handlers.ts(lines 1885-1925)For Users
After this fix:
git loggit log --onelineTesting Instructions for Maintainers