fix(unic-archon-dlc): explicit staging and host-agnostic repo pinning in the Archon Boxes - #307
Conversation
… in the Archon Boxes Two defects Archon 0.7.0 fixed in its own bundled defaults were live here. Blind staging: /explore's preserve-spike ran `git add -A` and /build's open-pr said "stage everything changed by the build". A Box runs in an isolated worktree that also carries the operator's copied `.archon/`, scratch files from earlier nodes, and a build-state.json the loop rewrites up to 60 times — so "everything changed" is not the change. Unpinned PR commands: no `gh`/`az` call passed a repository, so in a fork clone the CLI resolved to the upstream parent and a PR opened, a review posted or a defect filed landed in the wrong repository. Changes: - Stage named paths in preserve-spike, open-pr, both loop phases, and both gate paths of commands/specs.md and commands/tickets.md; prove it with `git status --porcelain` before committing - Commit build-state.json once, at open-pr — never in a loop iteration; it is ADR-0012's anti-cheat proof and died with the pruned worktree - Carry the deny list inline in every committing prompt: pr-body.md, *.tmp.md, *.scratch.md, $ARTIFACTS_DIR, .archon/ - Add optional `project.repo_ref`; every PR-touching node reads it via bootstrap and passes it host-agnostically (gh --repo, az repos --repository) - Add a guard-no-repo-ref cancel node to all four Box YAMLs (ADR-0011: expected precondition failures cancel, they do not fail) - Stop staging the docs/adr/ directory in commands/specs.md — name each new ADR - Add test/box-staging-and-repo-pinning.test.mjs and repo_ref schema tests Fixes #289 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Promotes the #289 changelog entries — explicit staging in every committing node and host-agnostic repo pinning via the new optional project.repo_ref key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔍 Comprehensive PR ReviewPR: #307 SummaryWell-targeted fix with a strong new test file ( Verdict:
🟠 High Issues
|
| Issue | Location | Suggestion |
|---|---|---|
build.yaml's bootstrap fallback phrasing diverges from the other 3 Boxes (functionally equivalent) |
unic-dlc-build.yaml:27-29 |
No fix required |
pr-review.yaml's post node describes GitHub review-comment endpoints loosely (pre-existing, not introduced here) |
unic-dlc-pr-review.yaml:481-482 |
No fix required for this PR |
✅ What's Good
guard-no-repo-refconsistently usescancel:(not a failing check) across all four Boxes, correctly following ADR-0011.az repos pr show/updatecorrectly use--idwhilepr create/pr listcorrectly use--repository— the two subcommand families are genuinely asymmetric and the PR gets this right throughout.lib/config-schema.mjs's handling ofrepo_refis exemplary: defaults tonull, deliberately excluded fromMANDATORY_PATHSwith a comment explaining why, survivesdeepMerge, andmigrateLegacytreats it like its sibling keys.docs/adr/directory-add elimination incommands/specs.mdis a good catch beyond the PR's literalgit add -Aframing.- CHANGELOG cites the specific upstream Archon issues (
coleam00/Archon#2361,#2229) that motivated the fix.
📋 Suggested Follow-up Issues
- "Scope repo-pinning test assertions to individual nodes, not whole-file" (P2, MEDIUM issue Fix release workflow, document Gitflow, and close post-launch open points #1)
Next Steps
- Fix both HIGH issues before merge.
- Decide on the two MEDIUM issues (both low-effort to fix now).
- LOW issues need no action.
Reviewed by Archon comprehensive-pr-review workflow
Artifacts: /Users/oriol.torrent/.archon/workspaces/unic/unic-agents-plugins/artifacts/runs/b1f2bd288cb04c6a14e11d33acfa2345/review/
Fixed: - pr-review.yaml: prep now depends_on [bootstrap, guard-no-repo-ref] — the DAG previously ran them as same-tier siblings, leaving prep's own "guaranteed non-empty" claim unbacked by an edge (HIGH, error-handling) - box-staging-and-repo-pinning.test.mjs: nodeSource() anchored the id split on a bare substring, so a prefix id (merge vs merge-gate) returned the wrong node's body — fixed while adding the per-node pinning tests below - box-staging-and-repo-pinning.test.mjs: BLIND_ADD doc-comment named the wrong distinguishing feature of its own regex (backtick vs line-start position) — reworded to match the regex and the CHANGELOG (MEDIUM, comment-quality) Tests added: - node-scoped repo-pinning assertions for every PR-touching node, closing the whole-file-granularity gap that would still pass a regression dropping the pin from one specific node (HIGH/MEDIUM, test-coverage) - warn-before-gh-pr-create ordering test for commands/specs.md and commands/tickets.md, covering the cancel-vs-warn distinction scope.md flagged as needing verification (HIGH, test-coverage) Skipped: none — the two LOW findings (build.yaml phrasing divergence, pr-review.yaml's pre-existing loose endpoint description) were marked "no fix required" by the reviewing agents themselves.
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (4 total)
View all fixes
Tests Added
Skipped (2)
Suggested Follow-up Issues(none — the one suggested follow-up was fixed directly in this pass) Validation✅ Type check | ✅ Lint/format (5 pre-existing infos, unrelated files) | ✅ Tests (232/232 passed) Self-fix by Archon · aggressive mode · fixes pushed to |
…ing/pinning test Windows checkouts convert LF to CRLF, so `- id: <name>\n` in nodeSource's split regex never matched (the character after the id was \r, not \n), failing every downstream node lookup and cascading into 10 test failures on windows-latest CI.
…all_done The `depends_on: [bootstrap, guard-no-repo-ref]` edge added for the prior review round broke the happy path. `guard-no-repo-ref` carries a `when:`, so when `project.repo_ref` IS set the guard is skipped, and a skipped dependency propagates its skipped state under Archon's default `all_success` join. `prep` would have been skipped for every correctly-configured Consumer, taking `review` → `synthesize` → `reconcile` → `post` with it and turning `/pr-review` into a no-op. `post`, `verify-pr-base` and `merge` already carry `trigger_rule: all_done` over their own gate edges for exactly this reason. A new test asserts the join for any node that depends on the guard, so a Box cannot take the edge without the join again. Verified red before green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔍 Comprehensive PR ReviewPR: #307 — SummaryThe staging half of #289 is done and provable: zero line-anchored blind adds survive anywhere in the plugin tree, every committing node carries its deny list inline per ADR-0023 §5, and The repo-pinning half is not ready. Three defects each break a whole class of Consumer:
The regression suite is the second concern. A mutation pass injected nine defects against the head and five survived a fully green 21/21 run — including the exact F1 DAG defect this PR just fixed, rewritten as a YAML block list. A malformed Verdict:
31 consolidated from 44 raw findings. Three agents returned 🧪 Mutation evidenceInjected one at a time into a clean checkout of
⚖️ The one decisionH1 and H3 are the same question asked twice, and both blocking.
Recommendation: keep the key and redefine ado's form. It preserves the invariant the whole PR is built on — one opaque string, the flag is the only host-specific thing — instead of carving a per-host exception into seven prompts, and it is documentation plus Everything else below is independent of that choice. 🟠 HIGH IssuesH1 — The guard cancels every Box for every existing Consumer, and the remedy it names cannot fix it📍 No Consumer config on disk carries On upgrade to 0.15.1, Fix, if the key staysAdd - **project** — `project.name`, `project.branching` (`gitflow | github-flow`), `project.pr_strategy`
(`merge | squash | rebase`), `project.repo_ref` (host-agnostic repository the Boxes pin PR commands
to: `OWNER/REPO` or `HOST/OWNER/REPO` for github, the repository name for ado). Suggest a value
parsed from `GIT_REMOTE` and ask the human to confirm it. _(mandatory: branching, pr_strategy)_H2 —
|
| # | Issue | Location | Suggestion |
|---|---|---|---|
| L1 | The command docs check null only; repo_ref: "" or " " passes the warning and fails at the CLI |
commands/specs.md:266, commands/tickets.md:310 |
Reword to "null or empty"; normalise in the loader: (g(…) ?? '').toString().trim() || null |
| L2 | "Confirm the inferred repository" has no command to run; both improvisations are wrong | commands/specs.md:266-296, commands/tickets.md:310-339 |
Print gh repo view --json nameWithOwner first, then run with no --repo; never --repo "null" |
| L3 | open-pr's stage-list source command has no failure path, and re-derives already-committed files |
unic-dlc-build.yaml:490-503 |
Name the unstaged git status --porcelain entries as the source; use origin/<expected_base>; never stage an empty list silently |
| L4 | gh --repo "<ref>" is not a runnable shape — --repo is a subcommand flag |
unic-dlc-build.yaml:364 |
gh issue view --repo "<ref>" |
| L5 | <ARTIFACTS_DIR> and $ARTIFACTS_DIR differ by one character and mean opposite things |
unic-dlc-build.yaml:494-507 |
Rename the config-derived constant to SESSION_DIR (already introduced at :481, never used) |
| L6 | "Anchors on the id as a whole line" describes an anchor the regex does not have | …test.mjs:45-56, CHANGELOG.md:30 |
Say "terminated by a newline" |
| L7 | AGENTS.md's "every PR-touching node pins" overstates; commands/cleanup.md:193 is unpinned and untested |
AGENTS.md:72, commands/cleanup.md:193 |
Scope to the six covered Boxes, name /cleanup as the open one, file a follow-up |
| L8 | The trigger_rule rationale sits outside the node's comment box and cites two nodes from another file |
unic-dlc-pr-review.yaml:128-132 |
Move inside the box; qualify verify-pr-base / merge as unic-dlc-qa.yaml |
| L9 | "Both gate paths" is asserted file-globally, so the stage-only path can lose its staging rule |
…test.mjs:241-272 |
Slice at the stage-only heading and assert per section, as the sibling test already does |
| L10 | The fixed nodeSource was not backported; two divergent copies now exist |
…test.mjs:54-57 vs archon-box-methods.test.mjs:70-72 |
Extract into test/helpers/workflows.mjs — three call sites, and M4's parse helper wants the same home |
| L11 | CONTEXT.md gains no vocabulary entry for repository pinning |
CONTEXT.md |
One entry following the file's existing shape, with an _Avoid_: line |
✅ What's good
- The staging fix is complete and provable. Zero line-anchored blind adds remain anywhere in the plugin tree; every remaining mention is prose that forbids the verb, and
BLIND_ADDis deliberately position-based so the forbidding prose does not trip it. build-state.jsonis handled exactly as ADR-0012 needs — and the loop test asserts both phases say they do not commit it, rather than asserting an absence a blind add would satisfy. The comment says why. That instinct is exactly what H6 asks for on the verb set.- F1 was found by review, fixed, and shipped with a regression test verified red before green. The
all_donefix is right and it generalised. H7 narrows its reach; it does not dispute the practice. cancel:over a failingbash:check in all four guards, so a missing key reads ascancelledand does not trigger auto-resume. The test asserts the node key itself, so a rewrite to a failing check breaks the suite. The right reading of ADR-0011.- Every cancel message is actionable: the missing key, both legal shapes, the concrete consequence ("in a fork clone is the upstream parent"), the remedy, how to restart, and each Box's own stake ("/qa is the box that MERGES"). The test enforces the key name and the remedy command — a much better bar than "a message exists".
- The
bootstrapfallback contract is explicit and complete in all four Boxes, so a clean cancel cannot degrade into a schema error. warns (not cancels) on a null repo_refchecks ordering, not presence. It would catch a reordering a presence check could not.- Node-scoped pinning assertions replace the old whole-file check, closing the exact hole where one node loses its flag while its file still passes.
- The config loader's named throw, with a comment explaining that the default
join(undefined, …)message "says nothing about what to do next". Error handling written for the person reading the output in six months. - Every new rule states its why, at the site that needs it. The deny lists say
$ARTIFACTS_DIR"resolves OUTSIDE the repo tree and is NOT the config'sartifacts_dir" — the exact confusion the rule prevents. Deliberate duplication with a written reason, justified by ADR-0023 §5. gh api's missing--repoflag is caught and explained rather than papered over with a symmetric-looking instruction.MANDATORY_PATHS' JSDoc explains an absence — why the key is deliberately not in the list, and what would have to change. The comments that stop a well-meaning "fix" later.nodeSource's prefix-collision fix is a real bug found while writing the tests, with themerge/merge-gatecase recorded at the helper.- CRLF normalisation on read keeps the string assertions honest on Windows, where CI runs.
- The change correctly avoids a new ADR — it implements ADR-0011, 0012, 0016 and 0023 rather than deciding anything new.
- Housekeeping is clean:
0.15.1in all three manifests,## [0.15.1] — 2026-08-05, conventional commits with package scope, basedevelop,MERGEABLE/CLEAN, 9/9 CI green, no new runtime deps, noLICENSEtouched.
📋 Suggested follow-up issues
| Title | Priority | Related |
|---|---|---|
project.repo_ref duplicates tracker.coords — collapse to one primitive |
P2 | H3 |
Pin /cleanup and /triage PR commands to project.repo_ref, with test coverage |
P2 | L7 |
Extract the shared workflow test helpers into test/helpers/workflows.mjs |
P3 | L10, M4 |
Record build-state.json's per-phase committed file list, and stage open-pr from it |
P3 | L3 |
Add a repo pinning vocabulary entry to the plugin CONTEXT.md |
P3 | L11 |
Next Steps
- Decide the
project.repo_refquestion. H1 and H3 both unblock from it; M1 and M8 shift with it. - Auto-fix the six mechanical HIGH findings: H2, H4, H5, H6, H7, H8.
- Re-run the mutation set. M1, M2, M3, M4, M5, M7 and M8 must all go red. That is the acceptance test for H6, H7, H8, M3, M4 and M5 — nothing else proves those fixes landed.
- Review the twelve MEDIUM findings and decide: fix now, create an issue, or skip. Ten are recommended fix-now at LOW effort.
- Re-run
pnpm --filter unic-archon-dlc test,pnpm typecheck,pnpm ci:check, and confirm CI stays 9/9 before merge.
Reviewed by Archon comprehensive-pr-review workflow
Artifacts: artifacts/runs/beb9b30606f43d287c6af1020ef87086/review/
…review All eight HIGH findings from the five-agent review. H1 and H3 were one design question — whether `project.repo_ref` survives — resolved under the review's stated recommendation: keep the key, define ado's form as the bare repository name. Reversible; the alternative (derive from `tracker.coords`) is filed as a follow-up. - H1 `/setup` now asks for `project.repo_ref`, suggesting a value parsed from GIT_REMOTE. README documents the key. The guard cancelled all four Boxes for every pre-0.15.1 Consumer while the remedy it named could not write the key, so the CHANGELOG entry moves to `### Breaking` with an upgrade step. - H2 `open-pr` commits and pushes what it stages, and proves build-state.json reached the commit. It staged, proved, then opened a PR — `gh pr create` exits 0 on the loop's last commit, so ADR-0012's proof died with the worktree while the node reported success. - H3 ado's `repo_ref` is the bare repository NAME, not "PROJECT/REPO": `az repos --repository` takes a name or ID with `--project` separate, and every prompt forbids splitting the value. Corrected at all 11 sites. - H4 A pinned call that errors STOPS and reports. The shortest retry an autonomous node finds is to drop the flag, which restores #289 with the guard still green. - H5 The null-REPO_REF check moves to Step 1 in both command docs, where the config is read. It sat in Step 10, after Step 9 had already filed one issue per slice against the inferred repository. Step 9 names the pin inline, and "confirm the inferred repository" now has a command to run. - H6 Staging rules and the BLIND_ADD barrier cover all six spellings. `git commit -am` stages every tracked modification without calling `git add`. - H7 The all_done regression test parses `depends_on` instead of string-matching the inline form only, and asserts non-vacuity — it found one node across four Boxes, so three of its four assertions were silent no-ops. - H8 A new test binds each bootstrap's output contract to the guard that reads it, and the guard also fires on a present-but-empty value. Incidental, in blocks rewritten for the above: the `gh api` host-segment split (HOST/OWNER/REPO 404s in the path), the `--repo`-on-`gh api` contradiction, the `gh --repo` top-level-flag shape, the `synthesize` join precedent, the nodeSource anchor claim, and AGENTS.md's overstated invariant. Mutation-verified: M1, M2, M3, M5 and M8 all previously survived a green 21/21 run and now fail. M4 and M7 still survive — they map to MEDIUM findings left for the maintainer. 1817 tests pass; typecheck, ci:check and verify:changelog clean. Review artifacts: artifacts/runs/beb9b30606f43d287c6af1020ef87086/review/
⚡ Auto-Fix ReportStatus: Fixes Applied
What was fixed
Tests
🧪 Mutation verification — the real acceptance testA green suite proves nothing here; these are the mutations that previously shipped past 21/21.
M3's first fix did not work — my field-extraction regex stopped at the first
|
| # | Issue | Options |
|---|---|---|
| M2 | /explore files an ado work item with az repos --repository |
Fix now / Create issue / Skip |
| M3 | Deny-list test pins 2 of 4 committing nodes — M7 still survives | Fix now / Create issue / Skip |
| M5 | Pin assertions check the flag, not the value — M4 still survives | Fix now / Create issue / Skip |
| M6 | az repos pr thread is not an az subcommand |
Fix now / Create issue / Skip |
| M7 | /qa's guard comment claims an ordering the DAG does not give |
Fix now / Create issue / Skip |
| M8 | repo_ref checked for presence, never shape — a typo costs a full run |
Fix now / Create issue / Skip |
| M10 | /build's guard comment names one node; the test names two |
Fix now / Create issue / Skip |
| M11 | README pipelines + 4 per-Box docs describe a DAG that no longer exists | Fix now / Create issue / Skip |
M6 is the one I would take next — two lines of prose, the correct form is already proven in unic-pr-review, and until it lands /pr-review's post cannot post on ado at all.
M4 (gh api path) and M9 (synthesize citation) are done. M12 is partly covered by the new README row. Of the 11 LOW findings, L1, L2, L4, L6 and L7 are done; six remain open.
📋 Suggested Follow-up Issues
project.repo_refduplicatestracker.coords— collapse to one primitive (P2) — the open half of the H3 assumptionaz repos pr threadis not a real subcommand (P2) — ADO threads needaz devops invoke- Pin
/cleanupand/triagePR commands, with test coverage (P2) — the gapAGENTS.mdnow names - Derive the deny-list and pin-value test node sets instead of hardcoding them (P2) — kills M4 + M7
- Validate
project.repo_ref's shape invalidateConfigand each bootstrap prompt (P3) - Extract the shared workflow test helpers into
test/helpers/workflows.mjs(P3) - Bring README node pipelines and the four per-Box docs back in step with the YAMLs (P3)
Validation
| Check | Command | Result |
|---|---|---|
| Plugin tests | pnpm --filter unic-archon-dlc test |
✅ 234/234, exit 0 |
| Monorepo tests | pnpm test |
✅ 1817/1817, 0 fail, exit 0 |
| Type check | pnpm typecheck |
✅ exit 0 |
| Lint + format | pnpm ci:check |
✅ exit 0 |
| Changelog | verify:changelog |
✅ exit 0 |
The 5 Biome infos in ci:check are pre-existing in pr-review / unic-pr-review and none are in changed files. This repo has no build step — tsc --checkJs --noEmit via pnpm typecheck is the whole compilation story.
Auto-fixed by Archon comprehensive-pr-review workflow
Fixes pushed to archon/task-feature-unic-archon-dlc-289-stage-explicitly-pin-r as c0fddbc
Full report: artifacts/runs/beb9b30606f43d287c6af1020ef87086/review/fix-report.md
1 participant
Fixes #289.
Why
Archon 0.7.0 removed
git add -Afrom its own bundled command defaults and pinned everyghcall to the origin repository (coleam00/Archon#2361, issue #2229). Both defects were live in this plugin's Boxes.Blind staging.
/explore'spreserve-spikerangit add -A;/build'sopen-prsaid "stage everything changed by the build". A Box runs in an isolated worktree that also carries the operator's copied.archon/directory, scratch files from earlier nodes, and abuild-state.jsonthe loop rewrites on every one of up to 60 iterations — so "everything changed" is not the change, and a blind add lands scratch and PR-body artefacts in the PR.Unpinned PR commands. No
ghorazinvocation passed a repository. In a fork clone the CLI resolves to the upstream parent, so a PR opened, a review posted, or a defect filed landed in the wrong repository./pr-reviewis the sharpest case: it holds the only review-posting authority in the lifecycle, so an inferred repository meant a whole review — summary comment and every inline thread — on a stranger's PR.What changed
unic-dlc-explore.yamlpreserve-spikestages named paths, then proves it withgit status --porcelain;spike-ticketpins the tracker callunic-dlc-build.yamlopen-prstages an explicit list (source/tests,PRD.md,issues.json,report.md,build-state.json, each new ADR by name) and pins the PR; both loop phases stage by name and refusebuild-state.jsonunic-dlc-qa.yamlverify-pr-base,mergeand UAT finding-capture pin the repositoryunic-dlc-pr-review.yamlprepandpostpin every read and every writecommands/specs.md·commands/tickets.mdgh pr create;specs.mdstops staging the wholedocs/adr/directorylib/config-schema.mjsproject.repo_ref, defaulting tonullguard-no-repo-refcancel nodebuild-state.jsonis now committed once, atopen-pr. It is the proof of ADR-0012's anti-cheat claim —red_exit,red_unexpected_pass, the per-slice phase order — and until now that proof died with the worktree when/cleanuppruned it.Both rules are stated inline in each prompt, not only as doctrine in
AGENTS.md: a Box node is self-contained and imports nothing from the Plugin (ADR-0023 §5), so a doctrine document is invisible to a running node.Two decisions worth reviewing
project.repo_refstays optional. It is not inMANDATORY_PATHSand/setupdoes not demand it. Promoting it belongs with the 0.7.0 adoption (AC 7 puts that out of scope), and a mandatory leaf would invalidate every existing Consumer config on upgrade. Absence is handled by aguard-no-repo-refnode that cancels rather than fails — an expected precondition failure (ADR-0011). The two command Boxes have a live human, so they warn and ask rather than cancel.The Azure DevOps flag follows the subcommand.
az repos pr createandaz repos pr listtake--repository;az repos pr showandaz repos pr updateidentify the PR by--id.gh apihas no--repoflag at all, so the reference is written into the API path instead of left togh's{owner}/{repo}placeholders, which resolve from the checkout. Each prompt names the form its own call needs; no host is hardcoded (ADR-0016).Verification
pnpm --filter unic-archon-dlc testpnpm --filter unic-archon-dlc typecheckpnpm --filter unic-archon-dlc verify:changelogpnpm ci:checkunic-pr-review, unrelated)New
test/box-staging-and-repo-pinning.test.mjsholds every Box to both rules. The blind-add check matches the verb as an invocation (at line start), not as a substring — every prompt has to namegit add -Ain order to forbid it, so a substring check would fail on exactly the rule that makes the file correct.🤖 Generated with Claude Code