feat(fathom): add stacked reviews - #10
Merged
Merged
Conversation
Gives the reserved stackedReviews forge capability its behavior: one issue may produce a short stack of dependent reviews when its breakdown is large enough, with the split proposed at breakdown time and confirmed by the user. Single-review behavior remains the default for every repository, forge, and tier that does not opt in.
Bump plugin.json to 2.2.0 and propagate it into marketplace.json and README.md via bin/sync-versions.sh. Fix a cross-file gap found during Step 5's side-by-side reading: execute/SKILL.md step 12's final summary said "the review URL" without accounting for a stacked issue, where multiple bundle reviews exist. It now reports every bundle's review URL in order when the issue was split into a stack.
Reorders step 8 so the sub-issues are planned, the split decided, and the bundling proposal confirmed before any task is created, then creates every task with its final deps in one pass. Guards the bundle-shaping and proposal bullets so a single-review run never reaches them, and only explains a missing split in the manual tier when the breakdown would otherwise have crossed the threshold.
A stacked issue records every bundle's branch under .fathom/, so the merge-closer Action would close the whole issue on whichever bundle merged first, and the next sweep would read it as complete. A split now writes a Merge-closer: suppressed marker into the plan document, both Action templates skip an issue carrying it, and closure is left to the sweep. The cost is stated at handoff.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFathom 2.2.0 adds opt-in stacked-review planning and execution. It records bundle state, chains tasks and reviews, supports recovery and restacking, delays issue closure until all reviews merge, and updates forge and tracker contracts. ChangesStacked review support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FathomExecute
participant PlanDocument
participant ForgeAdapter
participant TrackerSweep
FathomExecute->>PlanDocument: persist bundle and branch state
FathomExecute->>ForgeAdapter: open each review with dependency data
ForgeAdapter-->>FathomExecute: return or recover review records
TrackerSweep->>ForgeAdapter: inspect every bundle review
TrackerSweep->>PlanDocument: record closure state after all reviews merge
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: 5
🧹 Nitpick comments (1)
skills/fathom-shared/forges/github.md (1)
65-67: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winClarify the scope of “Never encode the dependency a second way.”
The shared stack contract requires
Depends on <previous review url>in every noninitial review body.skills/fathom-shared/conventions.mdlines 143-149 andskills/execute/SKILL.mdlines 237-238 apply this rule to GitHub too.State that this prohibition applies only to forge-side dependency metadata, such as labels or extra API calls. Do not imply that the required prose line should be removed.
🤖 Prompt for 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. In `@skills/fathom-shared/forges/github.md` around lines 65 - 67, Clarify the “Never encode the dependency a second way” guidance to prohibit only forge-side dependency metadata, such as labels or extra API calls. Explicitly preserve the required “Depends on <previous review url>” line in every noninitial review body, including GitHub reviews, while retaining the existing base-branch dependency mechanism.
🤖 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/execute/SKILL.md`:
- Around line 232-234: Update the bundle-processing sequence so it commits
leftover changes belonging to bundle k’s tasks before reconciling closed tasks
against the bundle’s final commit range. Reuse the existing single-review commit
behavior, leave unrelated working-tree edits untouched, then perform
reconciliation and stop without opening the review if the final commits
disagree.
- Around line 237-241: Update the review creation flow around openReview and
publishReview to handle a manual-handoff result with no review id before
publishing or recording anything. Stop and report the handoff, or otherwise
prevent stack proposals for unsupported adapters; only call publishReview and
write the Bundles Review record when a valid review id is returned, preserving
dependency propagation for subsequent bundles.
In `@skills/fathom-shared/approval.md`:
- Around line 35-38: Update the bundle-split rationale in execute to remove the
inaccurate claims that it runs before any branch, costs nothing, or can be
corrected by rerunning with stacking: never. Preserve the statement that
stacking: never or an absent stacking field suppresses the split itself, and
keep the stop on the skip list if that behavior remains intended.
In `@skills/fathom-shared/forges.md`:
- Around line 38-42: The review-recovery contract must provide enough evidence
to verify an exact review. In skills/fathom-shared/forges.md lines 38-42, update
findReviewByBranch or add a details operation to return bounded candidates
containing the review id, URL, and base; in skills/execute/SKILL.md lines
253-258, reuse a candidate only when its head and base match exactly, and when
the finder is absent or metadata is incomplete, stop and hold rather than
treating the review as missing or opening a duplicate.
In `@skills/fathom-shared/trackers.md`:
- Around line 106-108: Update legacy branch-only record handling to resolve IDs
through findReviewByBranch, add each resolved ID to the issue’s review set, and
call getReviewState for it before evaluating cases 1–5. If resolution fails,
leave the issue incomplete so case 2 prevents closure.
---
Nitpick comments:
In `@skills/fathom-shared/forges/github.md`:
- Around line 65-67: Clarify the “Never encode the dependency a second way”
guidance to prohibit only forge-side dependency metadata, such as labels or
extra API calls. Explicitly preserve the required “Depends on <previous review
url>” line in every noninitial review body, including GitHub reviews, while
retaining the existing base-branch dependency mechanism.
🪄 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: 7cb060b3-24ec-4a9d-9bbd-155b2ffe04f3
📒 Files selected for processing (16)
.claude-plugin/marketplace.json.claude-plugin/plugin.jsonREADME.mddocs/plans/2026-08-07-fathom-stacked-reviews-design.mddocs/plans/2026-08-07-fathom-stacked-reviews-plan.mdskills/execute/SKILL.mdskills/fathom-shared/approval.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.mdskills/fathom-shared/trackers.mdskills/fathom-shared/trackers/asana.mdskills/fathom-shared/trackers/linear.md
The per-bundle routine reconciled bundle k's closed tasks against its commit range before committing leftover changes belonging to those tasks, so a bundle with leftover work counted a range that was still missing a commit and stopped, and where the count happened to match anyway the later leftover commit was never reconciled at all. Swap the two so the leftover commit lands first and the reconciliation counts the bundle's final range. The single-review path carried the same ordering and is swapped with it.
openReview may return the manual-handoff result, which carries no review id. The per-bundle routine passed it straight to publishReview and wrote a - Review: record from it, recording an invalid entry and leaving later bundles without a dependsOn id. Branch on the result instead: skip publishReview, write no record, and stop and report the handoff. Noted as a guard against an adapter returning the handoff from a non-manual tier, since the manual tier never proposes a split.
The entry claimed the stop fires before any branch exists, but step 7 creates bundle 1's branch before step 8 runs, and it claimed a wrong answer is corrected by re-running with stacking: never, which does not roll back a split auto mode already applied. Rewrite the rationale in terms that hold and drop the re-run claim, matching section 3 of the design document, and drop the same overclaim from that section.
Bundle review recovery required matching on the exact head and base pair, but the operation was defined to return only a review id and the GitHub adapter returned only the newest pull request number, so the requirement was unimplementable and the backfill had no URL to record. Extend the operation to return bounded candidate records carrying the review id, its URL, and its base; the caller reuses only an exact head-and-base match. State in both files what happens when an adapter omits the operation entirely.
A branch-only legacy record resolved through findReviewByBranch had its id left out of the issue's review set, so the completeness requirement and the aggregate cases could run against an incomplete set and case 3 could close an issue on the records that happened to carry ids. Add the resolved id to the set and call getReviewState on it before any ordered case is evaluated, treat a record as resolved only on an exact single candidate, and leave the issue incomplete when resolution fails or the operation is unavailable so case 2 blocks closure.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 41-46: Update skills/fathom-shared/forges.md lines 41-46 so the
review finder exposes whether its bounded results are complete, or provide an
exact head/base lookup that can prove absence. Update skills/execute/SKILL.md
lines 260-265 to open a review only after that lookup proves no matching review
exists; when the result is non-exhaustive or ambiguous, stop and hold instead of
creating a duplicate.
In `@skills/fathom-shared/trackers.md`:
- Around line 115-116: Update the unresolved legacy-record handling in the
tracker flow to persist an idempotent marker keyed by the branch-only record
when no unique candidate exists or the adapter lacks findReviewByBranch. On
later sweeps, continue attempting resolution so newly available reviews can
resolve the record, but suppress duplicate unresolved reports while preserving
the incomplete state required by the missing Review record path.
- Around line 107-113: Define the durable persistence target for recovered
records in the sweep described around findReviewByBranch and getReviewState,
including the write, commit/push behavior, and failure handling. Ensure the
recovered review ID is persisted after successful resolution so subsequent
sweeps do not repeat recovery, while preserving the sweep’s existing read-only
behavior for records that are not recovered.
🪄 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: 6d9a12e7-2bb1-4f37-aa8a-3d66ce751e7c
📒 Files selected for processing (6)
docs/plans/2026-08-07-fathom-stacked-reviews-design.mdskills/execute/SKILL.mdskills/fathom-shared/approval.mdskills/fathom-shared/forges.mdskills/fathom-shared/forges/github.mdskills/fathom-shared/trackers.md
🚧 Files skipped from review as they are similar to previous changes (3)
- skills/fathom-shared/approval.md
- skills/fathom-shared/forges/github.md
- docs/plans/2026-08-07-fathom-stacked-reviews-design.md
Comment on lines
+115
to
+116
| When resolution fails, and when the resolved adapter does not implement that operation at all, that record yields no id and no state: report that once, naming the record, rather than guessing or treating a branch name as a review id. | ||
| Leave the issue incomplete in that case, exactly as a missing `- Review:` record leaves it, so case 2 below blocks closure on it instead of the aggregate cases deciding the issue on partial data. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make unresolved legacy-record reports idempotent.
When no unique candidate exists, or the adapter lacks findReviewByBranch, this path does not rewrite the record or create a durable marker. The same record will therefore be reported on every sweep, despite the requirement to report it once. Add a marker keyed by the branch-only record, while continuing lookup on later sweeps so a newly available review can still resolve it.
🤖 Prompt for 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.
In `@skills/fathom-shared/trackers.md` around lines 115 - 116, Update the
unresolved legacy-record handling in the tracker flow to persist an idempotent
marker keyed by the branch-only record when no unique candidate exists or the
adapter lacks findReviewByBranch. On later sweeps, continue attempting
resolution so newly available reviews can resolve the record, but suppress
duplicate unresolved reports while preserving the incomplete state required by
the missing Review record path.
Owner
Author
There was a problem hiding this comment.
Real finding, and out of scope for this PR.
Same pre-existing path as the note above. The report-once requirement was already on main before this branch. From git show 280bb22:skills/fathom-shared/trackers.md, line 96:
When the resolved adapter does not implement that operation, the record cannot be swept: report that once, naming the record, rather than guessing or treating a branch name as a review id.
So the gap between "report that once" and having no durable marker to make it once is older than this change. This branch added the ambiguous-candidate case to that same sentence, but the idempotency hole is inherited, and it only ever applies to legacy branch-only records, which no stacked-review path creates.
Closing it needs a durable marker plus the write path from the other note, so it belongs with that work rather than here. Tracking it separately.
There was a problem hiding this comment.
@crod951, agreed. The idempotency gap is pre-existing and applies only to legacy branch-only records. This PR extends that path but does not introduce it for stacked reviews.
I will leave this finding deferred for the separate follow-up. Would you like me to create the GitHub issue for it?
You are interacting with an AI system.
findReviewByBranch returns a bounded candidate list, so a lookup that finds no match is not proof that the branch has no review. The bundle recovery path in execute opened a new review on that basis, which can duplicate a review sitting outside the returned window. Execute now opens a bundle review only where the remote does not carry that bundle's branch at all, which is proof of absence that owes nothing to the lookup. When the branch exists and no candidate matches, or the adapter omits the operation, it stops and holds and tells the user a review may already exist for that branch. forges.md states the same distinction where the operation is defined: its result is bounded, absence cannot be read out of it, and a caller that would create something must establish absence elsewhere or hold.
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 `@skills/execute/SKILL.md`:
- Around line 260-261: Do not treat remote branch absence as proof that a bundle
has no review in the entry-processing routine. Replace the branch-existence
shortcut with an authoritative review lookup, or retain the unique bundle branch
until the - Review: record is durably committed; only openReview when the lookup
confirms no existing review.
🪄 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: 9dfa4461-2f8d-448e-9d82-91664f6f1d5c
📒 Files selected for processing (2)
skills/execute/SKILL.mdskills/fathom-shared/forges.md
Branch absence cannot prove a bundle has no review: a forge retains a review after its head branch is deleted, and nothing prevents a bundle branch being deleted or reused before its `- Review:` record is durable, so an interrupted run can leave a review with neither a branch nor a recorded id. The per-bundle routine could then open a duplicate. Add a write-ahead `- Review: pending (bundle k/N)` marker, committed as a bookkeeping commit before `openReview` is called and replaced by the real record once the review publishes. Recovery now reads the marker: no line at all means the call was never attempted, so open the review; a marker with no id means the outcome is unknown, so look it up and hold when that cannot settle it; a full record is reused as before. Document the marker in conventions.md, note that readers collecting review ids treat a marker-only bundle as having no record, and drop the branch-absence claim from the forges.md cross-reference.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/fathom-shared/conventions.md (1)
84-87: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake finalization recovery-safe across partial failure.
These lines persist only
Finalization: completeafter every closing action. If execution stops after a completion comment or another closing action but before the final closing commit, a resumed run sees no marker and can repeat earlier actions.Make each finalization action idempotent and reconcile it on resume, or persist per-action progress before performing non-idempotent actions.
🤖 Prompt for 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. In `@skills/fathom-shared/conventions.md` around lines 84 - 87, The finalization flow must recover safely when execution stops after individual closing actions but before the final “Finalization: complete” marker is committed. Update the finalization behavior described in the “Finalization” convention so each closing action is idempotent and reconciled during resume, or persist durable per-action progress before any non-idempotent action; ensure resumed execution does not repeat already completed actions.
🤖 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/execute/SKILL.md`:
- Around line 239-243: The instructions around the pending review marker must
not treat it as evidence that openReview was entered. Update the guidance near
the pending-marker handling and lines 266-274 to define pending as meaning the
call may or may not have started, while preserving the conservative hold
behavior and acknowledging that the marker alone cannot distinguish those
states.
- Around line 245-246: Update the openReview instructions around openReview so
GitHub bundles use the previous branch as the sole dependency mechanism: stop
passing dependsOn and remove the previous review URL from the review body.
Preserve the Part k of N and bundle-closing reference rules, and align the
wording with the GitHub contract in github.md.
- Around line 277-278: Update the uniquely matched candidate flow around
getReviewState to backfill the matched review ID (and retain its URL) when the
review is in a terminal successful state such as merged, before stopping
recovery. Use the returned state only to determine whether execution may
continue: proceed only for open, and stop/hold for every other state.
In `@skills/fathom-shared/conventions.md`:
- Around line 65-70: Update the pending-marker workflow described around
openReview so a marker written before the forge call can be safely recovered
after a crash. Add an idempotency key or equivalent forge lookup tied to the
bundle, use it to distinguish an unattempted call from an unknown outcome before
retrying, and document the resulting state transitions so recovery neither
blocks indefinitely nor creates duplicate reviews.
---
Outside diff comments:
In `@skills/fathom-shared/conventions.md`:
- Around line 84-87: The finalization flow must recover safely when execution
stops after individual closing actions but before the final “Finalization:
complete” marker is committed. Update the finalization behavior described in the
“Finalization” convention so each closing action is idempotent and reconciled
during resume, or persist durable per-action progress before any non-idempotent
action; ensure resumed execution does not repeat already completed actions.
🪄 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: d9652002-8ba7-4f65-80da-30570fc42ca4
📒 Files selected for processing (3)
skills/execute/SKILL.mdskills/fathom-shared/conventions.mdskills/fathom-shared/forges.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/fathom-shared/forges.md
The marker was described as proof that openReview had been entered, but it is committed before the call, so a run can stop between the commit and the call. Define it in execute/SKILL.md and conventions.md as meaning the call may or may not have started, with the marker's job being to force a lookup rather than to prove anything. Both files carry the same wording. Give the resulting hold a stated remedy: the hold reports the marker, the user confirms whether a review exists, and a confirmation that none does is recorded as a "- Review: none confirmed (bundle k/N)" line in the plan document's Bundles section, so the next run opens the review instead of holding again. The sweep counts that line among the missing records, the same way it counts the bare marker.
github.md forbade encoding a stack dependency a second way while the per-bundle routine wrote a "Depends on <url>" line into every review body after the first, so the two files contradicted each other. Resolve it in favor of the body line being prose: it is written for a human reviewer and carries no machine meaning, so it is not a second representation, and the prohibition covers machine-readable ones such as a label or an extra API call. github.md and conventions.md now carry that sentence identically, and execute/SKILL.md says the same where it passes dependsOn alongside the body line.
Recovery reused a uniquely matched review only when getReviewState said open and stopped on every other state without writing anything, so a bundle whose interrupted review had merged kept its id unrecorded and the sweep read that bundle as missing a record forever. Record the matched id and URL first, since the candidate came back keyed on this bundle's exact head and base and is true whatever the fate, then use the state only to decide continuation: continue on open and merged, hold on closed-unmerged and unknown, and push the backfilled record before any such hold.
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 `@skills/execute/SKILL.md`:
- Around line 293-300: Make pending-review recovery durable even when branch k
is deleted or unavailable: before replacing the pending marker with either the
matched review record or “none confirmed,” restore or recreate branch k and
check it out, then update the Bundles section and commit/push the bookkeeping
change on that branch. Preserve the existing explicit-path staging and
staged-diff verification requirements for both outcomes.
🪄 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: 557cb94e-1f0e-4b13-a902-36969a61936a
📒 Files selected for processing (3)
skills/execute/SKILL.mdskills/fathom-shared/conventions.mdskills/fathom-shared/forges/github.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/fathom-shared/forges/github.md
Comment on lines
+293
to
+300
| Say with it what unblocks the next run, since a hold with no stated remedy leaves the bundle blocked forever: when the user confirms that no review exists, replace bundle k's marker with a `- Review: none confirmed (bundle k/N)` line in the `Bundles` section, committed and pushed on branch k as the same bookkeeping commit the recording step below makes, and the next run opens that bundle's review rather than holding again. | ||
| When the user instead finds a review that does exist, record it as a full `- Review: <id> <url> (bundle k/N)` line the same way, and the next run reuses it. | ||
| Either confirmation belongs in the `Bundles` section, because that is where the next run reads it; a confirmation given only in conversation leaves the durable record unchanged and every later run holds identically. | ||
| When the resolved adapter omits `findReviewByBranch`, which `../fathom-shared/forges.md` says is legitimate, no lookup is possible at all: say once that the forge cannot be asked, then hold on exactly those same terms, since a pending marker with no id beside it says nothing about whether the call that followed it created a review. | ||
| Holding costs one confirmation on a run that was interrupted between marking bundle k pending and recording its review, while opening blindly costs a duplicate review that no record names and the sweep can never reconcile, so the hold is the cheaper of the two. | ||
| A matched candidate does not skip the two recording steps above it, whatever its state, and this is where a resumed run otherwise loses a bundle. | ||
| That bundle's entry carries the pending marker, since the marker is what brought this run into the lookup at all: replace it now with a real `- Review: <id> <url> (bundle k/N)` line built from the id and URL on the candidate record that matched, then commit it exactly as the recording step above does: stage only the plan document, by explicit path, verify the staged diff carries only that one replaced line before committing, and word it as the same bookkeeping commit. | ||
| Push it on branch k before anything else, including before any hold the state above calls for, since a record that never leaves this clone closes none of the gaps below. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'findReviewByBranch|none confirmed|Review: pending|push.*branch|branch.*(delete|reuse|restore|recreate)|git (branch|switch|checkout|push)' \
skills/execute/SKILL.md skills/fathom-shared docs/plansRepository: crod951/skills
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- branch lifecycle and recovery references ---'
rg -n -C 5 \
'deleted|delete|reused|reuse|restore|recreate|branch k|branch N|stack branch|head branch|recovery branch|record.*push|push.*record' \
skills/execute/SKILL.md skills/fathom-shared docs/plans \
| sed -n '1,260p'
printf '%s\n' '--- relevant section map ---'
ast-grep outline skills/execute/SKILL.md --match 'branch' --view compact 2>/dev/null | sed -n '1,160p'Repository: crod951/skills
Length of output: 44771
Make pending-review recovery durable when branch k is unavailable.
The procedure permits branch deletion or reuse before the - Review: record is durable, but requires both recovery outcomes to be committed and pushed on branch k. Add a guaranteed write path, or restore branch k before recording the review or none confirmed result.
🤖 Prompt for 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.
In `@skills/execute/SKILL.md` around lines 293 - 300, Make pending-review recovery
durable even when branch k is deleted or unavailable: before replacing the
pending marker with either the matched review record or “none confirmed,”
restore or recreate branch k and check it out, then update the Bundles section
and commit/push the bookkeeping change on that branch. Preserve the existing
explicit-path staging and staged-diff verification requirements for both
outcomes.
Owner
Author
There was a problem hiding this comment.
Real, and left open deliberately. This is the fifth review pass, and the automated loop has hit its cap, so it stops here and surfaces the remainder rather than continuing.
The finding is the tail of a compound edge case: branch k deleted or reused, AND its - Review: record not yet durable, AND recovery then needing to write its outcome to that same branch. Each pass in this area has closed one layer and exposed the next, which is the signal that bundle-review recovery with a missing record is a genuine distributed-state problem rather than a wording gap.
Nothing in the normal stacked-review path depends on it: a first run writes the pending marker and the real record on a branch it just created, and an ordinary resume finds one or the other. Flagging it for the maintainer to scope rather than patching a sixth layer unreviewed.
There was a problem hiding this comment.
@crod951, acknowledged. The finding remains valid and open.
This is a compound recovery case outside the normal stacked-review path. It needs separate design work for durable state when branch k is unavailable before the review record is written.
If you want, I can create a follow-up GitHub issue with the recovery requirements and acceptance criteria.
You are interacting with an AI system.
Both outcomes of the pending-review lookup, the backfilled review id and the "none confirmed" line, required a commit and push on branch k. A bundle branch may be deleted or reused before its record is durable, which is the very case the pending marker exists to survive, so branch k was not a guaranteed write path and the recovery outcome could never be made durable there. Every later run then repeated the same lookup and the same hold. Write both recovery records on the branch the run is currently on. Step 7 always resolves a branch that exists, and that branch is always one named in the Bundles section, which the sweep already fetches and reads in full, so nothing becomes unreachable. Also stop step 7 from recreating a missing bundle branch that already carries a review line, since recreating it from the branch below yields an empty branch and silently drops that bundle's commits. The run stays on the highest surviving bundle branch, records what the lookup learns, and then holds on the missing branch. conventions.md and trackers.md are updated to match: the collection rule now takes every review line each fetched branch carries rather than only the line for the bundle that branch belongs to.
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.
Summary
Gives Fathom's reserved
stackedReviewsforge capability real behavior, so one tracker issue can produce a short stack of dependent code reviews instead of exactly one.Originated from Ryan's thread about stacked PR support: GitHub supports stacked PRs, and even on a forge that does not, steering the agent to target the previous bundle's branch is useful.
How it works. When a breakdown produces five or more units of work and a valid boundary exists,
executeproposes splitting the issue into at most three bundles, each holding at least two units. Each bundle becomes one branch and one review, targeting the previous bundle's branch. The split is proposed at breakdown time and confirmed before any task, commit, or review is created.Opt-in only. A repository gets stacking by writing
stacking: proposeinto its.fathom/config.mdprofile. An absent field meansnever, so every existing repository, forge, and capability tier keeps behaving exactly as it does today until it opts in.Forge-agnostic.
openReviewgains one optionaldependsOnargument. Behavior is keyed off each adapter's declaredstackedReviewsvalue:retarget(GitHub) uses the base branch,declared-dependencyrecords an explicit dependency and scopes the commit range,nonechains the branches in git and states the relationship in the review body. No new forge operation, and the other four are unchanged.What changed
forges.mddependsOnargument, per-capability behavior table, tier-3 no-stack ruleforges/github.md,generic-git.md,TEMPLATE.mddependsOnbehavior, squash-merge caveatapproval.mdstackingprofile fieldconventions.mdBundlesplan-document section, per-bundle reconciliation, stack review bodiesmemory.mdtrackers.md,trackers/asana.md,trackers/linear.mdexecute/SKILL.mdPlugin version 2.1.0 to 2.2.0, propagated by
bin/sync-versions.sh.Verification
No unit tests exist in this repository, so verification is its own tooling plus cross-file consistency checks.
Invariants checked on the final tree:
execute/SKILL.md's numbered procedure runs 1 through 12, no gaps or duplicates.--force-with-lease. No bare force push anywhere. A rejected lease and a rebase conflict each stop and hold.TODO,TBD, or em dash underskills/.Review
Nine per-task reviews, a whole-branch review, and three local CodeRabbit rounds. The reviews found several defects that the plan alone would have shipped:
inReviewtwice. The two paths are now mutually exclusive with a durable finalization marker.Bundleslines. A stacked issue now recordsMerge-closer: suppressedand closure is left to the sweep.unknownreview state had no case. It was silently absorbed into "still entirely in review".Known follow-ups, deliberately not fixed here: an abandoned review id keeps the closed-unmerged case terminal until the user resolves it (user-mediated by design), and
execute/SKILL.md's parent-to-child dependency edges predate this change.docs/plans/2026-08-07-fathom-stacked-reviews-plan.mdis marked as a completed historical record; the design document and the skill files are authoritative.Summary by CodeRabbit
New Features
Documentation
Chores