fix(auto-mode): worktree defaults + local review + workflow default; ops cleanup#3802
Conversation
…rkflow default Stabilization fixes validated live this session: - auto-mode-service: useWorktrees defaults true in executeFeature/resumeFeature and the loop always dispatches with worktrees (was gated on executionMode != read-only). A stale 'read-only' executionMode previously let a scaffold agent run npm install in the main checkout and corrupt node_modules. Also adds 'config_corrupted' to the immediate-pause circuit breaker. - worktree-lifecycle-service: drop node_modules from BUILD_ARTIFACT_DIRS — each worktree runs its own npm ci and can no longer clobber the main install. - execution-service: run a local fresh-eyes (antagonistic) review of the staged worktree diff before the git-workflow/PR step; on BLOCK/CONCERN run one fix iteration. Fail-open, skips read-only. - project-orchestration-service: default phase workflow to 'standard' (was falling through fuzzy match-scoring into changelog-digest -> no PR). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- plugin: repo URL automaker->protoMaker; CodeRabbit->Quinn in ava/setuplab;
drop hardcoded /home/josh path from devops-health-check; README tool count;
handle-mcp-failure.sh fallback uses ${AUTOMAKER_ROOT:-$PWD} not a hardcoded path.
- scripts/launch-protomaker.sh: non-interactive prod build+serve for the
macOS LaunchAgent (frees stale ports, sources .env, NODE_ENV=production).
- tools/board_monitor.py: monitoring tweaks.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
|
Warning Review limit reached
More reviews will be available in 10 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR shifts protoMaker's execution defaults to worktrees-by-default with safety improvements, integrates Quinn for PR review across agent commands, and adds production deployment automation. Core service execution parameters flip to use worktrees by default, new Fresh Eyes review loop validates changes pre-git, and MCP agent specs switch from CodeRabbit to Quinn for review gating. ChangesWorktree-first execution, Fresh Eyes review gate, Quinn integration, and production deployment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![protoquinn[bot]](https://avatars.githubusercontent.com/in/3282126?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Audit — PR #3802 | fix(auto-mode): worktree defaults + local review + workflow default; ops cleanup
VERDICT: PASS
CI Status
- test: queued
- build: queued
- checks: queued
- Close external code contributions: skipped
Diff Review
Fourtargeted fixes across five files:
- auto-mode-service.ts (
executeFeature/resumeFeature):useWorktreesdefault flippedfalse → true; dispatch call site now always passestrueinstead of gating onread-only. This renders the stale-read-only→corrupt-main-node_modulesdefect impossible by design — pre-existing root cause addressed. - worktree-lifecycle-service.ts:
node_modulesremoved fromBUILD_ARTIFACT_DIRS. Each worktree now runs its ownnpm cirather than clobbering the host. Clear comment inlined explaining the deliberate non-symlink. - execution-service.ts: Introduces
reviewAndIterateWorktreeDiff— a local antagonistic diff review run after the agent finishes but before the git workflow opens the PR. Staged diff is extracted, sent to fresh-eyes, then one fix pass is triggered on BLOCK/CONCERN. Fail-open on read-only and any error. This implements a read-your-own-work gate without blocking shipping. - project-orchestration-service.ts: Unset
workflownow defaults to'standard'instead of dropping through fuzzy-match scoring intochangelog-digest. Fixes the silent mis-route that produced no-PR runs. - Ops cleanup (plugin config, scripts, monitor): path hygiene, naming, env-var usage — targeted, no scope creep.
Observations
- LOW: CI checks still queued (3/4 pending). Approval issued on code quality alone; merge gate follows CI resolution.
- LOW: clawpatch unavailable for this repo — structural analysis limited to diff-level review.
- LOW:
reviewAndIterateWorktreeDiffinvokessimpleQueryfollowed by anotherrunAgentpass — the two-model-call sequence is safe given the abortController propagation through the fix iteration, but no explicit test covers this code path. Recommend a coverage audit in a follow-up. - MEDIUM (process): The PR title references validation from an "uncommitted working tree" — the commit now introduces the same pattern from a formal branch, which is correct. No self-review concern on
protoquinn[bot]authorship.
The behavioral fixes (useWorktrees isolation-by-default, node_modules non-symlink, standard workflow default) collectively address three confirmed defects. The local review addition is additive with fail-open semantics. No security, data-loss, or correctness concerns.
— Quinn, QA Engineer
|
Submitted COMMENT review on #3802. |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
QA Audit — PR #3802 | fix(auto-mode): worktree defaults + local review + workflow default; ops cleanup
VERDICT: WARN
CI Status
- build: QUEUED
- checks: QUEUED
- test: IN_PROGRESS
Diff Review
Two commits targeting four areas:
-
auto-mode-service.ts —
useWorktreesdefaultstruein bothexecuteFeature(L1319) andresumeFeature(L1703). Removes theexecutionMode !== 'read-only'guard that previously let scaffold agents runnpm installin the main checkout. Change is minimal and correct. -
execution-service.ts — Adds
reviewAndIterateWorktreeDiff()(L2393–2480): stages the worktree diff, runs a fresh-eyes adversarial review viasimpleQuery, and on BLOCK/CONCERN triggers one fix iteration before the git workflow runs. Fail-open: wrapped in try/catch, skips onread-onlyexecutionMode, logs warnings rather than throwing. The prompt construction and verdict parsing look sound. -
project-orchestration-service.ts —
workflowdefault changed fromoptions.defaultWorkflow(could be undefined) tooptions.defaultWorkflow ?? 'standard'(L237). Prevents phases falling through to mis-scored read-only workflows likechangelog-digest. -
worktree-lifecycle-service.ts — Drops
node_modulesfromBUILD_ARTIFACT_DIRS. Adds a clear NOTE explaining why isolation is intentional: a worktree runningnpm installpreviously rewrote the host's deps. Thedist/buildsymlinks are retained.
Observations
- LOW:
reviewAndIterateWorktreeDiffcallsrunAgentwithphase: 'REVIEW'andmaxTurns: 30for the fix iteration. With the fresh-eyes pass already consuming a model call, the fix iteration adds a second full agent run on every feature. This is a meaningful compute cost per feature — worth monitoring in production once merged. - LOW: The
config_corruptedaddition to the circuit breaker (described in PR body but not visible in truncated diff) — confirm it is present inauto-mode-service.tsand maps to the correct pause path. - INFO: No unresolved CodeRabbit threads. Plugin/README/script cleanup in commit 2 is mechanical and low-risk.
Gaps
- Missing test coverage for
reviewAndIterateWorktreeDifffail-open path and verdict branching. config_corruptedcircuit breaker addition not confirmed in diff.
RECOMMENDATION: Re-check CI once test completes. If all green and config_corrupted is confirmed, upgrade to APPROVE. Otherwise address failures and re-notify.
— Quinn, QA Engineer
|
Submitted COMMENT review on #3802. |
Adds a Default column to the workflowSettings table; useWorktrees now true (see this PR's auto-mode-service change). Folds in the doc work from the 'Update docs after: protoMaker CLI' board feature. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
QA Audit — PR #3802 | fix(auto-mode): worktree defaults + local review + workflow default; ops cleanup
VERDICT: FAIL
CI Status
- checks: in_progress
- test: in_progress
- build: in_progress
- deploy: failure ← must clear before merge
Diff Review
Two commits: (1) three critical server behavior fixes — worktree isolation by default in executeFeature/resumeFeature to prevent main-checkout corruption (auto-mode-service.ts:1323,1719), node_modules dropped from symlink dirs (worktree-lifecycle-service.ts:771+), local fresh-eyes review before git workflow (execution-service.ts:1399+), workflow default to 'standard' (project-orchestration-service.ts:234); (2) plugin and ops cleanup. All changes are targeted and well-scoped.
Observations
- CLAWPATCH: unavailable for this repo (not in mounted set). Reviewed from diff alone.
- CRITICAL:
deploycheck is failing. While the PR description says these are stabilization fixes validated live during an autonomous session, the merge gate requires CI green. The failure must be diagnosed and cleared — if it's pre-existing infrastructure, that context needs to be on the record before approval. execution-service.ts:2476:execAsynccalled withcwd: workDirbut notimeoutarg — a long-running diff could hang the review step. Consider adding atimeout: 120_000guard for robustness, even though fail-open catches it.execution-service.ts:2467:git add -Abefore diff review stages everything, including untracked files, which is intentional per the comment. Good.execution-service.ts:2470: ifdiff.trim().length === 0the method returns early — correct, nothing to review.- Good fail-open on both the review loop and fix iteration (caught by try/catch, logged + proceeds).
Checks: 4
Passed: 3
Failed: 1 (deploy)
Gaps: 1
— Quinn, QA Engineer
|
Submitted REQUEST_CHANGES review on #3802. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/services/auto-mode-service.ts`:
- Line 1706: The route wiring is overriding resumeFeature's default by passing
useWorktrees ?? false from the /resume-feature handler; update the caller so it
does not coerce undefined to false — either stop passing the param when omitted
or pass the explicit boolean you intend (e.g., true) so the
resumeFeature(projectPath, featureId, useWorktrees = true) default actually
takes effect; look for the /resume-feature route handler that calls
resumeFeature and remove the "?? false" coalescing (or pass true explicitly) to
restore the new default.
In `@apps/server/src/services/project-orchestration-service.ts`:
- Line 239: The code currently falls back to the hardcoded string 'standard'
when resolving workflow (in the assignment using phase.workflow ??
options.defaultWorkflow ?? 'standard'); remove the literal and instead read the
fallback from configuration or a shared constant (e.g., import DEFAULT_WORKFLOW
from the project settings/config or call Settings.get('defaultWorkflow')), then
use phase.workflow ?? options.defaultWorkflow ?? DEFAULT_WORKFLOW (or the
settings call) so workflow-specific values are sourced from config rather than
hardcoded.
In `@apps/server/src/services/worktree-lifecycle-service.ts`:
- Around line 776-783: Update the stale comment and any agent-facing prompts
that warn "never to install in worktrees" to reflect the new isolation: clarify
node_modules is deliberately NOT symlinked and agents should use the
worktree-init script (npm ci) to install dependencies in the worktree; keep the
note that BUILD_ARTIFACT_DIRS (constant BUILD_ARTIFACT_DIRS) like
'dist','build','.next','.nuxt','out' remain symlinked and safe to read but not
mutated. Locate the top-of-file comment around BUILD_ARTIFACT_DIRS and any
message strings or prompt code that reference "never install" and replace them
with concise guidance to run worktree-init (npm ci) for dependency changes and
to avoid mutating symlinked build artifact dirs.
In `@scripts/launch-protomaker.sh`:
- Around line 20-24: Replace the hardcoded REPO="$HOME/dev/protomaker"
assignment with logic that prefers an environment variable (e.g.,
AUTOMAKER_ROOT) and falls back to a script-relative path; set REPO to
AUTOMAKER_ROOT if present, otherwise compute the repository root relative to the
launcher script location, then retain the existing cd "$REPO" || { echo "...
FATAL ..."; exit 1 } handling. Update references to REPO in the script only,
leaving the error message intact.
- Around line 56-61: The current loop that finds listeners into variable pids
and immediately runs xargs kill -9 should be changed to attempt a graceful
shutdown first: for each PID in pids (found by lsof -ti:"$port" -sTCP:LISTEN),
send SIGTERM (kill -TERM) and wait a short interval, then check if the PID is
still running and only then escalate to SIGKILL (kill -9); optionally inspect
the process command line (e.g., via /proc/$pid/cmdline or ps -o args=) to verify
it’s the expected protomaker-related process before killing. Update the loop
that references pids and the xargs kill -9 invocation to implement
TERM-then-KILL with a short wait and re-check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ee93f91f-da0b-4239-a93f-3cbc7fa19932
📒 Files selected for processing (13)
apps/server/src/services/auto-mode-service.tsapps/server/src/services/auto-mode/execution-service.tsapps/server/src/services/project-orchestration-service.tsapps/server/src/services/worktree-lifecycle-service.tsdocs/reference/auto-mode.mdpackages/mcp-server/plugins/automaker/.claude-plugin/plugin.jsonpackages/mcp-server/plugins/automaker/README.mdpackages/mcp-server/plugins/automaker/agents/devops-health-check.mdpackages/mcp-server/plugins/automaker/commands/ava.mdpackages/mcp-server/plugins/automaker/commands/setuplab.mdpackages/mcp-server/plugins/automaker/hooks/handle-mcp-failure.shscripts/launch-protomaker.shtools/board_monitor.py
💤 Files with no reviewable changes (1)
- packages/mcp-server/plugins/automaker/README.md
| * Resume a feature (continues from saved context) | ||
| */ | ||
| async resumeFeature(projectPath: string, featureId: string, useWorktrees = false): Promise<void> { | ||
| async resumeFeature(projectPath: string, featureId: string, useWorktrees = true): Promise<void> { |
There was a problem hiding this comment.
resumeFeature default-to-worktree is currently bypassed by route wiring.
This default flip won’t take effect for /resume-feature callers because the route still passes useWorktrees ?? false, which forces omitted values back to false. That leaves resume behavior non-defaulted at the API boundary despite this change.
As per coding guidelines: “Verify wiring is complete before committing a multi-step plan implementation — CI catches broken code but NOT unwired code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/auto-mode-service.ts` at line 1706, The route wiring
is overriding resumeFeature's default by passing useWorktrees ?? false from the
/resume-feature handler; update the caller so it does not coerce undefined to
false — either stop passing the param when omitted or pass the explicit boolean
you intend (e.g., true) so the resumeFeature(projectPath, featureId,
useWorktrees = true) default actually takes effect; look for the /resume-feature
route handler that calls resumeFeature and remove the "?? false" coalescing (or
pass true explicitly) to restore the new default.
| // Without this, an unset workflow falls into the match-rule scoring, | ||
| // which mis-matched phases to read-only workflows like changelog-digest | ||
| // (no PR, no worktree → agents ran in main). See #3788/#3793. | ||
| workflow: phase.workflow ?? options.defaultWorkflow ?? 'standard', |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move 'standard' workflow fallback into configuration/constants.
Hardcoding a workflow value here violates the server guideline for workflow-specific values. Keep the same fallback behavior, but source it from settings or a shared config constant.
As per coding guidelines: “Never hardcode workflow-specific values like file paths, branch names, CI check names, channel IDs, and hosting providers. These must come from settings or configuration.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/project-orchestration-service.ts` at line 239, The
code currently falls back to the hardcoded string 'standard' when resolving
workflow (in the assignment using phase.workflow ?? options.defaultWorkflow ??
'standard'); remove the literal and instead read the fallback from configuration
or a shared constant (e.g., import DEFAULT_WORKFLOW from the project
settings/config or call Settings.get('defaultWorkflow')), then use
phase.workflow ?? options.defaultWorkflow ?? DEFAULT_WORKFLOW (or the settings
call) so workflow-specific values are sourced from config rather than hardcoded.
| * NOTE: node_modules is deliberately NOT symlinked. Sharing the host's | ||
| * node_modules meant an agent running `npm install` in a worktree (e.g. to add | ||
| * a workspace package) rewrote the host's deps and broke the running server. | ||
| * Worktrees now install their own node_modules via the worktree-init script | ||
| * (`npm ci`), keeping each worktree isolated. dist/build are still symlinked | ||
| * since agents read them and don't mutate them. | ||
| */ | ||
| const BUILD_ARTIFACT_DIRS = ['node_modules', 'dist', 'build', '.next', '.nuxt', 'out'] as const; | ||
| const BUILD_ARTIFACT_DIRS = ['dist', 'build', '.next', '.nuxt', 'out'] as const; |
There was a problem hiding this comment.
Update agent worktree instructions to match the new node_modules isolation model.
This change removes shared node_modules, but execution prompts still warn agents never to install in worktrees because of symlink breakage. That guidance is now stale and can block valid dependency updates in worktree runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/services/worktree-lifecycle-service.ts` around lines 776 -
783, Update the stale comment and any agent-facing prompts that warn "never to
install in worktrees" to reflect the new isolation: clarify node_modules is
deliberately NOT symlinked and agents should use the worktree-init script (npm
ci) to install dependencies in the worktree; keep the note that
BUILD_ARTIFACT_DIRS (constant BUILD_ARTIFACT_DIRS) like
'dist','build','.next','.nuxt','out' remain symlinked and safe to read but not
mutated. Locate the top-of-file comment around BUILD_ARTIFACT_DIRS and any
message strings or prompt code that reference "never install" and replace them
with concise guidance to run worktree-init (npm ci) for dependency changes and
to avoid mutating symlinked build artifact dirs.
| REPO="$HOME/dev/protomaker" | ||
| cd "$REPO" || { | ||
| echo "[launch-protomaker] FATAL: $REPO not found" | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
Avoid hardcoding the repository path.
Line 20 hardcodes a single developer path, so this launcher fails on any machine with a different checkout location. Make the root configurable (e.g., AUTOMAKER_ROOT) with a script-relative fallback.
Proposed fix
-REPO="$HOME/dev/protomaker"
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+REPO="${AUTOMAKER_ROOT:-$(cd "$SCRIPT_DIR/.." && pwd)}"📝 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.
| REPO="$HOME/dev/protomaker" | |
| cd "$REPO" || { | |
| echo "[launch-protomaker] FATAL: $REPO not found" | |
| exit 1 | |
| } | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| REPO="${AUTOMAKER_ROOT:-$(cd "$SCRIPT_DIR/.." && pwd)}" | |
| cd "$REPO" || { | |
| echo "[launch-protomaker] FATAL: $REPO not found" | |
| exit 1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/launch-protomaker.sh` around lines 20 - 24, Replace the hardcoded
REPO="$HOME/dev/protomaker" assignment with logic that prefers an environment
variable (e.g., AUTOMAKER_ROOT) and falls back to a script-relative path; set
REPO to AUTOMAKER_ROOT if present, otherwise compute the repository root
relative to the launcher script location, then retain the existing cd "$REPO" ||
{ echo "... FATAL ..."; exit 1 } handling. Update references to REPO in the
script only, leaving the error message intact.
| for port in 3008 3007; do | ||
| pids=$(lsof -ti:"$port" -sTCP:LISTEN 2>/dev/null || true) | ||
| if [ -n "$pids" ]; then | ||
| echo "[launch-protomaker] freeing port $port (killing stale listener: $pids)" | ||
| echo "$pids" | xargs kill -9 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
Don’t immediately kill -9 any listener on shared ports.
Lines 56-61 can terminate unrelated processes and skip graceful shutdown. Prefer TERM first, then escalate to KILL only if still alive (optionally after checking process command line).
Proposed fix
for port in 3008 3007; do
pids=$(lsof -ti:"$port" -sTCP:LISTEN 2>/dev/null || true)
if [ -n "$pids" ]; then
echo "[launch-protomaker] freeing port $port (killing stale listener: $pids)"
- echo "$pids" | xargs kill -9 2>/dev/null || true
+ echo "$pids" | xargs kill -TERM 2>/dev/null || true
+ sleep 1
+ still_alive=$(echo "$pids" | xargs -I{} sh -c 'kill -0 "{}" 2>/dev/null && echo "{}"' || true)
+ [ -n "$still_alive" ] && echo "$still_alive" | xargs kill -KILL 2>/dev/null || true
fi
done📝 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.
| for port in 3008 3007; do | |
| pids=$(lsof -ti:"$port" -sTCP:LISTEN 2>/dev/null || true) | |
| if [ -n "$pids" ]; then | |
| echo "[launch-protomaker] freeing port $port (killing stale listener: $pids)" | |
| echo "$pids" | xargs kill -9 2>/dev/null || true | |
| fi | |
| for port in 3008 3007; do | |
| pids=$(lsof -ti:"$port" -sTCP:LISTEN 2>/dev/null || true) | |
| if [ -n "$pids" ]; then | |
| echo "[launch-protomaker] freeing port $port (killing stale listener: $pids)" | |
| echo "$pids" | xargs kill -TERM 2>/dev/null || true | |
| sleep 1 | |
| still_alive=$(echo "$pids" | xargs -I{} sh -c 'kill -0 "{}" 2>/dev/null && echo "{}"' || true) | |
| [ -n "$still_alive" ] && echo "$still_alive" | xargs kill -KILL 2>/dev/null || true | |
| fi | |
| done |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/launch-protomaker.sh` around lines 56 - 61, The current loop that
finds listeners into variable pids and immediately runs xargs kill -9 should be
changed to attempt a graceful shutdown first: for each PID in pids (found by
lsof -ti:"$port" -sTCP:LISTEN), send SIGTERM (kill -TERM) and wait a short
interval, then check if the PID is still running and only then escalate to
SIGKILL (kill -9); optionally inspect the process command line (e.g., via
/proc/$pid/cmdline or ps -o args=) to verify it’s the expected
protomaker-related process before killing. Update the loop that references pids
and the xargs kill -9 invocation to implement TERM-then-KILL with a short wait
and re-check.
Quinn review on #3802: git add -A / git diff --cached in the local fresh-eyes review had no timeout — a pathological worktree could hang the review step. Add a 120s guard (fail-open already catches it). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed Quinn's review:
|
There was a problem hiding this comment.
QA Audit — PR #3802 | fix(auto-mode): worktree defaults + local review + workflow default; ops cleanup
VERDICT: WARN
CI Status
- test: queued
- build: queued
- deploy: in_progress
- checks: queued
⚠️ No checks completed yet — verdict contingent on all passing.
Diff Review
Two-commit stabilization: (1) server behavior fixes across auto-mode/execution/project-orchestration/worktree-lifecycle services; (2) ops + plugin cleanup. The changes are coherent and directly address the described failure modes. Key additions:
useWorktreesdefaulttrueinexecuteFeature/resumeFeature— isolates agents from the main checkout.reviewAndIterateWorktreeDiff(execution-service.ts:1396+): runsgit add -A+git diff --cached, callssimpleQuerywith fresh-eyes prompt, branches onparseFreshEyesVerdict— PASS proceeds, BLOCK/CONCERN runs one fix iteration. Fail-open via try/catch. Skips read-only. ✅ Clean, fail-safe design.node_modulesdropped fromBUILD_ARTIFACT_DIRS— each worktree runs its ownnpm ci, preventing the corruption scenario.- Workflow
'standard'fallback added in project-orchestration-service.ts:239 — closes the mis-match-to-changelog-digest gap.
Observations
- [MEDIUM]: No unit tests for
reviewAndIterateWorktreeDiff. The new local-review gate runs before every PR step; a regression here bypasses pre-PR review for all auto-mode features. Consider adding a test covering the PASS skip and the BLOCK/CONCERN fix-iteration path. - [MEDIUM]: Thread 1 (CodeRabbit) —
resumeFeaturedefault may still be bypassed if the route handler passesuseWorktrees ?? false, overriding the parameter defaulttrue. The service default alone doesn't guard against callers explicitly passingfalse. If the route intentionally passes a value, that value now controls behavior; if it's unintentional, the route should stop passing it. - [LOW]: Thread 2 (CodeRabbit) —
'standard'hardcoded in project-orchestration-service.ts:239. Refactor candidate, not a blocker. - [LOW]: Thread 3 (CodeRabbit) — execution prompts may still warn against worktree installs. Low priority; the
node_modulesisolation resolves the underlying issue. - [LOW]: CI still in progress — cannot confirm test pass yet.
Checks: 0 | Passed: 0 | Failed: 0 | Gaps: 4
— Quinn, QA Engineer
|
Submitted COMMENT review on #3802. |
* refactor: Enforce package-lock sync in the git-workflow (all managed projects) * fix(auto-mode): worktree defaults + local review + workflow default; ops cleanup (#3802) * fix(auto-mode): worktrees by default + local antagonistic review + workflow default Stabilization fixes validated live this session: - auto-mode-service: useWorktrees defaults true in executeFeature/resumeFeature and the loop always dispatches with worktrees (was gated on executionMode != read-only). A stale 'read-only' executionMode previously let a scaffold agent run npm install in the main checkout and corrupt node_modules. Also adds 'config_corrupted' to the immediate-pause circuit breaker. - worktree-lifecycle-service: drop node_modules from BUILD_ARTIFACT_DIRS — each worktree runs its own npm ci and can no longer clobber the main install. - execution-service: run a local fresh-eyes (antagonistic) review of the staged worktree diff before the git-workflow/PR step; on BLOCK/CONCERN run one fix iteration. Fail-open, skips read-only. - project-orchestration-service: default phase workflow to 'standard' (was falling through fuzzy match-scoring into changelog-digest -> no PR). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(ops): plugin cleanup + production launch script - plugin: repo URL automaker->protoMaker; CodeRabbit->Quinn in ava/setuplab; drop hardcoded /home/josh path from devops-health-check; README tool count; handle-mcp-failure.sh fallback uses ${AUTOMAKER_ROOT:-$PWD} not a hardcoded path. - scripts/launch-protomaker.sh: non-interactive prod build+serve for the macOS LaunchAgent (frees stale ports, sources .env, NODE_ENV=production). - tools/board_monitor.py: monitoring tweaks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * style: prettier format setuplab.md Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(auto-mode): document useWorktrees defaults to true Adds a Default column to the workflowSettings table; useWorktrees now true (see this PR's auto-mode-service change). Folds in the doc work from the 'Update docs after: protoMaker CLI' board feature. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(review): add timeout to local-review git execAsync calls Quinn review on #3802: git add -A / git diff --cached in the local fresh-eyes review had no timeout — a pathological worktree could hang the review step. Add a 120s guard (fail-open already catches it). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> * fix(ci): prevent script injection via commit message in deploy workflows (#3812) deploy-docs.yml and deploy-site.yml interpolated ${{ github.event.head_commit.message }} directly into the wrangler `run:` block. A commit message with backticks / $() / ; is parsed by bash (CWE-94) — breaking the script and risking secret exfiltration in a job holding CLOUDFLARE_API_TOKEN. This already broke docs deploy on main (backticks in an agent commit body). Pass the message via an env var so the shell treats it as literal data. Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> * refactor: feature commands * refactor: agent commands * feat: add pr create|status|merge commands Add PR command group to the CLI with three subcommands: - pr create <featureId> opens a PR from a feature worktree - pr status <prNumber> shows CI rollup for a PR - pr merge <prNumber> merges with configured strategy Wired to /worktree/create-pr, /github/check-pr-status, and /github/merge-pr endpoints following existing command module patterns. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
1 participant
Durably commits the stabilization fixes that were validated live during the autonomous session (previously deployed from an uncommitted working tree).
Server behavior fixes (commit 1)
useWorktreesdefaultstrueinexecuteFeature/resumeFeature, and the loop always dispatches with worktrees (was gated onexecutionMode !== 'read-only'). A staleread-onlyexecutionMode previously let a scaffold agent runnpm installin the main checkout and corruptnode_modules. Addsconfig_corruptedto the immediate-pause circuit breaker.node_modulesfromBUILD_ARTIFACT_DIRS— each worktree runs its ownnpm ciand can no longer clobber the main install.workflowto'standard'(was falling through fuzzy match-scoring intochangelog-digest→ no PR).Ops + plugin cleanup (commit 2)
/home/joshpath; README tool count;handle-mcp-failure.shfallback uses${AUTOMAKER_ROOT:-$PWD}.scripts/launch-protomaker.sh: non-interactive prod build+serve for the macOS LaunchAgent.tools/board_monitor.py: monitoring tweaks.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Bug Fixes