feat(plan): enrich a brief on first read, and open it by naming the role - #92
Merged
Conversation
`plan brief` only ever READ a stored context pack, and the pack is written at dispatch time — so briefing a feat the runner had never reached printed the deterministic half alone, with no word about the missing one. That is indistinguishable from a pass that ran and found nothing, and it reads as broken enrichment. The pass now runs on first read and stores what it produced. A pack already on disk is reused as-is — even when the feat's row has changed since, which is only reported: briefing is what a human does repeatedly while editing a plan, and regenerating on every edit turns reading a brief into a recurring charge. `--refresh` is how you replace it, and it now refuses to run with the pass switched off instead of silently printing the stale one. `--enrich-model none` was documented as the way off and never worked here: the `none` → disabled mapping only existed in `plan run`, so `plan brief` spawned a session for a model literally named "none". Both commands share it now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARBSaXgJ7ssW1hvpy6r64y
The brief IS the prompt — it goes to `claude -p` on stdin with nothing in front of it — so whatever it opens with is where the session's role is set. It opened on a feat row, leaving the model to infer what it was supposed to be from a table of objectives. Four lines now name it, and a test keeps them four: the process this engineer follows stays in the plan-session CLAUDE.md and the `plan-dev` skill, so a preamble that starts explaining the cycle is the boundary TestBriefCarriesNoProcess guards eroding through a new door. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ARBSaXgJ7ssW1hvpy6r64y
📝 WalkthroughWalkthroughChangesPlan brief context-pack flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PlanCLI
participant StoredContextPack
participant EnrichmentHook
participant FeatBrief
User->>PlanCLI: Run plan brief with enrichment options
PlanCLI->>StoredContextPack: Load or remove stored pack
PlanCLI->>EnrichmentHook: Ensure pack when enabled and missing
StoredContextPack-->>PlanCLI: Return pack and possible staleness
PlanCLI->>FeatBrief: Render brief with selected context
FeatBrief-->>User: Print role-defining brief
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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/plan/brief_test.go`:
- Around line 200-212: The brief preamble assertions in the test must validate
the complete expected four-line role/mission text, not just its opening sentence
or maximum length. Replace the permissive checks around strings.HasPrefix and
the # Feat: boundary with an exact comparison that includes the required blank
separator immediately before "# Feat:", while preserving the existing diagnostic
output on failure.
In `@README.md`:
- Around line 305-307: The README’s earlier description of feat briefs
incorrectly says cited ADRs are inlined in full. Update that sentence to state
that briefs carry only adr:<slug> references, with sessions using csdd graph
explain to retrieve the ADR body, while preserving the surrounding explanation
of governing references.
🪄 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: a91d1e51-ce21-4a7c-b08f-1ba14a87d83e
📒 Files selected for processing (6)
README.mdinternal/cli/plan.gointernal/cli/plan_adr_test.gointernal/cli/plan_bridge_cli_test.gointernal/plan/brief.gointernal/plan/brief_test.go
Comment on lines
+200
to
+212
| if !strings.HasPrefix(out, "You are a senior software engineer.") { | ||
| t.Errorf("the brief must open by naming the role it is written for:\n%s", firstLines(out, 6)) | ||
| } | ||
| // The role sits BEFORE the feat header — everything after it reads as material | ||
| // for that role rather than as a document that happened to arrive. | ||
| role, feat := strings.Index(out, "You are a senior software engineer"), strings.Index(out, "# Feat:") | ||
| if feat < 0 || role > feat { | ||
| t.Errorf("the role must come before the feat header:\n%s", firstLines(out, 8)) | ||
| } | ||
| // Four lines, then the mission. A preamble long enough to need a heading has | ||
| // stopped being a preamble. | ||
| if head, _, _ := strings.Cut(out, "# Feat:"); strings.Count(head, "\n") > 6 { | ||
| t.Errorf("the role preamble should stay short, got:\n%s", head) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pin the complete four-line preamble.
The test fixes only the first sentence and permits extra lines; changes to the remaining role/mission text would still pass. Assert the exact preamble, including its blank separator before # Feat:.
Proposed test tightening
- if !strings.HasPrefix(out, "You are a senior software engineer.") {
+ const preamble = "You are a senior software engineer. You deliver ONE feat of an approved plan to\n" +
+ "completion on your own, in an autonomous session with no human at the gate.\n" +
+ "Everything below is the mission: what this feat is, what governs it, where it\n" +
+ "lives in this repository, and what verifies it.\n\n"
+ if !strings.HasPrefix(out, preamble) {
t.Errorf("the brief must open by naming the role it is written for:\n%s", firstLines(out, 6))
}
@@
- if head, _, _ := strings.Cut(out, "# Feat:"); strings.Count(head, "\n") > 6 {
- t.Errorf("the role preamble should stay short, got:\n%s", head)
- }📝 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 !strings.HasPrefix(out, "You are a senior software engineer.") { | |
| t.Errorf("the brief must open by naming the role it is written for:\n%s", firstLines(out, 6)) | |
| } | |
| // The role sits BEFORE the feat header — everything after it reads as material | |
| // for that role rather than as a document that happened to arrive. | |
| role, feat := strings.Index(out, "You are a senior software engineer"), strings.Index(out, "# Feat:") | |
| if feat < 0 || role > feat { | |
| t.Errorf("the role must come before the feat header:\n%s", firstLines(out, 8)) | |
| } | |
| // Four lines, then the mission. A preamble long enough to need a heading has | |
| // stopped being a preamble. | |
| if head, _, _ := strings.Cut(out, "# Feat:"); strings.Count(head, "\n") > 6 { | |
| t.Errorf("the role preamble should stay short, got:\n%s", head) | |
| const preamble = "You are a senior software engineer. You deliver ONE feat of an approved plan to\n" + | |
| "completion on your own, in an autonomous session with no human at the gate.\n" + | |
| "Everything below is the mission: what this feat is, what governs it, where it\n" + | |
| "lives in this repository, and what verifies it.\n\n" | |
| if !strings.HasPrefix(out, preamble) { | |
| t.Errorf("the brief must open by naming the role it is written for:\n%s", firstLines(out, 6)) | |
| } | |
| // The role sits BEFORE the feat header — everything after it reads as material | |
| // for that role rather than as a document that happened to arrive. | |
| role, feat := strings.Index(out, "You are a senior software engineer"), strings.Index(out, "# Feat:") | |
| if feat < 0 || role > feat { | |
| t.Errorf("the role must come before the feat header:\n%s", firstLines(out, 8)) | |
| } |
🤖 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/brief_test.go` around lines 200 - 212, The brief preamble
assertions in the test must validate the complete expected four-line
role/mission text, not just its opening sentence or maximum length. Replace the
permissive checks around strings.HasPrefix and the # Feat: boundary with an
exact comparison that includes the required blank separator immediately before
"# Feat:", while preserving the existing diagnostic output on failure.
Comment on lines
+305
to
+307
| What a session is handed is **the feat, not the method**. The brief opens by naming the role it is written for — the whole thing is fed to `claude -p` as the prompt, so its first lines are where the session's role is set — then 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. | ||
|
|
||
| Before a feat is dispatched, a **context pass** (`--enrich-model`, default `sonnet`) 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, into `.csdd/plan/<slug>/briefs/<feat>.json`. Every path and every `adr:`/`stack:` citation is checked against the workspace before it is stored — an unverifiable claim is dropped, not rendered — and the pack is keyed to the feat row, so a feat's later attempts reuse it instead of rediscovering the same tree at the orchestrator's price. `csdd plan brief <slug> --feat F --refresh` re-runs the pass and prints the brief, which is how you review (and, by editing the JSON, correct) what a session will be handed. `--enrich-model none` turns the pass off and briefs from the plan alone. | ||
| Before a feat is dispatched, a **context pass** (`--enrich-model`, default `sonnet`) 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, into `.csdd/plan/<slug>/briefs/<feat>.json`. Every path and every `adr:`/`stack:` citation is checked against the workspace before it is stored — an unverifiable claim is dropped, not rendered — and the pack is keyed to the feat row, so a feat's later attempts reuse it instead of rediscovering the same tree at the orchestrator's price. `csdd plan brief <slug> --feat F` prints the brief and runs the pass itself when the feat has no pack yet, which is how you review (and, by editing the JSON, correct) what a session will be handed. A pack already on disk is reused as-is — even when the feat's row has since changed, which is only reported, since briefing is something you do repeatedly while editing a plan and regenerating costs a model call; `--refresh` is how you replace it. `--enrich-model none` turns the pass off and briefs from the plan alone. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the ADR-body documentation.
This update says briefs carry governing refs, but README line 292 still says cited ADRs are inlined in full. FeatBrief emits only adr:<slug> references and directs the session to csdd graph explain; update that earlier sentence in this section.
🤖 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` around lines 305 - 307, The README’s earlier description of feat
briefs incorrectly says cited ADRs are inlined in full. Update that sentence to
state that briefs carry only adr:<slug> references, with sessions using csdd
graph explain to retrieve the ADR body, while preserving the surrounding
explanation of governing references.
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.
Two independent changes to what a plan session is handed, as two commits so either can be reverted alone.
1 · The context pass runs on first read
csdd plan briefonly ever read a stored context pack, and packs are written at dispatch time byplan run. So briefing a feat the runner had never reached printed the deterministic half alone — feat row, refs, Executor Notes, gates — with no word about the missing discovered half (Where this feat lives, Already there, Still missing, Pitfalls). Silent absence is indistinguishable from "the pass ran and found nothing", and reads as broken enrichment.Now:
adr:/stack:citation against the workspace, and stores it--enrich-model none--refreshReusing a stale pack rather than invalidating it is deliberate and differs from
plan run(which still invalidates on a changed row — what it dispatches a session to build has to match what it briefed that session with). Briefing is what a human does repeatedly while editing a plan; regenerating on every edit turns reading a brief into a recurring charge.Bug found by the new tests:
--enrich-model nonewas documented as the way to turn the pass off and never worked inplan brief— thenone→ disabled mapping lived only inplan run, so this spawned a session for a model literally namednone. Both commands now share one normalization.2 · The brief names the role
The brief is fed to
claude -pon stdin as the entire prompt, so its first lines are the only place the session's role is established. It opened on# Feat: <slug>, leaving the model to infer what it was supposed to be from a table of objectives.Four lines, and
TestBriefOpensByNamingTheRolekeeps them four — the process (authority, cycle, delegation, git, thedonegate) stays in the plan-sessionCLAUDE.mdand theplan-devskill, so a preamble that starts explaining the cycle is the boundaryTestBriefCarriesNoProcessguards eroding through a new door.Verification
make checkgreen (gofmt + vet +go test -raceacross every package).--refreshrefused with no model, the role preamble's content and its position before the feat header.plan brieftests now pass--enrich-model none— with the pass on by default they would spawn a realclaudeon any machine that has one installed.🤖 Generated with Claude Code
https://claude.ai/code/session_01ARBSaXgJ7ssW1hvpy6r64y
Summary by CodeRabbit
New Features
--refreshto regenerate a stored context pack.Documentation
csdd plan briefexamples to reflect enrichment, reuse, and refresh behavior.