handle-agy-tasks.sh lands the branch into the main clone for every repo — landing should be UI-repos-only - #687
Conversation
JoshuaVSherman
left a comment
There was a problem hiding this comment.
PR Review Summary
🛑 Changes Requested
Reviewed as the cross-model reviewer (Sonnet reviewing a Flash-High-authored PR) per skills/pr-review. The implementation correctly satisfies nearly every acceptance criterion in web-jam-tools#686 "handle-agy-tasks.sh lands the branch into the main clone for every repo — landing should be UI-repos-only", but the header-comment edit clobbers a pre-existing, unrelated sentence and leaves a broken/nonsensical paragraph behind — that must be fixed before merge.
🛑 Must Fix Items
-
🛑
scripts/handle-agy-tasks.shheader comment (around line 104) — deleted unrelated content and left a garbled sentence. The pre-existing paragraph read:...is enforced via the prompt instruction only — the design explicitly rejects a new create-draft-pr.sh guard for this (a brittle "is this ordered human steps?" regex would false-positive-reject).This PR deletes the second half of that sentence (
the design explicitly rejects a new create-draft-pr.sh guard...) — content unrelated to #686 and still true/relevant to the local-testing-recipe feature it documents — and grafts the newweb-jam-tools#686 — repo-conditional landing:block directly onto the dangling clause, producing:...is enforced via the prompt instruction only — the web-jam-tools#686 — repo-conditional landing: ...This is both a content-loss bug (an unrelated, still-valid design-rationale sentence about
create-draft-pr.shformat enforcement is gone with no replacement) and a broken/nonsensical sentence fragment left in the file. Fix: restore the original sentence intact, and add the new#686landing-behavior comment block as its own new paragraph (with a blank#line or clear separation) rather than splicing it into the middle of the existing one.
Checklist Verification
- Mergeability: ✅
mergeable: MERGEABLE, no conflicts withdev. (mergeStateStatus: BLOCKEDreflects the draft/no-review gate, not a conflict.) - CircleCI: ✅
ci/circleci: gate— SUCCESS (https://circleci.com/gh/WebJamApps/web-jam-tools/853). - Snyk: ✅ No separate Snyk check surfaced on this PR; nothing failing.
- Scope: ✅ Touches only
scripts/handle-agy-tasks.sh,test/handle_agy_tasks_no_land.test.ts,deno.json— matches the issue's declared file list exactly. - Acceptance criteria vs. web-jam-tools#686:
- ✅
UI_REPOS=("JaMmusic" "CollegeLutheran" "AppersonAuto" "TimShermanMusic" "HenricksonForSalem")— exact match to spec. - ✅ Non-UI repos never land, including under
--headless— theis_ui_repogate (script line ~941) runs unconditionally before theHEADLESS/prompt branch is ever reached, so headless can't bypass it. - ✅ Non-UI default path prints the worktree path and does not call
git worktree remove— confirmed by reading the landing block;git worktree removeonly happens inside the laterDO_LANDbranch, which the non-UI early-exit never reaches. - ✅ UI-repo path is provably unchanged:
is_ui_repotrue skips the new gate entirely, falling through to the original interactive Y/n prompt,--headlessauto-land (NEEDS-CONFIRMmarker), and the pre-existing dirty-main-clone refusal. - ✅
--landforces landing on a non-UI repo;NO_LANDis checked before the UI-repo gate, so--no-landsuppresses landing on both UI and non-UI repos, and passing both flags together suppresses (no-land wins) regardless of repo type — matches spec. - 🛑 Header comment updated but garbled (see Must Fix above); usage/help string (
--landadded,--no-landdocumented) is correct and clean. - ✅
test/handle_agy_tasks_no_land.test.tsgains exactly the three required cases (non-UI default skip, UI default land,--landforces non-UI land) plus two extra combinations (--no-landon UI, both flags on both repo types) — good coverage, no gaps found.- 🟡 No red/green evidence pasted in the PR body — the issue's AC explicitly asks for each new test to be "verified to FAIL against the unmodified script before the fix, then pass after." The PR's "Test evidence" section only shows post-fix passing runs. (Per
skills/pr-review's own Test Plan Integrity policy, a missing/thin test-evidence paste is not itself a defect, so this is not a blocker — flagging per the issue's specific AC wording, not as a generic evidence nit.)
- 🟡 No red/green evidence pasted in the PR body — the issue's AC explicitly asks for each new test to be "verified to FAIL against the unmodified script before the fix, then pass after." The PR's "Test evidence" section only shows post-fix passing runs. (Per
- ✅ Single version bump,
deno.json1.30.57→1.30.58, strictly ahead oforigin/dev's current1.30.57. - ✅ (duplicate of Scope above) — no stray files.
- ✅ AGENTS.md frontend guardrails (TS
any, required-asterisk wrap, mobile floating buttons, mobile OAuth redirect, footer logo flex, Cloudflare_headers, Mongoosesortstripping) — none apply; this is a bash script + Deno test change with no TSanyintroduced.
- ✅
- Package-lock engine alignment: N/A — no
engines/Node version change in this PR. - Test evidence integrity: ✅ Pasted
deno task fmt:check/lint/check/ targeted-suite / full-suite (1312 passed) /coverage:check(91.5% ≥ 90%) outputs look internally consistent with the diff; no fabrication signals.
🟡 Actionable Feedback & Suggestions
- 🟡 See the red/green test-evidence note above (item 7) — not a merge blocker per the skill's test-evidence policy, but worth a one-line note in the PR body if convenient.
- 🟡
scripts/handle-agy-tasks.shline ~104: once the header comment is fixed, double check the paragraph still reads coherently end-to-end (the original sentence + the new#686block as a clearly separated addition).
🤖 Cross-model review by Claude Code — Sonnet 5, per skills/pr-review.
JoshuaVSherman
left a comment
There was a problem hiding this comment.
PR Review Summary
✅ Approved
Re-review of the delta since the prior automated review (commit a4fb320 → head a2a529a). This evaluates only the new commit "docs: restore header comment rationale and fold /learn updates into AGENTS.md," which was pushed specifically to fix the sole Must Fix item from the previous review.
🛑 Must Fix Items
✅ None
The previously-flagged garbled header comment in scripts/handle-agy-tasks.sh (around line 104) is now fixed correctly:
- The original pre-existing sentence (
...is enforced via the prompt instruction only — the design explicitly rejects a new create-draft-pr.sh guard for this (a brittle "is this ordered human steps?" regex would false-positive-reject).) is restored intact and unmodified. - The new
web-jam-tools#686 — repo-conditional landing:block is added as its own clearly separated paragraph (blank#line, then a distinct bulleted block), not spliced into the middle of the existing sentence. - The paragraph now reads coherently end-to-end.
Checklist Verification
- Mergeability: ✅
mergeable: MERGEABLE, no conflicts withdev. (mergeStateStatus: BLOCKEDreflects the outstanding approving-review gate, not a conflict — the prior automated review wasCOMMENTED, notAPPROVED.) - CircleCI: ✅
ci/circleci: gate— PASS on the new commit (https://circleci.com/gh/WebJamApps/web-jam-tools/854). - Snyk: ✅ No separate Snyk check surfaced on this PR; nothing failing.
- Scope: ✅ This commit touches
scripts/handle-agy-tasks.sh(comment-only fix),AGENTS.md(new/learn-sourced lesson, explicitly exempt from scope review perskills/pr-review), andtest/handle_agy_tasks_no_land.test.ts/deno.jsonare unchanged in this delta (verified against the fetched diff — the diff shown reflects the full PR, and the test/version changes were already reviewed and approved-of-content in the prior pass). - AGENTS.md addition: ✅ "Header & Inline Comment Integrity" lesson is accurate, does not duplicate or contradict any adjacent entry, and is properly scoped as a
/learnguardrail exempt from the issue-scope check. - Semver: ✅
deno.jsonremains1.30.58, strictly ahead oforigin/dev's current1.30.57; this follow-up commit correctly does NOT re-bump sincedevdid not move. - Package-lock engine alignment: N/A — no
engines/Node version change in this PR. - Test evidence: ✅ No new test-evidence claims in this delta; prior evidence already verified consistent with the diff.
- Prior 🟡 note (red/green test evidence): Not addressed in this commit and remains non-blocking per the skill's Test Plan Integrity policy — carried forward as a suggestion only, not a defect.
🟡 Actionable Feedback & Suggestions
✅ None
🤖 Cross-model review by Claude Code — Sonnet 5, per skills/pr-review.
Summary
What changed
scripts/handle-agy-tasks.shconditional on the target repo being a UI repo.UI_REPOSlist inscripts/handle-agy-tasks.sh:JaMmusic,CollegeLutheran,AppersonAuto,TimShermanMusic,HenricksonForSalem.web-jam-tools,web-jam-back,WebJamSocketCluster) now skip landing by default, leaving the worktree in place at/tmp/agy-worktrees/<Repo>-<branch>and printing its path.--landflag to force landing on non-UI repos;--no-landcontinues to suppress landing on UI repos and takes precedence if both flags are passed.--landflag.test/handle_agy_tasks_no_land.test.tswith test cases covering non-UI default skipping, non-UI with--land, UI repo default landing, UI repo with--no-land, and both flags passed.deno.jsonto 1.30.58.Closes #686
How to test locally
Test Plan
Run all commands from
/home/joshua/WebJamApps/web-jam-tools:deno task lint && deno task checkdeno test --allow-env --allow-run --allow-read --allow-write test/handle_agy_tasks_no_land.test.ts--landchecks out branch into main clone.--no-landskips checkout into main clone.--landand--no-landskips checkout (--no-landwins).--landand--no-landskips checkout (--no-landwins).deno task testTest evidence
Test Evidence
deno task fmt:check:deno task lint && deno task check:deno test --allow-env --allow-run --allow-read --allow-write test/handle_agy_tasks_no_land.test.ts:deno task test:deno task coverage:check:🤖 Work by Antigravity — Gemini Flash (High)