diff --git a/CLAUDE.md b/CLAUDE.md index 02bbdd0..92fa972 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,8 +20,26 @@ These landed after phase 10, and all are documented in `design/orchestration.md` The profile also carries `PreloadsRules`, because where the rules go is scc's choice but what the harness then does with them is not. Claude Code loads `.claude/rules/*.md` at launch with the same priority as `CLAUDE.md`; Codex and opencode load nothing from `rules/`, which is scc's own directory there. The entry file branches on it: told to "read the rule when the concern is live", an agent that already has all nine in context re-reads them, putting the same ~26KB in twice — and learns that the one document it is meant to trust is wrong about its own environment. Confirm per harness with `/context`. - **`scc update` (phase 11), as replace-or-keep rather than the planned three-way merge.** It hashes every managed file against this build and against the manifest, prints the plan grouped by outcome, asks, and then replaces what is safe to replace. An edited file is kept and named; `--force` is the separate decision. `internal/merge` is still unbuilt. -- **`scc rtk`, and `scc init --rtk`.** Wires in [RTK](https://github.com/rtk-ai/rtk), the CLI proxy that filters command output: `cargo install` when the binary is missing, plus a splice of RTK's marker-delimited usage block into the entry file. Insert-only — a block already between the markers is left alone whatever version it claims, because RTK owns that text and `rtk init` refreshes it; `--force` is the separate decision. Opt-in in both places, since the block tells the agent to prefix every command with a binary the machine may not have. `--check` reports without writing and exits 2 when the block is missing. -- **`scc launch `.** Starts the harness this workspace was scaffolded for, from the workspace root, behind [Headroom](https://github.com/headroomlabs-ai/headroom)'s compression proxy (`headroom wrap `). Headroom is the *default* here, which is the deliberate opposite of how RTK is wired: RTK edits a file the user owns and changes how every later command is typed, while Headroom wraps one process for one session and changes nothing on disk. So it degrades instead of failing — missing binary, declined install, unattended run, or a harness Headroom does not wrap all end in the agent starting bare with a warning saying why. `--no-headroom` forces that path, and a missing binary prompts for `uv tool install` (`--yes`/`--no-install` are the unattended answers). This is the one command that does not obey the 0/1/2 exit-code contract; see the convention below. +- **`scc rtk`, and `scc init --rtk`.** Wires in [RTK](https://github.com/rtk-ai/rtk), the CLI proxy that filters command output: `cargo install` when the binary is missing, plus a splice of RTK's marker-delimited usage block into the entry file. **scc's block wins by default**, replacing whatever sits between the markers; `--keep` is the separate decision. The reason is size, not authorship: `rtk init` (measured on 0.42.4) writes 139 lines / 5140 bytes, scc ships 18 lines / ~900, both stamped `v2`, both saying the same thing — and the entry file is preloaded into every request of the session, so the difference is paid continuously rather than once. Between two blocks of the same version the condensed one is simply better, and leaving the larger one because it got there first is a standing cost. Where that costs something is a block claiming a *newer* version, which is a real downgrade: the run names the version it displaced and points at `--keep`. Opt-in in both places, since the block tells the agent to prefix every command with a binary the machine may not have. `--check` reports without writing and exits 2 when the block is missing. + + **scc uses RTK's own markers rather than namespacing its own**, and that is load-bearing: `rtk init` writes `` … `` into the project entry file (verified against rtk 0.42.4), so addressing the block by that pair is what makes `rtk init` and `scc rtk` converge on one copy. A namespaced `scc:rtk-instructions` would make each tool blind to the other's block and leave the file carrying both — which is the bug, not the fix. Headroom is the counter-example: it *does* namespace (`headroom:rtk-instructions`), which is why a workspace wired by both can end up with two blocks. scc detects that one and names it in the report (`rtkFile.Foreign`), with `headroom unwrap ` as the fix — reported, never touched, because that block belongs to Headroom. +- **`scc launch `.** Starts the harness this workspace was scaffolded for, from the workspace root, behind [Headroom](https://github.com/headroomlabs-ai/headroom)'s compression proxy (`headroom wrap `), with the workspace's symbol graph brought up to date first. Headroom is the *default* here, which is the deliberate opposite of how RTK is wired: RTK edits a file the user owns and changes how every later command is typed, while Headroom wraps one process for the length of one session. So it degrades instead of failing — missing binary, declined install, unattended run, or a harness Headroom does not wrap all end in the agent starting bare with a warning saying why. `--no-headroom` forces that path, and a missing binary prompts for `uv tool install` (`--yes`/`--no-install` are the unattended answers). This is the one command that does not obey the 0/1/2 exit-code contract; see the convention below. + + Two things about `wrap` are load-bearing and were originally documented wrong here: + + **It does write to disk.** `headroom wrap` registers MCP servers into the agent's own config (`~/.claude.json` and the Codex/opencode equivalents), and those registrations outlive the session that made them — which is why Headroom ships `unwrap` at all. So `scc launch` defaults to **`--headroom-mcp retrieve`**: Headroom's own retrieve tool stays, because the proxy's compression markers are unactionable without it, and the code-memory server it would otherwise install does not, because code intelligence in an scc workspace is CodeGraph's job. `all` keeps Headroom's defaults; `none` drops the retrieve tool too. + + **It also wants the entry file, and scc says no.** `headroom wrap`'s context-tool setup appends RTK guidance to `$PWD/CLAUDE.md` or `$PWD/AGENTS.md` — the same file `scc rtk` splices — behind its own marker pair, ``. Neither marker is a substring of the other, so both tools' idempotency checks pass and both append: an entry file carrying the same RTK instructions twice, in every request of the session. So `scc launch` passes `--no-context-tool` by default. Headroom already gates that injection behind `HEADROOM_RTK`, which makes this belt-and-braces — but only until an environment exports that variable for its own reasons, and `wrap claude` resolves `setup_context_tool = (context_tool or _rtk_opt_in()) and not no_rtk`, so the flag wins over both the env var and `--context-tool`. `--headroom-context-tool` hands it back. + + **The opt-out flags are discovered, not hardcoded.** `internal/headroom` reads `headroom wrap --help` and picks the spelling that build advertises. This is not defensiveness for its own sake: Headroom renamed this exact control once already (`--no-serena` → `--code-memory none`, and `--no-tokensave` in between), and the harness profiles disagree today — `wrap opencode` still takes `--no-serena` while `wrap claude` and `wrap codex` take `--code-memory`. A flag name compiled into scc turns that kind of release into a launch that dies on `no such option`, which is strictly worse than one unwanted MCP server. A build advertising no opt-out is reported, not overridden. + + **`--` reaches `wrap`, not only the agent.** `headroom wrap` parses every flag it recognizes out of the tail and forwards only the rest, so a pass-through argument that collides with one of Headroom's — `--verbose`, which both Claude Code and `wrap` define — is silently eaten. `WrapArgs` therefore takes scc's options and the pass-through as separate parameters and puts scc's first, so a colliding argument the user typed lands last and wins. To force something past `wrap` to the agent, use a second terminator: `scc launch claude -- -- -p`. + +- **`scc graph`, and the launch-time index.** A wrap over [CodeGraph](https://github.com/colbymchenry/codegraph) — `build | sync | status | query | explore` — plus the same index run automatically by `scc launch`: `codegraph init` when `.codegraph/` is absent, `sync` when it is there. Launch is the one moment where indexing is free, and it degrades exactly the way Headroom does, for the same reason: a graph is an enhancement, so a missing binary or a failed index still starts the agent. `--no-graph` opts out and a plan-only run (`--json`/`--dry-run`) reports without indexing. + + What scc adds over typing `codegraph` directly is the two things it already knows: the workspace root, so `scc graph build` from `specs/` indexes the repo rather than a subtree, and whether the binary is there at all. The graph itself is *not* an scc artifact — not in the manifest, never touched by `scc update`, and `.codegraph/` stays CodeGraph's directory on CodeGraph's schedule. Unlike the launch path, a missing binary in `scc graph` is a hard error: the whole command is the binary. + + npm is the only installer scc will run. CodeGraph's headline install pipes a remote script into a shell (`curl … | sh`, `irm … | iex`), which is a fine thing for a person to type and not a thing scc executes on their behalf — `InstallHint` names it and leaves the decision where it belongs. - **The four seeded `docs/` anchors** (`assets.Seeds`). `init` writes `glossary.md`, `stack.md`, `wiki/index.md`, and `wiki/changelog.md` — the knowledge base's only fixed-name documents, each holding the format its validator checks. A seed is written once and tracked nowhere: not in the manifest, not by `scc update`. `scc` is a redesign of `csdd` (`github.com/protonspy/csdd`), narrowed to spec-driven development and deliberately leaner. When reaching for something from there, port the *decision*, not the file. Already decided against: a TUI, an embedded web dashboard, an MCP server, a devcontainer. @@ -68,6 +86,8 @@ cmd/scc/main.go os.Exit(cli.Run(os.Args[1:])) plain files on disk: / · specs/ · plans/ · docs/ · CLAUDE.md|AGENTS.md ``` +Three packages sit off to the side of that tree — `rtk`, `headroom`, `codegraph` — reached only from `internal/cli`. They are the third-party integrations, and they are the only code that starts another process. + `internal/cli/cli.go` is the whole dispatcher: `Run(args)` switches on `args[0]` and hands off to `run` in a file named for that resource. Each handler owns its own `flag.FlagSet`. Adding a subcommand means adding a case there plus one file — nothing is registered dynamically, so the command set is readable in one place. | Package | Role | @@ -84,10 +104,11 @@ cmd/scc/main.go os.Exit(cli.Run(os.Args[1:])) | `internal/ears` | EARS requirement parsing, all five patterns plus complex. | | `internal/validate` | The eight validators, one file each, sharing `mdscan` and `finding`. The exception is `stack_manifests.go`: the seven dependency-file readers age on their own schedule, so they sit beside the rule rather than inside it. | | `internal/rtk` | RTK's marker pair and the idempotent splice of its block into the entry file, plus finding or `cargo install`ing the binary. | -| `internal/headroom` | Headroom's agent-slug table, the `wrap` argument vector, and finding or installing the binary (uv, then pip — never npm, which ships the SDK and no CLI). The slugs live here rather than on `paths.Harness` because they are Headroom's vocabulary, not scc's layout. | +| `internal/headroom` | Headroom's agent-slug table, the `wrap` argument vector, the MCP opt-out discovered from `wrap --help`, and finding or installing the binary (uv, then pip — never npm, which ships the SDK and no CLI). The slugs live here rather than on `paths.Harness` because they are Headroom's vocabulary, not scc's layout. | +| `internal/codegraph` | CodeGraph's argument vectors (`init`/`sync`/`index`/`status`/`query`/`explore`), the `.codegraph/` presence test, and finding or `npm install -g`ing the binary. Composes command lines and reads nothing inside the graph — the database is CodeGraph's schema on CodeGraph's schedule. | | `internal/cli` | The dispatcher and every command handler. | -`internal/rtk` and `internal/headroom` are the only packages that shell out to another program. Keep that boundary there rather than in a command handler: a third party's binary name, install command, and argument vocabulary all age on that third party's schedule, and one package per integration is what keeps a version bump from touching the dispatcher. +`internal/rtk`, `internal/headroom`, and `internal/codegraph` are the only packages that shell out to another program. Keep that boundary there rather than in a command handler: a third party's binary name, install command, and argument vocabulary all age on that third party's schedule, and one package per integration is what keeps a version bump from touching the dispatcher. Headroom's renamed MCP flag is the worked example — the fix stayed inside `internal/headroom`, and nothing else in the tree knows the flag exists. `go.mod` is stdlib-only. Keep it that way unless a dependency earns its place — the binary is distributed to six platforms and every dep is a supply-chain surface. diff --git a/internal/assets/assets.go b/internal/assets/assets.go index 1bcae63..b0d88df 100644 --- a/internal/assets/assets.go +++ b/internal/assets/assets.go @@ -71,7 +71,7 @@ import ( // each rule its own trigger line instead of running four of them together in a // sentence — project.md above all, since a build command nobody read is guessed — // and it stops telling a harness that preloads the rules to go and read them. -const Version = "9" +const Version = "10" // The embedded tree. "all:" so nothing is silently dropped for having a name the // default embed pattern skips. @@ -143,6 +143,7 @@ func Workspace(h paths.Harness) []File { "delivery.md", "specs.md", "knowledge-base.md", + "code-search.md", } { set = append(set, File{ Name: "rules/" + rule, diff --git a/internal/assets/templates/commands/scc-plan-run.md b/internal/assets/templates/commands/scc-plan-run.md index 3aab60d..3cc2f1d 100644 --- a/internal/assets/templates/commands/scc-plan-run.md +++ b/internal/assets/templates/commands/scc-plan-run.md @@ -1,6 +1,6 @@ --- -description: Run a plan under plans/ to completion group by group — worktree, implement, review, PR, CI, merge, then the next group -argument-hint: [the plan, plus any standing instruction for every group] +description: Run a plan under plans/ to completion — implement group by group, then deliver as one PR per group or one at the end, and settle CI before calling it delivered +argument-hint: [the plan, plus how to run it and any standing instruction] --- Use the `plan-run` skill. @@ -11,11 +11,17 @@ Read the plan and name the groups back, numbered and in order, before writing an code. The order is the one thing the user can correct cheaply now and expensively after three merges. -Then ask how this loop should run — automatic or gated, a worktree per group or the -current checkout, and what happens once each PR is open. Three questions, one -exchange, after they can see the groups they are agreeing to. **These are the -developer's calls.** Anything the plan's frontmatter already records is a proposed -answer to confirm, not a decision already made. +Then take every answer the line above already gave and ask only for what is left. +"Implement the whole plan, one PR at the end, delivered when CI is green" has settled +most of it; re-asking what someone just typed is the friction that stops people using +this at all. Restate what you took so a wrong reading is cheap to correct, then put +the remaining questions in one exchange — automatic or gated, one PR at the end or one +per group, a worktree per group or the current checkout, and what happens once a PR is +open. **These are the developer's calls.** Anything the plan's frontmatter already +records is a proposed answer to confirm, not a decision already made. + +The plan is delivered when CI is green on its pull request — never on the strength of +a passing local suite. Anything said above about *how* to implement is a standing instruction: it applies to every group, and you carry it into each one explicitly rather than trusting it to diff --git a/internal/assets/templates/entry.md b/internal/assets/templates/entry.md index 2a91a9a..4140031 100644 --- a/internal/assets/templates/entry.md +++ b/internal/assets/templates/entry.md @@ -29,6 +29,9 @@ Triggered by what you are about to touch: project's commands exist nowhere else: `scc` ships the file as a stub for the team to fill in, and runs none of them itself. A command that did not come from there is a guess, and a guessed test command that exits 0 looks exactly like a passing suite. +- `code-search.md` — **before you go looking for code you have not read yet.** This + workspace keeps a symbol graph, and a structural question answered from it costs one + call instead of a grep and six reads. - `specs.md` — writing requirements, design, or tasks for a spec - `tasks.md` — working through a spec's task list - `knowledge-base.md` — something was learned, or a decision was made diff --git a/internal/assets/templates/rules/code-search.md b/internal/assets/templates/rules/code-search.md new file mode 100644 index 0000000..324913e --- /dev/null +++ b/internal/assets/templates/rules/code-search.md @@ -0,0 +1,44 @@ +# Code search — ask the graph before you read the files + +This workspace keeps a symbol graph of its own code, in `.codegraph/`, rebuilt +whenever `scc launch` starts an agent. It exists so a structural question costs one +call instead of a grep and six reads. + +Reach for it **first**, when the question is about structure: + +| The question | Ask | +|---|---| +| Where does this behavior live, and what calls what? | `codegraph_explore`, or `scc graph explore ""` | +| What breaks if I change this symbol? | `scc graph impact ` | +| Who calls this / what does it call? | `scc graph query `, `callers`, `callees` | + +Read files directly when the question is about *this exact text* — a line you are +editing, a diff you are reviewing, a file you have just written. The graph is a map; +it is not the territory, and it does not replace reading the code you are about to +change. + +Two ways in, and they answer identically. Use the `codegraph_explore` tool where it +is registered. Use `scc graph explore` in a shell when it is not — from a subagent, +or from a harness with no MCP surface. + +## What the graph does not know + +**It indexes code, not this repository's knowledge.** `docs/` is Markdown and no part +of it is in the graph: not the glossary, not the wiki, not an ADR, not a `design.md`. + +That matters more here than it would elsewhere, because this project deliberately +keeps the *why* out of the code. A question the graph answers well — "where is this +implemented" — is a different question from the one the knowledge base answers — +"why is it like this, and what was ruled out". Asking the graph the second kind gets +you a confident answer about the wrong thing. See [knowledge-base.md](knowledge-base.md) +for where that half lives. + +## When it is not there + +A missing or stale graph is never a reason to stop. `scc launch` builds it on a best +effort and starts the agent either way, so a session may legitimately have none — +CodeGraph is not installed, the index failed, or someone passed `--no-graph`. + +Fall back to ordinary reading and say nothing about it. If a graph query returns +something that contradicts the file in front of you, the file wins and the index is +stale: `scc graph sync`. diff --git a/internal/assets/templates/skills/plan-run/SKILL.md b/internal/assets/templates/skills/plan-run/SKILL.md index 04bdad4..a8ea4e5 100644 --- a/internal/assets/templates/skills/plan-run/SKILL.md +++ b/internal/assets/templates/skills/plan-run/SKILL.md @@ -1,19 +1,29 @@ --- name: plan-run -description: Drive a whole plan under plans/ to completion group by group — read the plan, report the groups, ask the developer how the loop should run, then for each group branch from a green main, implement, open the PR, settle CI, merge, pull, and take the next. Resumes from what is on main rather than from memory. Use it when someone asks to implement an entire plan, to keep going until the plan is finished, or runs /scc-plan-run. Not for a single spec or a one-off change, which delivery.md already carries end to end on its own. +description: Drive a whole plan under plans/ to completion — read the plan, report the groups, take whatever the invocation already decided and ask only for the rest, then implement group by group and deliver either one PR per group or one at the end, settling CI before calling the plan delivered. Resumes from the repository rather than from memory. Use it when someone asks to implement an entire plan, to keep going until the plan is finished, or runs /scc-plan-run. Not for a single spec or a one-off change, which delivery.md already carries end to end on its own. --- -You run a plan to the end, one merged pull request per group. +You run a plan to the end. The mechanics of delivering *one* unit of work are not here — they are in `{{.Rules}}/delivery.md`, and repeating them would give this project two copies of one procedure. This skill owns only what a loop adds on top: choosing the -next group, branching every group from the merge the last one produced, knowing when -to stop, and finding your place again after a session dies. +next group, knowing when to stop, and finding your place again after a session dies. And it owns one thing that is nobody's default to assume — **how the loop runs is the developer's decision, taken once, after they have seen the groups.** +## What "delivered" means + +The plan is delivered when **CI is green on the pull request that carries it** — not +when you believe the work is done. + +That distinction is the point of this whole skill. Your own assessment of a finished +group is a claim; a green pipeline on a pushed branch is a fact about the repository +that a person can check without you. Every stopping rule below is written against +that fact. Never report a plan as delivered on the strength of a passing local suite: +say the PR is open, say where, and say what CI is doing. + ## What a group is A **group** is the smallest part of the plan that can merge on its own. @@ -29,21 +39,25 @@ wins — that heading exists precisely to say what must not be merged out of seq A plan with a flat, unnumbered checklist has exactly one group. Say so and run it once, rather than inventing a decomposition the author did not write. -## Before the first group — read, report, then ask +## Before the first group — read, report, then ask what is still open 1. **Read the plan and work out the groups.** Ask nothing yet. The questions below are only answerable by someone who can see what they are agreeing to. 2. **Name the groups back, numbered, in order.** Order is the one thing a person can correct cheaply now and expensively after three merges. -3. **Then ask, once, in one exchange — all three questions together.** How this loop - runs is the developer's call, not yours, and not a default you inferred from a - file. +3. **Take every answer the invocation already gave, and ask only for what is left.** + A prompt like *"implement the whole plan, open one PR at the end, and if CI passes + it is delivered"* has answered three of the four questions in one sentence. + Re-asking what somebody just typed is the friction that stops people using this + skill at all. Restate what you took, so a wrong reading is cheap to correct, then + ask for the remainder in a single exchange. | Ask | Answers | Recorded as | |---|---|---| | Run every group straight through, or stop at each group boundary for review? | automatic · gated | `autonomy: auto` · `gated` | +| One PR at the end of the plan, or one per group? | at the end · per group | `pr: per-plan` · `per-group` | | A git worktree per group, or a branch in the checkout you are already in? | worktree · in place | `worktree: per-group` · `in-place` | -| Once a group's PR is open — wait for CI and merge, merge without waiting, or stop and let me merge? | wait and merge · merge now · stop at the PR | `ci` + `merge`, below | +| Once a PR is open — wait for CI and merge, merge without waiting, or stop and let me merge? | wait and merge · merge now · stop at the PR | `ci` + `merge`, below | The plan's frontmatter may already carry `autonomy` and `ci` from when it was written. **Show those as the proposed answers and confirm them; do not ask blind, and do not @@ -61,14 +75,22 @@ that only shows up later: - **in place** (`worktree: in-place`) — one directory, and this session cannot run alongside another on the same repo. Right when the project's setup is expensive to duplicate; wrong when the user is running several features at once. - -**Write all four answers into the plan's frontmatter before starting**, then never ask +- **one PR at the end** (`pr: per-plan`) — the cheap shape, and measurably the fast + one: the groups become sequential commits on a single branch, the review subagents + run once over the whole diff instead of once per group, and CI settles once. What + it costs is granularity — a large diff to review, a red pipeline that does not say + which group broke it, and nothing landed on `main` if the run stops half way. Right + for a plan whose groups are one coherent piece of work; wrong when the groups ship + to users independently or when someone else has to review them as they land. + +**Write every answer into the plan's frontmatter before starting**, then never ask again for this plan: ```yaml --- autonomy: auto ci: wait +pr: per-plan worktree: per-group merge: auto --- @@ -83,7 +105,10 @@ interrogating the developer a second time. `scc validate` checks the values. ## The loop -For each group, in order: +Both shapes implement the same groups in the same order. What differs is how often +you stop to deliver. + +### `pr: per-group` — one merged pull request per group 1. **Start green.** In the primary checkout, `git switch main && git pull --ff-only`. Every group branches from the previous group's merge, which is the whole reason @@ -104,11 +129,34 @@ For each group, in order: settle and fix what is red before merging. `merge: auto` means you merge once that answer is satisfied; `merge: manual` means you open the PR, say where it is, and stop — the loop resumes when the developer's merge is on `main`. -7. **Back to a green main** — `git switch main && git pull --ff-only`. Every group - branches from the previous group's merge, which is what makes this a loop. +7. **Back to a green main** — `git switch main && git pull --ff-only`. 8. **Remove the worktree** now that its branch has landed, if there was one. 9. **Report the group in one line**, then start the next. +### `pr: per-plan` — one pull request at the end + +Branch once from a green `main`, then for each group in order: + +1. **Implement the group**, exactly as above — a leaf gets the spec cycle, a task + family its tasks in order. +2. **Run the suite, the lint, and `scc validate` before moving on.** These stay per + group and are not deferred with the rest. They are cheap, and they are what makes a + later failure attributable: a break caught at group 3 is group 3's, while the same + break found after group 9 costs a bisect. +3. **Commit the group on its own**, with the group in the subject, and tick its + checkboxes in the same commit. The commits are the granularity this shape gives up + in pull requests — do not squash the plan into one. +4. **Report the group in one line**, then start the next. Do not push a PR yet. + +Then, once — and only once every group is in: + +5. **Run both review subagents over the whole branch diff.** This is the deferral that + makes the shape cheap. Their findings are fixed on the same branch before anything + is pushed. +6. **Push and open one PR** covering the plan, its body naming every group it carries. +7. **CI and merge, exactly as answered** — the same rules as step 6 above, applied + once. Green CI here is the plan delivered. + ## Where the loop stops Stopping part-way is a legitimate outcome, and continuing past any of these is not: @@ -125,39 +173,49 @@ Stopping part-way is a legitimate outcome, and continuing past any of these is n Under `autonomy: gated`, stop at every group boundary and wait, having reported what merged. -Whenever you stop, say which groups merged, which one is open and where, and which -are untouched. A half-run plan the user cannot locate is worse than one that never -started. +Whenever you stop, say which groups are done, where the work is, and which are +untouched. A half-run plan the user cannot locate is worse than one that never +started — and under `pr: per-plan` that is the whole run, sitting unmerged on a +branch, so name the branch every time. ## Resuming -Derive your position from `main`, never from what you remember. A compaction, a -crash, or a fresh session must land in the same place: - -- Pull `main` and re-read the plan **there** — the copy in an old worktree is stale by - construction. -- **The four answers are in that frontmatter. Read them and carry on — do not ask - again.** They were the developer's call once; asking a second time because your - context died makes them pay for your problem. -- A task group whose boxes are ticked on `main` is done. -- A leaf whose spec's `tasks.md` is fully ticked on `main` is done. -- An open PR for this plan means that group is mid-flight. Under `merge: manual` that - is the expected resting state and the merge may simply not have happened yet. - Finish it before starting another; two open groups is the fan-out this loop exists - to avoid. -- A leftover worktree whose branch is already merged is debris. Remove it. - -A plan whose frontmatter carries no `worktree` or `merge` is a plan no loop has run -over. Ask the three questions. +Derive your position from the repository, never from what you remember. A compaction, +a crash, or a fresh session must land in the same place. **The answers are in the +plan's frontmatter — read them and carry on, do not ask again.** They were the +developer's call once; asking a second time because your context died makes them pay +for your problem. + +Where you read your position from depends on the shape: + +- **`pr: per-group` — read `main`.** Pull it and re-read the plan there; the copy in + an old worktree is stale by construction. A task group whose boxes are ticked on + `main` is done, as is a leaf whose spec's `tasks.md` is fully ticked there. An open + PR means that group is mid-flight — under `merge: manual` that is the expected + resting state. Finish it before starting another; two open groups is the fan-out + this loop exists to avoid. +- **`pr: per-plan` — read the plan's branch.** Nothing reaches `main` until the end, + so `main` will say no group is done and it will be wrong. Find the branch, read its + log for the per-group commits, and re-read the plan **there**. If every group is + committed but no PR is open, the run died between the last group and the review + pass: run the subagents and push. If the PR is open, the run died waiting on CI. + +A leftover worktree whose branch is already merged is debris. Remove it. + +A plan whose frontmatter carries no `pr`, `worktree`, or `merge` is a plan no loop has +run over. Ask what the invocation did not already answer. ## Degrading - **No remote, or no `gh`.** delivery.md stops at the branch, and a loop cannot — - the next group needs the last one on `main`. Say so *when you ask the three - questions*, not after the first group is written: "there is no remote here, so this - runs local-only — each group merges into your `main` with `git merge --no-ff`, no PR - is opened and no CI runs." That turns a degraded run into something the developer - chose. Never let it look like every group was reviewed. + under `pr: per-group` the next group needs the last one on `main`, and under + `pr: per-plan` there is no pipeline to make "delivered" mean anything. Say so *when + you ask the questions*, not after the first group is written: "there is no remote + here, so this runs local-only — the work merges into your `main` with `git merge + --no-ff`, no PR is opened and no CI runs, so the suite passing locally is all the + evidence you will get." That turns a degraded run into something the developer + chose. Never let it look like every group was reviewed, and never call such a plan + delivered. - **The plan grows while the loop runs.** Re-read it at each group boundary. The group list from step 2 is a report, not a contract, and a group appended after you started is still part of the plan. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 2800066..4b53069 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -64,6 +64,8 @@ func Run(args []string) int { return runRTK(args[1:]) case "launch": return runLaunch(args[1:]) + case "graph": + return runGraph(args[1:]) case "spec": return runSpec(args[1:]) case "plan": @@ -110,6 +112,7 @@ Commands: update Bring the managed files onto this build's templates, after showing the plan rtk Install RTK if missing and put its usage block in the entry file launch Start a harness in this workspace, through Headroom's compression proxy + graph The workspace's symbol graph — build | sync | status | query | explore spec Create and inspect specs — new | list | show | delete | validate plan Create and inspect plans — new | list | delete | validate skill Agent Skills conformance — validate diff --git a/internal/cli/graph.go b/internal/cli/graph.go new file mode 100644 index 0000000..91cddcd --- /dev/null +++ b/internal/cli/graph.go @@ -0,0 +1,327 @@ +package cli + +import ( + "flag" + "fmt" + "os" + "strings" + + "github.com/protonspy/spec-claude-code/internal/codegraph" + "github.com/protonspy/spec-claude-code/internal/render" +) + +// runGraph is the workspace's symbol graph: build it, refresh it, ask it things. +// +// A wrap over CodeGraph rather than a reimplementation of it, and thin on +// purpose. What scc adds is the two things it already knows and CodeGraph does +// not: which directory is the workspace root — so `scc graph build` from specs/ +// indexes the repo rather than a subtree — and whether the binary is there at +// all, answered once, the same way `scc rtk` answers it. +// +// The graph is not an scc artifact. It is not in the manifest, `scc update` never +// touches it, and `.codegraph/` is CodeGraph's directory on CodeGraph's schedule. +// scc's claim on it ends at composing the command line. +func runGraph(args []string) int { + if len(args) == 0 { + graphUsage() + return ExitError + } + switch args[0] { + case "help", "-h", "--help": + graphUsage() + return ExitOK + case "build": + return runGraphBuild(args[1:]) + case "sync": + return runGraphSync(args[1:]) + case "status": + return runGraphStatus(args[1:]) + case "query": + return runGraphQuery(args[1:]) + case "explore": + return runGraphExplore(args[1:]) + default: + render.Err(fmt.Sprintf("unknown graph subcommand %q", args[0])) + fmt.Fprintf(os.Stderr, "run `%s graph help` for the available subcommands\n", prog()) + return ExitError + } +} + +// runGraphBuild indexes the workspace. +// +// `init` when there is no graph and `index --force` when there is, because those +// are two different requests wearing one word: the first time is setup, and every +// later time is somebody saying the graph has gone wrong. Ordinary staleness is +// neither — that is `sync`, and mostly the watcher has already handled it. +func runGraphBuild(args []string) int { + fs := flag.NewFlagSet("graph build", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + noInstall := fs.Bool("no-install", false, "never install CodeGraph; use it only if it is already on PATH") + yes := fs.Bool("yes", false, "answer the install prompt with yes, for an unattended run") + force := fs.Bool("force", false, "rebuild from scratch even when a graph is already there") + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + if !noPositionals(rest, "graph build") { + return ExitError + } + target, bin, ok := graphTarget(*root, graphInstall{noInstall: *noInstall, yes: *yes}) + if !ok { + return ExitError + } + + cmd := codegraph.InitArgs() + switch { + case *force: + cmd = codegraph.IndexArgs() + case codegraph.Indexed(target): + render.Info("a graph is already there; rebuilding it incrementally — pass --force for a full re-index") + cmd = codegraph.SyncArgs() + } + return graphExec(bin, target, cmd) +} + +func runGraphSync(args []string) int { + fs := flag.NewFlagSet("graph sync", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + if !noPositionals(rest, "graph sync") { + return ExitError + } + target, bin, ok := graphTarget(*root, graphInstall{noInstall: true}) + if !ok { + return ExitError + } + if !requireGraph(target) { + return ExitError + } + return graphExec(bin, target, codegraph.SyncArgs()) +} + +// runGraphStatus reports what the graph holds. --check turns the same question +// into the findings code, so CI branches on "this checkout has no graph" the way +// it branches on `scc validate`. +func runGraphStatus(args []string) int { + fs := flag.NewFlagSet("graph status", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + check := fs.Bool("check", false, "report only; exit 2 when the workspace has no graph") + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + if !noPositionals(rest, "graph status") { + return ExitError + } + target, ok := resolveRoot(*root) + if !ok || !requireWorkspace(target) { + return ExitError + } + + if *check { + // Deliberately no binary lookup and no subprocess: --check answers whether + // this workspace has a graph, and a CI runner without CodeGraph installed + // still has a correct answer to that. + indexed := codegraph.Indexed(target) + if *jsonOut { + if c := emitJSON(struct { + Indexed bool `json:"indexed"` + Dir string `json:"dir"` + }{indexed, codegraph.Dir}); c != ExitOK { + return c + } + } else if indexed { + render.OK("the workspace has a graph in " + codegraph.Dir) + } else { + render.Warn(fmt.Sprintf("no graph in %s; run `%s graph build`", codegraph.Dir, prog())) + } + if !indexed { + return ExitFindings + } + return ExitOK + } + + _, bin, ok := graphTarget(target, graphInstall{noInstall: true}) + if !ok { + return ExitError + } + if !requireGraph(target) { + return ExitError + } + return graphExec(bin, target, codegraph.StatusArgs(*jsonOut)) +} + +func runGraphQuery(args []string) int { + fs := flag.NewFlagSet("graph query", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + kind := fs.String("kind", "", "restrict the search to one symbol kind, e.g. class or function") + limit := fs.Int("limit", 0, "stop after this many results (default: CodeGraph's own)") + jsonOut := addJSON(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + query, ok := graphQueryArg(rest, "query") + if !ok { + return ExitError + } + target, bin, ok := graphTarget(*root, graphInstall{noInstall: true}) + if !ok { + return ExitError + } + if !requireGraph(target) { + return ExitError + } + return graphExec(bin, target, codegraph.QueryArgs(query, *kind, *limit, *jsonOut)) +} + +// runGraphExplore is the question worth asking: the relevant symbols' source and +// the call paths between them, in one answer, without the agent reading files to +// find them. No --json, because explore emits the same agent-shaped text as the +// MCP tool it is the CLI face of. +func runGraphExplore(args []string) int { + fs := flag.NewFlagSet("graph explore", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + root := addRoot(fs) + rest, err := parseFlags(fs, args) + if err != nil { + return ExitError + } + query, ok := graphQueryArg(rest, "explore") + if !ok { + return ExitError + } + target, bin, ok := graphTarget(*root, graphInstall{noInstall: true}) + if !ok { + return ExitError + } + if !requireGraph(target) { + return ExitError + } + return graphExec(bin, target, codegraph.ExploreArgs(query)) +} + +// graphQueryArg takes the search terms. Joined rather than required to be one +// argument, so an unquoted question reaches CodeGraph as the sentence it was +// typed as instead of being rejected as too many positionals. +func graphQueryArg(rest []string, sub string) (string, bool) { + query := strings.TrimSpace(strings.Join(rest, " ")) + if query == "" { + render.Err(fmt.Sprintf("`%s graph %s` needs something to look for", prog(), sub)) + return "", false + } + return query, true +} + +type graphInstall struct { + noInstall bool + yes bool +} + +// graphTarget resolves the workspace root and finds the binary, installing it +// first when asked to. Unlike the Headroom path in `scc launch`, a missing binary +// here is a hard error rather than a degraded run: the whole command is the +// binary, so there is nothing left to fall back to. +func graphTarget(root string, opts graphInstall) (string, string, bool) { + target, ok := resolveRoot(root) + if !ok || !requireWorkspace(target) { + return "", "", false + } + bin, ok := ensureCodeGraph(opts) + if !ok { + return "", "", false + } + return target, bin, true +} + +// ensureCodeGraph finds the CLI, offering to install it when somebody is there to +// be asked. +func ensureCodeGraph(opts graphInstall) (string, bool) { + if p, ok := codegraph.Path(); ok { + return p, true + } + installer, available := codegraph.Available() + ask := !opts.noInstall && available && (opts.yes || interactive()) + if ask && !opts.yes { + render.Warn(fmt.Sprintf("%s is not on PATH — it is what builds and answers the graph", codegraph.Bin)) + render.Detail(" " + codegraph.Repo) + ask = confirm(promptIn, fmt.Sprintf("Install it now with `%s`?", installer.Cmd)) + } + if !ask { + render.Err(codegraph.Bin + " is not on PATH") + render.Detail(" install it with: " + codegraph.InstallHint()) + return "", false + } + + render.Info(fmt.Sprintf("installing %s: %s", codegraph.Bin, installer.Cmd)) + if err := codegraph.Install(installer, os.Stderr, os.Stderr); err != nil { + render.Err(err.Error()) + return "", false + } + p, ok := codegraph.Path() + if !ok { + render.Err(fmt.Sprintf("%s reported success but %s is still not on PATH", installer.Prog, codegraph.Bin)) + return "", false + } + render.OK(strings.TrimSpace(codegraph.Bin + " installed: " + p + " " + codegraph.Version(p))) + return p, true +} + +// requireGraph refuses to query a workspace that was never indexed, naming the +// command that fixes it. CodeGraph's own error for this is about a missing +// database, which is true and unhelpful. +func requireGraph(root string) bool { + if codegraph.Indexed(root) { + return true + } + render.Err(fmt.Sprintf("this workspace has no graph in %s", codegraph.Dir)) + render.Detail(fmt.Sprintf(" build it with: %s graph build", prog())) + return false +} + +// graphExec runs CodeGraph with this terminal attached. +// +// A child's non-zero exit becomes scc's 1: `scc graph` is a wrap, not a launcher, +// so the 0/1/2 contract holds here — the exemption `scc launch` has exists for a +// long-lived interactive session whose status a script has to see, which none of +// these are. It is a package var so tests can drive the surface without CodeGraph +// installed. +var graphExec = func(bin, root string, args []string) int { + code, err := codegraph.Run(bin, root, args, os.Stdout, os.Stderr) + if err != nil { + render.Err(fmt.Sprintf("could not run %s: %v", bin, err)) + return ExitError + } + if code != 0 { + return ExitError + } + return ExitOK +} + +func graphUsage() { + fmt.Fprintf(os.Stderr, `%s graph — the workspace's symbol graph, via CodeGraph + +Usage: + %s graph [flags] + +Subcommands: + build Index the workspace (--force for a full rebuild) + sync Bring an existing graph up to date incrementally + status Show what the graph holds (--check exits 2 when there is none) + query Search symbols by name (--kind, --limit) + explore Relevant symbols' source plus the call paths between them + +The graph lives in %s/ and belongs to CodeGraph: scc never tracks it in the +manifest and "%s update" never touches it. + + %s +`, render.Bold(prog()), prog(), codegraph.Dir, prog(), codegraph.Repo) +} diff --git a/internal/cli/graph_test.go b/internal/cli/graph_test.go new file mode 100644 index 0000000..5a39bd8 --- /dev/null +++ b/internal/cli/graph_test.go @@ -0,0 +1,234 @@ +package cli + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// withGraphExec replaces the CodeGraph runner with a recorder, so the whole +// subcommand surface is drivable without CodeGraph installed. +func withGraphExec(t *testing.T, code int) *[]string { + t.Helper() + var got []string + orig := graphExec + graphExec = func(bin, root string, args []string) int { + got = append([]string{bin, root}, args...) + return code + } + t.Cleanup(func() { graphExec = orig }) + return &got +} + +// graphArgs is what CodeGraph was asked to do, without the binary and root. +func graphArgs(got []string) string { + if len(got) < 2 { + return "" + } + return strings.Join(got[2:], " ") +} + +func indexWorkspace(t *testing.T, root string) { + t.Helper() + if err := os.Mkdir(filepath.Join(root, ".codegraph"), 0o755); err != nil { + t.Fatalf("Mkdir: %v", err) + } +} + +// `build` is two different requests wearing one word. The first time is setup; +// every later time is somebody saying the graph has gone wrong, which is a full +// re-index and has to be asked for. Ordinary staleness is neither. +func TestGraphBuildInitializesOnceThenRefreshes(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "codegraph") + got := withGraphExec(t, ExitOK) + + if _, stderr, code := run(t, "graph", "build", "--root", root); code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + if graphArgs(*got) != "init" { + t.Errorf("build on a fresh workspace = %q, want init", graphArgs(*got)) + } + + indexWorkspace(t, root) + if _, stderr, code := run(t, "graph", "build", "--root", root); code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + if graphArgs(*got) != "sync" { + t.Errorf("build on an indexed workspace = %q, want sync", graphArgs(*got)) + } + + if _, stderr, code := run(t, "graph", "build", "--root", root, "--force"); code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + if graphArgs(*got) != "index --force" { + t.Errorf("--force = %q, want a full re-index", graphArgs(*got)) + } +} + +// The workspace root, not the shell's directory: `scc graph build` from specs/ +// has to index the repository rather than a subtree, which is the whole reason +// this command exists rather than people typing `codegraph init`. +func TestGraphBuildsAtTheWorkspaceRoot(t *testing.T) { + root := initWorkspace(t) + sub := filepath.Join(root, "specs", "deep") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + isolatedPath(t, "codegraph") + got := withGraphExec(t, ExitOK) + t.Chdir(sub) + + if _, stderr, code := run(t, "graph", "build"); code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + if !sameDir(t, (*got)[1], root) { + t.Errorf("indexed %q, want the workspace root %q", (*got)[1], root) + } +} + +// --check answers one question — does this workspace have a graph — with the +// findings code, so CI branches on it the way it branches on `scc validate`. It +// deliberately runs no subprocess: a CI runner without CodeGraph installed still +// has a correct answer. +func TestGraphStatusCheckReportsFindingsWithoutTheBinary(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t) + + stdout, _, code := run(t, "graph", "status", "--root", root, "--check", "--json") + if code != ExitFindings { + t.Errorf("exit = %d, want %d for a workspace with no graph", code, ExitFindings) + } + var report struct { + Indexed bool `json:"indexed"` + } + if err := json.Unmarshal([]byte(stdout), &report); err != nil { + t.Fatalf("stdout is not valid JSON (%v): %q", err, stdout) + } + if report.Indexed { + t.Error("an unindexed workspace reported a graph") + } + + indexWorkspace(t, root) + if _, _, code := run(t, "graph", "status", "--root", root, "--check"); code != ExitOK { + t.Errorf("exit = %d, want %d once the graph is there", code, ExitOK) + } +} + +// CodeGraph's own error for an unindexed project is about a missing database, +// which is true and unhelpful. scc names the command that fixes it instead. +func TestGraphQueryRefusesAnUnindexedWorkspace(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "codegraph") + got := withGraphExec(t, ExitOK) + + _, stderr, code := run(t, "graph", "query", "UserService", "--root", root) + if code != ExitError { + t.Errorf("exit = %d, want %d", code, ExitError) + } + if !strings.Contains(stderr, "graph build") { + t.Errorf("stderr does not name the fix: %q", stderr) + } + if len(*got) != 0 { + t.Errorf("scc ran codegraph anyway: %v", *got) + } +} + +// Every flag scc forwards has to be one CodeGraph defines, and the ones it does +// not set stay CodeGraph's own defaults rather than becoming scc's. +func TestGraphQueryForwardsOnlyWhatWasAsked(t *testing.T) { + root := initWorkspace(t) + indexWorkspace(t, root) + isolatedPath(t, "codegraph") + got := withGraphExec(t, ExitOK) + + if _, _, code := run(t, "graph", "query", "UserService", "--root", root); code != ExitOK { + t.Fatalf("exit = %d", code) + } + if graphArgs(*got) != "query UserService" { + t.Errorf("bare query = %q, want no invented flags", graphArgs(*got)) + } + + if _, _, code := run(t, "graph", "query", "UserService", "--root", root, "--kind", "class", "--limit", "10", "--json"); code != ExitOK { + t.Fatalf("exit = %d", code) + } + if got, want := graphArgs(*got), "query UserService --kind class --limit 10 --json"; got != want { + t.Errorf("query = %q, want %q", got, want) + } +} + +// An unquoted question reaches CodeGraph as the sentence it was typed as, rather +// than being rejected as too many positionals. +func TestGraphExploreTakesAnUnquotedQuestion(t *testing.T) { + root := initWorkspace(t) + indexWorkspace(t, root) + isolatedPath(t, "codegraph") + got := withGraphExec(t, ExitOK) + + if _, stderr, code := run(t, "graph", "explore", "--root", root, "how", "does", "login", "work"); code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + if got, want := graphArgs(*got), "explore how does login work"; got != want { + t.Errorf("explore = %q, want %q", got, want) + } +} + +func TestGraphNeedsSomethingToLookFor(t *testing.T) { + root := initWorkspace(t) + indexWorkspace(t, root) + isolatedPath(t, "codegraph") + withGraphExec(t, ExitOK) + + for _, sub := range []string{"query", "explore"} { + _, stderr, code := run(t, "graph", sub, "--root", root) + if code != ExitError { + t.Errorf("%s with no query exited %d, want %d", sub, code, ExitError) + } + if !strings.Contains(stderr, sub) { + t.Errorf("%s: stderr does not name the subcommand: %q", sub, stderr) + } + } +} + +// The whole command is the binary, so a missing CodeGraph is a hard error here +// rather than the degraded run `scc launch` gives it: there is nothing left to +// fall back to. +func TestGraphReportsAMissingBinary(t *testing.T) { + root := initWorkspace(t) + indexWorkspace(t, root) + isolatedPath(t) + withoutTerminal(t) + + _, stderr, code := run(t, "graph", "sync", "--root", root) + if code != ExitError { + t.Errorf("exit = %d, want %d", code, ExitError) + } + if !strings.Contains(stderr, "codegraph") || !strings.Contains(stderr, "not on PATH") { + t.Errorf("stderr = %q, want it to name the missing binary", stderr) + } +} + +func TestGraphRequiresAWorkspace(t *testing.T) { + isolatedPath(t, "codegraph") + withGraphExec(t, ExitOK) + + _, stderr, code := run(t, "graph", "status", "--root", t.TempDir()) + if code != ExitError { + t.Errorf("exit = %d, want %d", code, ExitError) + } + if !strings.Contains(stderr, "not an scc workspace") { + t.Errorf("stderr = %q, want it to say the directory is not a workspace", stderr) + } +} + +func TestGraphRejectsAnUnknownSubcommand(t *testing.T) { + _, stderr, code := run(t, "graph", "nope") + if code != ExitError { + t.Errorf("exit = %d, want %d", code, ExitError) + } + if !strings.Contains(stderr, "nope") { + t.Errorf("stderr does not name the unknown subcommand: %q", stderr) + } +} diff --git a/internal/cli/launch.go b/internal/cli/launch.go index 3b48e5d..9f0d691 100644 --- a/internal/cli/launch.go +++ b/internal/cli/launch.go @@ -8,6 +8,7 @@ import ( "os/exec" "strings" + "github.com/protonspy/spec-claude-code/internal/codegraph" "github.com/protonspy/spec-claude-code/internal/headroom" "github.com/protonspy/spec-claude-code/internal/paths" "github.com/protonspy/spec-claude-code/internal/render" @@ -28,9 +29,15 @@ import ( // how RTK is wired. The difference is who bears the cost of being wrong: RTK's // block edits a file the user owns and tells the agent to prefix every command // with a binary the machine may not have, so it stays opt-in. Headroom wraps one -// process for the length of one session, changes nothing on disk, and degrades to -// starting the agent bare — so defaulting to it costs nothing when it is absent -// and saves context when it is there. +// process for the length of one session and degrades to starting the agent bare — +// so defaulting to it costs nothing when it is absent and saves context when it +// is there. +// +// What it does leave behind is MCP registrations in the agent's own config, which +// outlive the session that made them. That is why the default is +// --headroom-mcp=retrieve rather than Headroom's own defaults: the proxy needs +// its retrieve tool to make compression markers actionable, and the code-memory +// server it would otherwise install is a job this workspace gives CodeGraph. // // The agent's own exit code is passed straight through, which is the one place // scc's 0/1/2 contract does not apply — and it has to be. A launcher that @@ -48,14 +55,24 @@ func runLaunch(args []string) int { fs.SetOutput(os.Stderr) root := addRoot(fs) noHeadroom := fs.Bool("no-headroom", false, "start the agent directly, without Headroom's compression proxy") - noInstall := fs.Bool("no-install", false, "never install Headroom; use it only if it is already on PATH") - yes := fs.Bool("yes", false, "answer the install prompt with yes, for an unattended run") + mcp := fs.String("headroom-mcp", headroom.MCPRetrieve.String(), + "which MCP servers Headroom may register: all | retrieve (its own only) | none") + contextTool := fs.Bool("headroom-context-tool", false, + "let Headroom set up its own CLI context tool (RTK or lean-ctx) and append its guidance to the entry file") + noGraph := fs.Bool("no-graph", false, "start the agent without building or refreshing the symbol graph") + noInstall := fs.Bool("no-install", false, "never install anything; use Headroom and CodeGraph only if they are already on PATH") + yes := fs.Bool("yes", false, "answer the install prompts with yes, for an unattended run") dryRun := fs.Bool("dry-run", false, "print the command this would run, and run nothing") jsonOut := addJSON(fs) rest, err := parseFlags(fs, own) if err != nil { return ExitError } + mcpMode, err := headroom.ParseMCPMode(*mcp) + if err != nil { + render.Err(err.Error()) + return ExitError + } target, ok := resolveRoot(*root) if !ok { @@ -77,10 +94,12 @@ func runLaunch(args []string) int { // JSON document on stdout cannot both exist. plan := *jsonOut || *dryRun opts := headroomOptions{ - disabled: *noHeadroom, - noInstall: *noInstall || plan, - yes: *yes, - quiet: *jsonOut, + disabled: *noHeadroom, + noInstall: *noInstall || plan, + yes: *yes, + quiet: *jsonOut, + mcp: mcpMode, + contextTool: *contextTool, } cmd := launchCommand{Harness: harness.ID, Dir: target, Bin: harness.Bin, Args: passthrough} @@ -88,9 +107,16 @@ func runLaunch(args []string) int { cmd.Headroom = hr if hr.Wrapping { cmd.Bin = headroom.Bin - cmd.Args = headroom.WrapArgs(hr.Agent, passthrough) + cmd.Args = headroom.WrapArgs(hr.Agent, hr.Options, passthrough) } } + cmd.Graph = resolveGraph(target, graphOptions{ + disabled: *noGraph, + noInstall: *noInstall, + yes: *yes, + plan: plan, + quiet: *jsonOut, + }) if cmd.Args == nil { // A JSON consumer gets [] rather than null: the field is a command line, // and an empty one is still a list. @@ -127,6 +153,7 @@ type launchCommand struct { Bin string `json:"bin"` Args []string `json:"args"` Headroom *headroomReport `json:"headroom,omitempty"` + Graph *graphReport `json:"graph,omitempty"` } // String is the command as a person would type it. Not shell-quoted, because it @@ -151,6 +178,16 @@ type headroomReport struct { // describes the binary, which is a different question from what the command // ended up doing. Install string `json:"install"` + // MCP is the mode asked for: all | retrieve | none. + MCP string `json:"mcp"` + // ContextTool is whether Headroom was left free to set up its own CLI + // context tool — RTK or lean-ctx — and append its guidance to the entry file. + ContextTool bool `json:"context_tool"` + // Options is how this Headroom build spells everything above, reported apart + // from the rest of the command line because scc put it there and the user did + // not. Empty — with a warning on the human path — when the build advertises no + // way to decline what was declined. + Options []string `json:"options,omitempty"` // Reason names why a launch is not wrapping, for the run where that is a // surprise. Empty when it is. Reason string `json:"reason,omitempty"` @@ -161,6 +198,10 @@ type headroomOptions struct { noInstall bool yes bool quiet bool + mcp headroom.MCPMode + // contextTool lets Headroom set up RTK or lean-ctx and write its guidance + // into the entry file. Off by default: `scc rtk` owns that block here. + contextTool bool } // resolveHeadroom decides whether this launch goes through Headroom, installing @@ -181,10 +222,11 @@ func resolveHeadroom(h paths.Harness, opts headroomOptions) *headroomReport { if !wraps { return &headroomReport{Install: installSkipped, Reason: "Headroom does not wrap " + h.Label} } - report := &headroomReport{Agent: agent} + report := &headroomReport{Agent: agent, MCP: opts.mcp.String(), ContextTool: opts.contextTool} if p, ok := headroom.Path(); ok { - report.Wrapping, report.Path, report.Version, report.Install = true, p, headroom.Version(p), installPresent + report.Path, report.Version, report.Install = p, headroom.Version(p), installPresent + wrapWith(report, opts) return report } @@ -232,11 +274,188 @@ func resolveHeadroom(h paths.Harness, opts headroomOptions) *headroomReport { warnUnwrapped(report, opts) return report } - report.Wrapping, report.Path, report.Version, report.Install = true, p, headroom.Version(p), installInstalled + report.Path, report.Version, report.Install = p, headroom.Version(p), installInstalled render.OK(strings.TrimSpace(headroom.Bin + " installed: " + p + " " + report.Version)) + wrapWith(report, opts) return report } +// wrapWith settles the wrap: this launch is going through Headroom, and these are +// the options it goes through with. +// +// The opt-out flags are read off `headroom wrap --help` rather than +// compiled into scc, because they are Headroom's vocabulary and it has already +// renamed this one once — `--no-serena` became `--code-memory none`. A name +// hardcoded here would have turned that release into a launch that dies on "no +// such option", which is a strictly worse outcome than a launch that registers +// one MCP server too many. +func wrapWith(report *headroomReport, opts headroomOptions) { + report.Wrapping = true + if opts.mcp == headroom.MCPAll && opts.contextTool { + return + } + flags := headroom.HelpFlags(headroom.WrapHelp(report.Path, report.Agent)) + + declined, got := 0, 0 + if opts.mcp != headroom.MCPAll { + declined++ + if args := headroom.MCPOffArgs(flags, opts.mcp); len(args) > 0 { + report.Options, got = append(report.Options, args...), got+1 + } + } + if !opts.contextTool { + declined++ + if args := headroom.ContextToolOffArgs(flags); len(args) > 0 { + report.Options, got = append(report.Options, args...), got+1 + } + } + + if got == declined || opts.quiet { + return + } + // Something was declined and this build offers no way to decline it. Say so: + // the alternative is a user who set the flag, saw no error, and assumes it + // took. + render.Warn(fmt.Sprintf("this %s build advertises no way to decline everything scc asked it to; run it with --help to see what it takes", headroom.Bin)) + render.Detail(" `" + headroom.Bin + " wrap " + report.Agent + " --help`, and `" + headroom.Bin + " unwrap " + report.Agent + "` to undo what it registered") +} + +// graphReport says what happened to the symbol graph on the way to starting the +// agent. +type graphReport struct { + // Action is what scc did: built | synced | current | skipped | failed. + Action string `json:"action"` + // Indexed is whether the workspace had a graph before this launch. + Indexed bool `json:"indexed"` + Path string `json:"path,omitempty"` + Version string `json:"version,omitempty"` + // Reason names why nothing was built, for the run where that is a surprise. + Reason string `json:"reason,omitempty"` +} + +// The values graphReport.Action takes. +const ( + graphBuilt = "built" + graphSynced = "synced" + graphSkipped = "skipped" + graphFailed = "failed" +) + +type graphOptions struct { + disabled bool + noInstall bool + yes bool + plan bool + quiet bool +} + +// resolveGraph brings the workspace's symbol graph up to date before the agent +// starts, which is the only moment where doing so is free: the session is about +// to begin, nobody is waiting on a prompt yet, and the alternative is an agent +// whose first graph query answers out of an index from last week. +// +// It degrades exactly the way the Headroom path does, and for the same reason. A +// graph is an enhancement — the agent can still read files — so a missing binary, +// a declined install, or a failed index all end in the agent starting anyway with +// a line saying what happened. `scc launch` that refused to run because an index +// was stale would be scc putting its own tidiness above the thing the user asked +// for. +func resolveGraph(root string, opts graphOptions) *graphReport { + if opts.disabled { + return nil + } + report := &graphReport{Indexed: codegraph.Indexed(root)} + + bin, ok := codegraph.Path() + if !ok { + report.Action = graphSkipped + installer, available := codegraph.Available() + switch { + case opts.noInstall || opts.plan: + report.Reason = codegraph.Bin + " is not on PATH" + case !available: + report.Reason = fmt.Sprintf("npm is not on PATH, so %s cannot be installed", codegraph.Bin) + case opts.yes: + // Asked for by flag; no question to put. + case opts.quiet || !interactive(): + report.Reason = fmt.Sprintf("%s is not on PATH, and nobody is here to answer the install prompt", codegraph.Bin) + default: + render.Warn(fmt.Sprintf("%s is not on PATH — it gives the agent a symbol graph instead of file-by-file reading", codegraph.Bin)) + render.Detail(" " + codegraph.Repo) + if !confirm(promptIn, fmt.Sprintf("Install it now with `%s`?", installer.Cmd)) { + report.Reason = "install declined" + } + } + if report.Reason != "" { + warnNoGraph(report, opts) + return report + } + render.Info(fmt.Sprintf("installing %s: %s", codegraph.Bin, installer.Cmd)) + out := os.Stdout + if opts.quiet { + out = os.Stderr + } + if err := codegraph.Install(installer, out, os.Stderr); err != nil { + report.Action, report.Reason = graphFailed, err.Error() + warnNoGraph(report, opts) + return report + } + if bin, ok = codegraph.Path(); !ok { + report.Action = graphFailed + report.Reason = fmt.Sprintf("%s reported success but %s is still not on PATH", installer.Prog, codegraph.Bin) + warnNoGraph(report, opts) + return report + } + } + report.Path, report.Version = bin, codegraph.Version(bin) + + // A plan-only run reports what it would do and indexes nothing: --json has to + // leave stdout clean for the document, and --dry-run means what it says. + if opts.plan { + report.Action = graphSkipped + report.Reason = "plan-only run" + return report + } + + args, action, doing := codegraph.InitArgs(), graphBuilt, "building the symbol graph — the first index takes a while" + if report.Indexed { + args, action, doing = codegraph.SyncArgs(), graphSynced, "refreshing the symbol graph" + } + if !opts.quiet { + render.Info(codegraph.Bin + " " + doing) + } + // The indexer's own output goes to stderr in both streams when the caller is + // emitting JSON, because stdout carries the document and nothing else. + out := os.Stdout + if opts.quiet { + out = os.Stderr + } + code, err := codegraph.Run(bin, root, args, out, os.Stderr) + switch { + case err != nil: + report.Action, report.Reason = graphFailed, err.Error() + case code != 0: + report.Action = graphFailed + report.Reason = fmt.Sprintf("%s %s exited %d", codegraph.Bin, args[0], code) + default: + report.Action = action + return report + } + warnNoGraph(report, opts) + return report +} + +// warnNoGraph says, once, why the agent is starting without a fresh graph. +func warnNoGraph(report *graphReport, opts graphOptions) { + if opts.quiet { + return + } + render.Warn(fmt.Sprintf("starting without a fresh symbol graph: %s", report.Reason)) + if report.Action != graphFailed { + render.Detail(" install it with: " + codegraph.InstallHint()) + } +} + // warnUnwrapped says, once, why the agent is starting without compression. It is // a warning rather than a status line because the run is about to do less than // the user asked for, and silence there is how somebody spends a month wondering @@ -308,6 +527,13 @@ func harnessIDs(all []paths.Harness) string { // `--`. Everything after it is passed through untouched, so `scc launch claude -- // --dangerously-skip-permissions` reaches Claude Code rather than being rejected // here as an unknown flag. +// +// "Untouched by scc" is the whole promise, and it is worth being precise about +// what it is not: when the launch is wrapped, those arguments land after `wrap +// `, and `headroom wrap` takes every flag it recognizes for itself before +// forwarding the rest. A pass-through argument that collides with one of +// Headroom's — `--verbose` is defined by both — is eaten there, not here. A second +// terminator forces the issue: `scc launch claude -- -- -p`. func splitPassthrough(args []string) (own, rest []string) { for i, a := range args { if a == "--" { diff --git a/internal/cli/launch_test.go b/internal/cli/launch_test.go index d536623..869f6c1 100644 --- a/internal/cli/launch_test.go +++ b/internal/cli/launch_test.go @@ -16,19 +16,83 @@ import ( // machine happened to have Headroom already. Every test therefore replaces PATH // wholesale with a directory it controls — which also makes "Headroom is absent" // a fact of the test rather than a fact about the developer's laptop. -func isolatedPath(t *testing.T, bins ...string) { +func isolatedPath(t *testing.T, bins ...string) string { t.Helper() dir := t.TempDir() for _, bin := range bins { - script, name := "#!/bin/sh\necho '"+bin+" 0.0.0-stub'\n", bin + script, name := "#!/bin/sh\n"+stubHelp(bin, "sh")+"echo '"+bin+" 0.0.0-stub'\n", bin if runtime.GOOS == "windows" { - script, name = "@echo "+bin+" 0.0.0-stub\r\n", bin+".bat" + script, name = "@echo off\r\n"+stubHelp(bin, "bat")+"echo "+bin+" 0.0.0-stub\r\n", bin+".bat" } if err := os.WriteFile(filepath.Join(dir, name), []byte(script), 0o755); err != nil { t.Fatalf("stub %s: %v", bin, err) } } t.Setenv("PATH", dir) + return dir +} + +// stubHelp gives the headroom stub a --help that advertises the opt-out flags, +// because scc reads them off the binary rather than carrying them: a stub that +// answered nothing would make every MCP assertion below pass for the wrong +// reason. +// +// Shell builtins only — no `find`, no `grep`, no `printf`. isolatedPath replaces +// PATH wholesale, so a stub that shelled out to anything would find nothing there +// and silently report a Headroom with no flags at all. The text is abridged, and +// deliberately free of the `|` a real help puts in `--code-memory [serena|none]`, +// which in a .bat is a redirect rather than a character. +func stubHelp(bin, shell string) string { + if bin != "headroom" { + return "" + } + opts := []string{ + "Options:", + " --no-mcp Skip headroom MCP server registration", + " --code-memory Code-memory MCP to register: serena or none", + " --no-context-tool, --no-rtk Skip CLI context-tool setup", + } + if shell == "bat" { + s := ":sccargs\r\nif \"%~1\"==\"\" goto sccrun\r\nif \"%~1\"==\"--help\" goto scchelp\r\nshift\r\ngoto sccargs\r\n:scchelp\r\n" + for _, l := range opts { + s += "echo " + l + "\r\n" + } + return s + "exit /b 0\r\n:sccrun\r\n" + } + s := "for a in \"$@\"; do\n if [ \"$a\" = \"--help\" ]; then\n" + for _, l := range opts { + s += " echo '" + l + "'\n" + } + return s + " exit 0\n fi\ndone\n" +} + +// recordingStub replaces a stub with one that appends its arguments to a log, so +// a test can assert on what scc actually asked a third-party binary to do without +// that binary being installed. +func recordingStub(t *testing.T, dir, bin string) string { + t.Helper() + log := filepath.Join(dir, bin+".log") + script, name := "#!/bin/sh\necho \"$@\" >> '"+log+"'\nexit 0\n", bin + if runtime.GOOS == "windows" { + script, name = "@echo off\r\necho %* >> \""+log+"\"\r\nexit /b 0\r\n", bin+".bat" + } + if err := os.WriteFile(filepath.Join(dir, name), []byte(script), 0o755); err != nil { + t.Fatalf("stub %s: %v", bin, err) + } + return log +} + +// recorded is everything the stub was asked to do, or "" when it was never run. +func recorded(t *testing.T, log string) string { + t.Helper() + raw, err := os.ReadFile(log) + if os.IsNotExist(err) { + return "" + } + if err != nil { + t.Fatalf("read %s: %v", log, err) + } + return string(raw) } // withLaunchExec replaces the process launcher with a recorder, so the whole @@ -68,8 +132,8 @@ func TestLaunchWrapsWithHeadroomWhenItIsThere(t *testing.T) { if cmd.Bin != "headroom" { t.Errorf("bin = %q, want headroom", cmd.Bin) } - if strings.Join(cmd.Args, " ") != "wrap claude" { - t.Errorf("args = %v, want [wrap claude]", cmd.Args) + if got, want := strings.Join(cmd.Args, " "), "wrap claude --code-memory none --no-context-tool"; got != want { + t.Errorf("args = %q, want %q", got, want) } if cmd.Harness != paths.Claude.ID { t.Errorf("harness = %q, want claude", cmd.Harness) @@ -121,14 +185,85 @@ func TestLaunchNoHeadroomSkipsItEntirely(t *testing.T) { } } -// Everything after `--` belongs to the agent. scc must not parse it, reject it, -// or reorder it — it goes behind the wrap slug exactly as typed. +// Headroom's wrap registers MCP servers into the user's agent config, and those +// registrations outlive the session that made them. scc asks for its own retrieve +// tool and nothing else, so a launch does not quietly install a code-memory +// server the workspace has not asked for — this workspace's code intelligence is +// CodeGraph's job. +func TestLaunchTurnsHeadroomsCodeMemoryOffByDefault(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "headroom", "claude") + + cmd := launchJSON(t, "launch", "--root", root, "--json") + if cmd.Headroom == nil { + t.Fatal("no headroom report") + } + if cmd.Headroom.MCP != "retrieve" { + t.Errorf("mcp = %q, want retrieve", cmd.Headroom.MCP) + } + if got := strings.Join(cmd.Headroom.Options, " "); !strings.Contains(got, "--code-memory none") { + t.Errorf("options = %q, want them to decline the code-memory server", got) + } +} + +// Headroom's context-tool setup appends RTK guidance to the same entry file +// `scc rtk` splices, behind its own marker pair — so a workspace wired by both +// carries the instructions twice. Headroom already gates that behind +// HEADROOM_RTK; scc passes the flag anyway, so an environment that exports it for +// other reasons does not quietly turn every launch into a second copy. +func TestLaunchDeclinesHeadroomsContextToolByDefault(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "headroom", "claude") + + cmd := launchJSON(t, "launch", "--root", root, "--json") + if cmd.Headroom.ContextTool { + t.Error("context_tool = true, want Headroom's own setup declined") + } + if got := strings.Join(cmd.Args, " "); !strings.Contains(got, "--no-context-tool") { + t.Errorf("args = %q, want them to decline the context tool", got) + } + + // And the escape hatch hands it back, without scc having an opinion left. + cmd = launchJSON(t, "launch", "--root", root, "--headroom-context-tool", "--headroom-mcp", "all", "--json") + if !cmd.Headroom.ContextTool { + t.Error("context_tool = false under --headroom-context-tool") + } + if got, want := strings.Join(cmd.Args, " "), "wrap claude"; got != want { + t.Errorf("args = %q, want %q", got, want) + } +} + +// The two ends of the same flag: `all` is scc keeping its hands off Headroom's +// defaults, and `none` also drops the retrieve tool the proxy needs to make its +// compression markers actionable. +func TestLaunchHeadroomMCPModes(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "headroom", "claude") + + cmd := launchJSON(t, "launch", "--root", root, "--headroom-mcp", "all", "--json") + if got, want := strings.Join(cmd.Args, " "), "wrap claude --no-context-tool"; got != want { + t.Errorf("all: args = %q, want %q", got, want) + } + + cmd = launchJSON(t, "launch", "--root", root, "--headroom-mcp", "none", "--json") + if got, want := strings.Join(cmd.Args, " "), "wrap claude --code-memory none --no-mcp --no-context-tool"; got != want { + t.Errorf("none: args = %q, want %q", got, want) + } + + if _, stderr, code := run(t, "launch", "--root", root, "--headroom-mcp", "serena", "--json"); code != ExitError { + t.Errorf("an undefined mode exited %d, want %d (stderr: %s)", code, ExitError, stderr) + } +} + +// Everything after `--` belongs to the agent, and scc's own options go in front of +// it — so a user who names the same flag after `--` is the one who wins, since +// wrap takes the last occurrence. func TestLaunchPassesArgumentsThroughToTheAgent(t *testing.T) { root := initWorkspace(t) isolatedPath(t, "headroom", "claude") cmd := launchJSON(t, "launch", "claude", "--json", "--root", root, "--", "--resume", "--model", "opus") - if got, want := strings.Join(cmd.Args, " "), "wrap claude --resume --model opus"; got != want { + if got, want := strings.Join(cmd.Args, " "), "wrap claude --code-memory none --no-context-tool --resume --model opus"; got != want { t.Errorf("args = %q, want %q", got, want) } @@ -139,6 +274,102 @@ func TestLaunchPassesArgumentsThroughToTheAgent(t *testing.T) { } } +// Launch is the one moment where indexing is free: the session is about to begin, +// nobody is waiting on a prompt, and the alternative is an agent whose first graph +// query answers out of last week's index. A workspace with no graph gets the full +// build; one that already has a graph gets the incremental refresh. +func TestLaunchBuildsTheGraphAndThenRefreshesIt(t *testing.T) { + root := initWorkspace(t) + dir := isolatedPath(t, "claude") + log := recordingStub(t, dir, "codegraph") + withLaunchExec(t, 0) + + if _, stderr, code := run(t, "launch", "--root", root, "--no-headroom"); code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + if got := recorded(t, log); !strings.Contains(got, "init") { + t.Errorf("codegraph was asked to do %q, want an init", got) + } + + // Now the workspace has one, so the next launch refreshes rather than rebuilds. + if err := os.Mkdir(filepath.Join(root, ".codegraph"), 0o755); err != nil { + t.Fatalf("Mkdir: %v", err) + } + if err := os.Remove(log); err != nil { + t.Fatalf("Remove: %v", err) + } + if _, stderr, code := run(t, "launch", "--root", root, "--no-headroom"); code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + got := recorded(t, log) + if !strings.Contains(got, "sync") { + t.Errorf("codegraph was asked to do %q, want a sync", got) + } + if strings.Contains(got, "init") { + t.Errorf("codegraph re-initialized a workspace that already had a graph: %q", got) + } +} + +// A graph is an enhancement — the agent can still read files — so every way of not +// getting one ends in the agent starting anyway, with a line saying what happened. +// A launch that refused to run because an index was stale would put scc's +// tidiness above the thing the user asked for. +func TestLaunchStartsWithoutAGraphWhenCodeGraphIsMissing(t *testing.T) { + root := initWorkspace(t) + isolatedPath(t, "claude") + withoutTerminal(t) + got := withLaunchExec(t, 0) + + _, stderr, code := run(t, "launch", "--root", root, "--no-headroom") + if code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + if got.Bin != paths.Claude.Bin { + t.Errorf("launched %q, want the agent to have started anyway", got.Bin) + } + if !strings.Contains(stderr, "symbol graph") { + t.Errorf("stderr does not say the graph was skipped: %q", stderr) + } +} + +// --no-graph is the explicit "just start the agent": no lookup, no index, no +// report. +func TestLaunchNoGraphSkipsItEntirely(t *testing.T) { + root := initWorkspace(t) + dir := isolatedPath(t, "claude") + log := recordingStub(t, dir, "codegraph") + + cmd := launchJSON(t, "launch", "--root", root, "--no-headroom", "--no-graph", "--json") + if cmd.Graph != nil { + t.Errorf("graph = %+v, want it absent from the report", cmd.Graph) + } + if got := recorded(t, log); got != "" { + t.Errorf("--no-graph still ran codegraph: %q", got) + } +} + +// Indexing a repository is work, and a plan-only run has not been asked to do any. +// --json additionally cannot afford it: the indexer writes to stdout, which +// carries the document and nothing else. +func TestLaunchNeverIndexesOnAPlanOnlyRun(t *testing.T) { + root := initWorkspace(t) + dir := isolatedPath(t, "claude") + log := recordingStub(t, dir, "codegraph") + + for _, flag := range []string{"--json", "--dry-run"} { + if _, stderr, code := run(t, "launch", "--root", root, "--no-headroom", flag); code != ExitOK { + t.Fatalf("%s: exit = %d (stderr: %s)", flag, code, stderr) + } + if got := recorded(t, log); strings.Contains(got, "init") || strings.Contains(got, "sync") { + t.Errorf("%s indexed the workspace: %q", flag, got) + } + } + cmd := launchJSON(t, "launch", "--root", root, "--no-headroom", "--json") + if cmd.Graph == nil || cmd.Graph.Action != graphSkipped { + t.Errorf("graph = %+v, want the index reported as skipped", cmd.Graph) + } +} + // The agent's exit code is the command's exit code. A launcher that flattened the // status of what it launched into scc's own 0/1/2 would be unusable in a script. func TestLaunchPassesTheAgentsExitCodeThrough(t *testing.T) { diff --git a/internal/cli/rtk.go b/internal/cli/rtk.go index 34c6c7b..557c01d 100644 --- a/internal/cli/rtk.go +++ b/internal/cli/rtk.go @@ -27,7 +27,7 @@ func runRTK(args []string) int { root := addRoot(fs) check := fs.Bool("check", false, "report only, write nothing; exit 2 when the block is missing") noInstall := fs.Bool("no-install", false, "never run cargo; only write the block") - force := fs.Bool("force", false, "replace an existing block with the one this scc ships (default: leave it alone)") + keep := fs.Bool("keep", false, "leave an existing block alone (default: replace it with the one this scc ships)") jsonOut := addJSON(fs) rest, err := parseFlags(fs, args) if err != nil { @@ -45,7 +45,7 @@ func runRTK(args []string) int { return ExitError } - report, code := applyRTK(target, rtkOptions{check: *check, noInstall: *noInstall, force: *force, quiet: *jsonOut}) + report, code := applyRTK(target, rtkOptions{check: *check, noInstall: *noInstall, keep: *keep, quiet: *jsonOut}) if *jsonOut { if c := emitJSON(report); c != ExitOK { return c @@ -60,9 +60,10 @@ func runRTK(args []string) int { type rtkOptions struct { check bool noInstall bool - // force replaces a block that is already there. Off by default: the block - // between RTK's markers is RTK's, and `rtk init` is what refreshes it. - force bool + // keep leaves a block that is already there. Off by default: scc's block says + // what RTK's says in roughly a fifth of the bytes, and the entry file is + // preloaded into every request of the session. + keep bool // quiet suppresses the human status lines because the caller is emitting JSON // on stdout, which nothing else may touch. quiet bool @@ -84,8 +85,21 @@ type rtkFile struct { Path string `json:"path"` Action string `json:"action"` // added | present | replaced | missing // Block is the version the opening marker claims, for a file that had one. - // Reported, never compared: which version supersedes which is RTK's to say. Block string `json:"block,omitempty"` + // Was is the version of the block that got replaced, when it differed from the + // one scc ships. This is the honest half of preferring scc's block by default: + // between two v2 blocks the smaller one simply wins, but a v3 replaced by a v2 + // is a downgrade and has to be visible rather than inferred. + Was string `json:"was,omitempty"` + // Bytes and WasBytes size the block now in the file against the one it + // replaced — the whole argument for replacing it, stated in the unit that + // matters. + Bytes int `json:"bytes,omitempty"` + WasBytes int `json:"was_bytes,omitempty"` + // Foreign names another tool's RTK block found in the same file — Headroom + // writes one behind its own markers. Reported and never touched: scc does not + // own that block, and the file would carry the same instructions twice. + Foreign string `json:"foreign,omitempty"` } // The values rtkReport.Install takes. Not rtk.Action: these describe the binary, @@ -135,8 +149,8 @@ func applyRTK(root string, opts rtkOptions) (*rtkReport, int) { report.Files = append(report.Files, file) switch action := file.Action; action { case string(rtk.Present): - // Already wired. Whose block it is and which version it claims is RTK's - // business, so this reports and stops there. + // The block in the file is already scc's, or --keep asked for the one + // that was there. Either way there is nothing to do. if !opts.quiet { render.Info(strings.TrimSpace(fmt.Sprintf("%s — RTK block already there %s", entry, file.Block))) } @@ -157,7 +171,14 @@ func applyRTK(root string, opts rtkOptions) (*rtkReport, int) { } report.Changed++ if !opts.quiet { - render.OK(fmt.Sprintf("%s — RTK block %s", entry, action)) + render.OK(fmt.Sprintf("%s — RTK block %s%s", entry, action, sizeNote(file))) + } + // A replaced block that claimed a different version is the one case + // where preferring scc's costs something, so it is said out loud + // rather than left to be inferred from the file. + if file.Was != "" { + render.Warn(fmt.Sprintf("%s — the block it replaced claimed %s; scc ships %s", entry, file.Was, file.Block)) + render.Detail(fmt.Sprintf(" keep theirs with: %s rtk --keep", prog())) } } } @@ -174,6 +195,16 @@ func applyRTK(root string, opts rtkOptions) (*rtkReport, int) { return report, code } +// sizeNote is the whole argument for replacing a block, in the unit that makes it: +// bytes the entry file no longer spends in every request of the session. Silent +// when nothing was replaced, and when the replacement was not actually smaller. +func sizeNote(file rtkFile) string { + if file.WasBytes <= file.Bytes { + return "" + } + return fmt.Sprintf(" (%d → %d bytes)", file.WasBytes, file.Bytes) +} + // ensureBinary finds RTK, or builds it when it is missing and the user did not opt // out. Returns an error only when an install was attempted and failed — everything // else is a state, not a failure. @@ -239,7 +270,7 @@ func spliceEntry(root, entry, block string, opts rtkOptions) (rtkFile, error) { if err != nil { return rtkFile{}, err } - next, action, err := rtk.Splice(string(raw), block, opts.force) + next, action, err := rtk.Splice(string(raw), block, opts.keep) if err != nil { return rtkFile{}, err } @@ -249,7 +280,24 @@ func spliceEntry(root, entry, block string, opts rtkOptions) (rtkFile, error) { if action != rtk.Present { found = rtk.BlockVersion(block) } - file := rtkFile{Path: entry, Action: string(action), Block: found} + file := rtkFile{Path: entry, Action: string(action), Block: found, Bytes: len(block)} + if action == rtk.Present { + file.Bytes = len(rtk.Block(string(raw))) + } + if action == rtk.Replaced { + was := rtk.Block(string(raw)) + file.WasBytes = len(was) + // Only when it differs: naming the version on every replacement would bury + // the one case that actually needs reading. + if v := rtk.BlockVersion(was); v != found { + file.Was = v + } + } + if foreign, ok := rtk.ForeignBlock(string(raw)); ok { + file.Foreign = foreign.Tool + render.Warn(fmt.Sprintf("%s also carries %s's RTK block; the agent will read these instructions twice", entry, foreign.Tool)) + render.Detail(" remove that one with: " + foreign.Fix) + } if action == rtk.Present || opts.check { return file, nil } diff --git a/internal/cli/rtk_test.go b/internal/cli/rtk_test.go index 3a08025..cbbab49 100644 --- a/internal/cli/rtk_test.go +++ b/internal/cli/rtk_test.go @@ -58,6 +58,42 @@ func TestRTKAddsTheBlockToTheEntryFile(t *testing.T) { } } +// `headroom wrap --rtk` appends the same guidance to the same entry file behind +// its own marker pair, and neither marker is a substring of the other — so both +// tools' idempotency checks pass and the file ends up telling the agent the same +// thing twice. scc cannot address a block it does not own, so it does the one +// thing left: names it, says how to remove it, and touches nothing. +func TestRTKReportsHeadroomsCompetingBlock(t *testing.T) { + root := initWorkspace(t) + entry := filepath.Join(root, paths.Claude.EntryFile) + foreign := "\n\nPrefix every command with rtk.\n\n" + before := readEntry(t, root, paths.Claude.EntryFile) + foreign + if err := os.WriteFile(entry, []byte(before), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + stdout, stderr, code := run(t, "rtk", "--root", root, "--no-install", "--json") + if code != ExitOK { + t.Fatalf("exit = %d, want %d (stderr: %s)", code, ExitOK, stderr) + } + if !strings.Contains(stderr, "Headroom") || !strings.Contains(stderr, "headroom unwrap") { + t.Errorf("stderr does not name the competing block and its fix: %q", stderr) + } + + // Reported, never touched: that block is Headroom's to rewrite and remove. + if !strings.Contains(readEntry(t, root, paths.Claude.EntryFile), foreign) { + t.Error("scc modified Headroom's block") + } + + var report rtkReport + if err := json.Unmarshal([]byte(stdout), &report); err != nil { + t.Fatalf("stdout is not valid JSON (%v): %q", err, stdout) + } + if len(report.Files) == 0 || report.Files[0].Foreign != "Headroom" { + t.Errorf("files = %+v, want the foreign block named", report.Files) + } +} + // The command an agent runs at the top of a session has to be free to re-run: a // second pass reports the block as current and leaves the file byte-identical. func TestRTKIsIdempotent(t *testing.T) { @@ -89,15 +125,16 @@ func TestRTKIsIdempotent(t *testing.T) { } } -// The block between RTK's markers is RTK's. scc inserts one where there is none and -// otherwise keeps its hands off — a newer block, or one the user edited, survives -// every re-run. --force is the separate, explicit decision. -func TestRTKLeavesAnExistingBlockAlone(t *testing.T) { +// The block scc ships wins by default. `rtk init` writes the same v2 instruction +// in roughly five times the bytes, and the entry file is preloaded into every +// request of the session, so leaving the larger one in place because it got there +// first is a standing cost rather than deference. +func TestRTKReplacesAnExistingBlockWithItsOwn(t *testing.T) { root := initWorkspace(t) entry := filepath.Join(root, paths.Claude.EntryFile) base := readEntry(t, root, paths.Claude.EntryFile) - theirs := base + "\n\n## RTK\nnewer text\n\n" - if err := os.WriteFile(entry, []byte(theirs), 0o644); err != nil { + fat := "\n\n## RTK\n" + strings.Repeat("verbose guidance line\n", 200) + "\n" + if err := os.WriteFile(entry, []byte(base+fat), 0o644); err != nil { t.Fatalf("WriteFile: %v", err) } @@ -105,31 +142,77 @@ func TestRTKLeavesAnExistingBlockAlone(t *testing.T) { if code != ExitOK { t.Fatalf("exit = %d (stderr: %s)", code, stderr) } - if got := readEntry(t, root, paths.Claude.EntryFile); got != theirs { - t.Errorf("a newer block was rewritten:\n%s", got) + got := readEntry(t, root, paths.Claude.EntryFile) + if strings.Contains(got, "verbose guidance line") { + t.Error("the larger block survived") } + // Only the region between the markers: everything above it is the user's. + if !strings.HasPrefix(got, base) { + t.Error("the document above the block was rewritten") + } + var report rtkReport if err := json.Unmarshal([]byte(stdout), &report); err != nil { t.Fatalf("stdout is not valid JSON (%v): %q", err, stdout) } - if len(report.Files) != 1 || report.Files[0].Block != "v9" { - t.Errorf("files = %+v, want the v9 block reported as found", report.Files) + f := report.Files[0] + if f.Action != "replaced" { + t.Errorf("action = %q, want replaced", f.Action) } - // And --check calls it wired, because it is: the version is RTK's to judge. - if _, _, code := run(t, "rtk", "--root", root, "--check"); code != ExitOK { - t.Errorf("--check exit = %d, want %d with a block already in place", code, ExitOK) + if f.WasBytes <= f.Bytes { + t.Errorf("bytes = %d, was_bytes = %d, want the replacement to be the smaller one", f.Bytes, f.WasBytes) + } + // Same version on both sides, so there is nothing to warn about. + if f.Was != "" { + t.Errorf("was = %q, want it silent when the versions match", f.Was) } - // --force is the explicit "use the one this scc ships". - if _, stderr, code := run(t, "rtk", "--root", root, "--no-install", "--force"); code != ExitOK { - t.Fatalf("--force exit = %d (stderr: %s)", code, stderr) + // Re-running changes nothing: the block is now byte-identical to scc's. + if _, _, code := run(t, "rtk", "--root", root, "--no-install"); code != ExitOK { + t.Fatalf("second pass exit = %d", code) } - got := readEntry(t, root, paths.Claude.EntryFile) - if strings.Contains(got, "newer text") || strings.Contains(got, "v9") { - t.Error("--force did not replace the block") + if again := readEntry(t, root, paths.Claude.EntryFile); again != got { + t.Error("a second pass rewrote the file") } - if !strings.HasPrefix(got, base) { - t.Error("--force rewrote the document above the block") +} + +// Preferring scc's block costs something exactly once: when the block replaced +// claimed a newer version. That is a downgrade, so it is said out loud rather +// than left to be inferred, and --keep is the standing answer. +func TestRTKNamesTheVersionItDowngradesAndKeepCanRefuse(t *testing.T) { + root := initWorkspace(t) + entry := filepath.Join(root, paths.Claude.EntryFile) + base := readEntry(t, root, paths.Claude.EntryFile) + theirs := base + "\n\n## RTK\nnewer text\n\n" + if err := os.WriteFile(entry, []byte(theirs), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + // --keep leaves it exactly as it was. + if _, stderr, code := run(t, "rtk", "--root", root, "--no-install", "--keep"); code != ExitOK { + t.Fatalf("--keep exit = %d (stderr: %s)", code, stderr) + } + if got := readEntry(t, root, paths.Claude.EntryFile); got != theirs { + t.Errorf("--keep rewrote the block:\n%s", got) + } + + // Without it, scc's block wins — and the run says which version it displaced. + stdout, stderr, code := run(t, "rtk", "--root", root, "--no-install", "--json") + if code != ExitOK { + t.Fatalf("exit = %d (stderr: %s)", code, stderr) + } + if strings.Contains(readEntry(t, root, paths.Claude.EntryFile), "newer text") { + t.Error("the v9 block survived without --keep") + } + if !strings.Contains(stderr, "v9") || !strings.Contains(stderr, "--keep") { + t.Errorf("stderr does not name the downgrade and its escape: %q", stderr) + } + var report rtkReport + if err := json.Unmarshal([]byte(stdout), &report); err != nil { + t.Fatalf("stdout is not valid JSON (%v): %q", err, stdout) + } + if report.Files[0].Was != "v9" { + t.Errorf("was = %q, want v9", report.Files[0].Was) } } diff --git a/internal/codegraph/codegraph.go b/internal/codegraph/codegraph.go new file mode 100644 index 0000000..8373349 --- /dev/null +++ b/internal/codegraph/codegraph.go @@ -0,0 +1,195 @@ +// Package codegraph wires CodeGraph — the local symbol graph a coding agent +// queries instead of reading files one at a time — into scc. +// +// The same three concerns kept apart here as in the Headroom integration, for the +// same reason: naming the argument vectors, which is pure data; finding the +// binary, which is a PATH lookup; and installing it, which needs a network and +// can take a while. +// +// scc composes command lines and nothing else. The graph's schema, the file +// watcher, and the MCP server CodeGraph registers into the agent's own config are +// CodeGraph's, and scc deliberately knows none of them — `codegraph init` is a +// process to run, not a format to parse. +package codegraph + +import ( + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" +) + +// Repo is where CodeGraph is developed, for the error that has to send somebody +// somewhere. +const Repo = "https://github.com/colbymchenry/codegraph" + +// Bin is the executable's name, as it appears on PATH. +const Bin = "codegraph" + +// Pkg is the npm package that carries the CLI. +const Pkg = "@colbymchenry/codegraph" + +// Dir is the per-project directory CodeGraph builds the graph into. Its presence +// is the whole test for "this workspace has been initialized": scc never reads +// what is inside, because the contents are a SQLite database on CodeGraph's +// schedule, not a format scc has any business knowing. +const Dir = ".codegraph" + +// Indexed reports whether root has a graph. +func Indexed(root string) bool { + info, err := os.Stat(filepath.Join(root, Dir)) + return err == nil && info.IsDir() +} + +// Path reports where the codegraph binary is, and whether it is on PATH at all. +func Path() (string, bool) { + p, err := exec.LookPath(Bin) + if err != nil { + return "", false + } + return p, true +} + +// Version reports what `codegraph version` says, or "" when the binary cannot +// answer. Advisory only: it is printed, never branched on. +func Version(bin string) string { + out, err := exec.Command(bin, "version").Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + +// InitArgs builds the graph for the first time. One step: `init` creates the +// directory and does the full index, and afterwards CodeGraph's own watcher keeps +// it current. +func InitArgs() []string { return []string{"init"} } + +// SyncArgs is the incremental update, for a workspace that already has a graph +// and has been edited since the watcher last ran. +func SyncArgs() []string { return []string{"sync"} } + +// IndexArgs is the full rebuild from scratch, for a graph that has gone wrong +// rather than merely stale. +func IndexArgs() []string { return []string{"index", "--force"} } + +// StatusArgs reports the graph's statistics. +func StatusArgs(jsonOut bool) []string { return withJSON([]string{"status"}, jsonOut) } + +// QueryArgs searches symbols by name. kind and limit are omitted when zero, so +// the defaults stay CodeGraph's rather than becoming scc's. +func QueryArgs(query, kind string, limit int, jsonOut bool) []string { + args := []string{"query", query} + if kind != "" { + args = append(args, "--kind", kind) + } + if limit > 0 { + args = append(args, "--limit", strconv.Itoa(limit)) + } + return withJSON(args, jsonOut) +} + +// ExploreArgs is the one-shot question: the relevant symbols' source plus the +// call paths between them. +// +// No --json here, and that is CodeGraph's design rather than an omission: explore +// is the CLI face of the codegraph_explore MCP tool and emits the same +// agent-shaped text. Passing a flag it does not define would fail the command. +func ExploreArgs(query string) []string { return []string{"explore", query} } + +func withJSON(args []string, jsonOut bool) []string { + if jsonOut { + return append(args, "--json") + } + return args +} + +// Installer is one way to get the CLI onto PATH. +type Installer struct { + // Prog is the program that does the installing, looked up on PATH. + Prog string + // Args is the argument vector passed to Prog. + Args []string + // Cmd is the same command written the way a shell takes it, for printing. + Cmd string +} + +// Installers are the ways scc will install the CLI on the user's behalf. +// +// npm only, and the list is short on purpose. CodeGraph's headline install is a +// remote script piped into a shell — `curl … | sh`, `irm … | iex` — which is a +// fine thing for a person to type and not a thing scc gets to run for them: it +// executes whatever the URL serves at that moment, with no version pinned and +// nothing to inspect. InstallHint still names it, because somebody without npm +// needs a way through and that decision is theirs to make. +func Installers() []Installer { + return []Installer{ + { + Prog: "npm", + Args: []string{"install", "-g", Pkg}, + Cmd: "npm install -g " + Pkg, + }, + } +} + +// Available returns the first installer whose program is actually on this machine. +func Available() (Installer, bool) { + for _, i := range Installers() { + if _, err := exec.LookPath(i.Prog); err == nil { + return i, true + } + } + return Installer{}, false +} + +// InstallHint is what to tell somebody who has to do it themselves: what scc +// would have run, then the standalone bundle for a machine with no npm. +func InstallHint() string { + lines := make([]string, 0, len(Installers())+1) + for _, i := range Installers() { + lines = append(lines, i.Cmd) + } + lines = append(lines, "see "+Repo+" for the standalone bundle") + return strings.Join(lines, "\n or: ") +} + +// Install runs i, streaming its output — a global npm install is slow enough that +// a silent command reads as a hang. +// +// A missing program is reported as itself rather than as a failed install: the +// user has to get npm first, which is a different problem from an install that +// broke. +func Install(i Installer, stdout, stderr io.Writer) error { + prog, err := exec.LookPath(i.Prog) + if err != nil { + return fmt.Errorf("%s is not on PATH; install it, then run: %s", i.Prog, i.Cmd) + } + cmd := exec.Command(prog, i.Args...) + cmd.Stdout = stdout + cmd.Stderr = stderr + if err := cmd.Run(); err != nil { + return fmt.Errorf("%s: %w", i.Cmd, err) + } + return nil +} + +// Run executes the CLI in root with the given arguments, streaming its output. +// The exit code is the child's, so a caller can tell "the graph has no answer" +// from "the command could not run". +func Run(bin, root string, args []string, stdout, stderr io.Writer) (int, error) { + cmd := exec.Command(bin, args...) + cmd.Dir = root + cmd.Stdout, cmd.Stderr = stdout, stderr + if err := cmd.Run(); err != nil { + var exit *exec.ExitError + if errors.As(err, &exit) { + return exit.ExitCode(), nil + } + return 1, err + } + return 0, nil +} diff --git a/internal/codegraph/codegraph_test.go b/internal/codegraph/codegraph_test.go new file mode 100644 index 0000000..f5ee807 --- /dev/null +++ b/internal/codegraph/codegraph_test.go @@ -0,0 +1,125 @@ +package codegraph + +import ( + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +// The graph directory is the whole test for "this workspace is indexed", so it +// has to be a directory rather than anything that merely shares the name: a file +// called .codegraph is not a graph, and treating it as one would send every later +// command into CodeGraph's error instead of scc's. +func TestIndexedWantsADirectory(t *testing.T) { + root := t.TempDir() + if Indexed(root) { + t.Error("an empty workspace reported a graph") + } + + file := filepath.Join(root, Dir) + if err := os.WriteFile(file, []byte("not a graph"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if Indexed(root) { + t.Error("a regular file named .codegraph reported a graph") + } + + if err := os.Remove(file); err != nil { + t.Fatalf("Remove: %v", err) + } + if err := os.Mkdir(file, 0o755); err != nil { + t.Fatalf("Mkdir: %v", err) + } + if !Indexed(root) { + t.Error("a workspace with .codegraph/ reported no graph") + } +} + +// The argument vectors are the whole integration, and every flag in them has to +// be one CodeGraph actually defines — a wrap that invents a flag turns a working +// command into a usage error. +func TestArgVectors(t *testing.T) { + cases := []struct { + name string + got []string + want string + }{ + {"init", InitArgs(), "init"}, + {"sync", SyncArgs(), "sync"}, + {"index", IndexArgs(), "index --force"}, + {"status", StatusArgs(false), "status"}, + {"status json", StatusArgs(true), "status --json"}, + {"query", QueryArgs("UserService", "", 0, false), "query UserService"}, + {"query full", QueryArgs("UserService", "class", 10, true), "query UserService --kind class --limit 10 --json"}, + {"explore", ExploreArgs("how does login work"), "explore how does login work"}, + } + for _, c := range cases { + if got := strings.Join(c.got, " "); got != c.want { + t.Errorf("%s = %q, want %q", c.name, got, c.want) + } + } +} + +// explore is the CLI face of the MCP tool and emits the same agent-shaped text, +// so it defines no --json. Passing one would fail the command. +func TestExploreTakesNoJSONFlag(t *testing.T) { + for _, a := range ExploreArgs("anything") { + if a == "--json" { + t.Error("ExploreArgs passes --json, which explore does not define") + } + } +} + +// A remote script piped into a shell is a fine thing for a person to type and not +// a thing scc runs for them: it executes whatever the URL serves at that moment. +// The hint may name it; the installer list may not. +func TestInstallersNeverPipeAScriptIntoAShell(t *testing.T) { + for _, i := range Installers() { + if i.Prog == "curl" || i.Prog == "sh" || i.Prog == "powershell" || i.Prog == "irm" { + t.Errorf("installer %q runs a remote script", i.Prog) + } + if strings.Contains(i.Cmd, "|") { + t.Errorf("installer command pipes: %q", i.Cmd) + } + } + // But somebody without npm still needs a way through, and that decision is + // theirs to make. + if !strings.Contains(InstallHint(), Repo) { + t.Errorf("the hint does not point at the standalone bundle: %q", InstallHint()) + } +} + +func TestInstallHintNamesEveryInstaller(t *testing.T) { + hint := InstallHint() + for _, i := range Installers() { + if !strings.Contains(hint, i.Cmd) { + t.Errorf("the hint omits %s: %q", i.Prog, hint) + } + } +} + +// A missing npm is reported as a missing npm. Calling it a failed install would +// send the user looking at CodeGraph instead of at their own toolchain. +func TestInstallReportsAMissingProgram(t *testing.T) { + err := Install(Installer{Prog: "definitely-not-a-real-program", Cmd: "nope"}, io.Discard, io.Discard) + if err == nil { + t.Fatal("Install with a missing program returned no error") + } + if !strings.Contains(err.Error(), "not on PATH") { + t.Errorf("error = %q, want it to say the program is not on PATH", err) + } +} + +// Run reports the child's exit code rather than collapsing it, so a caller can +// tell "the graph has no answer" from "the command could not run". +func TestRunSeparatesAFailedExitFromAFailedStart(t *testing.T) { + code, err := Run("definitely-not-a-real-program", t.TempDir(), []string{"status"}, io.Discard, io.Discard) + if err == nil { + t.Error("Run with a missing binary returned no error") + } + if code != 1 { + t.Errorf("code = %d, want 1", code) + } +} diff --git a/internal/headroom/headroom.go b/internal/headroom/headroom.go index cc572b1..c8c7d03 100644 --- a/internal/headroom/headroom.go +++ b/internal/headroom/headroom.go @@ -16,6 +16,7 @@ import ( "fmt" "io" "os/exec" + "regexp" "strings" "github.com/protonspy/spec-claude-code/internal/paths" @@ -57,14 +58,155 @@ func Agent(h paths.Harness) (string, bool) { return slug, ok } -// WrapArgs is the whole argument vector: `wrap `, then whatever the caller -// is passing straight through to the agent itself. -func WrapArgs(agent string, rest []string) []string { - args := make([]string, 0, len(rest)+2) +// WrapArgs is the whole argument vector: `wrap `, then scc's own options +// to wrap, then whatever is passing through to the agent itself. +// +// The two argument lists are separate parameters rather than one because they are +// not interchangeable, and the command line hides that. `headroom wrap` parses +// every flag it recognizes out of the tail and forwards only the rest to the +// agent, so an agent flag that collides with one of Headroom's — `--verbose`, +// which both Claude Code and `wrap` define — is silently eaten. Keeping the two +// apart here means scc's options go first, where a colliding pass-through +// argument still lands last and wins. +func WrapArgs(agent string, opts, rest []string) []string { + args := make([]string, 0, len(opts)+len(rest)+2) args = append(args, "wrap", agent) + args = append(args, opts...) return append(args, rest...) } +// MCPMode says which of the MCP servers `headroom wrap` would register scc +// actually wants registered. +// +// scc names the intent; which flag expresses it is discovered from the binary +// (see WrapHelp). That split is the whole point: Headroom has already renamed +// this control once — `--no-serena` became `--code-memory none` — and a flag name +// compiled into scc would have turned that release into a launch that dies on +// "no such option" instead of one that starts an agent. +type MCPMode int + +const ( + // MCPAll leaves Headroom's own defaults alone. + MCPAll MCPMode = iota + // MCPRetrieve keeps Headroom's retrieve tool, which its proxy needs to make + // compression markers actionable, and drops the code-memory server it would + // otherwise install into the user's agent config. + MCPRetrieve + // MCPNone asks for no MCP server at all, retrieve included. + MCPNone +) + +// ParseMCPMode reads the mode from the spelling a user types. +func ParseMCPMode(s string) (MCPMode, error) { + switch strings.ToLower(strings.TrimSpace(s)) { + case "all": + return MCPAll, nil + case "retrieve": + return MCPRetrieve, nil + case "none": + return MCPNone, nil + default: + return MCPAll, fmt.Errorf("unknown mcp mode %q: want all, retrieve, or none", s) + } +} + +func (m MCPMode) String() string { + switch m { + case MCPRetrieve: + return "retrieve" + case MCPNone: + return "none" + default: + return "all" + } +} + +// longFlag matches an option as `--help` prints it. Deliberately loose: a flag +// named anywhere in the help text — in the options list or in an example — is a +// flag this build accepts, and that is the only question being asked. +var longFlag = regexp.MustCompile(`--[a-z0-9][a-z0-9-]*`) + +// WrapHelp is what `headroom wrap --help` prints, or "" when the binary +// cannot answer. Advisory: an empty answer means scc passes no options rather +// than guessing at them. +func WrapHelp(bin, agent string) string { + // Combined, because a build that routes help to stderr still answers the + // question, and getting this wrong would silently disable every opt-out. + out, err := exec.Command(bin, "wrap", agent, "--help").CombinedOutput() + if err != nil { + return "" + } + return string(out) +} + +// HelpFlags is the set of long options a help text advertises. +func HelpFlags(help string) map[string]bool { + found := map[string]bool{} + for _, f := range longFlag.FindAllString(help, -1) { + found[f] = true + } + return found +} + +// MCPOffArgs is how this build spells mode, given the flags it advertises. +// +// Empty is a legitimate answer, and the caller has to treat it as one: a build +// that offers no way to decline an MCP server has told scc that the answer is no, +// which is a thing to report rather than to force. +func MCPOffArgs(flags map[string]bool, mode MCPMode) []string { + if mode == MCPAll { + return nil + } + var args []string + // The code-memory server, under whichever name this build knows it by. Newest + // spelling first so a build that still accepts an old alias is driven by the + // one its own help leads with. + switch { + case flags["--code-memory"]: + args = append(args, "--code-memory", "none") + case flags["--no-serena"]: + args = append(args, "--no-serena") + } + // tokensave was a separate server in the builds between those two spellings, + // with its own opt-out and no entry in --code-memory. + if flags["--no-tokensave"] { + args = append(args, "--no-tokensave") + } + if mode == MCPNone && flags["--no-mcp"] { + args = append(args, "--no-mcp") + } + return args +} + +// ContextToolOffArgs is how this build spells "set up no CLI context tool", or +// empty when it offers no way to say it. +// +// scc passes this by default, and the reason is a collision rather than a +// preference. Headroom's context-tool setup appends RTK guidance to +// `$PWD/CLAUDE.md` or `$PWD/AGENTS.md` — the same entry file `scc rtk` splices — +// behind its own marker pair, ``. Neither +// marker is a substring of the other, so each tool's idempotency check passes and +// both append: an entry file carrying the same RTK instructions twice, which is +// pure wasted context in every request of the session. +// +// Headroom already gates that injection behind HEADROOM_RTK, so it is off unless +// asked for. Passing the flag anyway is what makes it off *here*: an environment +// that exports HEADROOM_RTK=1 for other reasons would otherwise turn every +// `scc launch` into a second copy of a block the workspace already has. +// +// --no-context-tool first: it is the primary spelling, and it covers lean-ctx as +// well as RTK. --no-rtk is the same option's older alias. +func ContextToolOffArgs(flags map[string]bool) []string { + switch { + case flags["--no-context-tool"]: + return []string{"--no-context-tool"} + case flags["--no-rtk"]: + return []string{"--no-rtk"} + default: + return nil + } +} + // Path reports where the headroom binary is, and whether it is on PATH at all. func Path() (string, bool) { p, err := exec.LookPath(Bin) diff --git a/internal/headroom/headroom_test.go b/internal/headroom/headroom_test.go index 0ca003f..cba9795 100644 --- a/internal/headroom/headroom_test.go +++ b/internal/headroom/headroom_test.go @@ -37,7 +37,7 @@ func TestAgentRejectsAnUnknownHarness(t *testing.T) { // into their backing array would corrupt what the caller still holds. func TestWrapArgsPrefixesWithoutAliasing(t *testing.T) { rest := []string{"--resume", "--model", "opus"} - got := WrapArgs("claude", rest) + got := WrapArgs("claude", nil, rest) want := []string{"wrap", "claude", "--resume", "--model", "opus"} if strings.Join(got, " ") != strings.Join(want, " ") { @@ -48,11 +48,92 @@ func TestWrapArgsPrefixesWithoutAliasing(t *testing.T) { t.Errorf("WrapArgs wrote through to the caller's slice: %v", rest) } - if got := WrapArgs("codex", nil); strings.Join(got, " ") != "wrap codex" { + if got := WrapArgs("codex", nil, nil); strings.Join(got, " ") != "wrap codex" { t.Errorf("WrapArgs with no pass-through = %v", got) } } +// scc's own options go in front of the pass-through, so a user who names the same +// flag after `--` is the one who wins: click takes the last occurrence, and the +// argument the user typed has to beat the default scc supplied. +func TestWrapArgsPutsSCCsOptionsBeforeThePassthrough(t *testing.T) { + got := WrapArgs("claude", []string{"--code-memory", "none"}, []string{"--code-memory", "serena"}) + want := "wrap claude --code-memory none --code-memory serena" + if strings.Join(got, " ") != want { + t.Errorf("WrapArgs = %q, want %q", strings.Join(got, " "), want) + } +} + +// The opt-out flags are read off the binary's own help rather than compiled in, +// because Headroom has already renamed this control once. Each spelling has to +// resolve to the vector that build actually accepts. +func TestMCPOffArgsFollowsTheBuildsOwnSpelling(t *testing.T) { + current := HelpFlags(" --no-mcp Skip it\n --code-memory [serena|none] Code-memory MCP\n") + if got := strings.Join(MCPOffArgs(current, MCPRetrieve), " "); got != "--code-memory none" { + t.Errorf("retrieve on a current build = %q", got) + } + if got := strings.Join(MCPOffArgs(current, MCPNone), " "); got != "--code-memory none --no-mcp" { + t.Errorf("none on a current build = %q", got) + } + + // The older vocabulary, which opencode's wrap still speaks. + older := HelpFlags(" --no-mcp Skip\n --no-serena Never register Serena\n --no-tokensave Skip tokensave\n") + if got := strings.Join(MCPOffArgs(older, MCPRetrieve), " "); got != "--no-serena --no-tokensave" { + t.Errorf("retrieve on an older build = %q", got) + } + + // MCPAll is scc keeping its hands off, whatever the build offers. + if got := MCPOffArgs(current, MCPAll); len(got) != 0 { + t.Errorf("all = %v, want no arguments", got) + } +} + +// A build that advertises no opt-out has answered the question, and scc has to +// take the answer: inventing a flag would trade one unwanted MCP server for a +// launch that dies on "no such option". +func TestMCPOffArgsInventsNothing(t *testing.T) { + for _, mode := range []MCPMode{MCPRetrieve, MCPNone} { + if got := MCPOffArgs(HelpFlags("Usage: headroom wrap claude [OPTIONS]\n"), mode); len(got) != 0 { + t.Errorf("%s against a build with no opt-out = %v, want nothing", mode, got) + } + } +} + +// Headroom's context-tool setup writes RTK guidance into the same entry file +// `scc rtk` splices, behind its own marker pair — so the file ends up carrying +// the instructions twice. --no-context-tool is the primary spelling and covers +// lean-ctx too; --no-rtk is the same option's older alias. +func TestContextToolOffArgsPrefersThePrimarySpelling(t *testing.T) { + both := HelpFlags(" --no-context-tool, --no-rtk Skip CLI context-tool setup\n") + if got := strings.Join(ContextToolOffArgs(both), " "); got != "--no-context-tool" { + t.Errorf("with both spellings = %q, want --no-context-tool", got) + } + + older := HelpFlags(" --no-rtk Skip rtk setup\n") + if got := strings.Join(ContextToolOffArgs(older), " "); got != "--no-rtk" { + t.Errorf("with only the alias = %q, want --no-rtk", got) + } + + if got := ContextToolOffArgs(HelpFlags("Usage: headroom wrap claude\n")); len(got) != 0 { + t.Errorf("against a build with no opt-out = %v, want nothing", got) + } +} + +func TestParseMCPModeRoundTrips(t *testing.T) { + for _, want := range []MCPMode{MCPAll, MCPRetrieve, MCPNone} { + got, err := ParseMCPMode(want.String()) + if err != nil { + t.Fatalf("ParseMCPMode(%q): %v", want.String(), err) + } + if got != want { + t.Errorf("ParseMCPMode(%q) = %v", want.String(), got) + } + } + if _, err := ParseMCPMode("serena"); err == nil { + t.Error("ParseMCPMode accepted a mode scc does not define") + } +} + // uv is what Headroom's docs lead with and what puts the CLI in an isolated // environment, so it has to be tried first on a machine that has both. func TestInstallersPreferUV(t *testing.T) { diff --git a/internal/rtk/rtk.go b/internal/rtk/rtk.go index 2b5875d..522df1f 100644 --- a/internal/rtk/rtk.go +++ b/internal/rtk/rtk.go @@ -30,27 +30,79 @@ const Bin = "rtk" // The markers RTK itself writes. The opening one carries a version, so it is // matched by prefix: a block stamped v1 or v9 is still the block, and replacing it // with the one this build ships is exactly what an update means. +// +// Sharing RTK's markers rather than namespacing scc's own is the load-bearing +// choice here, and it is worth naming what it buys: `rtk init` writes this exact +// pair into the project's entry file, so addressing the block by these markers is +// what makes `rtk init` and `scc rtk` converge on one copy. A marker of scc's own +// — `scc:rtk-instructions`, say — would make each tool blind to the other's block +// and leave the file carrying both. const ( openPrefix = "" ) +// Foreign is a marker some other tool writes for the same guidance. +// +// Headroom's context-tool setup appends RTK instructions to the same entry file +// behind its own namespaced pair. scc cannot address that block — it is +// Headroom's, written by Headroom, refreshed on Headroom's schedule — and it must +// not silently ignore it either, because a file carrying both blocks tells the +// agent the same thing twice in every request of the session. So: detected, +// named, and left exactly where it is. +type Foreign struct { + // Tool is what to call it when telling somebody it is there. + Tool string + // Open is the marker that proves it. + Open string + // Fix is the command that removes it, run by the tool that owns it. + Fix string +} + +// foreigners are the blocks scc knows to look for. A short list on purpose: a +// marker that turns out not to be there costs one Contains call, and a marker +// nobody warns about costs a duplicated block in every session. +var foreigners = []Foreign{ + { + Tool: "Headroom", + Open: "", + Fix: "headroom unwrap ", + }, +} + +// ForeignBlock reports the other tool's RTK block in doc, if there is one. +func ForeignBlock(doc string) (Foreign, bool) { + for _, f := range foreigners { + if strings.Contains(doc, f.Open) { + return f, true + } + } + return Foreign{}, false +} + // Action is what splicing the block did to a document. type Action string const ( // Added: the document carried no block, so one was appended. Added Action = "added" - // Present: it already carried a block, which was left exactly as it was. - // - // This is the default outcome for an existing block whatever its version says, - // and the reason is ownership: RTK writes that block, stamps its own version - // into the opening marker, and `rtk init` is what refreshes it. An scc that - // rewrote it on every run would silently downgrade a v3 block to whatever this - // build happens to ship, and it would do it to a file the user owns. + // Present: the block in the document is already the one scc ships, byte for + // byte, or the caller asked for an existing block to be kept. Present Action = "present" - // Replaced: the caller passed force, so an existing block was overwritten with - // the one this build ships. + // Replaced: the document carried a different block, and scc's replaced it. + // + // This is the default, and the reason is size. `rtk init` and scc both stamp + // v2 and give the agent the same instruction, but RTK's own block spends + // roughly five times the bytes doing it — and the entry file is preloaded into + // every request of the session, so the difference is paid continuously rather + // than once. Between two blocks of the same version, the condensed one is + // simply better, and leaving the larger one in place because it got there first + // is not deference, it is a standing cost. + // + // What that does give up is version ordering: a future `rtk init` writing v3 + // would be overwritten by scc's v2. Splice therefore keeps the replaced block's + // version visible so the caller can say so, and Keep is the standing answer for + // anyone who has deliberately curated their own. Replaced Action = "replaced" ) @@ -62,19 +114,19 @@ func InstallCmd() string { return "cargo install --git " + Repo } // Splice returns doc with the block present exactly once, and what it had to do to // get there. // -// Insert only when the marker is absent. A document that already carries a block is -// returned untouched and reported Present, because that block is RTK's — see the -// Action constants. force is the explicit "replace it with what this scc ships", -// and it is the only path that ever overwrites one. +// The block scc ships wins by default — see Replaced for why. keep is the standing +// "leave whatever is already there", for a document whose block somebody curated +// on purpose. // -// It preserves the document's own line endings: an entry file checked out CRLF -// stays CRLF, because scc is a guest in this file and rewriting every line of -// someone else's document is not a change they asked for. +// Only the region between the markers is ever rewritten. Everything outside them +// is untouched, always, and the document's own line endings are preserved: an +// entry file checked out CRLF stays CRLF, because scc is a guest in this file and +// rewriting every line of someone else's document is not a change they asked for. // // A document carrying an opening marker with no closing one is malformed rather // than blockless, and it is an error: appending a second block there would leave // the file with two openings and one close, which no tool could then update. -func Splice(doc, block string, force bool) (string, Action, error) { +func Splice(doc, block string, keep bool) (string, Action, error) { block = strings.TrimRight(textutil.NormalizeNewlines(block), "\n") eol := "\n" if strings.Contains(doc, "\r\n") { @@ -100,12 +152,33 @@ func Splice(doc, block string, force bool) (string, Action, error) { return "", "", fmt.Errorf("found %s with no closing %s", openPrefix+" …", closeTag) } end += len(closeTag) - if !force || rest[:end] == block { + if keep || rest[:end] == block { return doc, Present, nil } return doc[:start] + block + doc[start+end:], Replaced, nil } +// Block returns the marker-delimited block in doc, markers included, or "" when +// doc carries none. +// +// It exists so a caller can measure what is already there against what it would +// have written. That comparison matters more than it sounds: `rtk init` and scc +// both stamp v2 and say the same thing, but RTK's own block spends roughly five +// times the bytes doing it, and the entry file is preloaded into every request of +// the session. Version ordering cannot separate those two — only size can. +func Block(doc string) string { + start := strings.Index(doc, openPrefix) + if start < 0 { + return "" + } + rest := doc[start:] + end := strings.Index(rest, closeTag) + if end < 0 { + return "" + } + return rest[:end+len(closeTag)] +} + // BlockVersion reports what the opening marker in doc claims — "v2" for // `` — or "" when doc carries no block. // diff --git a/internal/rtk/rtk_test.go b/internal/rtk/rtk_test.go index 8aeef35..5d91066 100644 --- a/internal/rtk/rtk_test.go +++ b/internal/rtk/rtk_test.go @@ -31,6 +31,67 @@ func TestShippedBlockIsMarkerDelimited(t *testing.T) { } } +// scc shares RTK's markers rather than namespacing its own, and that is what +// makes `rtk init` and `scc rtk` converge on one copy: `rtk init` writes this +// exact pair into the project's entry file. A marker of scc's own would make each +// tool blind to the other's block and leave the file carrying both. +func TestTheMarkersAreRTKsOwn(t *testing.T) { + if openPrefix != "" { + t.Errorf("markers are %q / %q, which is not what `rtk init` writes", openPrefix, closeTag) + } + // And the namespaced variant somebody will eventually propose must not match, + // or scc would claim a block it does not own. + if _, _, err := Splice("\nx\n\n", block(t), false); err != nil { + t.Errorf("a namespaced block confused the splice: %v", err) + } +} + +// Headroom writes the same guidance behind its own marker pair, into the same +// entry file. scc cannot address that block — it is Headroom's — but a file +// carrying both tells the agent the same thing twice in every request, so the one +// thing scc must not do is fail to notice. +func TestForeignBlockFindsHeadroomsCopy(t *testing.T) { + doc := "# CLAUDE.md\n\n\nuse rtk\n\n" + f, ok := ForeignBlock(doc) + if !ok { + t.Fatal("Headroom's block went undetected") + } + if f.Tool != "Headroom" || f.Fix == "" { + t.Errorf("foreign = %+v, want it named with a way to remove it", f) + } + + // scc's own block is not foreign, and neither is a document with no block. + if _, ok := ForeignBlock(block(t)); ok { + t.Error("scc's own block was reported as another tool's") + } + if _, ok := ForeignBlock("# CLAUDE.md\n\nnothing here\n"); ok { + t.Error("a blockless document reported a foreign block") + } +} + +// Neither marker is a substring of the other, which is why both tools' idempotency +// checks pass and both append. Splice must leave Headroom's block exactly where it +// is and add scc's alongside — anything else would be scc editing a document it +// does not own. +func TestSpliceLeavesAForeignBlockAlone(t *testing.T) { + foreign := "\nuse rtk\n" + doc := "# CLAUDE.md\n\n" + foreign + "\n" + + got, action, err := Splice(doc, block(t), false) + if err != nil { + t.Fatalf("Splice: %v", err) + } + if action != Added { + t.Errorf("action = %q, want %q", action, Added) + } + if !strings.Contains(got, foreign) { + t.Error("Splice modified Headroom's block") + } + if !strings.Contains(got, openPrefix) { + t.Error("Splice did not add scc's own block") + } +} + func TestSpliceAppendsToADocumentWithoutABlock(t *testing.T) { doc := "# CLAUDE.md\n\nSome rules.\n" got, action, err := Splice(doc, block(t), false) @@ -72,49 +133,65 @@ func TestSpliceIsIdempotent(t *testing.T) { } } -// The block between the markers is RTK's: `rtk init` writes it and stamps its own -// version into the opening marker. scc inserts one only where there is none, so a -// block this build does not recognize — a newer one, or one the user edited — -// survives contact. -func TestSpliceLeavesAnExistingBlockAlone(t *testing.T) { - doc := "# CLAUDE.md\n\n\n## RTK\nnewer text\n\n" +// The block scc ships wins by default, replacing whatever is between the markers +// — in place, between the markers and nowhere else. +func TestSpliceReplacesAnExistingBlockInPlace(t *testing.T) { + doc := "# CLAUDE.md\n\nAbove.\n\n\n## RTK\nold text\n\n\nBelow.\n" got, action, err := Splice(doc, block(t), false) if err != nil { t.Fatalf("Splice: %v", err) } - if action != Present { - t.Errorf("action = %q, want %q", action, Present) + if action != Replaced { + t.Errorf("action = %q, want %q", action, Replaced) } - if got != doc { - t.Errorf("an existing block was rewritten without force:\n%s", got) + if strings.Contains(got, "old text") || strings.Contains(got, "v1") { + t.Error("the old block survived") } - if v := BlockVersion(doc); v != "v9" { - t.Errorf("BlockVersion = %q, want %q", v, "v9") + if !strings.Contains(got, "# CLAUDE.md\n\nAbove.\n") || !strings.HasSuffix(got, "\nBelow.\n") { + t.Errorf("the user's own prose did not survive: %q", got) } - if v := BlockVersion("# CLAUDE.md\n\nno block here.\n"); v != "" { - t.Errorf("BlockVersion of a blockless document = %q, want empty", v) + if strings.Count(got, openPrefix) != 1 { + t.Errorf("the document carries %d opening markers, want 1", strings.Count(got, openPrefix)) } } -// force is the explicit "use the one this scc ships". It replaces the block where -// it stands, between the markers and nowhere else. -func TestSpliceReplacesAnOlderBlockInPlaceWithForce(t *testing.T) { - doc := "# CLAUDE.md\n\nAbove.\n\n\n## RTK\nold text\n\n\nBelow.\n" +// keep is the standing "leave whatever is already there", for a block somebody +// curated on purpose — or one whose version is ahead of what this build ships. +func TestSpliceKeepsAnExistingBlockWhenAsked(t *testing.T) { + doc := "# CLAUDE.md\n\n\n## RTK\nnewer text\n\n" got, action, err := Splice(doc, block(t), true) if err != nil { t.Fatalf("Splice: %v", err) } - if action != Replaced { - t.Errorf("action = %q, want %q", action, Replaced) + if action != Present { + t.Errorf("action = %q, want %q", action, Present) } - if strings.Contains(got, "old text") || strings.Contains(got, "v1") { - t.Error("the old block survived") + if got != doc { + t.Errorf("keep rewrote the block:\n%s", got) } - if !strings.Contains(got, "# CLAUDE.md\n\nAbove.\n") || !strings.HasSuffix(got, "\nBelow.\n") { - t.Errorf("the user's own prose did not survive: %q", got) + if v := BlockVersion(doc); v != "v9" { + t.Errorf("BlockVersion = %q, want %q", v, "v9") } - if strings.Count(got, openPrefix) != 1 { - t.Errorf("the document carries %d opening markers, want 1", strings.Count(got, openPrefix)) + if v := BlockVersion("# CLAUDE.md\n\nno block here.\n"); v != "" { + t.Errorf("BlockVersion of a blockless document = %q, want empty", v) + } +} + +// Block is what lets a caller measure what is there against what it would write, +// which is the whole basis for replacing it: `rtk init` and scc both stamp v2 and +// say the same thing, but one spends roughly five times the bytes. +func TestBlockExtractsWhatIsThere(t *testing.T) { + inner := "\nold text\n" + doc := "# CLAUDE.md\n\nAbove.\n\n" + inner + "\n\nBelow.\n" + if got := Block(doc); got != inner { + t.Errorf("Block = %q, want %q", got, inner) + } + if got := Block("# CLAUDE.md\n\nnothing here.\n"); got != "" { + t.Errorf("Block of a blockless document = %q, want empty", got) + } + // A half block is not a block: the opening marker without its close. + if got := Block("\ndangling\n"); got != "" { + t.Errorf("Block of a half block = %q, want empty", got) } } diff --git a/internal/validate/plan.go b/internal/validate/plan.go index 698a51b..d390551 100644 --- a/internal/validate/plan.go +++ b/internal/validate/plan.go @@ -117,10 +117,11 @@ func Plan(root, name string) (*finding.Set, error) { var loopValues = map[string]map[string]bool{ "worktree": {"per-group": true, "in-place": true}, "merge": {"auto": true, "manual": true}, + "pr": {"per-group": true, "per-plan": true}, } func checkLoopAnswers(set *finding.Set, file string, fm mdscan.Frontmatter) { - for _, key := range []string{"worktree", "merge"} { + for _, key := range []string{"worktree", "merge", "pr"} { value, ok := fm.Get(key) if !ok { continue diff --git a/internal/validate/plan_test.go b/internal/validate/plan_test.go index 656079a..25ef31c 100644 --- a/internal/validate/plan_test.go +++ b/internal/validate/plan_test.go @@ -114,16 +114,31 @@ func TestPlanKickoffAnswers(t *testing.T) { } } -// The two answers `plan-run` writes back before it starts a loop. A wrong value is -// worth a finding because the skill writes these and every later session reads them — +// The answers `plan-run` writes back before it starts a loop. A wrong value is worth +// a finding because the skill writes these and every later session reads them — // `worktree: yes` would quietly decide how the rest of the plan gets built. func TestPlanLoopAnswers(t *testing.T) { root := t.TempDir() writePlan(t, root, "sweep", - "---\nworktree: yes\nmerge: whenever\n---\n\n# Sweep\n\n- [ ] 1.1 (Unit) Do it\n") + "---\nworktree: yes\nmerge: whenever\npr: sometimes\n---\n\n# Sweep\n\n- [ ] 1.1 (Unit) Do it\n") got := planFindings(t, root, "sweep") - if n := count(got, "plan.loop-invalid"); n != 2 { - t.Errorf("rules = %v, want two plan.loop-invalid findings, got %d", got, n) + if n := count(got, "plan.loop-invalid"); n != 3 { + t.Errorf("rules = %v, want three plan.loop-invalid findings, got %d", got, n) + } +} + +// `pr` decides whether the loop opens a PR per group or one at the end, which is the +// difference between the review subagents running once and running once per group. +// Both spellings have to survive, and a resumed session reads this key to know +// whether `main` or the plan's branch is where its position lives. +func TestPlanPRShapeAcceptsBothLoops(t *testing.T) { + root := t.TempDir() + for _, shape := range []string{"per-group", "per-plan"} { + writePlan(t, root, "sweep", + "---\npr: "+shape+"\n---\n\n# Sweep\n\n- [ ] 1.1 (Unit) Do it\n") + if got := planFindings(t, root, "sweep"); contains(got, "plan.loop-invalid") { + t.Errorf("pr: %s reported %v", shape, got) + } } } @@ -137,7 +152,7 @@ func TestPlanLoopAnswersAreOptional(t *testing.T) { t.Errorf("rules = %v, want no plan.loop-invalid", got) } writePlan(t, root, "run", - "---\nautonomy: auto\nci: wait\nworktree: per-group\nmerge: auto\n---\n\n# Run\n\n- [ ] 1.1 (Unit) Do it\n") + "---\nautonomy: auto\nci: wait\npr: per-plan\nworktree: per-group\nmerge: auto\n---\n\n# Run\n\n- [ ] 1.1 (Unit) Do it\n") if got := planFindings(t, root, "run"); len(got) != 0 { t.Errorf("a plan carrying every valid answer reported %v", got) }