feat(plan): brief the feat, not the method, and discover its context once - #89
Merged
Conversation
…once A feat's brief was 94% development process — the mission steps, the approval authority, the forbidden actions, the verdict protocol and the feat-exit checklist — all of it already in the plan-session CLAUDE.md the runner writes into every worktree and in the plan-dev skill. The copies had begun to disagree about the feat-exit checks, and the authority block argued against STOP rules that the worktree's CLAUDE.md no longer contains. The brief now carries the feat and what governs it; the process stays in the one place that already loads every turn. What replaces the removed prose is a per-feat context pass that reads the worktree once, on a cheap model, and records what the orchestrator would otherwise rediscover per attempt. The pass answers into a closed schema rather than prose, and that is the load-bearing part: asked in prose for "a prompt for this feat", both a cheap and a strong model wrote the process instead of the context — and the stronger one wrote it worse, faithfully reproducing the INTERACTIVE contract (create a branch, have a human approve each phase, open a PR). "Have a human approve" deadlocks a session that has no human. A schema with no free-text field cannot carry any of it. - FeatBrief drops the methodology and renders the feat's stored context pack; it still reads only from disk, so the brief stays byte-deterministic (R7.3) - enrich.go: bounded schema, deterministic prompt, VerifyPack (every path must exist, every adr:/stack: must resolve through the same helpers the validators use, `declared` is corrected from the feat row), cache keyed to the feat row - the pass runs after the worktree is prepared, so a feat with dependencies is described against the tree that actually has them merged in - enrichment never fails a dispatch: any failure is logged and the brief renders without it, which is also what --enrich-model none gets - the plan-session CLAUDE.md absorbs what only the brief carried: spec-author delegation, the cycle-skill fallback, graph analyze --strict, the reviewers at feat exit, the Tier-3 evidence command, recording decisions in stack/ADR - plan brief --refresh re-runs the pass, so a human can review and correct the pack before the run that will use it On a real 13-feat plan the brief goes from ~10.8 KB (7.5 KB of it methodology) to 8.2 KB, of which 3.6 KB is verified, feat-specific context. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Atx6C8EMdJxUBKsijj7wpN
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesPlan enrichment and briefing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant PlanRunner
participant FeatWorktree
participant EnsurePack
participant Claude
participant FeatBrief
CLI->>PlanRunner: run plan with enrich model
PlanRunner->>FeatWorktree: prepare feat worktree
PlanRunner->>EnsurePack: load or create context pack
EnsurePack->>Claude: request constrained JSON enrichment
Claude-->>EnsurePack: return context pack
EnsurePack-->>PlanRunner: return verified or empty pack
PlanRunner->>FeatBrief: generate plan and context brief
FeatBrief-->>CLI: print feat brief
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Atx6C8EMdJxUBKsijj7wpN
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@internal/cli/plan.go`:
- Around line 568-577: In the refresh flow around plan.EnrichHook, build and
validate the enrichment hook before calling plan.RemovePack. If the hook is nil,
return the existing --refresh model error without deleting the stored pack; only
invoke RemovePack after validation succeeds.
In `@internal/plan/enrich.go`:
- Around line 362-378: The stale-pack handling spans
internal/plan/enrich.go:362-378 and internal/plan/brief.go:114-122. In
EnsurePack, remove the stored pack via RemovePack when its key mismatches
PackKey(doc, feat) before attempting enrichment, so disabled or failed
enrichment leaves no stale file. In FeatBrief, validate a loaded pack’s Key
against PackKey(doc, feat) and ignore mismatches instead of rendering them.
- Around line 166-172: Handle the return value from fmt.Fprintf in PackKey so
errcheck no longer flags the hash-writing operation. Since the hash.Hash writer
cannot fail, explicitly discard the returned count and error while preserving
the existing input order and digest generation.
In `@internal/plan/runner.go`:
- Line 484: Update the EnsurePack call in the runner flow so its verification
root uses dir, matching the worktree passed to the enricher and allowing
worktree-only files to survive verification, while preserving opts.Root as the
storage root for the resulting pack.
- Around line 480-484: Move the EnsurePack enrichment call out of the
synchronous openDispatch/squad-filling path and execute it within the
per-dispatch goroutine so a slow claude call cannot block other feats. Preserve
the existing enrichment arguments and failure-handling behavior, while allowing
multiple dispatched feats to enrich concurrently up to the squad limit.
In `@internal/templater/templates/plan/CLAUDE.md.tmpl`:
- Around line 94-98: Update the completion criteria wording in the plan template
to say “all checks below” instead of “the three checks below,” preserving the
existing four checklist conditions including csdd graph analyze --strict and
spec validate.
In `@README.md`:
- Line 305: Update the README section around “What a session is handed” to match
the canonical plan-session contract in CLAUDE.md.tmpl: remove claims that
sessions own branches or open PRs, and state that process authority is
centralized in the plan-session CLAUDE.md rather than also living in plan-dev.
Preserve the distinction that sessions receive the feat and plan inputs while
following the worktree contract for development actions.
🪄 Autofix (Beta)
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 Plus
Run ID: 69326c60-5a0c-4ecd-9cb4-919445067eca
📒 Files selected for processing (13)
README.mdinternal/cli/plan.gointernal/cli/plan_adr_test.gointernal/cli/plan_bridge_cli_test.gointernal/plan/adr_test.gointernal/plan/brief.gointernal/plan/brief_test.gointernal/plan/enrich.gointernal/plan/enrich_test.gointernal/plan/runner.gointernal/plan/runner_exec.gointernal/templater/templater_test.gointernal/templater/templates/plan/CLAUDE.md.tmpl
💤 Files with no reviewable changes (2)
- internal/cli/plan_adr_test.go
- internal/plan/adr_test.go
Comment on lines
+568
to
+577
| if refresh { | ||
| if err := plan.RemovePack(r, doc.Slug, target.Slug); err != nil { | ||
| render.Err(err.Error()) | ||
| return 1 | ||
| } | ||
| hook := plan.EnrichHook(enrichModel) | ||
| if hook == nil { | ||
| render.Err("--refresh needs a model: pass --enrich-model") | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
--refresh deletes the stored pack before validating the model.
With --enrich-model none (or empty), RemovePack has already run when the nil-hook check fails, so the user loses the pack — including one they hand-edited, which the --refresh help text explicitly invites. Build and validate the hook first.
🔧 Proposed reorder
if refresh {
+ hook := plan.EnrichHook(enrichModel)
+ if hook == nil {
+ render.Err("--refresh needs a model: pass --enrich-model")
+ return 1
+ }
if err := plan.RemovePack(r, doc.Slug, target.Slug); err != nil {
render.Err(err.Error())
return 1
}
- hook := plan.EnrichHook(enrichModel)
- if hook == nil {
- render.Err("--refresh needs a model: pass --enrich-model")
- return 1
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if refresh { | |
| if err := plan.RemovePack(r, doc.Slug, target.Slug); err != nil { | |
| render.Err(err.Error()) | |
| return 1 | |
| } | |
| hook := plan.EnrichHook(enrichModel) | |
| if hook == nil { | |
| render.Err("--refresh needs a model: pass --enrich-model") | |
| return 1 | |
| } | |
| if refresh { | |
| hook := plan.EnrichHook(enrichModel) | |
| if hook == nil { | |
| render.Err("--refresh needs a model: pass --enrich-model") | |
| return 1 | |
| } | |
| if err := plan.RemovePack(r, doc.Slug, target.Slug); err != nil { | |
| render.Err(err.Error()) | |
| return 1 | |
| } |
🤖 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 `@internal/cli/plan.go` around lines 568 - 577, In the refresh flow around
plan.EnrichHook, build and validate the enrichment hook before calling
plan.RemovePack. If the hook is nil, return the existing --refresh model error
without deleting the stored pack; only invoke RemovePack after validation
succeeds.
Comment on lines
+362
to
+378
| key := PackKey(doc, feat) | ||
| if p, err := LoadPack(root, doc.Slug, feat.Slug); err == nil && p != nil && p.Key == key { | ||
| return p | ||
| } | ||
| if enrich == nil { | ||
| return nil | ||
| } | ||
| out, err := enrich(EnrichRequest{ | ||
| Feat: feat, | ||
| Prompt: enrichPrompt(doc, feat), | ||
| Schema: enrichSchema, | ||
| Dir: dir, | ||
| }) | ||
| if err != nil { | ||
| logf(" · context enrichment for %s failed (%v); briefing without it", feat.Slug, err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale context packs can be briefed as current. Nothing enforces EnrichPack.Key freshness at render time: EnsurePack returns nil for a stale pack but leaves the file on disk, and FeatBrief loads it regardless — so an edited feat row plus a disabled or failing enricher yields a brief carrying the previous row's discovery.
internal/plan/enrich.go#L362-L378: on a key mismatch, discard the stored pack (RemovePack) before attempting re-enrichment, so a failed or disabled pass leaves nothing stale behind.internal/plan/brief.go#L114-L122: ignore a loaded pack whoseKeydoes not equalPackKey(doc, feat)instead of rendering it unconditionally.
📍 Affects 2 files
internal/plan/enrich.go#L362-L378(this comment)internal/plan/brief.go#L114-L122
🤖 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 `@internal/plan/enrich.go` around lines 362 - 378, The stale-pack handling
spans internal/plan/enrich.go:362-378 and internal/plan/brief.go:114-122. In
EnsurePack, remove the stored pack via RemovePack when its key mismatches
PackKey(doc, feat) before attempting enrichment, so disabled or failed
enrichment leaves no stale file. In FeatBrief, validate a loaded pack’s Key
against PackKey(doc, feat) and ignore mismatches instead of rendering them.
Comment on lines
+480
to
+484
| // Discovery, once per feat rather than once per attempt: EnsurePack reuses the | ||
| // stored pack whenever the feat row that produced it has not changed. It cannot | ||
| // fail the dispatch — enrichment is an optimization over the deterministic brief, | ||
| // so every failure inside it is logged and the brief renders without it. | ||
| EnsurePack(opts.Root, doc, feat, dir, opts.Hooks.Enrich, logf) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Enrichment runs on the dispatch loop goroutine.
openDispatch is called from the squad-filling loop, so a claude enrichment call — bounded only by enrichIdle (3 min) — blocks every other feat from being dispatched, including with --squad-limit > 1. Consider running the pass inside the per-dispatch goroutine, or at least noting the serialization.
🤖 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 `@internal/plan/runner.go` around lines 480 - 484, Move the EnsurePack
enrichment call out of the synchronous openDispatch/squad-filling path and
execute it within the per-dispatch goroutine so a slow claude call cannot block
other feats. Preserve the existing enrichment arguments and failure-handling
behavior, while allowing multiple dispatched feats to enrich concurrently up to
the squad limit.
| // stored pack whenever the feat row that produced it has not changed. It cannot | ||
| // fail the dispatch — enrichment is an optimization over the deterministic brief, | ||
| // so every failure inside it is logged and the brief renders without it. | ||
| EnsurePack(opts.Root, doc, feat, dir, opts.Hooks.Enrich, logf) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
VerifyPack checks paths against the shared root, not the worktree the enricher read.
EnsurePack receives opts.Root as the verification root but dir as the enrichment cwd. VerifyPack stats filepath.Join(root, path), so a file that only exists in the feat's worktree (created by a merged dependency — exactly the case the comment on Line 471 motivates) is dropped as "does not exist", while a path that exists only on the base branch survives. Verify against dir and keep the pack stored under opts.Root.
🤖 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 `@internal/plan/runner.go` at line 484, Update the EnsurePack call in the
runner flow so its verification root uses dir, matching the worktree passed to
the enricher and allowing worktree-only files to survive verification, while
preserving opts.Root as the storage root for the resulting pack.
Comment on lines
+94
to
+98
| - `csdd graph analyze --strict` passes (spec ↔ task ↔ component traceability holds) | ||
| - every task box in `specs/<feat>/tasks.md` is `[x]` | ||
| - `specs/<feat>/test-report.json` is green with no open attentions | ||
|
|
||
| The loop checks these three on disk before accepting your verdict. It does not | ||
| The loop checks the last three on disk before accepting your verdict. It does not |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the stale “three checks” count.
After adding csdd graph analyze --strict, the checklist now has four conditions, but Lines 51-52 still say done requires “the three checks below.” Change that wording to “all checks below” so the session cannot interpret the new graph check or spec validate as optional.
Suggested wording
-4. **Verdict.** `done` only when the three checks below hold;
+4. **Verdict.** `done` only when all checks below hold;🤖 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 `@internal/templater/templates/plan/CLAUDE.md.tmpl` around lines 94 - 98,
Update the completion criteria wording in the plan template to say “all checks
below” instead of “the three checks below,” preserving the existing four
checklist conditions including csdd graph analyze --strict and spec validate.
|
|
||
| The session's verdict declares one of three intents: `done` (the whole feat is delivered and the session's own checks — `csdd spec validate`, `csdd graph analyze --strict`, the plan's Quality Gates — pass), `continue` (honest partial work; the summary is the handoff to the next session on the same feat), or `blocked` (the feat turned out to need another feat of this plan that has not landed — it parks without spending an attempt, and the discovered edge is recorded in `.csdd/plan/<slug>/discovered-deps.json`, never written back into `plan.md`). | ||
|
|
||
| What a session is handed is **the feat, not the method**. The brief carries the feat row, its governing refs, the seeds, the Executor Notes and the plan's Quality Gates; the development process — the session's authority to approve its own phases, the cycle, what it delegates, the git it owns and what makes a `done` acceptable — lives in the plan-session `CLAUDE.md` written into each worktree and in the `plan-dev` skill, both of which the session reads every turn anyway. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align this section with the canonical plan-session contract.
The surrounding text still says the session owns branches and opens PRs, while internal/templater/templates/plan/CLAUDE.md.tmpl says it must not create or switch branches, push, or open a PR. It also says the process lives in both CLAUDE.md and plan-dev, conflicting with the stated centralization in CLAUDE.md. Update this section so users and autonomous sessions receive one consistent contract.
🤖 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 `@README.md` at line 305, Update the README section around “What a session is
handed” to match the canonical plan-session contract in CLAUDE.md.tmpl: remove
claims that sessions own branches or open PRs, and state that process authority
is centralized in the plan-session CLAUDE.md rather than also living in
plan-dev. Preserve the distinction that sessions receive the feat and plan
inputs while following the worktree contract for development actions.
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.
What
A feat's brief was 94% development process — mission steps, approval authority, forbidden actions, verdict protocol, feat-exit checklist — all of it already in the plan-session
CLAUDE.mdthe runner writes into every worktree (tree.go: writeEntryDoc) and in theplan-devskill. The copies had drifted: the brief named feat-exit checks and sub-agents the entry doc did not, and its authority block spent 1.1 KB arguing against STOP rules that the worktree's CLAUDE.md no longer contains.The brief now carries the feat and what governs it. The process stays in the one place that already loads every turn.
What replaces the removed prose is a per-feat context pass: before a feat is dispatched, a cheap model reads its worktree once and records what the feat touches, which decisions and stack rows govern it, what is already there and what is not — the discovery the orchestrator would otherwise redo on every attempt, at orchestrator prices.
Why a schema and not a prompt
Asked in prose for "a prompt to develop this feat", both a cheap and a strong model wrote the process instead of the context — and the stronger one wrote it worse, faithfully reproducing the repository's INTERACTIVE contract: create a branch, have a human approve each phase, open a PR. "Have a human approve" deadlocks a session that has no human, and would cost a whole attempt.
A closed schema with no free-text field cannot carry any of it. The failure is removed by construction rather than forbidden by wording.
Changes
FeatBriefdrops the methodology and renders the feat's stored pack. It still reads only from disk, so the brief stays byte-deterministic (R7.3).enrich.go: bounded schema, deterministic prompt,VerifyPack(every path must exist, everyadr:/stack:must resolve through the same helpers the validators use,declaredis corrected from the feat row), cache keyed to the feat row.--enrich-model nonegets.CLAUDE.mdabsorbs what only the brief carried:spec-authordelegation, the cycle-skill fallback,graph analyze --strict, the reviewers at feat exit, the Tier-3 evidence command, recording decisions in stack/ADR.plan brief --refreshre-runs the pass, so a human can review and correct the pack before the run that uses it.Evidence
Verified end-to-end against a real 13-feat plan in another workspace (
plan brief --refresh, realclaude -pspawn, exit 0, zero claims dropped by the verifier). The pass found two governing ADRs the plan row never declared, the wikilog.mdthe feat would have forgotten, and this pitfall — both dates confirmed by hand:Brief size on that feat: ~10.8 KB (7.5 KB methodology) → 8.2 KB, of which 3.6 KB is verified feat-specific context.
Known limitation
VerifyPackcheckstouches[].pathandgovernors[].id— the fields that are citations.exists,missingandtrapsare free text and are not mechanically checkable; one trap in the real run carried an imprecise claim about which component boundaries ship aMANIFEST.md. Tightening this would mean requiring anevidencepath per item, which changes the pack format — deliberately left for a follow-up.Test plan
make checkgreen (gofmt + vet +go test -race),internal/planat 85.2% coverage.enrich_test.go: verification drops (nonexistent path,..escape, unresolvable ADR, undecided stack row, bad flow),declaredcorrection in both directions, per-feat caching and re-enrichment on an edited row, every failure mode leaving the dispatch intact, envelope parsing, schema bounds.TestPlanSessionEntryCarriesTheLoopContract, which fails if the plan-session CLAUDE.md loses any instruction of the loop.TestBriefCarriesNoProcessis the new boundary: it fails if process reappears in the brief.🤖 Generated with Claude Code
https://claude.ai/code/session_01Atx6C8EMdJxUBKsijj7wpN
Summary by CodeRabbit
plan run, including--enrich-model(defaulting tosonnet, withnoneto disable) and cached workspace-validated references.plan brief --refreshplus--enrich-modelto regenerate feature context.plan runsession loop guidance (technology choice recording, stronger quality/security-review dispatch, and addedcsdd graph analyze --strict).plan briefto treat ADRs as citation-only (no inline ADR body content).