Add self-review, auto-merge, failure recovery, and board enhancements#763
Add self-review, auto-merge, failure recovery, and board enhancements#763robobuidl wants to merge 30 commits intoAutoMaker-Org:mainfrom
Conversation
…cked, In Review, Waiting Approval, Verified, Done Extends the board from 4 fixed columns to 9 to support a full workflow lifecycle. Pipeline steps now route to 'in_review' after completion instead of directly to 'waiting_approval'/'verified'. New CSS variables, status badges, drag-drop handlers, and graph view nodes for all new statuses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy of the generic project bootstrapper template for easy access when starting new projects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Auto-create git worktrees when branchName is set but no worktree exists, instead of silently falling back to the main project directory. Updated setup template with branchName rules, pipeline model notes, and OAuth one-time code exchange pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Features now keep branchName=null until execution starts, preventing them from being filtered out of the main worktree view in the UI. The auto-mode-service generates and persists a branch name when a feature begins executing with worktrees enabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pre-setting branchName on backlog features causes them to be filtered out of the main worktree view in the UI. Updated guidance to leave branchName null — Automaker auto-generates it at execution time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The auto-branchName generation was persisting to feature.json and updating the running feature tracker, which moved features out of the main worktree scope. This caused getRunningCountForWorktree to report 0 running, allowing the loop to start ALL backlog features instead of respecting the maxConcurrency limit. Fix: generate branchName locally for worktree creation without persisting it, and preserve the original branchName in the running feature tracker for accurate concurrency counting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Features must be manually moved to Ready before auto-mode will execute them. This gives users explicit control over which features run, matching the intended kanban workflow: Backlog → Ready → execute. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Auto-mode picks from Ready only. Users must manually move features from Backlog to Ready to queue them for execution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When auto-mode verifies a feature in a worktree, the branch now automatically squash-merges back to main so dependent features can start from updated code. Adds a safety net that blocks dependent features until their dependencies are actually merged (mergedToMain flag). Extracts reusable merge/cleanup utilities from the worktree merge route into @automaker/git-utils. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The loadPendingFeatures filter still included status === 'pending' which maps to backlog. Now only 'ready' features are picked up, requiring manual move from Backlog to Ready before auto-mode runs them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The missing dependency removal logic compared against pendingFeatures (only ready/pending features) instead of allFeatures. Dependencies that were in_progress, verified, or any other non-pending status were incorrectly flagged as "missing" and permanently deleted from feature.json. Now only truly missing dependencies (not in allFeatures at all) are removed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s toggle UI - Set feature.startedAt timestamp when status changes to in_progress so the UI timer displays elapsed time instead of infinite spinner - Append Auto-Merge section to agent-output.md after successful merge (timestamp, branch, merge type, cleanup status) - Add autoMergeOnVerify toggle to settings UI with sync to server Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Clear startedAt when feature leaves in_progress (prevents stale timers) - Handle "nothing to squash" in merge utility when branches are identical - Show merged features on main board (mergedToMain bypasses worktree filter) - Fix useState ordering in worktree-panel (autoModeConfirmWorktree TDZ error) - Add auto-mode confirmation dialog with settings summary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The extractAutoMergeSection regex was rewritten by an agent with literal newlines, breaking esbuild transform. Restored proper \n escapes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When auto-mode features fail, they now automatically retry with error context injection and optional model escalation (e.g., sonnet -> opus) instead of silently moving to backlog. After all retries are exhausted, features move to 'failed' status for clear visibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds an optional self-review step that runs after implementation (and pipeline steps) but before setting final status. Uses simpleQuery for a lightweight read-only review of the diff, then runAgent for an agentic fix pass if issues are found. Controlled by enableSelfReview setting (default: true). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughUpdates default Claude Opus mapping to 4.6, adds a debug endpoint for resolved models, expands feature statuses and UI columns, implements auto-mode self-review/retry/auto-merge flows, centralizes git merge/cleanup utilities, and wires new settings and token-usage tracking across services and UI. Changes
Sequence Diagram(s)sequenceDiagram
participant AutoSvc as AutoModeService
participant Merge as mergeWorktreeBranch
participant Cleanup as cleanupWorktree
participant Store as FeatureStore
participant Logger as EventLogger
AutoSvc->>AutoSvc: detect verified feature
AutoSvc->>Merge: mergeWorktreeBranch(projectPath, branchName, 'main')
Merge-->>AutoSvc: MergeResult{success, hasConflicts, error}
alt success
AutoSvc->>Cleanup: cleanupWorktree(projectPath, worktreePath, branchName)
Cleanup-->>AutoSvc: CleanupResult{worktreeDeleted, branchDeleted}
AutoSvc->>Store: set feature.mergedToMain = true
AutoSvc->>Logger: emit auto-merge success
else hasConflicts
AutoSvc->>Logger: emit merge conflict (409)
else error
AutoSvc->>Logger: emit merge failure (400)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 @robobuidl, 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 introduces a suite of significant enhancements to the automated development workflow, focusing on increased autonomy, resilience, and visibility. The core purpose is to streamline the agentic development lifecycle by automating review, merge, and recovery processes, while providing a more granular and informative project management interface. These changes aim to reduce manual intervention, improve the reliability of automated feature delivery, and offer better insights into the state and progress of development tasks. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
![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 is a substantial pull request that introduces several powerful new features, including self-review, auto-merge, and failure recovery with retries. The expansion of the Kanban board to 9 columns and the automatic creation of worktrees are also great enhancements to the workflow. I'm particularly impressed with the refactoring in apps/server/src/routes/worktree/routes/merge.ts to use shared git utilities, which greatly improves code clarity and maintainability. The updates to the Claude Opus model version are consistent across the codebase. The new debug endpoint for model resolution is a thoughtful addition for troubleshooting. My feedback is minor and focuses on improving type safety in the new debug route. Overall, this is an excellent set of improvements.
|
|
||
| const defaultFeatureModelKey = settings.defaultFeatureModel?.model; | ||
|
|
||
| const phaseModels = settings.phaseModels || ({} as any); |
There was a problem hiding this comment.
Using as any bypasses type safety. Since settings.phaseModels can be undefined, using the nullish coalescing operator ?? {} is a safer way to provide a default empty object. The properties on phaseModels can then be accessed with optional chaining (e.g., phaseModels?.specGenerationModel), which resolvePhaseModel already handles gracefully for undefined inputs.
| const phaseModels = settings.phaseModels || ({} as any); | |
| const phaseModels = settings.phaseModels ?? {}; |
| const legacyValidationModelKey = (settings as any).validationModel; | ||
| const legacyEnhancementModelKey = (settings as any).enhancementModel; |
There was a problem hiding this comment.
Using as any to access legacy properties bypasses type safety. A safer alternative is to cast to Record<string, unknown> to indicate that you are accessing properties that may not be part of the defined type. This makes the intent clearer and is slightly safer than a full any cast.
| const legacyValidationModelKey = (settings as any).validationModel; | |
| const legacyEnhancementModelKey = (settings as any).enhancementModel; | |
| const legacyValidationModelKey = (settings as Record<string, unknown>).validationModel; | |
| const legacyEnhancementModelKey = (settings as Record<string, unknown>).enhancementModel; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/ui/src/components/views/graph-view/components/task-node.tsx (1)
159-169:⚠️ Potential issue | 🟡 MinorNew statuses fall through to the default border color.
The nested ternary only checks for
border-border,status-in-progress,status-waiting, andstatus-success. The five new statuses (ready,assigned,blocked,in_review,done) have border classes likeborder-[var(--status-ready)]that don't match any branch, so they all resolve tovar(--border)instead of their status-specific color.This causes the graph node's inline border to ignore the status-specific border color for these new columns.
🔧 Proposed fix — extract the CSS variable from the border class generically
const borderColor = data.error ? 'var(--status-error)' - : config.borderClass.includes('border-border') - ? 'var(--border)' - : config.borderClass.includes('status-in-progress') - ? 'var(--status-in-progress)' - : config.borderClass.includes('status-waiting') - ? 'var(--status-waiting)' - : config.borderClass.includes('status-success') - ? 'var(--status-success)' - : 'var(--border)'; + : (() => { + const match = config.borderClass.match(/border-\[var\((--[^)]+)\)\]/); + return match ? `var(${match[1]})` : 'var(--border)'; + })();libs/types/src/model-display.ts (1)
208-220:⚠️ Potential issue | 🟠 MajorMissing display-name entries for the new GPT-5.3 models.
CODEX_MODEL_MAP.gpt53Codex('codex-gpt-5.3-codex') andCODEX_MODEL_MAP.gpt53('codex-gpt-5.3') were added toCODEX_MODELS(lines 74–81, 106–113) but are not mapped ingetModelDisplayName(). These will fall through to the raw model ID string.Proposed fix
const displayNames: Record<string, string> = { haiku: 'Claude Haiku', sonnet: 'Claude Sonnet', opus: 'Opus 4.6', 'claude-opus-4-6': 'Opus 4.6', + [CODEX_MODEL_MAP.gpt53Codex]: 'GPT-5.3-Codex', [CODEX_MODEL_MAP.gpt52Codex]: 'GPT-5.2-Codex', [CODEX_MODEL_MAP.gpt51CodexMax]: 'GPT-5.1-Codex-Max', [CODEX_MODEL_MAP.gpt51CodexMini]: 'GPT-5.1-Codex-Mini', + [CODEX_MODEL_MAP.gpt53]: 'GPT-5.3', [CODEX_MODEL_MAP.gpt52]: 'GPT-5.2', [CODEX_MODEL_MAP.gpt51]: 'GPT-5.1', };libs/types/src/model.ts (1)
76-81:⚠️ Potential issue | 🟠 MajorNew GPT-5.3 models missing from
REASONING_CAPABLE_MODELS.
gpt53Codexandgpt53are both declared withhasReasoning: trueinmodel-display.ts(lines 80, 112), but they're not included inREASONING_CAPABLE_MODELS. This meanssupportsReasoningEffort()will returnfalsefor these models, preventing users from configuring reasoning effort.Proposed fix
export const REASONING_CAPABLE_MODELS = new Set([ + CODEX_MODEL_MAP.gpt53Codex, CODEX_MODEL_MAP.gpt52Codex, CODEX_MODEL_MAP.gpt51CodexMax, + CODEX_MODEL_MAP.gpt53, CODEX_MODEL_MAP.gpt52, CODEX_MODEL_MAP.gpt51, ]);Based on learnings: "When modeling AI capabilities, add per-model flags to model definitions (e.g., supportsThinking: boolean) and check capabilities by model ID rather than assuming all models from a provider share the same features."
apps/ui/src/lib/agent-context-parser.ts (1)
35-49:⚠️ Potential issue | 🟡 Minor
model.includes('opus')short-circuits Cursor model matching — and new GPT-5.3 models are unhandled.Two issues in
formatModelName:
The broad
.includes('opus')check on line 37 matchescursor-opus-*models before thestartsWith('cursor-opus')check on line 55 is reached, so Cursor Opus models incorrectly display as "Opus 4.6". (This ordering issue pre-dates this PR but is worth fixing alongside the label change.)The new
codex-gpt-5.3-codexandcodex-gpt-5.3models (added inmodel-display.ts) have no entries here, so they'll fall through to the generic split-and-capitalize fallback on line 62.Proposed fix
export function formatModelName(model: string): string { + // Cursor models first (more specific prefixes) + if (model === 'cursor-auto' || model === 'auto') return 'Cursor Auto'; + if (model === 'cursor-composer-1' || model === 'composer-1') return 'Composer 1'; + if (model.startsWith('cursor-sonnet')) return 'Cursor Sonnet'; + if (model.startsWith('cursor-opus')) return 'Cursor Opus'; + if (model.startsWith('cursor-gpt')) return model.replace('cursor-', '').replace('gpt-', 'GPT-'); + if (model.startsWith('cursor-gemini')) + return model.replace('cursor-', 'Cursor ').replace('gemini', 'Gemini'); + if (model.startsWith('cursor-grok')) return 'Cursor Grok'; + // Claude models if (model.includes('opus')) return 'Opus 4.6'; if (model.includes('sonnet')) return 'Sonnet 4.5'; if (model.includes('haiku')) return 'Haiku 4.5'; // Codex/GPT models - specific formatting + if (model === 'codex-gpt-5.3-codex') return 'GPT-5.3 Codex'; + if (model === 'codex-gpt-5.3') return 'GPT-5.3'; if (model === 'codex-gpt-5.2-codex') return 'GPT-5.2 Codex'; ... - - // Cursor models - if (model === 'cursor-auto' || model === 'auto') return 'Cursor Auto'; - ...libs/types/src/cursor-models.ts (1)
35-45:⚠️ Potential issue | 🟠 MajorLegacy migration entries for
opus-4.5/opus-4.5-thinkingare missing.The
LegacyCursorModelIdtype andLEGACY_CURSOR_MODEL_MAPexist to migrate old settings to canonical IDs. The current version only includes'opus-4.6'and'opus-4.6-thinking', but lacks entries for the previous'opus-4.5'variants. Users who have'opus-4.5'stored in their configuration will not be migrated — the old ID will not match any entry in the legacy map. The legacy type and map should retain old entries alongside new ones.Proposed fix: add legacy opus-4.5 entries
export type LegacyCursorModelId = | 'auto' | 'composer-1' | 'sonnet-4.5' | 'sonnet-4.5-thinking' + | 'opus-4.5' + | 'opus-4.5-thinking' | 'opus-4.6' | 'opus-4.6-thinking' | 'opus-4.1' | 'gemini-3-pro' | 'gemini-3-flash' | 'grok';And in
LEGACY_CURSOR_MODEL_MAP:'sonnet-4.5-thinking': 'cursor-sonnet-4.5-thinking', + 'opus-4.5': 'cursor-opus-4.6', + 'opus-4.5-thinking': 'cursor-opus-4.6-thinking', 'opus-4.6': 'cursor-opus-4.6', 'opus-4.6-thinking': 'cursor-opus-4.6-thinking',apps/ui/src/components/views/board-view/constants.ts (1)
104-109:⚠️ Potential issue | 🟡 Minor
EMPTY_STATE_CONFIGS.defaultis undefined — fallback returnsundefinedat runtime.
getEmptyStateConfigfalls back toEMPTY_STATE_CONFIGS.defaulton line 108, but no'default'key exists in the record. If a non-pipelinecolumnIddoesn't match any key, this returnsundefineddespite theEmptyStateConfigreturn type. This is pre-existing but now more likely to trigger with the expanded status set.Suggested fix
export function getEmptyStateConfig(columnId: string): EmptyStateConfig { if (columnId.startsWith('pipeline_')) { return EMPTY_STATE_CONFIGS.pipeline_default; } - return EMPTY_STATE_CONFIGS[columnId] || EMPTY_STATE_CONFIGS.default; + return EMPTY_STATE_CONFIGS[columnId] || EMPTY_STATE_CONFIGS.pipeline_default; }Alternatively, add an explicit
defaultentry toEMPTY_STATE_CONFIGS.apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts (1)
151-151:⚠️ Potential issue | 🟡 MinorNo-op ternary — both branches return the same value.
worktreeData.isMain ? targetBranch : targetBranchalways evaluates totargetBranch. This looks like an oversight — theisMaincase was likely intended to display something different (e.g.,"main"or the repo's default branch name).Suggested fix
- const branchDisplay = worktreeData.isMain ? targetBranch : targetBranch; + const branchDisplay = targetBranch;Or, if the intent was to label main differently:
- const branchDisplay = worktreeData.isMain ? targetBranch : targetBranch; + const branchDisplay = worktreeData.isMain ? 'main' : targetBranch;
🤖 Fix all issues with AI agents
In `@apps/server/src/routes/debug/index.ts`:
- Around line 23-85: The async route handler registered with
router.get('/resolved-models') must catch errors from await
settingsService.getGlobalSettings() (and other async calls); update the handler
signature to include next: NextFunction, wrap the existing body in try/catch,
and on error call next(err) so Express error middleware handles it. Ensure you
import NextFunction from express if needed and keep all existing logic
(resolvePhaseModel/resolveModelString and res.json(result)) inside the try
block.
In `@apps/server/src/routes/features/routes/update.ts`:
- Line 14: Update the stale inline comment that references "verified or
completed" to reflect the current SYNC_TRIGGER_STATUSES array which includes
'verified', 'done', and 'completed'; locate the comment near the
SYNC_TRIGGER_STATUSES constant (symbol: SYNC_TRIGGER_STATUSES) and change the
wording to mention "verified, done, or completed" (or an equivalent phrase) so
the comment matches the actual values.
In `@apps/server/src/services/auto-mode-service.ts`:
- Around line 3571-3574: The code sets a feature status of 'error' on merge
conflicts which isn't part of the board's status set and will hide the feature;
update the call in the conflict branch (where result.hasConflicts is checked and
logger.error is called) to use the canonical status 'failed' instead by changing
updateFeatureStatus(projectPath, featureId, 'error') to
updateFeatureStatus(projectPath, featureId, 'failed') so the feature appears in
the board's columns.
- Around line 3357-3365: The fire-and-forget call to
this.executeFeature(projectPath, featureId, useWorktrees, isAutoMode, undefined,
{ continuationPrompt: retryPrompt }) lacks a .catch(), risking unhandled promise
rejections; append a .catch handler that logs the error (e.g. this.logger.error
or processLogger.error) and any relevant context (featureId, projectPath) so
failures during retry are observed, mirroring the pattern used for other
fire-and-forget executeFeature calls in this file.
- Around line 3491-3506: Auto-merge currently hardcodes 'main' as the merge
target in mergeWorktreeBranch, which breaks repos whose primary branch is
'master' or another name; update the auto-merge logic to resolve the actual
default/primary branch (use the existing getCurrentBranch(projectPath) or
another helper) and pass that resolved branch as the target instead of the
literal 'main'. Ensure the code still skips merging when branchName is missing
or equal to the resolved primary branch, and keep the squash/message behavior
(references: mergeWorktreeBranch, getCurrentBranch, featureId, branchName,
settingsService.getGlobalSettings).
- Around line 3162-3199: createWorktreeForBranch currently interpolates
user-controlled branchName into shell commands via execAsync which allows shell
injection; fix by avoiding shell interpretation when running git: stop using
string-interpolated execAsync calls for rev-parse and worktree add and instead
call a child-process API that accepts an argv array (e.g., execFile/spawn with
args) or a safe execAsync wrapper that passes ["git", "rev-parse", "--verify",
branchName] and ["git","worktree","add", ...] (use sanitizedName only for
filesystem paths, keep branchName validated against a strict pattern or reject
unsafe characters before passing as an arg), and update the code paths inside
createWorktreeForBranch so all git invocations use argument arrays rather than
shell strings.
In `@apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx`:
- Around line 210-218: The stats for the Auto‑Mode confirmation dialog are
currently computed from global values (storeFeatures, maxConcurrency,
autoMergeOnVerify, useWorktreesSetting) but need to be scoped to the selected
worktree/branch; update the component to select the current worktree (e.g., via
a useAppStore selector for selectedWorktree or currentBranch) and then compute
readyFeaturesCount from selectedWorktree.features.filter(f => f.status ===
'ready').length and pull maxConcurrency/autoMergeOnVerify/useWorktrees from
selectedWorktree.settings (or per-worktree fields) instead of the global values
so the dialog reflects what will actually run for that worktree.
In `@docs/automaker-setup-prompt-template.md`:
- Around line 254-256: Remove the project-specific "Security Pattern — OAuth
One-Time Code Exchange" section from the generic prompt template (the header
titled exactly "Security Pattern — OAuth One-Time Code Exchange") and relocate
it into a separate OAuth-specific security guide or snippet; update the template
so that any OAuth-specific guidance is conditionally included only when the Step
1 analysis detects OAuth in the tech stack (i.e., make inclusion of the OAuth
snippet contingent on the Step 1 outcome), and ensure the generic template no
longer contains hardcoded OAuth implementation details.
- Around line 197-200: The model tier table currently lists ranges (20-30% opus,
55-65% sonnet, 10-15% haiku) that conflict with the verification checklist
values (~25% opus, ~60% sonnet, ~15% haiku); update one of them so both the
"model tier table" and the "verification checklist" use the exact same target
percentages (e.g., set the table to ~25% opus, ~60% sonnet, ~15% haiku) and
ensure the wording around those symbols matches exactly to avoid inconsistency.
- Line 176: Update the docs to resolve the status inconsistency by expanding the
feature.json status examples or adding a clarifying note: either list all
possible status values (e.g., include verified, backlog, ready, assigned,
in_progress, etc.) in the `"status": "..."` example on the schema line that
currently shows `"status": "verified|backlog"`, or add a short explanatory
sentence after the schema (referencing the "status" field and the auto-mode
workflow) stating that `verified` and `backlog` are initial creation statuses
and that `ready` and other statuses are assigned by the Kanban/auto-mode
workflow.
In `@docs/server/utilities.md`:
- Around line 260-265: The example for resolveModelString shows a fallback of
"claude-opus-4-6" which is inconsistent with the actual DEFAULT_MODELS.claude
value used by ModelResolver; update the documentation example so the call
resolveModelString('invalid-model') and its logged output reflect the real
default (DEFAULT_MODELS.claude), e.g., "claude-sonnet-4-20250514" (or change
DEFAULT_MODELS.claude to match the example if the intent was to keep
"claude-opus-4-6"), and make sure the log line string "[ModelResolver] Unknown
model key \"invalid-model\", using default: \"<DEFAULT_MODELS.claude>\"" matches
exactly.
In `@libs/git-utils/src/merge.ts`:
- Around line 165-173: The code incorrectly sets worktreeDeleted = true after
running 'git worktree prune' even though prune may not remove the specific
worktree; modify the catch block around the execAsync('git worktree prune', ...)
so that you do not assume success — instead verify removal explicitly (for
example by checking fs.existsSync(worktreePath) or running execAsync('git
worktree list --porcelain') and confirming the worktreePath is absent) and only
set worktreeDeleted = true when that verification confirms removal; update the
logger messages in this block to reflect the verification result and keep the
original error path if the worktree still exists (using the same symbols
worktreeDeleted, worktreePath, execAsync, logger).
- Around line 88-96: The merge command builds mergeCmd using a caller-supplied
mergeMsg (options.message) and runs it with execAsync, allowing shell injection;
change the implementation to call a safe execFile variant (e.g., execFileAsync
or child_process.execFile with promisify) and pass the git executable and
arguments as an array (e.g., ['merge','-m', mergeMsg, branchName] or
['merge','--squash', branchName] for squash), and apply the same change for the
squash commit path that uses squashMsg so user input is never interpolated into
a shell string; keep mergeMsg and squashMsg as plain strings but never
concatenate them into a single shell command.
In `@restart-automaker.cmd`:
- Line 6: The comment contains a hardcoded developer path
("C:\Users\Robo1\.openclaw\workspace\automaker") in the REM line; replace it
with a non-personalized instruction or remove it—e.g., change the REM line to
"REM Run from the automaker repository root" or simply delete the REM line—so
update the REM comment in restart-automaker.cmd accordingly.
- Around line 1-2: The batch script restart-automaker.cmd currently has Unix
(LF) line endings which can break Windows batch parsing (e.g., GOTO/CALL labels
like :killPort); convert this file to CRLF line endings and ensure future
commits enforce CRLF by adding a .gitattributes entry such as
"restart-automaker.cmd text eol=crlf" (or update a global .gitattributes) and
re-save the file with Windows/CRLF line endings before committing so labels and
batch control flow (e.g., :killPort) work correctly on Windows.
In `@status-automaker.cmd`:
- Around line 1-18: The status-automaker.cmd file uses LF-only line endings
which can break Windows batch parsing; convert status-automaker.cmd to CRLF line
endings (use your editor or a line-ending conversion tool) and update the
repository text attributes to enforce CRLF for batch files by adding entries for
*.cmd and *.bat with text eol=crlf to .gitattributes, then normalize and
recommit the file so the repository stores and checks out the file with CRLF
endings.
🧹 Nitpick comments (22)
libs/platform/src/system-paths.ts (1)
997-1011: Minor DRY opportunity: reusegetVoltaDirs()to derive Volta binary check paths.The
hasVolta()closure manually constructs paths tovolta.exe/volta.cmdin the same directories thatgetVoltaDirs()already returns. Consider deriving fromgetVoltaDirs()to keep the two in sync and reduce duplication.♻️ Suggested refactor
- const hasVolta = async (): Promise<boolean> => { - const homeDir = os.homedir(); - const localAppData = process.env.LOCALAPPDATA || path.join(homeDir, 'AppData', 'Local'); - const pathsToCheck = [ - path.join(localAppData, 'Volta', 'bin', 'volta.exe'), - path.join(localAppData, 'Volta', 'bin', 'volta.cmd'), - path.join(homeDir, '.volta', 'bin', 'volta.exe'), - path.join(homeDir, '.volta', 'bin', 'volta.cmd'), - ]; - - for (const p of pathsToCheck) { - if (await systemPathAccess(p)) return true; - } - return false; - }; + const hasVolta = async (): Promise<boolean> => { + const voltaBinaries = getVoltaDirs().flatMap((dir) => [ + path.join(dir, 'bin', 'volta.exe'), + path.join(dir, 'bin', 'volta.cmd'), + ]); + + for (const p of voltaBinaries) { + if (await systemPathAccess(p)) return true; + } + return false; + };apps/ui/src/hooks/use-settings-sync.ts (1)
646-646: Consider adding a fallback default forautoMergeOnVerify.When refreshing from a server that hasn't seen this field before,
serverSettings.autoMergeOnVerifywill beundefined, overwriting whatever default the Zustand store provides. Neighboring fields likedefaultSkipTestshave the same pattern, so this is consistent—but if the intended default isfalse, an explicit?? falsewould be more defensive.apps/server/src/services/pipeline-service.ts (1)
253-261:skipTestsbypasses the newin_reviewflow when there are no pipeline steps.The stated flow is
in_progress → [pipeline steps] → in_review → ..., but whenskipTestsis true and there are no pipeline steps, the code returnswaiting_approvaldirectly (line 260), skippingin_review. The comment acknowledges this as "legacy behavior," but it creates an inconsistency: features with pipeline steps always pass throughin_review, while those without can skip it entirely based on a test-related flag.If this is intentional for backward compatibility, consider adding a brief inline comment explaining why
skipTestsshould bypass review (not just that it does).restart-automaker.cmd (2)
21-21: Nested quote escaping is fragile when%ROOT%contains spaces or special characters.The double-double-quote pattern
""%ROOT%apps\server""insidecmd /k "..."works for simple paths but will break ifROOTever contains&,(,), or%. Since%ROOT%comes from%~dp0, it's relatively safe but consider simplifying:🔧 Proposed fix — use pushd/popd to avoid nested quoting
-start "Automaker Backend" cmd /k "cd /d ""%ROOT%apps\server"" && npx.cmd --yes tsx src/index.ts" +start "Automaker Backend" cmd /k "pushd "%ROOT%apps\server" && npx.cmd --yes tsx src/index.ts"
32-38: Port-matching pattern could match unintended substrings.
findstr ":%PORT%"will also match ports containing the target as a substring (e.g.,:30070when looking for:3007). For a developer convenience script this is low risk, but a tighter match would be safer:🔧 Proposed fix — anchor the port match
-for /f "tokens=5" %%a in ('netstat -ano ^| findstr ":%PORT%" ^| findstr LISTENING') do ( +for /f "tokens=5" %%a in ('netstat -ano ^| findstr /R ":%PORT% " ^| findstr LISTENING') do (libs/git-utils/src/merge.ts (1)
30-32: DuplicateisValidBranchName— consider importing fromcommon.tsor extracting to a shared location.This function is duplicated from
apps/server/src/routes/worktree/common.ts(which usesMAX_BRANCH_NAME_LENGTH) while this copy uses a hardcoded250. Since this library (@automaker/git-utils) is intended to be the shared location for git operations, consider moving the canonical implementation here and having the server route import from this package instead.libs/types/src/model-display.ts (1)
60-66: Inconsistent label pattern: "Opus 4.6" vs "Claude Haiku" / "Claude Sonnet".The other Claude model labels use the "Claude [Name]" pattern (lines 48, 55), but the opus entry dropped the "Claude" prefix. This will display inconsistently in the UI model selector. Consider using "Claude Opus 4.6" for consistency, or removing "Claude" from the others if the intent is shorter labels.
apps/server/tests/unit/providers/provider-factory.test.ts (1)
60-63: Consider updating the case-insensitivity test to use the new model string.Line 61 still references
'CLAUDE-OPUS-4-5-20251101'. While the test is still valid (it tests prefix-based routing), updating it to'CLAUDE-OPUS-4-6'would keep the test suite consistent with the current model set.Proposed diff
it('should be case-insensitive for claude models', () => { - const provider = ProviderFactory.getProviderForModel('CLAUDE-OPUS-4-5-20251101'); + const provider = ProviderFactory.getProviderForModel('CLAUDE-OPUS-4-6'); expect(provider).toBeInstanceOf(ClaudeProvider); });apps/server/tests/unit/lib/model-resolver.test.ts (2)
36-42: Hardcoded'claude-opus-4-6'is inconsistent with the haiku/sonnet tests above.Lines 28 and 33 use
CLAUDE_MODEL_MAP.haikuandCLAUDE_MODEL_MAP.sonnetfor expectations, but this test hardcodes the opus string. UsingCLAUDE_MODEL_MAP.opuswould avoid needing to update this test every time the model version changes (as just happened with 4.5 → 4.6).Proposed diff
it("should resolve 'opus' alias to full model string", () => { const result = resolveModelString('opus'); - expect(result).toBe('claude-opus-4-6'); + expect(result).toBe(CLAUDE_MODEL_MAP.opus); expect(consoleSpy.log).toHaveBeenCalledWith( expect.stringContaining('Migrated legacy ID: "opus" -> "claude-opus"') ); });
118-121: Same hardcoding issue here — preferCLAUDE_MODEL_MAP.opus.Consistent with the suggestion above; using the constant makes this test resilient to future model version bumps.
Proposed diff
it('should prioritize explicit model over session and default', () => { const result = getEffectiveModel('opus', 'haiku', 'gpt-5.2'); - expect(result).toBe('claude-opus-4-6'); + expect(result).toBe(CLAUDE_MODEL_MAP.opus); });apps/ui/src/components/views/board-view/dialogs/auto-mode-settings-popover.tsx (1)
65-67: Consider updating the helper text to mention the "pause" behavior.The description says "Higher values process more features in parallel" but doesn't mention that setting to 0 pauses auto-mode. A brief note like "Set to 0 to pause." would improve discoverability.
📝 Suggested text update
<p className="text-[10px] text-muted-foreground"> - Higher values process more features in parallel but use more API resources. + Higher values process more features in parallel but use more API resources. Set to 0 to pause. </p>libs/dependency-resolver/src/resolver.ts (1)
232-241:getBlockingDependenciesdoesn't account forrequireMerged.
areDependenciesSatisfiednow considersmergedToMainwhenrequireMergedis set, butgetBlockingDependencies(andgetBlockingDependenciesFromMapat Line 266) still only check status. If these are used to display why a feature is blocked, they may miss the "not yet merged" reason. Consider whether these helper functions should accept the same options for consistency.libs/types/src/pipeline.ts (1)
22-33: Clarify the distinction between'done'and'completed'terminal statuses or remove one.Both
'done'and'completed'exist in the union type, but usage across the codebase is heavily skewed:'completed'appears in 18+ locations (dependency resolver, auto-mode-service, UI components) consistently paired with'verified'for terminal status checks, while'done'appears only once in the drag-drop handler. The baseFeatureStatustype includes both, but the inconsistent adoption suggests'done'may be incompletely integrated. Either clarify the intended distinction between these terminal statuses or consolidate to a single canonical value to prevent confusion and ensure consistent status handling.apps/ui/src/components/views/board-view/components/list-view/status-badge.tsx (1)
240-259: Using4.5forfailedordering works but is unconventional.Fractional ordering values like
4.5are a code smell that suggests the ordering scheme may need to be re-spaced as more statuses are added. Consider using integer spacing (e.g., multiples of 10) to leave room for insertions, or just reindex to sequential integers now that the full set is known.libs/model-resolver/src/escalation.ts (1)
14-18: Consider derivingESCALATION_CHAINfromCLAUDE_MODEL_MAPfor consistency.The
ESCALATION_CHAINcurrently hardcodes model strings that are identical to entries inCLAUDE_MODEL_MAP. While this works correctly today becauseresolveModelStringreturns the exact same strings, deriving the chain directly from the model map would eliminate maintenance risk ifCLAUDE_MODEL_MAPis updated in the future. Additionally, no tests currently validate the escalation logic.apps/ui/src/components/views/board-view/constants.ts (1)
62-71:blockedandfailedboth use the'ban'icon.Consider using a distinct icon for
failed(e.g.,'shield'or a new icon) to visually differentiate it fromblockedon the board.apps/ui/src/components/views/board-view/hooks/use-board-drag-drop.ts (2)
200-215:persistFeatureUpdatepromise is fire-and-forget insidedoMove.
doMoveis synchronous but callspersistFeatureUpdate(which returns aPromise) without awaiting it. If the persist fails, the error is silently lost — the UI shows the feature moved (optimistic update viamoveFeature) but the server state diverges. Consider makingdoMoveasync and awaiting the persist, or at minimum adding a.catch()handler to surface the error via toast.Suggested approach
- const doMove = ( + const doMove = async ( status: typeof targetStatus, label: string, variant: 'info' | 'success' = 'info' ) => { moveFeature(featureId, status); - persistFeatureUpdate(featureId, { status, justFinishedAt: undefined }); + try { + await persistFeatureUpdate(featureId, { status, justFinishedAt: undefined }); + } catch (err) { + logger.error('Failed to persist feature update', err); + toast.error('Failed to save status change'); + } if (variant === 'success') {Then
await doMove(...)at each call site (the containing callback is alreadyasync).
278-282: Fallback path doesn't show a toast, unlike alldoMovepaths.The generic fallback directly calls
moveFeature+persistFeatureUpdatewithout a toast notification. This creates an inconsistent UX for any status transition that hits this branch. Consider usingdoMovehere as well.Suggested fix
} else { - // Generic fallback for any other status combination - moveFeature(featureId, targetStatus); - persistFeatureUpdate(featureId, { status: targetStatus }); + // Generic fallback for any other status combination + doMove(targetStatus, targetStatus.replace(/_/g, ' ')); }apps/server/src/services/auto-mode-service.ts (3)
3240-3245:startedAtis cleared when transitioning away fromin_progress.Setting
feature.startedAt = undefinedfor every non-in_progressstatus means that once a feature moves towaiting_approvalorverified, its original start timestamp is lost. If the UI or analytics needs to show how long a feature took from start to completion, this data is no longer available.Consider preserving
startedAtand only clearing it when moving back to an earlier state (e.g.,backlog), or storing it under a different field likecompletedAtfor the duration calculation.
1680-1684:'NO_ISSUES_FOUND'marker detection is fragile.The check
reviewResponse.includes('NO_ISSUES_FOUND')at line 1682 relies on the model outputting this exact string. If the model wraps it in markdown (e.g.,`NO_ISSUES_FOUND`), includes it within a longer word, or rephrases it, the detection fails — leading to an unnecessary fix pass. Consider using a regex with word boundaries or prompting the model to output it on its own line.
1583-1762: Self-review and settings are fetched repeatedly — consider caching within a single execution.
this.settingsService?.getGlobalSettings()is called multiple times per feature execution flow: once inexecuteFeature(line 1324), once perfollowUpFeature(line 2412), once perresumeFromPipelineStep(line 2100), and also insidehandleRetryOrFail(line 3305). Each call hits the settings store. A single settings load at the start of the execution flow passed through as a parameter would reduce redundant I/O.Also applies to: 2411-2422, 2099-2110, 3323-3334
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
533-571: Consider extracting the Auto‑Mode confirmation dialog to a shared render helper.
The dialog JSX is duplicated in mobile/desktop branches; a small helper or component would reduce drift risk.Also applies to: 782-820
| router.get('/resolved-models', async (_req: Request, res: Response) => { | ||
| const settings = await settingsService.getGlobalSettings(); | ||
|
|
||
| const defaultFeatureModelKey = settings.defaultFeatureModel?.model; | ||
|
|
||
| const phaseModels = settings.phaseModels || ({} as any); | ||
|
|
||
| const specGeneration = resolvePhaseModel( | ||
| phaseModels.specGenerationModel, | ||
| DEFAULT_MODELS.claude | ||
| ); | ||
| const backlogPlanning = resolvePhaseModel( | ||
| phaseModels.backlogPlanningModel, | ||
| DEFAULT_MODELS.claude | ||
| ); | ||
| const validation = resolvePhaseModel(phaseModels.validationModel, DEFAULT_MODELS.claude); | ||
|
|
||
| // Also show what the legacy "validationModel" / "enhancementModel" shortcuts are set to (if present) | ||
| const legacyValidationModelKey = (settings as any).validationModel; | ||
| const legacyEnhancementModelKey = (settings as any).enhancementModel; | ||
|
|
||
| const result = { | ||
| now: new Date().toISOString(), | ||
| defaults: { | ||
| DEFAULT_MODELS, | ||
| }, | ||
| configured: { | ||
| defaultFeatureModelKey, | ||
| phaseModels: { | ||
| specGenerationModel: phaseModels.specGenerationModel, | ||
| backlogPlanningModel: phaseModels.backlogPlanningModel, | ||
| validationModel: phaseModels.validationModel, | ||
| }, | ||
| legacy: { | ||
| validationModelKey: legacyValidationModelKey, | ||
| enhancementModelKey: legacyEnhancementModelKey, | ||
| }, | ||
| }, | ||
| resolved: { | ||
| defaultFeatureModel: { | ||
| key: defaultFeatureModelKey, | ||
| resolved: resolveModelString(defaultFeatureModelKey, DEFAULT_MODELS.claude), | ||
| }, | ||
| phaseModels: { | ||
| specGenerationModel: specGeneration, | ||
| backlogPlanningModel: backlogPlanning, | ||
| validationModel: validation, | ||
| }, | ||
| legacy: { | ||
| validationModel: { | ||
| key: legacyValidationModelKey, | ||
| resolved: resolveModelString(legacyValidationModelKey, DEFAULT_MODELS.claude), | ||
| }, | ||
| enhancementModel: { | ||
| key: legacyEnhancementModelKey, | ||
| resolved: resolveModelString(legacyEnhancementModelKey, DEFAULT_MODELS.claude), | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| res.json(result); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Express 4 async route handlers error handling promise rejection
💡 Result:
In Express 4, an async route handler returns a Promise, but Express 4 does not observe/await the returned promise, so a thrown error or rejected promise won’t automatically reach your error middleware (often showing up as an unhandled rejection instead). [1]
Option A (Express 4): wrap and forward to next(err)
const asyncHandler = (fn) => (req, res, next) =>
Promise.resolve(fn(req, res, next)).catch(next);
app.get('/users/:id', asyncHandler(async (req, res) => {
const user = await findUser(req.params.id); // if rejects -> caught -> next(err)
res.json(user);
}));
app.use((err, req, res, next) => {
res.status(500).json({ error: err.message });
});This pattern is widely used to ensure rejections get forwarded to Express’s error handler. [2]
Option B (Express 4): use express-async-errors
require('express-async-errors'); // once, before routesThis patches Express so errors thrown in async handlers are routed to your error middleware. [3]
Option C: upgrade to Express 5
Express 5 handles rejected promises from middleware/handlers by forwarding them to error-handling middleware (effectively like next(err)). [4]
Sources:
[1] Stack Overflow explanation of why Express 4 doesn’t catch async rejections: it doesn’t use the returned promise https://stackoverflow.com/questions/49417580/express-middleware-cannot-trap-errors-thrown-by-async-await-but-why
[2] Example wrapper pattern (Promise.resolve(...).catch(next)) https://betterstack.com/community/guides/scaling-nodejs/error-handling-express/
[3] express-async-errors usage/docs https://www.npmjs.com/package/express-async-errors
[4] Express 5 migration guide: rejected promises handled from middleware/handlers https://expressjs.com/ko/guide/migrating-5.html
🏁 Script executed:
cat -n apps/server/src/routes/debug/index.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 3649
🏁 Script executed:
rg "express-async-errors" --type ts --type tsx --type jsRepository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
rg "asyncHandler|async.*catch|Promise\.resolve.*catch" apps/server/src --type ts --type tsx -A 2Repository: AutoMaker-Org/automaker
Length of output: 94
🏁 Script executed:
find apps/server/src/routes -name "*.ts" -o -name "*.tsx" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -30 {}'Repository: AutoMaker-Org/automaker
Length of output: 4580
Add error handling to async route handler.
Express 4 does not automatically catch rejected promises from async route handlers—if settingsService.getGlobalSettings() throws, the unhandled rejection will bypass error middleware. Wrap the handler body in try/catch to match the error handling pattern used throughout the codebase (see apps/server/src/routes/agent/routes/*.ts).
🛡️ Proposed fix
router.get('/resolved-models', async (_req: Request, res: Response) => {
+ try {
const settings = await settingsService.getGlobalSettings();
// ... existing body ...
res.json(result);
+ } catch (error) {
+ res.status(500).json({ error: 'Failed to resolve models' });
+ }
});🤖 Prompt for AI Agents
In `@apps/server/src/routes/debug/index.ts` around lines 23 - 85, The async route
handler registered with router.get('/resolved-models') must catch errors from
await settingsService.getGlobalSettings() (and other async calls); update the
handler signature to include next: NextFunction, wrap the existing body in
try/catch, and on error call next(err) so Express error middleware handles it.
Ensure you import NextFunction from express if needed and keep all existing
logic (resolvePhaseModel/resolveModelString and res.json(result)) inside the try
block.
|
|
||
| // Statuses that should trigger syncing to app_spec.txt | ||
| const SYNC_TRIGGER_STATUSES: FeatureStatus[] = ['verified', 'completed']; | ||
| const SYNC_TRIGGER_STATUSES: FeatureStatus[] = ['verified', 'done', 'completed']; |
There was a problem hiding this comment.
Stale inline comment on Line 74.
The comment on Line 74 says "verified or completed" but SYNC_TRIGGER_STATUSES now also includes 'done'. Consider updating it to match.
📝 Suggested fix
- // Trigger sync to app_spec.txt when status changes to verified or completed
+ // Trigger sync to app_spec.txt when status changes to verified, done, or completed🤖 Prompt for AI Agents
In `@apps/server/src/routes/features/routes/update.ts` at line 14, Update the
stale inline comment that references "verified or completed" to reflect the
current SYNC_TRIGGER_STATUSES array which includes 'verified', 'done', and
'completed'; locate the comment near the SYNC_TRIGGER_STATUSES constant (symbol:
SYNC_TRIGGER_STATUSES) and change the wording to mention "verified, done, or
completed" (or an equivalent phrase) so the comment matches the actual values.
| private async createWorktreeForBranch( | ||
| projectPath: string, | ||
| branchName: string | ||
| ): Promise<string | null> { | ||
| try { | ||
| const sanitizedName = branchName.replace(/[^a-zA-Z0-9_-]/g, '-'); | ||
| const worktreesDir = path.join(projectPath, '.worktrees'); | ||
| const worktreePath = path.join(worktreesDir, sanitizedName); | ||
|
|
||
| // Create worktrees directory if it doesn't exist | ||
| await secureFs.mkdir(worktreesDir, { recursive: true }); | ||
|
|
||
| // Check if branch already exists in git | ||
| let branchExists = false; | ||
| try { | ||
| await execAsync(`git rev-parse --verify "${branchName}"`, { cwd: projectPath }); | ||
| branchExists = true; | ||
| } catch { | ||
| // Branch doesn't exist yet | ||
| } | ||
|
|
||
| // Create the worktree | ||
| if (branchExists) { | ||
| await execAsync(`git worktree add "${worktreePath}" "${branchName}"`, { | ||
| cwd: projectPath, | ||
| }); | ||
| } else { | ||
| await execAsync(`git worktree add -b "${branchName}" "${worktreePath}" HEAD`, { | ||
| cwd: projectPath, | ||
| }); | ||
| } | ||
|
|
||
| return path.resolve(worktreePath); | ||
| } catch (error) { | ||
| logger.error(`Failed to create worktree for branch "${branchName}":`, error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shell injection risk — branchName is interpolated into shell commands inside double quotes.
branchName originates from user-supplied feature data and is passed unsanitized into execAsync template literals (lines 3177, 3185, 3189). Double-quoting in shell does not prevent expansion of $(...), backticks, or " within the string. A crafted branch name like `malicious-command` or $(rm -rf /) would be executed.
The sanitizedName on line 3167 is only used for the filesystem path, not for the git commands.
Consider using execFile (which bypasses shell interpretation) or sanitizing branchName for the git commands as well.
Suggested approach using execFile
+import { execFile } from 'child_process';
+const execFileAsync = promisify(execFile);
private async createWorktreeForBranch(
projectPath: string,
branchName: string
): Promise<string | null> {
try {
const sanitizedName = branchName.replace(/[^a-zA-Z0-9_-]/g, '-');
+ // Also sanitize branchName for git commands (reject dangerous characters)
+ if (/[`$"\\]/.test(branchName)) {
+ logger.error(`Rejecting suspicious branch name: "${branchName}"`);
+ return null;
+ }
const worktreesDir = path.join(projectPath, '.worktrees');
const worktreePath = path.join(worktreesDir, sanitizedName);
// ...
- await execAsync(`git rev-parse --verify "${branchName}"`, { cwd: projectPath });
+ await execFileAsync('git', ['rev-parse', '--verify', branchName], { cwd: projectPath });
// ...
- await execAsync(`git worktree add "${worktreePath}" "${branchName}"`, { cwd: projectPath });
+ await execFileAsync('git', ['worktree', 'add', worktreePath, branchName], { cwd: projectPath });
// ...
- await execAsync(`git worktree add -b "${branchName}" "${worktreePath}" HEAD`, { cwd: projectPath });
+ await execFileAsync('git', ['worktree', 'add', '-b', branchName, worktreePath, 'HEAD'], { cwd: projectPath });🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 3162 - 3199,
createWorktreeForBranch currently interpolates user-controlled branchName into
shell commands via execAsync which allows shell injection; fix by avoiding shell
interpretation when running git: stop using string-interpolated execAsync calls
for rev-parse and worktree add and instead call a child-process API that accepts
an argv array (e.g., execFile/spawn with args) or a safe execAsync wrapper that
passes ["git", "rev-parse", "--verify", branchName] and ["git","worktree","add",
...] (use sanitizedName only for filesystem paths, keep branchName validated
against a strict pattern or reject unsafe characters before passing as an arg),
and update the code paths inside createWorktreeForBranch so all git invocations
use argument arrays rather than shell strings.
| // Remove from running features before re-executing to avoid "already running" error | ||
| this.runningFeatures.delete(featureId); | ||
|
|
||
| // Re-execute with error context (fire-and-forget, executeFeature handles its own errors) | ||
| this.executeFeature(projectPath, featureId, useWorktrees, isAutoMode, undefined, { | ||
| continuationPrompt: retryPrompt, | ||
| }); | ||
|
|
||
| return 'retrying'; |
There was a problem hiding this comment.
Missing .catch() on fire-and-forget executeFeature call during retry.
Line 3361 calls this.executeFeature(...) without .catch(), creating an unhandled promise rejection risk if the call fails. Other fire-and-forget executeFeature calls in this file (e.g., line 711–718) correctly attach .catch().
Suggested fix
// Remove from running features before re-executing to avoid "already running" error
this.runningFeatures.delete(featureId);
// Re-execute with error context (fire-and-forget, executeFeature handles its own errors)
- this.executeFeature(projectPath, featureId, useWorktrees, isAutoMode, undefined, {
+ this.executeFeature(projectPath, featureId, useWorktrees, isAutoMode, undefined, {
continuationPrompt: retryPrompt,
- });
+ }).catch((retryExecError) => {
+ logger.error(`Retry execution failed for feature ${featureId}:`, retryExecError);
+ });🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 3357 - 3365, The
fire-and-forget call to this.executeFeature(projectPath, featureId,
useWorktrees, isAutoMode, undefined, { continuationPrompt: retryPrompt }) lacks
a .catch(), risking unhandled promise rejections; append a .catch handler that
logs the error (e.g. this.logger.error or processLogger.error) and any relevant
context (featureId, projectPath) so failures during retry are observed,
mirroring the pattern used for other fire-and-forget executeFeature calls in
this file.
| // Only auto-merge on verified status | ||
| if (status !== 'verified') return; | ||
|
|
||
| // Nothing to merge if no branch or on main/master | ||
| if (!branchName || branchName === 'main' || branchName === 'master') return; | ||
|
|
||
| // Check if auto-merge is enabled | ||
| const settings = await this.settingsService?.getGlobalSettings(); | ||
| if (!settings?.autoMergeOnVerify) return; | ||
|
|
||
| logger.info(`Auto-merging verified feature ${featureId} branch "${branchName}" to main`); | ||
|
|
||
| const result = await mergeWorktreeBranch(projectPath, branchName, 'main', { | ||
| squash: true, | ||
| message: `Merge feature/${featureId} (squash)`, | ||
| }); |
There was a problem hiding this comment.
Hardcoded 'main' as merge target — fails on projects using master or other primary branches.
Line 3503 always merges into 'main', but line 3495 only skips for 'main' and 'master'. If a project's primary branch is 'master' or 'develop', the squash-merge targets a potentially non-existent 'main' branch. Consider using getCurrentBranch(projectPath) (already defined in this file) to resolve the actual primary branch.
Suggested fix
private async autoMergeIfVerified(
projectPath: string,
featureId: string,
status: string,
branchName: string | null,
worktreePath: string | null
): Promise<void> {
if (status !== 'verified') return;
- if (!branchName || branchName === 'main' || branchName === 'master') return;
+ const primaryBranch = await getCurrentBranch(projectPath) || 'main';
+ if (!branchName || branchName === primaryBranch) return;
const settings = await this.settingsService?.getGlobalSettings();
if (!settings?.autoMergeOnVerify) return;
logger.info(`Auto-merging verified feature ${featureId} branch "${branchName}" to main`);
- const result = await mergeWorktreeBranch(projectPath, branchName, 'main', {
+ const result = await mergeWorktreeBranch(projectPath, branchName, primaryBranch, {
squash: true,
message: `Merge feature/${featureId} (squash)`,
});📝 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.
| // Only auto-merge on verified status | |
| if (status !== 'verified') return; | |
| // Nothing to merge if no branch or on main/master | |
| if (!branchName || branchName === 'main' || branchName === 'master') return; | |
| // Check if auto-merge is enabled | |
| const settings = await this.settingsService?.getGlobalSettings(); | |
| if (!settings?.autoMergeOnVerify) return; | |
| logger.info(`Auto-merging verified feature ${featureId} branch "${branchName}" to main`); | |
| const result = await mergeWorktreeBranch(projectPath, branchName, 'main', { | |
| squash: true, | |
| message: `Merge feature/${featureId} (squash)`, | |
| }); | |
| // Only auto-merge on verified status | |
| if (status !== 'verified') return; | |
| // Nothing to merge if no branch or on primary branch | |
| const primaryBranch = await getCurrentBranch(projectPath) || 'main'; | |
| if (!branchName || branchName === primaryBranch) return; | |
| // Check if auto-merge is enabled | |
| const settings = await this.settingsService?.getGlobalSettings(); | |
| if (!settings?.autoMergeOnVerify) return; | |
| logger.info(`Auto-merging verified feature ${featureId} branch "${branchName}" to main`); | |
| const result = await mergeWorktreeBranch(projectPath, branchName, primaryBranch, { | |
| squash: true, | |
| message: `Merge feature/${featureId} (squash)`, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 3491 - 3506,
Auto-merge currently hardcodes 'main' as the merge target in
mergeWorktreeBranch, which breaks repos whose primary branch is 'master' or
another name; update the auto-merge logic to resolve the actual default/primary
branch (use the existing getCurrentBranch(projectPath) or another helper) and
pass that resolved branch as the target instead of the literal 'main'. Ensure
the code still skips merging when branchName is missing or equal to the resolved
primary branch, and keep the squash/message behavior (references:
mergeWorktreeBranch, getCurrentBranch, featureId, branchName,
settingsService.getGlobalSettings).
| const mergeMsg = options?.message || `Merge ${branchName} into ${mergeTo}`; | ||
| const mergeCmd = options?.squash | ||
| ? `git merge --squash ${branchName}` | ||
| : `git merge ${branchName} -m "${mergeMsg}"`; | ||
|
|
||
| let mergeOutput = ''; | ||
| try { | ||
| const result = await execAsync(mergeCmd, { cwd: projectPath }); | ||
| mergeOutput = `${result.stdout || ''} ${result.stderr || ''}`; |
There was a problem hiding this comment.
Command injection via message parameter in shell commands.
mergeMsg (and squashMsg on line 122) originate from options.message which is caller-supplied. Embedding it directly in a shell string allows injection via ", `, or $(…). For example, a message like "; rm -rf / # would break out of the double-quoted string.
Use execFile (or pass args as an array) instead of exec with string interpolation, or at minimum sanitize the message to strip shell metacharacters.
🔒 Proposed fix using execFile for safe argument passing
Replace execAsync (promisified exec) with execFileAsync (promisified execFile) for commands where user-controlled strings are involved:
-import { exec } from 'child_process';
+import { exec, execFile } from 'child_process';
import { promisify } from 'util';
const execAsync = promisify(exec);
+const execFileAsync = promisify(execFile);Then for the merge command:
- const mergeMsg = options?.message || `Merge ${branchName} into ${mergeTo}`;
- const mergeCmd = options?.squash
- ? `git merge --squash ${branchName}`
- : `git merge ${branchName} -m "${mergeMsg}"`;
-
- let mergeOutput = '';
- try {
- const result = await execAsync(mergeCmd, { cwd: projectPath });
+ const mergeMsg = options?.message || `Merge ${branchName} into ${mergeTo}`;
+ const mergeArgs = options?.squash
+ ? ['merge', '--squash', branchName]
+ : ['merge', branchName, '-m', mergeMsg];
+
+ let mergeOutput = '';
+ try {
+ const result = await execFileAsync('git', mergeArgs, { cwd: projectPath });Apply the same pattern for the squash commit on line 122:
- await execAsync(`git commit -m "${squashMsg}"`, { cwd: projectPath });
+ await execFileAsync('git', ['commit', '-m', squashMsg], { cwd: projectPath });📝 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.
| const mergeMsg = options?.message || `Merge ${branchName} into ${mergeTo}`; | |
| const mergeCmd = options?.squash | |
| ? `git merge --squash ${branchName}` | |
| : `git merge ${branchName} -m "${mergeMsg}"`; | |
| let mergeOutput = ''; | |
| try { | |
| const result = await execAsync(mergeCmd, { cwd: projectPath }); | |
| mergeOutput = `${result.stdout || ''} ${result.stderr || ''}`; | |
| import { exec, execFile } from 'child_process'; | |
| import { promisify } from 'util'; | |
| const execAsync = promisify(exec); | |
| const execFileAsync = promisify(execFile); | |
| // ... [earlier code] ... | |
| const mergeMsg = options?.message || `Merge ${branchName} into ${mergeTo}`; | |
| const mergeArgs = options?.squash | |
| ? ['merge', '--squash', branchName] | |
| : ['merge', branchName, '-m', mergeMsg]; | |
| let mergeOutput = ''; | |
| try { | |
| const result = await execFileAsync('git', mergeArgs, { cwd: projectPath }); | |
| mergeOutput = `${result.stdout || ''} ${result.stderr || ''}`; |
| const mergeMsg = options?.message || `Merge ${branchName} into ${mergeTo}`; | |
| const mergeCmd = options?.squash | |
| ? `git merge --squash ${branchName}` | |
| : `git merge ${branchName} -m "${mergeMsg}"`; | |
| let mergeOutput = ''; | |
| try { | |
| const result = await execAsync(mergeCmd, { cwd: projectPath }); | |
| mergeOutput = `${result.stdout || ''} ${result.stderr || ''}`; | |
| await execFileAsync('git', ['commit', '-m', squashMsg], { cwd: projectPath }); |
🤖 Prompt for AI Agents
In `@libs/git-utils/src/merge.ts` around lines 88 - 96, The merge command builds
mergeCmd using a caller-supplied mergeMsg (options.message) and runs it with
execAsync, allowing shell injection; change the implementation to call a safe
execFile variant (e.g., execFileAsync or child_process.execFile with promisify)
and pass the git executable and arguments as an array (e.g., ['merge','-m',
mergeMsg, branchName] or ['merge','--squash', branchName] for squash), and apply
the same change for the squash commit path that uses squashMsg so user input is
never interpolated into a shell string; keep mergeMsg and squashMsg as plain
strings but never concatenate them into a single shell command.
| } catch { | ||
| // Try with prune if remove fails | ||
| try { | ||
| await execAsync('git worktree prune', { cwd: projectPath }); | ||
| worktreeDeleted = true; | ||
| } catch { | ||
| logger.warn(`Failed to remove worktree: ${worktreePath}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
git worktree prune doesn't guarantee the specific worktree was removed.
Setting worktreeDeleted = true after prune is misleading — prune only removes stale worktree entries, not necessarily the target one. If the initial remove --force failed because the worktree is still in use or locked, prune won't help and the result will incorrectly report success.
Proposed fix
} catch {
// Try with prune if remove fails
try {
await execAsync('git worktree prune', { cwd: projectPath });
- worktreeDeleted = true;
+ // prune only cleans stale entries; we can't confirm the target was removed
+ logger.warn(`Worktree remove failed; ran prune as fallback: ${worktreePath}`);
} catch {
logger.warn(`Failed to remove worktree: ${worktreePath}`);
}
}📝 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.
| } catch { | |
| // Try with prune if remove fails | |
| try { | |
| await execAsync('git worktree prune', { cwd: projectPath }); | |
| worktreeDeleted = true; | |
| } catch { | |
| logger.warn(`Failed to remove worktree: ${worktreePath}`); | |
| } | |
| } | |
| } catch { | |
| // Try with prune if remove fails | |
| try { | |
| await execAsync('git worktree prune', { cwd: projectPath }); | |
| // prune only cleans stale entries; we can't confirm the target was removed | |
| logger.warn(`Worktree remove failed; ran prune as fallback: ${worktreePath}`); | |
| } catch { | |
| logger.warn(`Failed to remove worktree: ${worktreePath}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@libs/git-utils/src/merge.ts` around lines 165 - 173, The code incorrectly
sets worktreeDeleted = true after running 'git worktree prune' even though prune
may not remove the specific worktree; modify the catch block around the
execAsync('git worktree prune', ...) so that you do not assume success — instead
verify removal explicitly (for example by checking fs.existsSync(worktreePath)
or running execAsync('git worktree list --porcelain') and confirming the
worktreePath is absent) and only set worktreeDeleted = true when that
verification confirms removal; update the logger messages in this block to
reflect the verification result and keep the original error path if the worktree
still exists (using the same symbols worktreeDeleted, worktreePath, execAsync,
logger).
| @echo off | ||
| setlocal EnableExtensions EnableDelayedExpansion |
There was a problem hiding this comment.
Unix line endings will break this batch script on Windows.
The static analysis tool detected LF-only line endings. The Windows batch parser can fail at GOTO/CALL label boundaries (e.g., :killPort at line 32) when the file uses Unix-style \n instead of \r\n. This can cause the script to silently malfunction.
Ensure the file uses CRLF line endings. You can add a .gitattributes entry to enforce this:
restart-automaker.cmd text eol=crlf
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 39 LF sequences found
(E018)
🤖 Prompt for AI Agents
In `@restart-automaker.cmd` around lines 1 - 2, The batch script
restart-automaker.cmd currently has Unix (LF) line endings which can break
Windows batch parsing (e.g., GOTO/CALL labels like :killPort); convert this file
to CRLF line endings and ensure future commits enforce CRLF by adding a
.gitattributes entry such as "restart-automaker.cmd text eol=crlf" (or update a
global .gitattributes) and re-save the file with Windows/CRLF line endings
before committing so labels and batch control flow (e.g., :killPort) work
correctly on Windows.
|
|
||
| REM restart-automaker.cmd | ||
| REM Kills anything listening on ports 3007 (UI) and 3008 (backend), then starts both. | ||
| REM Run from: C:\Users\Robo1\.openclaw\workspace\automaker |
There was a problem hiding this comment.
Hardcoded developer-specific path in the comment.
The comment references C:\Users\Robo1\.openclaw\workspace\automaker, which is a specific developer's machine path. Either generalize it (e.g., REM Run from the automaker repository root) or remove it.
🤖 Prompt for AI Agents
In `@restart-automaker.cmd` at line 6, The comment contains a hardcoded developer
path ("C:\Users\Robo1\.openclaw\workspace\automaker") in the REM line; replace
it with a non-personalized instruction or remove it—e.g., change the REM line to
"REM Run from the automaker repository root" or simply delete the REM line—so
update the REM comment in restart-automaker.cmd accordingly.
| @echo off | ||
| setlocal | ||
|
|
||
| echo === Automaker status (%DATE% %TIME%) === | ||
| echo. | ||
|
|
||
| echo Port 3007 (UI): | ||
| netstat -ano | findstr LISTENING | findstr ":3007" || echo (not listening) | ||
|
|
||
| echo. | ||
| echo Port 3008 (Backend): | ||
| netstat -ano | findstr LISTENING | findstr ":3008" || echo (not listening) | ||
|
|
||
| echo. | ||
| echo Try open: | ||
| echo http://localhost:3007/ | ||
| echo http://localhost:3008/api/health | ||
| echo. |
There was a problem hiding this comment.
Convert to Windows (CRLF) line endings — batch scripts can malfunction with LF-only.
Windows cmd.exe parser can misparse labels, GOTO, and CALL targets when .cmd/.bat files use Unix (LF) line endings instead of CRLF. The static analyzer detected 18 LF-only sequences. Configure .gitattributes to enforce CRLF for batch files:
*.cmd text eol=crlf
*.bat text eol=crlf🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 18 LF sequences found
(E018)
🤖 Prompt for AI Agents
In `@status-automaker.cmd` around lines 1 - 18, The status-automaker.cmd file uses
LF-only line endings which can break Windows batch parsing; convert
status-automaker.cmd to CRLF line endings (use your editor or a line-ending
conversion tool) and update the repository text attributes to enforce CRLF for
batch files by adding entries for *.cmd and *.bat with text eol=crlf to
.gitattributes, then normalize and recommit the file so the repository stores
and checks out the file with CRLF endings.
Track token consumption and execution time from Claude Agent SDK result messages across all agent call sites (implementation, pipeline steps, self-review, follow-ups). Usage data is persisted to feature.json and displayed on kanban cards and in the summary dialog breakdown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx (1)
1-1:⚠️ Potential issue | 🟠 Major
@ts-nocheckdisables all type safety — remove or scope narrowly.This suppresses every TypeScript error in the file, hiding potential bugs in the newly added token usage code. If there's a specific type issue, use targeted
@ts-expect-errorcomments instead.apps/server/src/services/auto-mode-service.ts (3)
2150-2184:⚠️ Potential issue | 🟠 MajorToken usage from pipeline resume and self-review is silently discarded.
In
resumeFromPipelineStep,executePipelineStepsreturnsTokenUsageandexecuteSelfReviewreturnsUsageEntry, but neither return value is captured or persisted. Compare with the mainexecuteFeaturepath (lines 1300–1437) andfollowUpFeature(lines 2454–2501), both of which accumulate and persist usage.This means features resumed from a pipeline step will show incomplete or zero token usage on the kanban card.
Proposed fix — capture and persist token usage
// Execute remaining pipeline steps (starting from crashed step) - await this.executePipelineSteps( + const pipelineUsage = await this.executePipelineSteps( projectPath, featureId, feature, stepsToExecute, workDir, abortController, autoLoadClaudeMd ); + let resumeUsage: TokenUsage | undefined; + if (pipelineUsage) { + for (const entry of pipelineUsage.entries) { + resumeUsage = this.accumulateUsage(resumeUsage, entry); + } + } // Self-review pass after pipeline resume const globalSettingsResume = await this.settingsService?.getGlobalSettings(); if (globalSettingsResume?.enableSelfReview !== false) { - await this.executeSelfReview( + const reviewEntry = await this.executeSelfReview( projectPath, featureId, feature, workDir, abortController, autoLoadClaudeMd ); + resumeUsage = this.accumulateUsage(resumeUsage, reviewEntry); } // Determine final status const finalStatus = feature.skipTests ? 'waiting_approval' : 'verified'; await this.updateFeatureStatus(projectPath, featureId, finalStatus); // Auto-merge verified feature branch back to main if enabled await this.autoMergeIfVerified( projectPath, featureId, finalStatus, branchName ?? null, worktreePath ); console.log('[AutoMode] Pipeline resume completed successfully'); + // Persist accumulated token usage (merge with any existing usage) + if (resumeUsage) { + try { + const currentFeature = await this.loadFeature(projectPath, featureId); + if (currentFeature?.tokenUsage) { + resumeUsage = { + inputTokens: currentFeature.tokenUsage.inputTokens + resumeUsage.inputTokens, + outputTokens: currentFeature.tokenUsage.outputTokens + resumeUsage.outputTokens, + cacheReadTokens: currentFeature.tokenUsage.cacheReadTokens + resumeUsage.cacheReadTokens, + cacheWriteTokens: currentFeature.tokenUsage.cacheWriteTokens + resumeUsage.cacheWriteTokens, + durationMs: currentFeature.tokenUsage.durationMs + resumeUsage.durationMs, + numTurns: currentFeature.tokenUsage.numTurns + resumeUsage.numTurns, + entries: [...currentFeature.tokenUsage.entries, ...resumeUsage.entries], + }; + } + await this.featureLoader.update(projectPath, featureId, { tokenUsage: resumeUsage }); + } catch (usageError) { + logger.warn(`Failed to persist token usage for pipeline resume ${featureId}:`, usageError); + } + } + this.emitAutoModeEvent('auto_mode_feature_complete', { featureId, featureName: feature.title,
1169-1210:⚠️ Potential issue | 🟠 MajorAuto-generated branch/worktree is never persisted when auto-merge is disabled, orphaning the worktree.
When
useWorktrees=trueand the feature has nobranchName, line 1177 generatesfeature/${feature.id}and a worktree is created (line 1190). However, this generatedbranchNameis never written tofeature.json— line 1210 explicitly preserves the originalnull. The only path that persists it is insideautoMergeIfVerified(line 3602), which only runs whenautoMergeOnVerifyis enabled.If auto-merge is disabled, the worktree and branch exist on disk but the feature has no record of them. On the next resume/retry,
findExistingWorktreeForBranchwon't find the worktree (it looks upfeature.branchNamewhich is stillnull), so a new worktree may be created, and the old one is leaked.Consider persisting the generated
branchNametofeature.jsonimmediately after worktree creation, or at minimum before thefinallyblock. The concurrency concern mentioned in the comment (lines 1172–1175) can be addressed by updating only the feature file without changingtempRunningFeature.branchName(which controls the running-count scoping).
2274-2295:⚠️ Potential issue | 🟡 MinorSame worktree orphan risk in
followUpFeature.Line 2278 generates a fallback
branchName(feature/${featureId}) when the feature has none, and lines 2287–2294 auto-create a worktree for it. This branch name is stored in theRunningFeature(line 2363) but never persisted tofeature.jsonunless auto-merge fires. Same orphan concern as inexecuteFeature.
🤖 Fix all issues with AI agents
In `@apps/server/src/services/auto-mode-service.ts`:
- Around line 3320-3325: The current code in auto-mode-service.ts clears
feature.startedAt for any status not equal to 'in_progress', which loses the
timer when transitioning to pipeline steps; update the logic in the
updateFeatureStatus/feature status assignment so that feature.startedAt is set
when status === 'in_progress' or status.startsWith('pipeline_step') (only set it
if not already present), and only clear feature.startedAt for truly terminal
statuses (e.g., 'completed' or 'failed') instead of every non-in_progress
status.
🧹 Nitpick comments (4)
apps/ui/src/components/views/board-view/components/kanban-card/agent-info-panel.tsx (1)
51-58: DuplicateformatTokenCount— extract to a shared utility.This exact function is duplicated in
summary-dialog.tsx(lines 16-20). Extract it to a shared location (e.g.,@/lib/format-utils.ts) and import from both files to stay DRY.apps/ui/src/components/views/board-view/components/kanban-card/summary-dialog.tsx (2)
16-28: DuplicateformatTokenCountand co-locatedformatDurationshould live in a shared utility.
formatTokenCountis identical to the one inagent-info-panel.tsx. Both helpers are good candidates for a shared@/lib/format-utils.tsmodule, keeping these components focused on rendering.
74-103: Token usage section looks good overall.One thing to note: the Total row (lines 92-93) reads from
feature.tokenUsage.inputTokens + outputTokens(pre-aggregated on the backend) while individual rows sum per-entry values. If these ever drift out of sync, the total won't match the visible entries. Consider deriving the total from entries client-side for consistency, or document that the backend is the source of truth.Derive totals from entries
- {formatTokenCount( - feature.tokenUsage.inputTokens + feature.tokenUsage.outputTokens - )}{' '} - tokens + {formatTokenCount( + feature.tokenUsage.entries.reduce((sum, e) => sum + e.inputTokens + e.outputTokens, 0) + )}{' '} + tokens- <span>{formatDuration(feature.tokenUsage.durationMs)}</span> + <span>{formatDuration(feature.tokenUsage.entries.reduce((sum, e) => sum + e.durationMs, 0))}</span>apps/server/src/services/auto-mode-service.ts (1)
1640-1823: Self-review implementation looks solid overall; one gap in issue counting.The flow (get diff → lightweight review → optional fix pass → append results) is well-structured with proper error handling and event emission at each stage.
Minor note: line 1814 always emits
issuesFound: noIssues ? 0 : 1, regardless of how many issues the review actually found. If the UI or analytics use this count, it will undercount.
| // Set startedAt timestamp when moving to in_progress (for UI timer) | ||
| if (status === 'in_progress') { | ||
| feature.startedAt = new Date().toISOString(); | ||
| } else { | ||
| feature.startedAt = undefined; | ||
| } |
There was a problem hiding this comment.
startedAt is cleared when transitioning to pipeline steps.
Setting startedAt = undefined for any status other than in_progress means that when a feature moves to pipeline_step_X (also set via updateFeatureStatus), the timer reference is lost. If the UI uses startedAt to show elapsed time during the entire execution (not just the in_progress phase), this will cause the timer to reset or disappear during pipeline steps.
Consider preserving startedAt for pipeline statuses as well:
if (status === 'in_progress') {
feature.startedAt = new Date().toISOString();
- } else {
+ } else if (!status.startsWith('pipeline_')) {
feature.startedAt = undefined;
}🤖 Prompt for AI Agents
In `@apps/server/src/services/auto-mode-service.ts` around lines 3320 - 3325, The
current code in auto-mode-service.ts clears feature.startedAt for any status not
equal to 'in_progress', which loses the timer when transitioning to pipeline
steps; update the logic in the updateFeatureStatus/feature status assignment so
that feature.startedAt is set when status === 'in_progress' or
status.startsWith('pipeline_step') (only set it if not already present), and
only clear feature.startedAt for truly terminal statuses (e.g., 'completed' or
'failed') instead of every non-in_progress status.
1 participant
Summary
simpleQuery(), then runsrunAgent()to fix any problems found. Controlled byenableSelfReviewsetting (default: true).autoMergeOnVerifyis enabled, with UI toggle in settings.Test plan
npm run build:packages— all packages compilenpm run build:server— server compilesnpm run test:packages— 490/493 pass (3 pre-existing symlink failures on Windows)npm run test:server— 1240/1248 pass (8 pre-existing failures: symlink, timing, hostname)enableSelfReview: true, verify review step runs and results appear in agent-output.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Updates