fix: accept AGENTS.md or legacy CLAUDE.md in the pre-claim preflight check #495
No reviewers
Labels
No labels
Agent
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
1 participant
Notifications
Total time spent: 6 minutes 59 seconds
Due date
igor
6 minutes 59 seconds
No due date set.
Dependencies
No dependencies set
Reference
joshtronic/igor!495
Loading…
Reference in a new issue
No description provided.
Delete branch "agent/493-fix-pre-claim-context-file-check-must-accept"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What this PR does
rc_context_file_exists_at(lib/repo-checks.sh) so tick.sh's preflight and its tests share one implementationAGENTS.mdfirst,CLAUDE.mdas the legacy fallbackbin/site-work-block.sh's prompt feed (prefers the repo'sAGENTS.mddossier, falls back toCLAUDE.md)bin/tick.sh, which also namesCLAUDE.mdspecificallyAGENTS.md-> claimable, repo with onlyCLAUDE.md-> claimable, both present -> claimable, repo with neither -> blocked, plus a check that the block message wording is AGENTS.md-firstDeliverable 2 sweep report
grep -n "CLAUDE.md" bin/tick.sh bin/site-work-block.shafter this change:Left as-is:
bin/tick.sh:381andbin/tick.sh:4647are comments documenting that Claude Code's own CLI auto-loads a repo's context file from the worktree root -- that auto-load is Claude Code's behavior, not this harness's code, so it already picks up whichever filename is present with no change needed here.Out of scope per the issue: the worker-contract skill's prose (Distillery-side), validation's advisory
check_claude_md(lib/repo-checks.sh:70, still gates only the advisory checklist, untouched), and un-blockingjoshtronic/distillery#1itself.Test plan
bin/test-repo-checks.sh-- new cases: AGENTS.md-only, CLAUDE.md-only, both, neither, plus the block-message-wording assertionmake testpasses (full suite, including the new cases)make lintpasses (shellcheck+mdl)Closes #493
🤖 Review —
REQUEST_CHANGES(automated)CI for
95b98277: successSmall, well-scoped change: preflight now accepts root
AGENTS.mdor legacyCLAUDE.mdvia a newrc_context_file_exists_atinlib/repo-checks.sh, with matching message/prompt updates and four new unit cases. The logic itself reads correct and the sweep report matches the diff. One thing blocks me.Blocking
bin/tick.sh:4598callsrc_context_file_exists_at, but nothing in this diff showsbin/tick.shsourcinglib/repo-checks.sh. If it doesn't already source it, the call returns 127,if ! ...swallows it (noset -etrip in a condition), and every repo gets blocked at preflight — a total work stoppage that no test in this PR would catch. CI is green, butmake test→check-sync.shonly runsbin/test-*.sh; the new cases exercise the library function directly and the fifth case onlygreps tick.sh's message string. Nothing executes tick.sh's preflight path.. "$AGENT_HOME/lib/repo-checks.sh"line number in tick.sh (if it's already there, that resolves this outright), or add it; and ideally add a check that tick.sh actually sources the lib it now depends on, so the wiring — not just the wording — is covered.rc_local_init/_RC_REF(per its own doc comment), which is a hint that repo-checks.sh's other consumers are the validation path, not tick.sh. That's what makes the sourcing worth proving rather than assuming.Non-blocking notes
bin/site-work-block.sh:186-192: the header now says "No harness worker-contract AGENTS.md -- non-issue-work surface" while the code reads$WORKTREE/AGENTS.mdas "repo conventions". For any repo where AGENTS.md is a worker-contract-style document (igor itself, percheck-sync.sh's fallback), this surface would now ingest exactly what the comment says it excludes. Probably fine for the site repo, but worth a sentence confirming which worktrees this runs against.REPO_CLAUDE_MDis now a misnomer (it may hold AGENTS.md). Renaming would be a drive-by, so leaving it is the right call — just flagging it for the follow-up ticket if one exists.grep -qF '\AGENTS.md` (or legacy ...'`). It matches the escaped backticks in tick.sh:4599 as written, but it's a brittle wording test — it will break on any innocuous rephrasing while still not covering the branch's behavior.new_fixture/commit_fixture/ok/noexist with the assumed semantics and default branchmaster(CI green is decent evidence they do), and the tick.sh source line above.Checklist vs. diff: every checked item corresponds to real changes, and the deliverable-2 sweep report matches what's in
bin/tick.sh/bin/site-work-block.sh. No scope creep, no comment-contract issues, no security surface touched.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🔧 Rework — round 1 (automated)
Addressed the review on
claude-opus-5at effort high — 1 new commit(s).🧑⚖️ Rework — findings dismissed (automated)
Dismissed: the blocking finding (tick.sh doesn't source
lib/repo-checks.sh)It already sources it, and has since long before this PR -- which is why the
line isn't in the diff.
bin/tick.sh:89-90:grep -n "repo-checks.sh" bin/tick.shreturns exactly those two lines. Thesource block sits at the top of the file, above every function definition and
~4500 lines above the preflight, so the function is defined by the time the
preflight runs. No 127, no fleet-wide block.
The reviewer's own escape hatch ("if it's already there, that resolves this
outright") applies. I did take the second half of the suggestion, though: the
wiring is now covered by tests rather than assumed.
bin/test-repo-checks.shasserts both halves -- that tick.sh carries the source line, and that the
sourced lib actually defines
rc_context_file_exists_at. A future edit thatdrops either one now fails
make test.On the doc-comment hint: the helper bypasses
rc_local_init/_RC_REFbecausetick.sh's preflight reads a DIFFERENT clone (the work clone at
origin/$PR_BASE) than the validation path's clone, not because tick.sh is astranger to the lib. tick.sh already calls
validate_repo_localand friendsfrom the same file.
Fixed (from the non-blocking notes)
grep -qFon the full literal sentenceto
grep -qE 'AGENTS\.md.*legacy.*CLAUDE\.md.*missing at the repo root', soan innocuous rephrasing doesn't fail it while the AGENTS-first-with-legacy-
fallback intent still is asserted.
bin/site-work-block.shheader ambiguity: the "No harness worker-contractAGENTS.md" line now says explicitly that it means THIS repo's worker contract,
and that the worktree is always carved from
WEBSITE_REPO. That worktreecomes from
$WEBSITE_PATH'sorigin/master(site-work-block.sh:178), sothe only AGENTS.md it can ever read is the website repo's own conventions
dossier -- never igor's.
REPO_CLAUDE_MDmisnomer: renamed toREPO_CONTEXT_MD. The reviewer calledleaving it the right call, but this PR is what made the name wrong, the
variable is local to one script with four references, and three of the four
lines were already mine.
The rest of the findings were addressed in the commits on this branch. The reviewer will re-review the new head.
🤖 Review —
APPROVE(automated)CI for
d3b49024: successSmall, focused fix: the pre-claim preflight now accepts root
AGENTS.mdor legacyCLAUDE.mdvia a newrc_context_file_exists_athelper, with the same either/or applied tosite-work-block.sh's prompt feed and the vacuous-test block message. Scope matches issue #493's three deliverables exactly; CI is green.What I checked
lib/repo-checks.sh:65-77, preflight rewritebin/tick.sh:4583-4601, prompt feedbin/site-work-block.sh:186-193, vacuous-test messagebin/tick.sh:4917, five new test cases). No fabricated items.bin/test-repo-checks.shnow greps for the exact source line intick.shand assertscommand -v rc_context_file_exists_at. Green CI means both matched, so the "127 blocks every repo" failure mode is now covered rather than assumed. Point dropped.rc_context_file_exists_atis a conventional helper doc comment plus a genuine why (path/ref taken explicitly because tick's preflight reads a different clone thanrc_local_init's). Thetick.shpreflight comment edits are why-comments on an existing block, not narration. No violations.REPO_CLAUDE_MD→REPO_CONTEXT_MDrename is a drive-by in the strict sense, but this PR is what made the old name wrong and it's confined to the three lines the change already owns. Acceptable.cat-file -eagainstorigin/$PR_BASE(not the working tree) is preserved; the short-circuit||gives the intended AGENTS-first-else-legacy semantics; the "neither" case still blocks with the reworded message. No widening of privilege, no CI-config changes, no security surface.Non-blocking notes
bin/site-work-block.shhas no test coverage, so the rename's completeness rests onshellcheck(SC2154 would fire on a stray${REPO_CLAUDE_MD});make lintis reported green and CI agrees, but I couldn't confirm by reading — the grep in the PR body wouldn't have caught a leftover either, sinceCLAUDE.mddoesn't matchREPO_CLAUDE_MD.master; the positive assertions passing in CI proves that, but a future fixture-helper change tomainwould silently turnno "neither -> absent"vacuous. Not worth churn now.check_claude_md(lib/repo-checks.sh:70, untouched) truly only feeds the advisory checklist rather than gating discovery — if it gated eligibility, distillery#1 still wouldn't be claimable after this merges. The issue explicitly scopes it out and the author reports it as advisory, so I'm taking that at face value; flagging only so the human knows it's the one link in the chain neither of us confirmed.AGENTS\.md.*legacy.*CLAUDE\.md.*missing at the repo root) rather than a literal-sentence match — good call, it asserts intent without breaking on rephrasing.Nothing blocking. Approving.
Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.