fix(wrappers): move per-side rebind after lib-auth source (INV-37/38)#159
Merged
Conversation
Both wrappers (autonomous-dev.sh, autonomous-review.sh) used to apply their per-side AGENT_CMD / AGENT_LAUNCHER_ARGV rebind immediately after sourcing lib-agent.sh — and crucially BEFORE sourcing lib-auth.sh. That order is broken: lib-auth.sh transitively sources lib-config.sh::load_autonomous_conf, which re-sources autonomous.conf. Operator confs unconditionally declare AGENT_CMD="claude" (the default CLI), so the re-source silently reverted any per-side rebind. The bug was invisible whenever AGENT_DEV_CMD == AGENT_REVIEW_CMD == AGENT_CMD; it surfaced only when an operator set a non-trivial AGENT_REVIEW_CMD (e.g. kiro for cheaper review passes). Discovered when a downstream consumer's review wrapper kept invoking claude with model 'claude-sonnet-4.6' (a kiro model name) and failing 404 — operator had set AGENT_REVIEW_CMD=kiro and the rebind was being undone seven seconds into every invocation. Two consecutive review- wrapper crashes triggered the dev agent's loop-break protection, removing the autonomous label from the issue. Fix: move the rebind below `source lib-auth.sh` (and, for review, below the review-specific libs too — none of them currently re-source autonomous.conf, but keeping all rebind below all lib sources is the robust contract). lib-auth.sh and friends do not read AGENT_CMD or AGENT_LAUNCHER_ARGV, so deferring the rebind is safe. Tests: - tests/unit/test-wrapper-rebind-order.sh — new behavioral regression. Builds a sandbox conf with AGENT_DEV_CMD=claude, AGENT_REVIEW_CMD=kiro, AGENT_DEV_LAUNCHER=cc-bridge; sources each wrapper's source/rebind block and asserts post-rebind state. T2 directly reproduces the downstream consumer review misroute. - tests/unit/test-lib-agent-per-side-cmd.sh — PSC-S9/S10 rewritten to assert "rebind AFTER source lib-auth.sh" instead of "rebind within ≤4/5 lines of source lib-agent.sh". The old line-window assertion was the bug shape. - tests/unit/test-lib-agent-per-side-launcher.sh — PSL-S9/S10 rewritten in the same shape. Docs: - invariants.md INV-37 description corrected (and INV-38 cross-ref). - per-side-agent-cmd.md implementation step 2/3 corrected with a load-bearing-rationale paragraph naming the bug. - per-side-launcher.md resolution-order block updated; PSL-S9/S10 test description updated.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR correctly fixes INV-37/INV-38 rebind order by moving the per-side overrides after all library sources. The structural change ensures autonomous.conf re-sourcing via lib-auth.sh no longer silently reverts the rebind. Comprehensive test coverage (including behavioral regression) and clear documentation make this ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
agy review of #159 surfaced three issues: P2 (test bug, now P1 in effect): test-wrapper-rebind-order.sh used a hardcoded `sed -n '22,40p'` extraction range for the wrapper init block. Review-side launcher rebind sits at line 41 of the post-fix autonomous-review.sh, so the hardcoded range silently truncated the launcher rebind out of the simulation. Default-empty AGENT_REVIEW_LAUNCHER_ARGV made the count assertion pass vacuously. Fix: - Replace hardcoded range with content-anchored awk extraction (from `SCRIPT_DIR=` to before `: "${PROJECT_ID:?...}"`). Stays in lock-step with wrapper edits. - Switch from `bash -c "..."` to writing the script to a tmpfile and feeding `bash <file>`. The extracted block contains markdown backticks (e.g. `AGENT_CMD="claude"` in WHY comments). bash -c treats backticks as command substitution under double-quoting, which would silently execute the comment payload. - Add T3 — separate sandbox where AGENT_REVIEW_CMD=claude (so INV-38 guard accepts) and AGENT_REVIEW_LAUNCHER is non-empty. Asserts both AGENT_CMD and the launcher rebind execute. Closes the vacuously-passing-T2 hole. P3 (test gap): test-lib-agent-per-side-launcher.sh PSL-S9/S10 only asserted "rebind lands AFTER lib-auth.sh", missing the symmetric "rebind does NOT precede lib-auth.sh" check that PSC-S9/S10 has. Added the matching pre-source assertions. Verified by mutating the review wrapper to revert the bug shape: PSL-S10 now FAILs with "REBIND_BEFORE_LIB_AUTH", catching what the AFTER-only check missed. P2 (doc typo): per-side-agent-cmd.md test-summary table had a stale duplicate row for PSC-S10 referencing the old "within 5 lines" assertion. Removed.
Owner
Author
Addressed agy review findings (commit
|
agy follow-up review on PR #159 noted that the awk end-anchor was coupled to the specific variable name PROJECT_ID. If a future edit reorders the validation block (e.g. validates REPO before PROJECT_ID) or renames PROJECT_ID, the extraction would fail to terminate and the test would silently capture wrapper code beyond the init block. Generalize the regex to '\${[A-Z_]+:\\?' — matches any uppercase variable name in the `: "${VAR:?...}"` validation idiom that the wrappers use to gate startup on required config.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Both wrappers (
autonomous-dev.sh,autonomous-review.sh) used to rebindAGENT_CMDandAGENT_LAUNCHER_ARGVto per-side values between sourcinglib-agent.shandlib-auth.sh. That order is broken:lib-auth.shtransitively re-sourcesautonomous.conf(vialib-config.sh::load_autonomous_conf), which silently reverts the per-side rebind because operator confs declareAGENT_CMD="claude"unconditionally.Fix: move the rebind to after
source lib-auth.sh(and the review-specific libs).Bug shape
A downstream consumer's review wrapper kept invoking
claudewith modelclaude-sonnet-4.6(a kiro model name) and failing 404. Operator had setAGENT_REVIEW_CMD=kiro. Two consecutive review-wrapper crashes triggered the dev agent's loop-break protection, which removed theautonomouslabel from the issue.The bug was invisible whenever
AGENT_DEV_CMD == AGENT_REVIEW_CMD == AGENT_CMD(the common case). It surfaced only on mixed-CLI deployments — exactly the deployment INV-37 was supposed to enable.Root cause
Files changed
skills/autonomous-dispatcher/scripts/autonomous-dev.sh— move rebind belowsource lib-auth.sh+ 5-line WHY comment.skills/autonomous-dispatcher/scripts/autonomous-review.sh— same shape, after all four lib sources.tests/unit/test-wrapper-rebind-order.sh— new behavioral regression. Builds a sandbox conf withAGENT_DEV_CMD=claude+AGENT_REVIEW_CMD=kiro+AGENT_DEV_LAUNCHER=cc-bridge; sources each wrapper's source/rebind block; asserts post-rebind state. T2 reproduces the downstream consumer review misroute.tests/unit/test-lib-agent-per-side-cmd.sh— PSC-S9/S10 rewritten: assert "rebind AFTER source lib-auth.sh" instead of "within ≤4/5 lines of source lib-agent.sh" (the old line-window assertion was the bug shape).tests/unit/test-lib-agent-per-side-launcher.sh— PSL-S9/S10 same shape.docs/pipeline/invariants.md— INV-37 description corrected with "Why after lib-auth.sh" rationale; INV-38 cross-ref.docs/pipeline/per-side-agent-cmd.md— implementation step 2/3 corrected.docs/pipeline/per-side-launcher.md— resolution-order block updated; PSL-S9/S10 test description updated.Test plan
bash tests/unit/test-lib-agent-per-side-cmd.sh→ 17/17 PASSbash tests/unit/test-lib-agent-per-side-launcher.sh→ 14/14 PASSbash tests/unit/test-wrapper-rebind-order.sh→ 4/4 PASS (T2 directly reproduces the downstream consumer misroute)test-lib-agent-*.shandtest-autonomous-{dev,review}-*.sh→ still PASSnpx skills update -g -y autonomous-dispatcherto refresh user-scope skill on dev box; rerun review wrapper end-to-end against a real PR to confirm the misroute is gone.Backwards compat
Pure structural reordering of three lines per wrapper. No public API change. No conf-format change. Default deployment (
AGENT_DEV_CMD == AGENT_REVIEW_CMD == AGENT_CMD == claude) is byte-for-byte identical — the bug only manifested on mixed-CLI deployments where the rebind was load-bearing.