fix: preserve seed file modes + pause on verify env faults (rc 126/127)#130
Conversation
_copy_traversable copied every file via write_bytes, which drops the source mode: seeded gitignored trees (vendor/bin/*, node_modules/.bin/*) arrived non-executable and the first verify command died rc=127. Real-filesystem sources now go through shutil.copy2 (mode-preserving, matching the repo's other copy sites); the byte-only walk remains as the fallback for zip-imported Traversables, which expose no stat. Side effects: copy2 also carries mtimes (nothing reads seeded mtimes), and install_into/_copy_skills inherit the same mode preservation. Symlinked seeds are still materialized as target content, now with the target's mode.
… run (#126) The shell's 126 (found, not executable) and 127 (not found) are the run environment's failures, not the story's: deterministic for a given tree and shared by every story's verify commands. They were collapsed into the generic fixable retry, burning max_dev_attempts on repair sessions that could not fix anything and eventually deferring the story. verify_commands_outcome now escalates them (new VerifyOutcome.env_fault flag): the dev leg already pauses on CRITICAL regardless of budget, and runs.rearm_escalation resets attempt=0, so the story re-drives with a fresh budget once the human fixes the environment. The review loop, skip-review path, and _fix_phase gain matching escalate branches so an env fault surfacing there pauses instead of burning review cycles / dispatching doomed fix sessions (this also stops a shared-gate GitError escalate from silently burning review cycles). Journal events carry the env_fault flag. Timeouts (rc -1) keep the charged retry: a hang is plausibly the story's own tests. Windows cmd signals these faults as 9009/1, so it keeps today's behavior — a documented no-op, not a regression.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes preserve executable modes during worktree seeding, classify verification return codes 126 and 127 as environment faults, and pause development, review, commit, and repair flows without continuing additional repair attempts. ChangesEnvironment fault handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VerifyCommand
participant verify_commands_outcome
participant VerifyOutcome
participant EnginePhase
VerifyCommand->>verify_commands_outcome: return rc=126 or rc=127
verify_commands_outcome->>VerifyOutcome: create non-retryable env_fault outcome
VerifyOutcome->>EnginePhase: deliver verification failure
EnginePhase->>EnginePhase: journal env_fault and pause
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bmad_loop/verify.py (1)
1431-1451: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not let an earlier ordinary failure mask a later environment fault.
run_verify_commandscollects every command result, but this loop returns on the first non-zero result. With commands such as("exit 1", "missing-command"), therc=1result is classified as fixable, so the engine can start a repair session even though the same verification run already observedrc=127. Scan all results forENV_FAULT_RCSbefore selecting an ordinary retryable failure, and add a mixed-command regression test.Proposed fix
- for result in run_verify_commands(policy, cwd): + results = run_verify_commands(policy, cwd) + for result in results: if result.returncode in ENV_FAULT_RCS: return VerifyOutcome.escalate( ... env_fault=True, ) + for result in results: if result.returncode != 0: return VerifyOutcome.retry( ... )🤖 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 `@src/bmad_loop/verify.py` around lines 1431 - 1451, Update verify_commands_outcome to scan all results from run_verify_commands for ENV_FAULT_RCS before returning any ordinary retry outcome, ensuring a later environment fault takes precedence over earlier failures. Preserve the existing escalation details and ordinary retry behavior when no environment fault exists, and add a regression test covering an ordinary failure followed by a missing-command environment fault.src/bmad_loop/engine.py (1)
1844-1853: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRecord
env_faulton the skip-review verification failure path.When review is disabled, this branch escalates directly without emitting a verification-failure journal event. Consequently, journal consumers cannot distinguish an environment fault from another non-retryable failure, while the review-enabled and fix paths record
env_fault.Add a failure event here, or extend the existing skip-review event, with
env_fault=outcome.env_fault; add the corresponding assertion to the skip-review engine test.Proposed fix
if not outcome.ok: + self.journal.append( + "review-verify-failed", + story_key=task.story_key, + reason=outcome.reason, + env_fault=outcome.env_fault, + ) if not outcome.retryable:🤖 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 `@src/bmad_loop/engine.py` around lines 1844 - 1853, Update the skip-review failure handling in the engine flow around _verify_review and _defer to emit or extend the verification-failure journal event with env_fault=outcome.env_fault before escalation or deferral. Add the corresponding assertion to the skip-review engine test, preserving existing behavior for retryable and non-retryable outcomes.
🤖 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.
Outside diff comments:
In `@src/bmad_loop/engine.py`:
- Around line 1844-1853: Update the skip-review failure handling in the engine
flow around _verify_review and _defer to emit or extend the verification-failure
journal event with env_fault=outcome.env_fault before escalation or deferral.
Add the corresponding assertion to the skip-review engine test, preserving
existing behavior for retryable and non-retryable outcomes.
In `@src/bmad_loop/verify.py`:
- Around line 1431-1451: Update verify_commands_outcome to scan all results from
run_verify_commands for ENV_FAULT_RCS before returning any ordinary retry
outcome, ensuring a later environment fault takes precedence over earlier
failures. Preserve the existing escalation details and ordinary retry behavior
when no environment fault exists, and add a regression test covering an ordinary
failure followed by a missing-command environment fault.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64528143-28fc-47ea-9459-d64557f8f476
📒 Files selected for processing (7)
src/bmad_loop/engine.pysrc/bmad_loop/install.pysrc/bmad_loop/verify.pytests/test_engine.pytests/test_escalation.pytests/test_install.pytests/test_verify.py
…ip-review env_fault journal (#126) CodeRabbit review round on PR #130, both findings confirmed: - verify_commands_outcome returned on the first non-zero rc, so an ordinary failure (rc=1) earlier in the command list masked an env fault (rc=126/127) later in the same run — charging a repair session in a broken environment, the exact #126 failure mode. Scan the collected results for ENV_FAULT_RCS first. - _skip_review_and_commit escalated/deferred without a structured env_fault journal field; emit the same review-verify-failed event as the review-enabled loop.
|
Validated both review findings against the code — both confirmed, fixed in bc391b4:
Full suite 2224 green + trunk clean. |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
1 participant
Fixes #126.
Problem
Both claims in #126 validated against HEAD:
_copy_traversablecopied every seeded file viadst.write_bytes(src.read_bytes()), discarding file modes. Every[scm] worktree_seed/ pluginseed_globstree (e.g.vendor/,node_modules/) arrived in the worktree with non-executable binaries, and the first verify command died rc=127.max_dev_attemptsexactly like broken story code — spawning no-op repair sessions and, on the reference run, falsely deferring a story.Fix
Commit 1 — root cause. Real-filesystem sources now copy via
shutil.copy2(mode-preserving, matching every other copy site in the repo); the byte-only walk remains solely for zip-imported Traversables, which expose nostat. Seededvendor/bin/*keep+x.Commit 2 — misclassification.
verify_commands_outcomeclassifies rc 126/127 as a CRITICAL escalate carrying a newVerifyOutcome.env_faultflag, so the run pauses at the first environment fault instead of burning story budgets. This deliberately diverges from the issue's "retry without decrementing" proposal: these rcs are deterministic for a given tree and shared by every story's verify commands, so uncharged retries either loop forever or replay the same wall on all N stories. The uncharged semantics land via existing machinery —rearm_escalationresetstask.attempt = 0, so the story re-drives with a fresh budget once the environment is fixed. The review loop, skip-review path, and_fix_phasegain matching escalate branches (no fix sessions dispatched against an unfixable environment; also closes a latent hole where a shared-gateGitErrorescalate burned review cycles). Journaldev-decision/review-verify-failed/fix-decisionevents carryenv_fault.Out of scope, documented in-code: timeouts (rc -1) stay charged (a hang is plausibly the story's own tests); Windows
cmdsignals these faults as 9009/1 and keeps today's behavior.The issue's shipped workarounds (
chmod +x vendor/bin/*guard,@php vendor/bin/...) become unnecessary.Tests
seed_files+seed_globs), plus azipfile.Pathfallback guard; the exec-bit tests fail on the old copy code.attempt == 1; review-gate rc 126 escalates with no fix session; skip-review rc 126 escalates instead of deferring; mid-fix-loop rc 126 stops the loop with budget left. All verified to fail without the fix.trunk checkclean.Summary by CodeRabbit