fix(fathom): close six contract defects in the forge, tracker, and memory adapters - #9
Merged
Merged
Conversation
…mory adapters All six shipped in 1.1.0 and were deferred from the rename PR as pre-existing. None is a regression from that rename. Merge-closer branch matching (trackers/asana.md, trackers/linear.md). The templates searched every record under .fathom/ with an unanchored grep -rlF for the branch name. A plan document that mentions another branch in prose produced a false second match, so the workflow took its refusing-to-guess path and exited 1, closing nothing; a bare substring also made feat/tes-5 match feat/tes-50. Matching now requires a labeled Branch line whose value equals the branch exactly, with the branch regex-escaped before use. Records written before that line was required still resolve through the original search, which now logs the files it matched so a wrong pick is visible. One issue legitimately has two records, the plan document and, in checklist mode, the task file, both named <ISSUE-REF>.md. The ambiguity check therefore collapses on the file name; counting files would have refused every checklist-mode merge. Selection prefers .fathom/tasks/ by anchored prefix, so a nested .fathom/.workbench/tasks/ left by a botched rename cannot sort ahead of the live record. Every grep in the block that can legitimately match nothing now ends in || true. Actions' default shell is bash -e with pipefail off, but a consumer who adds shell: bash gets pipefail, where a no-match grep would abort the job instead of taking the skip path. Plan document labels (conventions.md). The plan document is the only record the merge-closer has on a beads-backed issue, so its Issue, Ref, and Branch labels are now pinned exactly. A natural-reading label such as Issue link or Tracker URL matches neither grep, and the workflow then exits successfully having closed nothing. Beads createTask durability (memory/beads.md). Task creation, external ref, and dependency edges were three writes that could fail apart, leaving an unidentifiable orphan that the retry could not recognize, so it created a duplicate. It is now one bd create carrying --external-ref and --deps. Verified on bd 0.49.0 that --deps produces the same bd ready exclusion as bd dep add. Forge preflight (forges/generic-git.md). verifyForge used git ls-remote --exit-code origin HEAD, which fails on a reachable remote that does not advertise HEAD and so rejected a working forge. It now asks only whether the remote answers; the branch-specific check stays in resolveBase. Adapter resolution (forges.md). forge: local with no .fathom/forge.md fell through toward a nonexistent forges/local.md. It now stops with repair instructions, as does a bundled adapter named by the profile that does not exist. Push ownership (forges.md, execute/SKILL.md, forges/TEMPLATE.md). pushesForYou was stated unscoped, but the final closing commit happens after openReview, so on such an adapter it had no defined publish path and would sit local while the review read as complete. The capability is now scoped to the review-opening push. Manual-handoff reporting (forges/generic-git.md). The handoff now states all three facts together: no review was opened, the tracker phase is still inReview, and closure is manual. Also: an installed merge-closer is a copy taken at accept time, so a repository that accepted it before a template fix would keep the old copy forever. Both adapters now compare the installed workflow against the current template on profile load and offer once to rewrite it, recording the decline so the offer is not repeated. The discovery block carries fathom:discovery-block markers so its byte-identity across the two templates is mechanically checkable. Verified by extracting each template's run: script from its YAML and running it against fixtures under GNU grep 3.12: 32 cases, both templates, under bash -e and bash -eo pipefail, all passing. Covers checklist mode, beads mode, genuine two-issue ambiguity, the prefix collision, the legacy fallback, the stale .workbench copy, and an unknown branch. shellcheck clean on both. Minor bump: behavior changes and a newly required plan-document field, all backward compatible.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates the Fathom plugin to version 2.1.0 and refines plan metadata, merge-closer workflow updates, tracker file discovery, forge lifecycle guidance, manual handoffs, and Beads task creation. ChangesFathom workflow updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MergeCloser
participant InstalledWorkflow
participant PlanTaskFiles
participant Tracker
MergeCloser->>InstalledWorkflow: compare discovery block
MergeCloser->>PlanTaskFiles: find labeled Branch and tracker labels
PlanTaskFiles-->>MergeCloser: resolve preferred matching file
MergeCloser->>Tracker: extract labeled tracker reference
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/fathom-shared/forges.md`:
- Around line 21-28: Update the openReview push-ownership contract in forges.md
so the adapter owns the opening push only when pushesForYou is true; when false,
the caller must push before invoking openReview. Align the generic adapter’s
openReview documentation with this conditional rule, while preserving the
separate requirement that callers push later commits.
In `@skills/fathom-shared/trackers/asana.md`:
- Around line 182-193: Update the task-file selection logic in
skills/fathom-shared/trackers/asana.md (lines 182-193) and
skills/fathom-shared/trackers/linear.md (lines 171-182) to prefer matches under
.fathom/tasks/, then .fathom/plans/, before falling back to the first sorted
match; preserve the no-match handling and grep safeguards in both templates.
- Around line 105-106: Update the parity validation covering the discovery and
ref-extraction blocks in skills/fathom-shared/trackers/asana.md:105-106 and
skills/fathom-shared/trackers/linear.md:88-89. Normalize and compare the blocks
while allowing only the marker filename, TASK_FILE/FILE, task/issue wording, and
wrong issue/wrong one differences; reject any other drift and keep both
templates synchronized.
In `@skills/fathom-shared/trackers/linear.md`:
- Line 29: Update the shared guidance in the non-GitHub forge arrangement rule
to document both skip conditions: `ciHooks: false` and any resolved forge other
than GitHub, including the `merge-closer: none (forge is not GitHub)` outcome.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9dd3413a-9e6a-4f71-b207-e4e978f69fcb
📒 Files selected for processing (13)
.claude-plugin/marketplace.json.claude-plugin/plugin.jsonREADME.mddocs/fathom.mdskills/execute/SKILL.mdskills/fathom-shared/conventions.mdskills/fathom-shared/forges.mdskills/fathom-shared/forges/TEMPLATE.mdskills/fathom-shared/forges/generic-git.mdskills/fathom-shared/forges/github.mdskills/fathom-shared/memory/beads.mdskills/fathom-shared/trackers/asana.mdskills/fathom-shared/trackers/linear.md
Four findings, all valid. Record selection could still pick a stale copy. Anchoring on .fathom/tasks/ covered the case where a live task file exists, but the final fallback took the first sorted match, and .fathom/.workbench/ sorts ahead of .fathom/plans/. A beads-backed issue in a repository left holding a nested .workbench/ from a botched rename therefore closed against the stale record. Reproduced, then fixed by preferring .fathom/plans/ before the unanchored fallback. Verified both templates now select the live record under bash -e and bash -eo pipefail. Push ownership was stated unconditionally. The contract said openReview owns the push that opens the review, while pushesForYou: false means the caller pushes first, which is what both bundled adapters declare and what execute does. An agent reading only that sentence would skip the push or perform it twice. Ownership is now stated as conditional on the capability in both directions, and generic-git.md no longer reads as though its openReview performs the push. The documented parity allowlist for the two discovery blocks listed three differences where there are four. docs/fathom.md gave ciHooks: false as the only reason the merge-closer question is skipped. linear.md also skips it on any non-GitHub forge, since both closure paths are GitHub-specific.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/fathom.md`:
- Around line 360-368: Update the closure-mechanism documentation around the
“sweep is the only mechanism” claim to account for the resolved adapter’s
reviewLookup capability. State that the sweep-only path applies only when
reviewLookup is by-id; when it is none, document that no automated closure is
available. Keep the existing GitHub forge and CI-hook conditions for the
integration and merge-closer options.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99eb6d3c-8024-46ce-a8dd-2cfc9e831016
📒 Files selected for processing (5)
docs/fathom.mdskills/fathom-shared/forges.mdskills/fathom-shared/forges/generic-git.mdskills/fathom-shared/trackers/asana.mdskills/fathom-shared/trackers/linear.md
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/fathom-shared/forges.md
- skills/fathom-shared/forges/generic-git.md
- skills/fathom-shared/trackers/linear.md
- skills/fathom-shared/trackers/asana.md
…g it always covers non-GitHub forges An adapter declaring reviewLookup: none, which includes the bundled generic-git fallback, cannot look a review up by id, so the sweep does not run there. The three closure mechanisms therefore do not cover every forge between them: where none applies there is no automated closure at all, and the doc now says that instead of leaving the sweep to imply it.
This was referenced Aug 8, 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.
Fixes all six findings CodeRabbit raised on #8. Every one shipped in 1.1.0 and was deferred there as pre-existing, so none is a regression from the rename.
The one that actually bites
The merge-closer templates found a branch's record with an unanchored
grep -rlF "$BRANCH" .fathom/. Reproduced: a plan document mentioning another branch in prose yields a false second match, the workflow takes its "refusing to guess" path,exit 1- a red check that closes nothing. A bare substring also makesfeat/tes-5matchfeat/tes-50.Matching now requires a labeled
Branchline equal to the branch exactly, with the branch regex-escaped. Legacy records still resolve through the original search, which now logs the files it matched so a wrong pick is visible.Two follow-on hazards found by testing rather than reading:
<ISSUE-REF>.md. Counting files would have refused every checklist-mode merge. The ambiguity check collapses on the file name instead; selection prefers.fathom/tasks/by anchored prefix so a nested.fathom/.workbench/tasks/from a botched rename cannot sort ahead of the live record.bash -ewith pipefail off, but a consumer addingshell: bashgets pipefail, where a no-matchgrepaborts the job instead of taking the skip path. Every grep that can legitimately match nothing now ends in|| true.The other five
- Issue:/- Ref:/- Branch:pinned exactly - it's the only record a beads-backed issue hascreateTaskbd create --external-ref --depsverifyForgels-remote --exit-code origin HEADrejected a reachable remote not advertising HEADforge: localforges/local.mdpushesForYouinReview, closure manualPlus: an installed merge-closer is a copy taken at accept time, so a repo that accepted it before a template fix keeps the old copy forever. Both adapters now compare it on profile load and offer once to rewrite, recording the decline so it isn't re-asked.
Verification
Each template's
run:script extracted from its YAML and executed against fixtures under GNU grep 3.12, as onubuntu-latest:32 cases, all passing - 8 scenarios x 2 templates x
bash -eandbash -eo pipefail. Scenarios: checklist mode (plan+task), beads mode (plan only), two different issues claiming one branch (must refuse), thetes-5/tes-50collision, legacy fallback, unknown branch, stale.workbenchcopy must lose, and a**Tracker**bold label.shellcheck -s bashclean on both extracted scriptsfathom:discovery-blockmarkers so that's mechanically checkablebd 0.49.0: verified--depsgives the samebd readyexclusion asbd dep addskills/now at zero em dashesThree review rounds; round 1 caught the checklist-mode regression, round 2 the stale-
.workbenchselection, round 3 confirmed.Note on one deferred claim
CodeRabbit's beads finding also said daemon mode drops
--external-refand--no-daemonis required. It does not reproduce onbd 0.49.0-bd infoconfirmedMode: daemonand the ref persisted through bothbd createand a separatebd update. I fixed the durability half only and did not encode the unverified claim.Breaking
None. The new
- Branch:requirement is covered for old records by the legacy fallback. Minor bump to 2.1.0.Summary by CodeRabbit
New Features
Bug Fixes