Repository navigation
task-done: one canonical body + tracker modules, deleting 7 near-identical copies (#236) - #244
Merged
Merged
Conversation
Owner
Author
Spec complianceIssue #236 is fully satisfied. Checking every deliverable from the spec:
Spec had one acknowledged accepted-change that the implementation then designed out: Code-review findingsCorrectness: CI ran and passed on head SHA Nit 1 — stale comment in This was accurate pre-#236, but the canonical Nit 2 — test count off-by-one between PR body and CHANGELOG PR body table: Observation (UX, not a bug) — double error on missing module
source "$(aw_tracker_module "$AW_ROOT" "$TRACKER")" || exit 1When the module is absent, Cleaner two-liner that avoids the double error: TRACKER_MODULE=$(aw_tracker_module "$AW_ROOT" "$TRACKER") || exit 1
source "$TRACKER_MODULE"This is a nice-to-have, not a blocker — the helpful message still reaches the user and the script still exits. Positive observation — two-step verification approach Retargeting all 67 tests at the canonical script first (1392/1392 green) before retiring the duplication turns behaviour preservation from a hope into an observation. The failure demonstration (deleting Verdictclean-with-nits Two cosmetic nits (stale comment, off-by-one in documented numbers) and one UX nicety (double error on missing module). None block merge. Fix all three in this PR if possible — they're small enough — but they don't affect correctness. |
martin-conur
added a commit
that referenced
this pull request
Oct 2, 2026
…ment, count parity (#244) Three items from the clean-with-nits review on PR #244. 1. tests/task_done_dispatcher.bats:56 said "kiro-notion's task-done prints branch/base lines", true pre-#236 and not after. The comment now says what the test pins: detection reached the notion module and the SHARED body produced those lines, there being exactly one task-done since #236. 2. The test count disagreed between the two durable records -- PR body 1380 -> 1360, CHANGELOG 1381 -> 1361. Nets agreed at -20, absolute baselines did not, which undercuts the credibility the retirement table exists for. Cause found rather than a side picked: `bats --count` and `grep -c '^@test'` differ by one because tests/radio_home_isolation.bats:105 has an @test inside a heredoc that writes a generated probe.bats fixture, so grep counts a test bats never runs. The measured count is authoritative; the CHANGELOG now states which tool produced it and shows the arithmetic. 3. The composition call site was source "$(aw_tracker_module "$AW_ROOT" "$TRACKER")" || exit 1 which reads as resolve-then-source and is not: on a missing module the helper writes its diagnosis and exits 1, but the substitution has already yielded the empty string, so `source ""` runs and bash appends `: No such file or directory` naming a line in task-done before `|| exit 1` is reached -- inviting a reader to debug the wrong file. Now two lines. The part worth recording is why the existing test missed it. `aw_tracker_module names the missing file rather than letting source fail` passed either way, because it exercised the helper directly and never went through the call site: a name promising more than it checked, so a green suite would have told the next reader something untrue. Same shape as the misnamed table row this branch caught in its own test rename. Both helper tests are renamed to say they test the helper, and two new cases run the real bin/task-done against a checkout with a module removed and refute the shell noise -- one for gh, one sweeping all four trackers. Both were verified to FAIL against the old call site before landing, which the first draft did not: it asserted `source: : not found`, the message the review described, rather than the one bash actually emits. That draft passed against the very call site it was written to reject. Counts: 1380 baseline - 32 retired + 14 added = 1362, and bats --count returns 1362. Suite exit 0, 1362 ok, 0 not ok. check-drift 25 groups. shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tical copies (#236) task-done was a three-variant file wearing seven copies. claude-gh, claude-notion, kiro-gh and kiro-notion were byte-identical; claude-local and kiro-local were byte-identical to each other; claude-jira differed by seven lines. 1525 lines expressing about 249 distinct ones, held in step by ten of the 35 groups in tools/check-drift.sh. The part that genuinely differed was the part with no sentinel on it: the `gh pr view` / `gh pr create` block sat between two drift-guarded regions and was guarded by neither, so jira's Jira-key uppercasing could have broken in one copy unnoticed. The local board-regen region was nested inside confirm-and-cleanup, and extract_region is a plain awk window, so the outer extract swallowed the inner sentinels and passed only because both local copies happened to match. There is no agent axis here at all, and that shaped the design: removing a worktree, sweeping radio state and closing a tab never depended on which agent was in the tab. So a loadout is {tracker x agent} and the modules are keyed on the axes rather than the combo -- lib/trackers/{gh,jira,notion,local}.sh and lib/agents/{claude,kiro}.sh, four files and two, not seven of each. kiro-jira (#92) reuses jira.sh x kiro.sh with zero new module files for this tool. Three hooks carry it and four of the seven loadouts override none: aw_tracker_pr_section (jira), aw_tracker_usage_steps and aw_tracker_post_cleanup (local). lib/trackers/_default.sh holds what the four byte-identical copies actually had, so gh.sh and notion.sh override nothing rather than restating an identical PR section -- otherwise seven copies would merely have become four. aw_tracker_module / aw_agent_module live in lib/detect-impl.sh beside aw_all_impls, and aw_all_trackers / aw_all_agents derive the axis names from that list rather than restating them. No behaviour changes. One candidate was designed out rather than accepted: aw_regenerate_board defaults its `self` to ${BASH_SOURCE[1]}, which from a tracker module resolves to a file with no task-board beside it, so the sibling fallback would silently find nothing. aw_tracker_post_cleanup now takes the impl and names that loadout's own task-done, keeping $PATH-first-then-sibling resolution byte-identical. The two local board tests caught it on the first run. Tests ran 1380 -> 1360: 32 duplicated cases retired, 12 added. The port was done in two steps on purpose -- retarget all 67 task_done cases at the canonical script and confirm green first, then retire -- so behaviour preservation is an observation rather than a hope. tests/loadout_modules.bats proves every module LOADS (derived from aw_all_impls, failing on a missing file, an orphan file, or a module missing a hook) and tests/task_done.bats proves the right one RAN. Both were demonstrated to fail, not assumed to. Upgrading: no task-init re-run needed. All seven installers already symlink bin/task-done and never their own copy, so no installer-written artifact changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ment, count parity (#244) Three items from the clean-with-nits review on PR #244. 1. tests/task_done_dispatcher.bats:56 said "kiro-notion's task-done prints branch/base lines", true pre-#236 and not after. The comment now says what the test pins: detection reached the notion module and the SHARED body produced those lines, there being exactly one task-done since #236. 2. The test count disagreed between the two durable records -- PR body 1380 -> 1360, CHANGELOG 1381 -> 1361. Nets agreed at -20, absolute baselines did not, which undercuts the credibility the retirement table exists for. Cause found rather than a side picked: `bats --count` and `grep -c '^@test'` differ by one because tests/radio_home_isolation.bats:105 has an @test inside a heredoc that writes a generated probe.bats fixture, so grep counts a test bats never runs. The measured count is authoritative; the CHANGELOG now states which tool produced it and shows the arithmetic. 3. The composition call site was source "$(aw_tracker_module "$AW_ROOT" "$TRACKER")" || exit 1 which reads as resolve-then-source and is not: on a missing module the helper writes its diagnosis and exits 1, but the substitution has already yielded the empty string, so `source ""` runs and bash appends `: No such file or directory` naming a line in task-done before `|| exit 1` is reached -- inviting a reader to debug the wrong file. Now two lines. The part worth recording is why the existing test missed it. `aw_tracker_module names the missing file rather than letting source fail` passed either way, because it exercised the helper directly and never went through the call site: a name promising more than it checked, so a green suite would have told the next reader something untrue. Same shape as the misnamed table row this branch caught in its own test rename. Both helper tests are renamed to say they test the helper, and two new cases run the real bin/task-done against a checkout with a module removed and refute the shell noise -- one for gh, one sweeping all four trackers. Both were verified to FAIL against the old call site before landing, which the first draft did not: it asserted `source: : not found`, the message the review described, rather than the one bash actually emits. That draft passed against the very call site it was written to reject. Counts: 1380 baseline - 32 retired + 14 added = 1362, and bats --count returns 1362. Suite exit 0, 1362 ok, 0 not ok. check-drift 25 groups. shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dy (#236) Rebase onto origin/main after #243 (#242) merged first. The two PRs overlap on nine files -- CHANGELOG.md, README.md and all 7 */bin/task-done -- and this one DELETES those seven, so a resolution that took either side whole would have lost a P0's work or this one's. Carried forward rather than resolved by deletion: 1. #243's _TAB_ID_MISS block and the second skip line, which name WHICH source came up empty and print the `grep 'tab-id:'` that finds the launch-time log line. Seven copies of that became one when task-done was inverted, so it belongs in the canonical body. 2. Both README edits: this PR's module-shape section and #243's runbook row. #243's nine runbook docs -- 7 templates, README, and this repo's own .claude/gh-workflow.md -- are otherwise untouched; verified all nine. 3. Both CHANGELOG entries: this one under Changed, #243's under Fixed. 4. A fourth item not on the carry-forward list: #243's three task-done cases in tests/tab_id_capture.bats ran through $CLAUDE_GH_TASK_DONE, a helper var this PR retires. Retargeted to `env AW_IMPL=claude-gh "$TASK_DONE"`, the same treatment the other suites got. THEIR ASSERTIONS ARE UNCHANGED AND THEY PASS AGAINST THIS SHAPE, which is the evidence the port is faithful rather than plausible. Counts move again, and the previous figures were wrong against the wrong baseline: origin/main is now 1397 because #243 added 17 tests of its own, not the 1380 this branch had been quoting. Measured by checking origin/main out into a throwaway worktree and counting it rather than inferring: 1397 baseline - 32 retired + 14 added = 1379 bats --count tests/*.bats = 1379 ./run_tests.sh exit 0 -- 1379 ok, 0 not ok The CHANGELOG now names the REF as well as the tool, since "1380" was correct against the wrong baseline -- a third variant of the count-credibility problem raised in review, after the grep-vs-bats one. check-drift 25 groups. shellcheck clean on the full CI glob. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
martin-conur
force-pushed
the
task/loadout-hook-inversion
branch
from
October 2, 2026 14:49
fd4d254 to
b8550aa
Compare
This was referenced Oct 2, 2026
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.
Closes #236. Epic #122 Phase 2, first half. Sub-issue #237 (
task-work) builds on the module loader and parity suite this adds.What changed
task-donewas a three-variant file wearing seven copies:1525 lines expressing ~249 distinct ones. It is now one canonical
bin/task-donecomposing one tracker module.task-donefilescheck-driftgroupsNet −1021 lines across 31 files.
The part that genuinely differed was the part with no sentinel on it
The
gh pr view/gh pr createblock (claude-gh/bin/task-done:87-97) is the only tracker-specific code in the file, and it sat between two drift-guarded regions while being guarded by neither — so jira's Jira-key uppercasing could have broken in one copy with nothing to say so. Separately, the localboard-regenregion was nested insideconfirm-and-cleanup, andextract_region(tools/check-drift.sh:52) is a plain awk window: the outer extract swallowed the inner sentinels and body, and passed only because both local copies happened to be identical.There is no agent axis, and that shaped the design
diff claude-gh/bin/task-done kiro-gh/bin/task-donewas empty; neither copy ever mentionedclaudeorkiro-cli. Removing a worktree, sweeping radio state and closing a tab does not depend on which agent was in the tab.So a loadout is
{tracker × agent}and the modules are keyed on the axes, not the combo —lib/trackers/{gh,jira,notion,local}.sh×lib/agents/{claude,kiro}.sh, four files and two, not seven of each. #122's body says "per-loadouthooks.sh" in one place and "{tracker module × agent module}: 4 + 2 small files" in another; the second is right. Per-loadout hooks would have kept the file count and kept the gh verbs duplicated betweenclaude-ghandkiro-gh.kiro-jira(#92) now reusesjira.sh × kiro.shwith zero new module files for this tool.lib/trackers/_default.shholds the behaviour the four byte-identical copies actually had, sogh.shandnotion.shoverride nothing rather than restating an identical PR section — otherwise seven copies would merely have become four.No behaviour changes — including one the spec said was unavoidable
The spec declared the
aw_regenerate_boardsibling-fallback resolution as an accepted change. It was wrong:lib/board-regen.sh:30defaultsselfto${BASH_SOURCE[1]}, which from a tracker module resolves to a file with notask-boardbeside it, so the sibling fallback silently finds nothing. The two local board tests failed on the first run.aw_tracker_post_cleanupnow takes the impl and names that loadout's owntask-done, keeping$PATH-first-then-sibling resolution byte-identical. #238 moves that line when it consolidates the twotask-boardcopies.Everything else is preserved byte-for-byte: jira's uppercasing, local's five-step usage text, every message and exit code.
How the port was verified
Two steps, on purpose. First retarget all 67
task_donecases at the canonical script withAW_IMPLand confirm 1392/1392 green — proving the port behaviour-preserving while the duplication was still there. Only then retire. So behaviour preservation is an observation, not a hope.tests/loadout_modules.bats(14 new cases) proves every module LOADS. Derived fromaw_all_impls, never a second copy of the list. It fails on a missing file, on an orphan file, and on a module that loads but forgot a hook.tests/task_done.batsproves the right one RAN. That is #206's lesson restated — a region can be byte-identical and still inert, and structural parity cannot see it.Both halves were demonstrated to fail, not assumed to:
That demonstration is the floor that makes a falling test count trustworthy; a retirement rule without it is an honour system.
Test count: 1380 → 1362
32 of the 67
task_donecases asserted shared-body behaviour two or three times over, once per loadout. Each was retired only where an identical assertion survives. Note the raw diff shows 51 removed@testlines — 19 of those are thekiro:→shared:rename, not retirements.Retired, with surviving counterpart
claude-jira: reviewer worktree force-deletes branch (PR_NUMBER set)claude-gh: reviewer worktree force-deletes branch (PR_NUMBER set)claude-local: --remove-worktree sweeps its own radio mailboxclaude-gh: --remove-worktree sweeps its own radio mailboxclaude-local: ambient PR_NUMBER export does NOT trigger force-delete on workerclaude-gh: ambient PR_NUMBER export does NOT trigger force-delete on workerclaude-local: reviewer worktree force-deletes branch (PR_NUMBER set)claude-gh: reviewer worktree force-deletes branch (PR_NUMBER set)claude-notion: --remove-worktree --force skips all promptsshared: --remove-worktree --force skips all promptsclaude-notion: --remove-worktree alone exits 0 without reading stdinshared: --remove-worktree alone exits 0 without reading stdinclaude-notion: --remove-worktree skips PR sectionshared: --remove-worktree skips PR sectionclaude-notion: deletes .info file after removalshared: deletes .info file after removalclaude-notion: deletes local branch when fully merged (no new commits)shared: deletes local branch when fully merged (no new commits)claude-notion: fails when run from main reposhared: fails when run from main repoclaude-notion: keeps local branch when it has unmerged commitsshared: keeps local branch when it has unmerged commitsclaude-notion: no prompt when --force is setshared: no prompt when --force is setclaude-notion: reads custom BASE_BRANCH from .info fileshared: reads custom BASE_BRANCH from .info fileclaude-notion: removes worktree containing initialized submodules without warningshared: removes worktree containing initialized submodules without warningclaude-notion: removes worktree directoryshared: removes worktree directoryclaude-notion: reviewer worktree force-deletes branch (PR_NUMBER set)claude-gh: reviewer worktree force-deletes branch (PR_NUMBER set)claude-notion: shows branch and base branchshared: shows branch and base branchclaude-notion: shows commit count ahead of baseshared: shows commit count ahead of baseclaude-notion: shows existing PR URL instead of create commandshared: shows existing PR URL instead of create commandclaude-notion: shows gh pr create with correct --base when no PR existsshared: shows gh pr create with correct --base when no PR existsclaude-notion: skips zellij close-tab when no radio session (no \$ZELLIJ env)shared: skips zellij close-tab when no radio session (no \$ZELLIJ env)claude-notion: warns about uncommitted changesshared: warns about uncommitted changesjira: --remove-worktree alone exits 0 without reading stdinshared: --remove-worktree alone exits 0 without reading stdinjira: --remove-worktree skips PR sectionshared: --remove-worktree skips PR sectionjira: deletes local branch when fully merged (no new commits)shared: deletes local branch when fully merged (no new commits)jira: fails when run from main reposhared: fails when run from main repojira: keeps local branch when it has unmerged commitsshared: keeps local branch when it has unmerged commitsjira: removes worktree containing initialized submodules without warningshared: removes worktree containing initialized submodules without warningjira: shows branch and base branchshared: shows branch and base branchkiro-gh: reviewer worktree force-deletes branch (PR_NUMBER set)claude-gh: reviewer worktree force-deletes branch (PR_NUMBER set)kiro-local: reviewer worktree force-deletes branch (PR_NUMBER set)claude-gh: reviewer worktree force-deletes branch (PR_NUMBER set)kiro: (kiro-notion) reviewer worktree force-deletes branch (PR_NUMBER set)claude-gh: reviewer worktree force-deletes branch (PR_NUMBER set)Kept, because it distinguishes an impl
jira: PR title uppercases Jira key slug/jira: PR title uses raw slug for non-Jira branches— the only tracker override in the file.claude-local:/kiro-local: task-done regenerates tasks/_board.md— the local module's board +state.jsonwork.<impl>: task-done unregisters the radio session— deliberately kept at seven rows. It is the only place every impl runs the full script end to end, so it is what would catch a module that loads but never fires. A new combo must add a row.Surviving total: 35 (19
shared:+ 16 impl-specific).Green
.github/workflows/ci.ymlneeded one fix: the lint job globbedlib/*.sh, which would never have reached the new subdirectories. Both are now linted.Also in this PR
The install label in all seven installers:
task-done (shared dispatcher)is no longer true, so it reads(canonical + tracker modules)with the explanatory comment updated. Theinstall-shared-symlinksstanza is drift-guarded, so an incomplete edit would have failed CI; verified all seven identical.Upgrading: no
task-initre-run needed. All seven installers already symlink$SCRIPT_DIR/../bin/task-doneand never their own copy, so no installer-written artifact changes.🤖 Generated with Claude Code
Review round 1 (commit
fd4d254)Three items from the clean-with-nits verdict, all fixed in this PR per the tight-PR norm.
1. Stale comment (
tests/task_done_dispatcher.bats:56) — said "kiro-notion's task-done prints branch/base lines", true pre-#236 and not after. Now says what the test pins: detection reached the notion module and the shared body produced those lines.2. Test count parity. The two durable records disagreed — PR body
1380 → 1360, CHANGELOG1381 → 1361. Nets agreed at −20, absolute baselines did not. Cause found rather than a side picked:bats --countandgrep -c '^@test'differ by one, becausetests/radio_home_isolation.bats:105has an@testinside a heredoc that writes a generatedprobe.batsfixture — grep counts a test bats never runs. The measured count is authoritative; the CHANGELOG now names the tool and shows the arithmetic.3. The call-site double error — not the nit it looked like. Now two lines:
Why the existing test missed it:
aw_tracker_module names the missing file rather than letting source failpassed either way, because it exercised the helper directly and never went through the call site — a name promising more than it checked. Both helper tests are renamed to say they test the helper, and two new cases run the realbin/task-doneagainst a checkout with a module removed and refute the shell noise (one forgh, one sweeping all four trackers).Both were verified to fail against the old call site before landing — and the first draft did not. It asserted
source: : not found, the message the review described, rather than what bash actually emits:So the first draft passed against the very call site it was written to reject — the same defect the review was asking me to fix, reproduced inside the fix. Corrected, then confirmed 14/14 with the fix and tests 13–14 failing without it.
Counts after this round
One scope note: the call-site fix changes stderr on a broken checkout, so it does not widen the "no behaviour changes" claim, which is relative to pre-#236 where no module existed to be missing. That distinction is recorded in the CHANGELOG rather than glossed.