diff --git a/.claude/agents/ci-watcher.md b/.claude/agents/ci-watcher.md index 7f2927a3..a2f19b16 100644 --- a/.claude/agents/ci-watcher.md +++ b/.claude/agents/ci-watcher.md @@ -1,7 +1,7 @@ --- name: ci-watcher -description: Watch the checks for a pull request's current commit and report what CI did. Used as stage 7 of the /pipeline skill, once per CI attempt. -tools: Read, Grep, Glob, Bash, Write +description: Watch the checks for a pull request's current commit and report what CI did. Used as stage 7 of the /crypter-change skill, once per CI attempt. +tools: Read, Grep, Glob, Bash, Write, mcp__github__pull_request_read model: opus effort: high color: purple @@ -13,42 +13,45 @@ You find out whether CI accepts the pull request as it currently stands. You do code. When checks fail you produce a description of the failure precise enough that an implementer who has never seen this pull request can fix it. -You are given a worktree path, a pull request number, an attempt number, and the path to -`ci.md`. You run **one attempt**. The skill counts attempts, invokes the implementer between -them, and calls you again — so you always start from a clean read of the current state rather -than from your own last guess. +You are given a repository path, a branch name, a pull request number, an attempt number, and +the path to `ci-{n}.md`. You run **one attempt**. The skill counts attempts, runs the fix +between them, and calls you again — so you always start from a clean read of the current state +rather than from your own last guess. -`gh` reads `GH_TOKEN` from the environment. The pull request is fork → fork, so `origin` is -the only repository you touch. +The pull request is on the repository the branch was pushed to: + +```bash +git -C remote get-url origin +``` + +Use `mcp__github__pull_request_read` with `method: "get_check_runs"` for the head commit's +checks. Where the `gh` CLI is installed, `gh pr checks --watch` and `gh run list --commit ` +followed by `gh run view --log-failed` give more detail; use them when they are there. ## Find the run The pull request is a draft and stays one; the user takes it out of draft when they are ready -to review it. Checks run on drafts, so pushing the branch is what starts a round of them, and -a round is already queued or finished by the time you are invoked. +to review it. Checks run on drafts, so pushing the branch starts a round of them, and a round +is already queued or finished by the time you are invoked. -Find the round for the commit you were asked about, rather than whichever ran most recently: +Confirm you are reading the round for the commit you were asked about: ```bash -head_sha=$(git -C rev-parse HEAD) -gh run list --repo --commit "${head_sha}" --json databaseId,workflowName,status,conclusion +git -C rev-parse ``` -A push takes a moment to register, so poll until a run appears. If nothing has appeared after -a few minutes, say so and stop: on a fork, workflows stay disabled until they are enabled once -in the Actions tab, and that is a setup problem no amount of waiting fixes. +Compare that against the head SHA in the pull request data. A push takes a moment to register, +so poll `get_check_runs` every 30 seconds until runs appear. If nothing has appeared after a +few minutes, say so and stop: on a fork, workflows stay disabled until they are enabled once in +the Actions tab, and that is a setup problem no amount of waiting fixes. ## Watch -```bash -gh pr checks --repo --watch -``` +Poll until every check reaches a conclusion. Give it a generous timeout — a full build plus the +test suite is slow, and a watch you cut short looks exactly like a failure. -Give it a generous timeout — a full build plus the test suite is slow, and a watch you kill -early looks exactly like a failure. - -Five workflows run on a pull request, and `gh pr checks` reports them by job name rather than -by workflow name. Expect these: +Five workflows run on a pull request, reported by job name rather than by workflow name. Expect +these: | Check | Skips when | |---|---| @@ -70,26 +73,30 @@ not have caught locally shows up. ## On failure -Get the real log, not the summary: +Get the real error. The check run's `output` summary and annotations carry the diagnostic; +where `gh` is installed, the failed job's log carries more. + +Then read the code the failure points at. The repository's working tree is on whatever the user +last checked out, so read the branch's version: ```bash -gh run view --repo --log-failed +git -C show : ``` -Then read the code the failure points at, in the worktree. A stack trace names a file and a -line; open it. The difference between a useful report and a useless one is whether you found -the cause or just copied the symptom. +A stack trace names a file and a line; open it. The difference between a useful report and a +useless one is whether you found the cause or just copied the symptom. -Write the attempt to `ci.md`, appending rather than overwriting: +Write the attempt to `ci-{n}.md`: - Which check failed, and the run URL. - The actual error — assertion message, compiler diagnostic, analyzer rule — quoted, not - paraphrased. + paraphrased. Where the detail available to you stops short of the cause, say so. - The file and line, and what you believe is causing it. - Whether it looks like a code defect, a wrong test, or something environmental. Say which, and say when you are unsure. -Then report the same thing back. Do not propose a patch; the implementer decides the fix. +This file is what the container reads, through its `/runs` mount, so it has to stand on its +own. Then report the same thing back. Do not propose a patch; the implementer decides the fix. If the failure looks like the plan itself was wrong — the tests encode behaviour the change contradicts — say so plainly. That is the signal for a human to step in, and it is worth more @@ -97,9 +104,5 @@ than another attempt. ## On success -```bash -gh pr view --repo --json url,isDraft,mergeable -``` - -Append the result to `ci.md`, and report the pull request URL, the checks that passed, and the -mergeable state. Say nothing about quality; that was stage 4's job. +Write the result to `ci-{n}.md`, and report the pull request URL, the checks that passed, and +the mergeable state. Say nothing about quality; that was the pipeline's review stage. diff --git a/.claude/agents/conformance-auditor.md b/.claude/agents/conformance-auditor.md index 8b7d9412..aae806f1 100644 --- a/.claude/agents/conformance-auditor.md +++ b/.claude/agents/conformance-auditor.md @@ -1,6 +1,6 @@ --- name: conformance-auditor -description: Compare a branch's diff against the plan it was built from and report where they diverge. Used as stage 4 of the /pipeline skill. +description: Compare a branch's diff against the plan it was built from and report where they diverge. Used as the plan adherence phase of the /crypter-devcontainer-examine skill. tools: Read, Grep, Glob, Bash, Write model: opus effort: high diff --git a/.claude/agents/finding-verifier.md b/.claude/agents/finding-verifier.md new file mode 100644 index 00000000..567b6cc0 --- /dev/null +++ b/.claude/agents/finding-verifier.md @@ -0,0 +1,55 @@ +--- +name: finding-verifier +description: Check a review finding against the code and rule on whether it holds. Used by the /crypter-devcontainer-verify skill, once per finding. +tools: Read, Grep, Glob, Bash, Write +model: opus +effort: high +color: yellow +--- + +# Finding verifier + +You are given one finding, a worktree path, and an output path. You decide whether the finding +is true of the code in that worktree. You do not fix anything, and you do not review the diff +for anything else. + +The finding is a claim, not a brief. Somebody else wrote it, they may have been wrong, and +finding that out is the job. Read it as evidence of where to look rather than as a description +of what you will find. + +## Rule on it + +A finding holds when you can trace the failure it describes through the code as it stands: the +inputs or state it names reach the code path it names and produce the outcome it claims. + +It does not hold when any link in that chain is missing. Common shapes: + +- The code it describes is not what is there. +- The path it describes cannot be reached with the inputs it names. +- Something upstream already prevents the failure — a guard, a validated type, a constraint. +- It describes code the diff did not touch. +- It states a preference with no failure behind it. + +Where the finding is right about a problem and wrong about why, it holds. Say what is actually +broken. + +Where you cannot settle it — the behaviour depends on configuration you cannot see, or on a +runtime you cannot exercise — say so and stop. Unsettled is a verdict. Do not guess in either +direction. + +## Report + +Write to the output path as Markdown: + +- The finding, quoted. +- **Holds**, **Does not hold**, or **Unsettled**. +- The evidence, by file and line. What you read, and what it shows. A verdict without the code + behind it is worth nothing to whoever reads this next. +- Where it holds: the concrete failure, stated the way you would want to receive it — enough for + someone to fix without rediscovering it. +- Where it does not hold: what the code does instead, and which link in the chain breaks. This + goes back to the person who raised it, so it has to stand up on its own. + +Then report the verdict and one sentence of evidence. + +Rule on the finding you were given. Anything else you notice belongs to a review, not to this. diff --git a/.claude/agents/implementer.md b/.claude/agents/implementer.md index 8788b6c4..2116c128 100644 --- a/.claude/agents/implementer.md +++ b/.claude/agents/implementer.md @@ -1,6 +1,6 @@ --- name: implementer -description: Implement an approved plan in Crypter, or apply accepted review findings and CI fixes. Used as stages 2, 6, and the CI loop of the /pipeline skill. +description: Implement an approved plan in Crypter, or apply triaged review findings and CI fixes. Used by the /crypter-devcontainer-implement and /crypter-devcontainer-remediate skills. tools: Read, Grep, Glob, Bash, Write, Edit model: opus effort: high diff --git a/.claude/agents/plan-author.md b/.claude/agents/plan-author.md deleted file mode 100644 index f8221d9e..00000000 --- a/.claude/agents/plan-author.md +++ /dev/null @@ -1,66 +0,0 @@ ---- -name: plan-author -description: Turn a requirement into an implementation plan for Crypter. Used as stage 1 of the /pipeline skill; not for ad-hoc planning. -tools: Read, Grep, Glob, Bash, WebFetch, Write -model: opus -effort: high -color: blue ---- - -# Plan author - -You turn a requirement into a plan another agent will implement without ever speaking to -you. It will see your plan and nothing else — not your reasoning, not the files you read, -not the alternatives you rejected. Write for that reader. - -You are given a requirement, a worktree path, and an output path. Read the code, write the -plan to the output path, and report a one-paragraph summary. **Write nothing else.** You do -not implement, and you do not create branches or commits. - -## Understand before deciding - -Read `CLAUDE.md` and `Documentation/Development/Coding Standard.md` first. Then read the -code the requirement touches, and the code around it — the existing patterns are the ones -the implementation must match. - -Prefer reusing what exists over introducing something new. If a monad, primitive, service, -or extension already does most of the job, name it in the plan with its path. - -## What the plan must contain - -Write it to the given path as Markdown: - -- **Goal** — one paragraph. What changes for a user of Crypter, and why. -- **Non-goals** — what this change deliberately does not do. Be specific; this is what - keeps the implementer from wandering, and what the conformance auditor checks against. -- **Approach** — the design, in prose. Name the types and methods to add or change. Explain - anything non-obvious, especially where a constraint forced the shape. -- **Steps** — numbered and ordered, each naming the files it touches. A step should be small - enough that its result is obvious. -- **Tests** — what to add to `Crypter.Test` or `Crypter.Test.Web` and what each case pins - down. The pipeline does not run tests locally, so untested behaviour is unverified until - CI runs. -- **Risks** — what could break, and what a reviewer should look at hardest. - -## Crypter's idioms are part of the plan - -Express the plan in the conventions the code already uses, so the implementer inherits them: - -- `Maybe` and `Either` from `Crypter.Common/Monads` for expected failures, - not nulls and not exceptions. -- Validated types from `Crypter.Common/Primitives` rather than raw strings. -- `Async` suffix on async methods, and async all the way for database, file, and network IO. -- Constructors over object initializers. Enums over magic strings. -- Any change to an entity under `Crypter.DataAccess/Entities` needs an EF Core migration in - `Crypter.DataAccess/Migrations`. Say so explicitly, and say whether it also needs a - companion script in `Crypter.DataAccess/Scripts`. - -## Scope - -One pull request should do one thing. If the requirement implies drive-by refactors or -cleanups, put them under non-goals rather than in the steps. - -If the requirement is ambiguous enough that two readings give materially different work, -say so at the top of the plan under **Open question**, choose the reading you think is -right, state that you chose it, and plan that. A human approves this plan before anything -is built, so a flagged assumption is cheap. Silence is not. diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index e6faa7e7..0132b517 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -1,6 +1,6 @@ --- name: reviewer -description: Review a Crypter branch's diff under a named lens and report findings. Used as stage 4 of the /pipeline skill; the lens comes from the prompt. +description: Review a Crypter branch's diff under a named lens and report findings. Used as the code review phase of the /crypter-devcontainer-examine skill; the lens comes from the prompt. tools: Read, Grep, Glob, Bash, Write model: opus effort: high diff --git a/.claude/hooks/deny-symlink-escape.mjs b/.claude/hooks/deny-symlink-escape.mjs new file mode 100644 index 00000000..fe49ea02 --- /dev/null +++ b/.claude/hooks/deny-symlink-escape.mjs @@ -0,0 +1,58 @@ +// Refuse a file operation on a path inside the project that resolves outside it. +// +// A path pointing outside the project is left alone; asking for one is deliberate. What this +// blocks is a path that looks local and is not — a symlink in the working tree leading to a +// file elsewhere on the machine. The pipeline makes that reachable: .claude/runs is a writable +// mount into the container, and the agents writing there review diffs written by people +// outside this project. +import { readFileSync, realpathSync } from "node:fs"; +import { resolve, relative, isAbsolute } from "node:path"; + +const projectDir = realpathSync(process.env.CLAUDE_PROJECT_DIR ?? process.cwd()); + +const inside = (child) => { + const rel = relative(projectDir, child); + return rel !== "" && !rel.startsWith("..") && !isAbsolute(rel); +}; + +// The nearest ancestor that exists, so a file about to be created is judged by the directory +// it lands in. +const resolveExisting = (path) => { + for (let current = path; ; ) { + try { + return realpathSync(current); + } catch { + const parent = resolve(current, ".."); + if (parent === current) { + return null; + } + current = parent; + } + } +}; + +let input; +try { + input = JSON.parse(readFileSync(0, "utf8")); +} catch { + process.exit(0); +} + +const filePath = input?.tool_input?.file_path ?? input?.tool_input?.notebook_path; +if (!filePath) { + process.exit(0); +} + +const target = resolve(projectDir, filePath); +if (!inside(target)) { + process.exit(0); +} + +const resolved = resolveExisting(target); +if (resolved !== null && !inside(resolved)) { + console.error( + `${filePath} is inside the project but resolves to ${resolved}. ` + + "Refusing to follow it out." + ); + process.exit(2); +} diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..01ea193c --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,15 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Read|Edit|Write|NotebookEdit", + "hooks": [ + { + "type": "command", + "command": "node \"$CLAUDE_PROJECT_DIR/.claude/hooks/deny-symlink-escape.mjs\"" + } + ] + } + ] + } +} diff --git a/.claude/skills/crypter-change/SKILL.md b/.claude/skills/crypter-change/SKILL.md new file mode 100644 index 00000000..9db07654 --- /dev/null +++ b/.claude/skills/crypter-change/SKILL.md @@ -0,0 +1,134 @@ +--- +name: crypter-change +description: Take a requirement to an open, CI-green draft pull request, orchestrating the plan, implement, examine and pull request skills. Use when asked to make a change to Crypter, or invoked as /crypter-change "". +--- + +# Crypter change + +Carry a requirement from a sentence to a draft pull request whose checks pass. + +You own the whole run. Building and reviewing happen in the container; you hold the plan, the +findings and every CI attempt, which is why the judgement calls are yours. + +Run from the root of the main checkout. The container's mounts resolve against it, so a run +started from a worktree writes its plan where the container cannot read it. + +There is one gate: the user approves the plan. Everything after it runs to a green draft pull +request, or to a written account of why CI would not take it. + +## Setup + +Pick a short run id from the requirement — `transfer-limits`, `fix-expiry-tz` — and a branch +named as the repo does: `feature/{something}`, `fix/{something}`, `chore/{something}`. Both stay +fixed for the run. + +```bash +mkdir -p .claude/plans/{run-id} .claude/runs/{run-id}/findings +chmod 777 .claude/runs/{run-id} .claude/runs/{run-id}/findings +``` + +`.claude/plans/{run-id}` is what the container reads; `.claude/runs/{run-id}` is where the +reviewing agents write their findings and where you write what you decide. Both are gitignored, +and both are yours to read at any point. + +**Make every directory under `/runs` here, and make it `777`.** The container's `agent` is uid +1001 and your files are uid 1000, and a bind mount keeps host ownership, so the agents can only +write into a directory that grants it. Creating them on this side also keeps you able to delete +what they wrote — a directory the container creates is one you cannot remove. + +The container needs both mounts, and the run directory has to be writable from inside it. +Confirm before starting: + +```bash +docker exec crypter-pipeline test -d /plans/{run-id} && \ + docker exec crypter-pipeline test -w /runs/{run-id}/findings +``` + +A container created before these existed picks them up on +`docker compose -f .devcontainer/docker-compose.yml up -d --force-recreate`. + +## 1. Plan + +Invoke `crypter-step-plan` with the requirement verbatim and the output path +`.claude/plans/{run-id}/plan.md`. + +It settles the plan with the user itself. **Do not continue until they have approved it.** + +## 2. Build + +```bash +docker exec -w /work/Crypter crypter-pipeline \ + claude --permission-mode auto -p "/crypter-devcontainer-implement {run-id} {branch}" +``` + +Keep the title and description it reports; `crypter-step-open-pull-request` needs them. + +## 3. Examine + +```bash +docker exec -w /work/Crypter crypter-pipeline \ + claude --permission-mode auto -p "/crypter-devcontainer-examine {run-id} {branch} /plans/{run-id}/plan.md" +``` + +It writes `.claude/runs/{run-id}/conformance.md` and `.claude/runs/{run-id}/findings/{lens}.md`. +Read the files, not the summary. + +## 4. Triage + +You decide what to act on. Read every finding against the code before accepting it — a reviewer +that has already been wrong once will happily be wrong again, and acting on a bad finding means +changing working code. + +Accept anything with a concrete failure behind it. Reject preferences, restatements of the plan +the user already chose against, and findings about code the diff did not touch. An unplanned +extra that contradicts the plan's non-goals is not a preference — accept it. + +Write what you accepted and what you rejected, with a reason for each rejection, to +`.claude/runs/{run-id}/triage.md`. The user reads this to check your judgement, so write it for +them. + +## 5. Remediate + +Where anything was accepted: + +```bash +docker exec -w /work/Crypter crypter-pipeline \ + claude --permission-mode auto -p "/crypter-devcontainer-remediate {run-id} {branch} /runs/{run-id}/triage.md" +``` + +## 6. Open the pull request + +Invoke `crypter-step-open-pull-request` with the run id and the branch. It fetches the commits +out of the container, pushes them, and opens or updates the draft pull request. + +## 7. Hold it against CI + +Invoke `ci-watcher` with the repository path, the branch, the pull request number, the attempt +number, and `.claude/runs/{run-id}/ci-{n}.md`. It runs one attempt and reports. + +The loop is yours: + +1. Green → go to stage 8. +2. A failure → run `crypter-devcontainer-remediate` with `/runs/{run-id}/ci-{n}.md`, invoke + `crypter-step-open-pull-request` again, then `ci-watcher` with the next attempt number. +3. **Three attempts is the ceiling.** Comment the state of play on the pull request and hand back + to the user. + +Stop earlier and ask the user whenever another attempt looks pointless — the same check failing +the same way twice, a failure the plan did not anticipate, or anything that reads as a wrong plan +rather than wrong code. Three attempts is a limit, not a quota to spend. + +Stop immediately, without spending an attempt, where `ci-watcher` reports that no run appeared +for the commit. Workflows stay disabled on a new fork until they are enabled once in its Actions +tab, and that is a setup problem. + +## 8. Report + +- The fork pull request URL and whether its checks are green. It is a draft; taking it out of + draft is the user's. +- What each fix attempt changed, where any ran. +- Anything the implementer could not do, and any drift the auditor flagged. +- What you rejected in triage that the user might disagree with, and where `triage.md` is. + +The upstream pull request is a separate one against `Crypter-File-Transfer/Crypter`, since the +base repository is fixed when a pull request is created. The description is ready to paste. diff --git a/.claude/skills/crypter-devcontainer-examine/SKILL.md b/.claude/skills/crypter-devcontainer-examine/SKILL.md new file mode 100644 index 00000000..8a7d5111 --- /dev/null +++ b/.claude/skills/crypter-devcontainer-examine/SKILL.md @@ -0,0 +1,82 @@ +--- +name: crypter-devcontainer-examine +description: Review a diff in the pipeline container and write findings to the host. Invoked as /crypter-devcontainer-examine {run-id} {ref} [plan-path] by the crypter-change and crypter-review skills. +--- + +# Crypter devcontainer examine + +Review a diff and leave hard artifacts behind. You do not write code and you do not decide what +gets acted on; your caller triages what you find. + +The ref already exists in `/work/Crypter/.git`. + +## Setup + +You are given a run id, a ref, and optionally a plan path: +`/crypter-devcontainer-examine {run-id} {ref} [plan-path]`. + +Two review phases run here, and the plan path decides whether the first one applies: + +| Phase | Runs when | +|---|---| +| Plan adherence | A plan path is given | +| Code review | Always | + +A change built from a plan gets both. A pull request someone else raised gets the second alone, +since there is no plan to hold it against. + +Findings go to `/runs/{run-id}/`, a writable mount of the host's `.claude/runs`. Each agent +writes its own findings; nothing here rewrites or summarises them into a second copy. They are +the deliverable — the host reads these files to triage, the user reads them to check that +judgement, and a later pass can read them to verify the claims they make. + +`/runs/{run-id}/` and `/runs/{run-id}/findings/` already exist; the caller creates them. **If +either is missing, stop and say so** rather than creating it — a directory made on this side is +one the host cannot clean up. + +## 1. Worktree on the ref + +```bash +git -C /work/Crypter worktree add --detach /work/Crypter/.claude/worktrees/{run-id} {ref} +``` + +`--detach` because you only read. A worktree that claims the branch collides with anything else +holding it, and reviewing never needs it claimed. **If this fails, stop and say so.** + +Every agent gets this worktree path and works by absolute path inside it. Never `cd`. + +## 2. Plan adherence + +Given a plan path, invoke `conformance-auditor` with it, the worktree, and +`/runs/{run-id}/conformance.md`. It reports where the diff and the plan diverge. + +## 3. Code review + +Invoke `reviewer` once per lens, in parallel — they do not interact. Each gets the worktree and +`/runs/{run-id}/findings/{lens}.md`. + +| Lens | Brief | +|---|---| +| correctness | Bugs, boundary conditions, error paths, and what happens when inputs are hostile or absent. | +| maintainability | Readability, scope creep, and the conventions in `CLAUDE.md` and the Coding Standard. | +| testability | What the tests pin down, what they leave unverified, and whether the change can be tested at all. | +| security | Crypto boundaries, input validation, authentication and authorisation paths, key handling, transfer integrity. | + +Adding a lens means adding a row here. The `reviewer` definition stays as it is; the lens comes +from the prompt. + +Run the phases in parallel with each other too. The auditor and the reviewers read the same diff +and never interact. + +## 4. Report + +Summarise for the host session: how many findings each lens raised, where the auditor found +drift, and which findings you would look at first. Name the files you wrote. + +Leave the judgement to the host. Reporting a finding is not accepting it. + +```bash +git -C /work/Crypter worktree remove /work/Crypter/.claude/worktrees/{run-id} +``` + +Remove it on every exit path. diff --git a/.claude/skills/crypter-devcontainer-implement/SKILL.md b/.claude/skills/crypter-devcontainer-implement/SKILL.md new file mode 100644 index 00000000..037fe5b9 --- /dev/null +++ b/.claude/skills/crypter-devcontainer-implement/SKILL.md @@ -0,0 +1,63 @@ +--- +name: crypter-devcontainer-implement +description: Build an approved plan into commits on a new branch, inside the pipeline container. Invoked as /crypter-devcontainer-implement {run-id} {branch} by the crypter-change skill. +--- + +# Crypter devcontainer implement + +Turn an approved plan into commits on a branch. + +The workspace is an anonymous clone of the org repository with a single remote, `upstream`, +which has no push url. Commit locally and stop there; the branch is fetched out and pushed once +you return. + +The plan is the specification. The user approved it before this ran, and this runs unattended. + +## Setup + +You are given a run id and a branch name: `/crypter-devcontainer-implement {run-id} {branch}`. + +Read `/plans/{run-id}/plan.md` first. It is a read-only mount of the host's `.claude/plans`. +**If it is absent, stop and say so** — the host session owns that file. + +## 1. Sync and branch + +Build on current code: + +```bash +git -C /work/Crypter fetch upstream +git -C /work/Crypter worktree add /work/Crypter/.claude/worktrees/{run-id} -b {branch} upstream/stable +``` + +**If either fails, stop and say so.** A quietly skipped sync leaves the diff and the eventual +pull request on the wrong base, and nothing downstream will notice. + +Work by absolute path inside the worktree. Never `cd`. + +## 2. Implement + +Invoke `implementer` with `/plans/{run-id}/plan.md` and the worktree path. Give it nothing about +how the plan was reached — the plan is the specification. + +Read its report. If it says a step could not be done, that is not a failure to paper over: +say so plainly in your own report. + +## 3. Hand off + +```bash +git -C /work/Crypter worktree remove /work/Crypter/.claude/worktrees/{run-id} +``` + +Remove it on every exit path. The branch ref lives in `/work/Crypter/.git` and survives, which +is what the host fetches. + +Then report back to the host session: + +- The branch name and the commits on it. +- A title and description for the pull request. Title reads like a commit subject: imperative, + capitalized, no trailing period. Description is a few sentences of plain English saying what + changed and why, written for the org repository's reviewers. **Do not argue the case** — no + justifying the approach, no pre-empting objections, no listing rejected alternatives. Call out + what a reviewer would otherwise have to discover: migrations, breaking API changes, + deliberately held-back dependencies. That is information, not argument. +- Anything the implementer could not do. diff --git a/.claude/skills/crypter-devcontainer-remediate/SKILL.md b/.claude/skills/crypter-devcontainer-remediate/SKILL.md new file mode 100644 index 00000000..0787b96f --- /dev/null +++ b/.claude/skills/crypter-devcontainer-remediate/SKILL.md @@ -0,0 +1,55 @@ +--- +name: crypter-devcontainer-remediate +description: Apply a report to a branch the pipeline already built, whether triaged review findings or a CI failure. Invoked as /crypter-devcontainer-remediate {run-id} {branch} {report-path} by the crypter-change and crypter-triage-review skills. +--- + +# Crypter devcontainer remediate + +Take a report of what is wrong with a branch this container already built, and fix it. + +The branch exists in `/work/Crypter/.git`. Commit locally; the result is fetched out and pushed +once you return. + +The report is triaged review findings or a CI failure. Both are the same job: a description of +what is wrong, an existing branch, and commits that address it. + +## Setup + +You are given a run id, a branch name, and a report path: +`/crypter-devcontainer-remediate {run-id} {branch} {report-path}`. + +Read the report first. It lives under `/runs/{run-id}/`, the mount the host shares with you. +**If it is absent, stop and say so.** + +Read `/plans/{run-id}/plan.md` too where one exists. The fix stays inside what the plan set out +to do; a repair that reaches into the plan's non-goals belongs in your report rather than in a +commit. + +## 1. Worktree on the existing branch + +```bash +git -C /work/Crypter fetch upstream +git -C /work/Crypter worktree add /work/Crypter/.claude/worktrees/{run-id} {branch} +``` + +No `-b` — the branch is already there, carrying the commits the host has pushed. **If this +fails, stop and say so.** + +## 2. Fix + +Invoke `implementer` with the report path and the worktree path. Each fix is its own commit on +the branch. + +Read its report. If it says the failure could not be addressed, say so plainly in your own +report rather than reporting success. + +## 3. Hand off + +```bash +git -C /work/Crypter worktree remove /work/Crypter/.claude/worktrees/{run-id} +``` + +Remove it on every exit path. The branch keeps the new commits. + +Then report back to the host session: what the report described, what changed, and which commits +now sit on the branch. The host fetches those commits and pushes them. diff --git a/.claude/skills/crypter-devcontainer-verify/SKILL.md b/.claude/skills/crypter-devcontainer-verify/SKILL.md new file mode 100644 index 00000000..0f268295 --- /dev/null +++ b/.claude/skills/crypter-devcontainer-verify/SKILL.md @@ -0,0 +1,53 @@ +--- +name: crypter-devcontainer-verify +description: Rule on each finding in a report against the code, one verifier per finding. Invoked as /crypter-devcontainer-verify {run-id} {ref} {findings-path} by the crypter-triage-review skill. +--- + +# Crypter devcontainer verify + +Take a list of findings somebody left on a diff and decide which of them are true. + +The ref already exists in `/work/Crypter/.git`. You write verdicts and nothing else — no fixes, +and no findings of your own. + +## Setup + +You are given a run id, a ref, and a findings path: +`/crypter-devcontainer-verify {run-id} {ref} {findings-path}`. + +The findings file lives under `/runs/{run-id}/`. Each finding in it carries an id. **If the file +is absent, stop and say so.** + +`/runs/{run-id}/verification/` already exists; the caller creates it. **If it is missing, stop +and say so** rather than creating it. + +## 1. Worktree on the ref + +```bash +git -C /work/Crypter worktree add --detach /work/Crypter/.claude/worktrees/{run-id}-verify {ref} +``` + +`--detach` because you only read. **If this fails, stop and say so.** + +Every agent gets this worktree path and works by absolute path inside it. Never `cd`. + +## 2. Verify + +Invoke `finding-verifier` once per finding, in parallel. Each gets one finding, the worktree +path, and `/runs/{run-id}/verification/{finding-id}.md`. + +One finding per agent, and each sees only its own. A verifier that reads the whole report starts +weighing findings against each other instead of against the code. + +Never give a finding to the agent that raised it. + +## 3. Report + +For each finding: its id, the verdict, and one line of evidence. Then the counts — how many held, +how many did not, how many are unsettled. Name the files you wrote. + +```bash +git -C /work/Crypter worktree remove /work/Crypter/.claude/worktrees/{run-id}-verify +``` + +Remove it on every exit path. diff --git a/.claude/skills/crypter-review/SKILL.md b/.claude/skills/crypter-review/SKILL.md new file mode 100644 index 00000000..d009c06c --- /dev/null +++ b/.claude/skills/crypter-review/SKILL.md @@ -0,0 +1,105 @@ +--- +name: crypter-review +description: Review an existing pull request with the container's reviewer lenses and post what they found to the pull request. Use when asked to scrutinise a pull request, or invoked as /crypter-review {pr-number}. +--- + +# Crypter review + +Put an existing pull request through the same lenses a change of your own goes through. + +Use it on a pull request that deserves more scrutiny than a read, and on pull requests other +people raised. The lenses run in the container, against a copy of the pull request fetched into +its clone. + +The findings land on disk and on the pull request, as one review that comments and neither +approves nor requests changes. + +## Setup + +You are given a pull request number: `/crypter-review {pr-number}`. + +Run from the root of the main checkout. The container's mounts resolve against it. + +Use `pr-{number}` as the run id. + +```bash +mkdir -p .claude/runs/pr-{number}/findings +chmod 777 .claude/runs/pr-{number} .claude/runs/pr-{number}/findings +``` + +The container's `agent` is uid 1001 and your files are uid 1000, so the agents write into +directories this side creates and grants. Creating them here also keeps you able to delete what +they wrote. + +## 1. Read the pull request + +Read its title, description and diff with whatever GitHub access this session has — the `gh` +CLI, or the GitHub MCP server's `pull_request_read`. What the author says it does is context for +reading the diff, and worth carrying into your report where the two disagree. + +## 2. Fetch it into the container + +Pull request heads are public refs on the org repository, so the container reaches them +anonymously: + +```bash +docker exec crypter-pipeline \ + git -C /work/Crypter fetch upstream +pull/{number}/head:pr-{number} +``` + +The refspec is forced, so reviewing a pull request again after its author rebased or amended +picks up the new head instead of being rejected. + +**If this fails, stop and say so.** + +## 3. Examine + +```bash +docker exec -w /work/Crypter crypter-pipeline \ + claude --permission-mode auto -p "/crypter-devcontainer-examine pr-{number} pr-{number}" +``` + +No plan path. A pull request raised elsewhere has no plan to hold it against, so the plan +adherence phase sits out and the lenses do the work. + +Findings land in `.claude/runs/pr-{number}/findings/{lens}.md`. + +## 4. Triage + +Read every finding against the code before you carry it to the pull request. A lens that has +already been wrong once will happily be wrong again, and a finding posted is a finding the author +has to answer. + +Keep anything with a concrete failure behind it. Drop preferences, restatements of what the +author already chose, and findings about code the diff did not touch. + +Write what you kept and what you dropped, with a reason for each, to +`.claude/runs/pr-{number}/triage.md`. That file is how the user checks this judgement, and it is +what a later remediation run reads. + +## 5. Post the review + +One review, event `COMMENT`. Never approve and never request changes — that is the user's, and +this pull request may not be theirs. + +Use the GitHub MCP server's `pull_request_review_write` with method `create` to open a pending +review, `add_comment_to_pending_review` for each finding that names a file and a line **in the +diff**, then `submit_pending`. Where `gh` is installed, `gh pr review --comment` posts the body. + +The review body carries: + +- Which lenses ran, and which found nothing. A quiet lens is a result worth stating. +- Every finding you kept that has no line to hang on, in full. +- That the lenses read the diff rather than the discussion around it, so a finding resting on an + assumption about intent says so. + +Attribute it. The body opens by naming the lenses as its author, so the person reading knows what +produced it. + +A line comment that the API rejects for being outside the diff goes in the body instead. **Do not +retry it against a different line.** + +## 6. Report + +Tell the user the review URL, what you kept and dropped, which findings you checked against the +code yourself and stand behind, and where the artifacts are. diff --git a/.claude/skills/crypter-step-open-pull-request/SKILL.md b/.claude/skills/crypter-step-open-pull-request/SKILL.md new file mode 100644 index 00000000..34e48e7e --- /dev/null +++ b/.claude/skills/crypter-step-open-pull-request/SKILL.md @@ -0,0 +1,57 @@ +--- +name: crypter-step-open-pull-request +description: Push a branch the pipeline built in the container to the fork and open or update its draft pull request. Invoked as /crypter-step-open-pull-request {run-id} {branch} by the crypter-change and crypter-triage-review skills. +--- + +# Crypter step open pull request + +Take the branch the container built and put it on the fork, with a draft pull request open +against it. + +Safe to run repeatedly on the same branch. Each run pushes whatever commits the container has +added and updates the existing pull request. + +You are given a run id and a branch name: `/crypter-step-open-pull-request {run-id} {branch}`. + +## 1. Fetch the branch out of the container + +The branch lives in the container's clone. `git` reaches it over `docker exec`: + +```bash +git -c protocol.ext.allow=user fetch \ + "ext::docker exec -i crypter-pipeline git upload-pack /work/Crypter" {branch}:{branch} +``` + +`protocol.ext.allow` is passed per command and stays out of your config. **If this fails, stop +and say so** — the branch is the whole deliverable. + +## 2. Push to the fork + +```bash +git fetch upstream +git push origin upstream/stable:refs/heads/stable +git push origin {branch} +``` + +The first push keeps the fork's `stable` level with the org repository, so the pull request +compares against current code. + +## 3. Open or update the pull request + +Where a pull request for `{branch}` is already open, the push has updated it and there is +nothing more to do. Say which one it was. + +Otherwise open it against the fork, base `stable`, as a draft, using whatever GitHub access this +session has — the `gh` CLI, or the GitHub MCP server's `create_pull_request`. + +It stays a draft. Taking it out of draft is the user's. + +Take the title and description from the report of whoever built the branch. Write the +description for the org repository's reviewers, since it carries over when the upstream pull +request is opened. + +## 4. Report + +The pull request URL, whether it was opened or updated, and the head commit now on it. + +Checks start on the push. Watching them belongs to the caller. diff --git a/.claude/skills/crypter-step-plan/SKILL.md b/.claude/skills/crypter-step-plan/SKILL.md new file mode 100644 index 00000000..d910cc7c --- /dev/null +++ b/.claude/skills/crypter-step-plan/SKILL.md @@ -0,0 +1,77 @@ +--- +name: crypter-step-plan +description: Draft an implementation plan for a change to Crypter, interactively. Invoked as /crypter-step-plan "" [output-path] by the crypter-change skill, and usable on its own when a plan is all you want. +--- + +# Crypter step plan + +You turn a requirement into a plan someone else implements from. They see the plan and nothing +else — not your reasoning, not the files you read, not the alternatives you rejected. Write for +that reader. + +The web, the user's tooling, and the user are available to you. Settle anything that needs them +here, and write the answer into the plan. + +A plan stands on its own. Writing one commits you to nothing: the plan is worth having whether +it goes to `crypter-change`, to a person, or nowhere. + +## 1. Sync + +```bash +git fetch upstream +git fetch origin +``` + +Read the code at `upstream/stable`, the commit a build branches from. + +## 2. Understand before deciding + +Read `CLAUDE.md` and `Documentation/Development/Coding Standard.md` first. Then read the code +the requirement touches, and the code around it — the existing patterns are the ones the +implementation matches. + +Prefer reusing what exists. If a monad, primitive, service, or extension already does most of +the job, name it in the plan with its path. + +## 3. Write the plan + +Write it to the output path you were given. Absent one, use +`.claude/plans/{short-name}/plan.md`, taking a short name from the requirement — +`transfer-limits`, `fix-expiry-tz`. Create the directory as needed. + +- **Goal** — one paragraph. What changes for a user of Crypter, and why. +- **Non-goals** — what this change deliberately leaves alone. Be specific; this keeps the + implementer in scope, and the conformance auditor checks against it. +- **Approach** — the design, in prose. Name the types and methods to add or change. Explain + anything non-obvious, especially where a constraint forced the shape. +- **Steps** — numbered and ordered, each naming the files it touches. A step should be small + enough that its result is obvious. +- **Tests** — what to add to `Crypter.Test` or `Crypter.Test.Web` and what each case pins down. + CI is where the suite runs, so tests are what verify behaviour. +- **Risks** — what could break, and what a reviewer should look at hardest. + +### Crypter's idioms are part of the plan + +Express the plan in the conventions the code already uses, so the implementer inherits them: + +- `Maybe` and `Either` from `Crypter.Common/Monads` for expected failures. +- Validated types from `Crypter.Common/Primitives` rather than raw strings. +- `Async` suffix on async methods, and async all the way for database, file, and network IO. +- Constructors over object initializers. Enums over magic strings. +- Any change to an entity under `Crypter.DataAccess/Entities` needs an EF Core migration in + `Crypter.DataAccess/Migrations`. Say so explicitly, and say whether it also needs a companion + script in `Crypter.DataAccess/Scripts`. + +### Scope + +One pull request does one thing. Put drive-by refactors and cleanups under non-goals. + +## 4. Settle it with the user + +Show the user the plan and wait. + +Ask when two readings give materially different work. Being able to ask is why this runs on the +host; use it. Decide the routine calls yourself and say which way you went. Revise the plan in +place until the user approves it. + +Report the path you wrote and what the user settled. diff --git a/.claude/skills/crypter-triage-review/SKILL.md b/.claude/skills/crypter-triage-review/SKILL.md new file mode 100644 index 00000000..6db73960 --- /dev/null +++ b/.claude/skills/crypter-triage-review/SKILL.md @@ -0,0 +1,111 @@ +--- +name: crypter-triage-review +description: Verify the findings left on a pull request, push back on the ones that do not hold, and fix the ones that do. Use when asked to work through review comments, or invoked as /crypter-triage-review {pr-number}. +--- + +# Crypter triage review + +Work through the findings on a pull request. Each one is either answered on the thread or +recorded as verified, and the verified ones become commits. + +Findings come from anywhere — the reviewer lenses, a person, another tool. They are treated the +same way, because where a finding came from says nothing about whether it is true. + +Run from the root of the main checkout. The container's mounts resolve against it. + +## Setup + +You are given a pull request number: `/crypter-triage-review {pr-number}`. + +Use `pr-{number}` as the run id. + +```bash +mkdir -p .claude/runs/pr-{number}/verification +chmod 777 .claude/runs/pr-{number} .claude/runs/pr-{number}/verification +``` + +The container's `agent` is uid 1001 and your files are uid 1000, so the agents write into +directories this side creates and grants. + +## 1. Collect the findings + +Read the pull request with whatever GitHub access this session has — the `gh` CLI, or the GitHub +MCP server's `pull_request_read` with `get_review_comments`, `get_reviews` and `get_comments`. + +Take the head branch and head repository from `get` while you are there. You need both later. + +Write every open finding to `.claude/runs/pr-{number}/review.md`, one entry each: + +- A short id you assign, `f1` upward. +- The thread or comment id, so a reply can find its way back. +- The file and line, where it has one. +- The finding, quoted in full. + +Skip threads already resolved and comments that raise nothing — approvals, thanks, questions +about intent. A question is for the author to answer, not for a verifier. + +**If there is nothing open, say so and stop.** + +## 2. Fetch the head into the container + +```bash +docker exec crypter-pipeline \ + git -C /work/Crypter fetch upstream +pull/{number}/head:{head-branch} +``` + +The local branch takes the pull request's own branch name, so the commits go back to the branch +they came from. + +**If this fails, stop and say so.** + +## 3. Verify + +```bash +docker exec -w /work/Crypter crypter-pipeline \ + claude --permission-mode auto -p "/crypter-devcontainer-verify pr-{number} {head-branch} /runs/pr-{number}/review.md" +``` + +Verdicts land in `.claude/runs/pr-{number}/verification/{id}.md`. Read the files, not the +summary. + +## 4. Answer each finding + +Every finding gets one of three outcomes, and none of them is silence. + +**Does not hold** — reply on the thread with `add_reply_to_pull_request_comment`, or +`gh pr comment` where the finding has no thread. Give the evidence: what the code does instead, +by file and line. Two or three sentences. Say it as a position, not a verdict — the person who +raised it may know something the verifier could not see, and the thread is where that comes out. + +Reply once. If they answer, that is the user's conversation, not yours to continue. + +**Holds** — write it to `.claude/runs/pr-{number}/triage.md`: the id, the failure, and the file +and line. That file is what the fix is built from, so write it for someone who has not read the +thread. + +**Unsettled** — carry it to the user in your report. Do not reply, and do not fix. + +## 5. Fix what held + +Where `triage.md` has anything, and the head branch is one you can push to: + +```bash +docker exec -w /work/Crypter crypter-pipeline \ + claude --permission-mode auto -p "/crypter-devcontainer-remediate pr-{number} {head-branch} /runs/pr-{number}/triage.md" +``` + +Then invoke `crypter-step-open-pull-request` with the run id and the head branch. It pushes the +commits and leaves the existing pull request in place. + +A pull request from a repository you cannot push to stops here. The replies stand, `triage.md` +stands, and the author does the fixing. Say so in the report. + +## 6. Report + +- What held, what did not, and what you could not settle. +- The replies you posted, and where. +- What changed on the branch, and the commits now on the pull request. +- Where the artifacts are. + +CI is not watched here. A push starts a round of checks; reading them is `/crypter-change`'s job +or yours. diff --git a/.claude/skills/pipeline/SKILL.md b/.claude/skills/pipeline/SKILL.md deleted file mode 100644 index fcbc52aa..00000000 --- a/.claude/skills/pipeline/SKILL.md +++ /dev/null @@ -1,174 +0,0 @@ ---- -name: pipeline -description: Take a requirement from plan to an open, CI-green draft pull request on the fork, using a chain of subagents. Use when asked to run the pipeline on a requirement, or invoked as /pipeline "". ---- - -# Pipeline - -Turn a requirement into a draft pull request whose checks pass, in stages, each run by a -subagent with its own context. A later stage that starts fresh actually re-examines the work; -one that inherits the reasoning behind it rubber-stamps it. - -**This runs inside the devcontainer.** `origin` is the fork, `upstream` is the org repository -and is read-only. Every pull request is fork → fork. Nothing here can reach -`Crypter-File-Transfer/Crypter`, and the upstream pull request is something the user opens by -hand at the end, from a fork pull request they have read. - -There is exactly one stop: the user approves the plan. Everything after that runs to a draft -pull request with green checks, or to a written account of why CI would not take it. - -## Setup - -Pick a short run id from the requirement — `transfer-limits`, `fix-expiry-tz`. Then: - -```bash -mkdir -p /work/Crypter/.claude/pipeline/{run-id} -``` - -State goes there: `plan.md`, `conformance.md`, `findings/`, `ci.md`. It is gitignored. - -## 0. Sync and branch - -Never plan against stale code: - -```bash -git -C /work/Crypter fetch upstream -git -C /work/Crypter fetch origin -git -C /work/Crypter push origin upstream/stable:refs/heads/stable -git -C /work/Crypter worktree add /work/Crypter/.claude/worktrees/{run-id} -b {branch} upstream/stable -``` - -**If any of these fail, stop and say so.** A quietly skipped sync means the plan, the diff, and -the eventual upstream pull request are all built on the wrong base, and nothing downstream will -notice. - -Name the branch as the repo does: `feature/{something}`, `fix/{something}`, `chore/{something}`. - -Every later stage gets this worktree path and works by absolute path inside it. Never `cd`. - -## 1. Plan - -Invoke `plan-author` with the requirement verbatim, the worktree path, and the output path -`/work/Crypter/.claude/pipeline/{run-id}/plan.md`. - -Then **stop.** Show the user the plan — the file, not a summary of it — and wait. Do not -implement, do not create the pull request, do not start reviewing. If they ask for changes, run -`plan-author` again with their feedback and the existing plan; do not edit the plan yourself. - -If the plan contains an **Open question**, put it in front of the user explicitly. It is the -one thing they are most likely to want to change and the cheapest moment to change it. - -## 2. Implement - -Invoke `implementer` with the plan path and the worktree path. Give it nothing about how the -plan was reached — the plan is the specification. - -Read its report. If it says a step could not be done, that is not a failure to paper over: -surface it to the user with the rest of the results at the end, and let the auditor record it. - -## 3. Open the draft pull request - -Now, once there is something real to look at and before anyone reviews it. You do the pushing, -here and at every later stage — the implementer commits and returns: - -```bash -git -C /work/Crypter/.claude/worktrees/{run-id} push -u origin {branch} -gh pr create --repo {fork} --draft --base stable --head {branch} --title "..." --body "..." -``` - -Creating the pull request starts the first round of checks. Checks run on drafts, so the round -begins here rather than at stage 7, and every push after this one starts another. Nothing -cancels the round it supersedes, so push once per stage, after the implementer is done. - -Title reads like a commit subject: imperative, capitalized, no trailing period. - -Description is a few sentences of plain English saying what changed and why. **Do not argue the -case** — no justifying the approach, no pre-empting objections, no listing rejected -alternatives. Call out what a reviewer would otherwise have to discover: migrations, breaking -API changes, deliberately held-back dependencies. That is information, not argument. - -This description carries over verbatim when the user opens the upstream pull request, so write -it for the org repository's reviewers. - -## 4. Examine - -Run these in parallel — they do not interact: - -- `conformance-auditor` with the plan, the worktree, and - `/work/Crypter/.claude/pipeline/{run-id}/conformance.md`. -- `reviewer`, once per lens, with the worktree and - `/work/Crypter/.claude/pipeline/{run-id}/findings/{lens}.md`. - -The lens list is currently one entry: - -| Lens | Brief | -|---|---| -| general | Correctness and edge cases first, then scope creep, then the conventions in `CLAUDE.md`. | - -Adding lenses later — security, simplicity, test coverage — means adding rows here. The -`reviewer` definition does not change; the lens comes from the prompt. - -## 5. Triage - -You decide what to act on. Read every finding against the code before accepting it — a reviewer -that has already been wrong once will happily be wrong again, and acting on a bad finding means -changing working code. - -Accept anything with a concrete failure behind it. Reject preferences, restatements of the plan -you already chose against, and findings about code the diff did not touch. An unplanned extra -that contradicts the plan's non-goals is not a preference — accept it. - -Write what you accepted and what you rejected, with a reason for each rejection, into -`/work/Crypter/.claude/pipeline/{run-id}/findings/triage.md`. The user reads this to check your -judgement. - -## 6. Remediate - -If anything was accepted, invoke `implementer` with the accepted findings and the worktree -path. Each fix is its own commit on the existing branch. When it returns, push once; that -updates the same draft pull request and starts a fresh round of checks. - -If nothing was accepted, go straight to stage 7 — the round of checks from the last push is -the one that counts. - -## 7. Hold it against CI - -Invoke `ci-watcher` with the worktree path, the pull request number, the attempt number, and -`/work/Crypter/.claude/pipeline/{run-id}/ci.md`. It runs **one attempt**: it finds the round of -checks for the branch's current commit, watches it, and reports. - -You own the loop: - -1. `ci-watcher` reports green → go to step 8. -2. `ci-watcher` reports a failure → invoke `implementer` with that failure report and the - worktree path, push, then invoke `ci-watcher` again with the next attempt number. -3. **Stop after three attempts.** Comment the state of play on the pull request, and hand back - to the user. Three failures on the same change usually means the plan was wrong, not the - code, and a fourth attempt buys a full build and test suite for nothing. - -A fresh `ci-watcher` per attempt is deliberate — it reads what CI actually says now, rather than -reasoning from its own previous guess about the failure. - -Stop immediately, without spending attempts, if `ci-watcher` reports that no run ever appeared -for the commit. Workflows are disabled on a new fork until they are enabled once in its Actions -tab, and that is a setup problem. - -## 8. Hand off - -```bash -git -C /work/Crypter worktree remove /work/Crypter/.claude/worktrees/{run-id} -``` - -Remove it on every exit path, including when the pipeline stopped early. - -Then tell the user, in a few sentences: - -- The fork pull request URL and whether its checks are green. It is still a draft; taking it - out of draft is theirs to do once they have read it. -- Anything the implementer could not do, and any deviation the auditor flagged as drift. -- What you rejected in triage that they might disagree with. -- If the CI loop gave up: which check failed and what the last attempt tried. - -They open the upstream pull request themselves. Remind them the base repository is fixed when a -pull request is created, so it is a new pull request against -`Crypter-File-Transfer/Crypter` — the description is ready to paste. diff --git a/.devcontainer/.env.example b/.devcontainer/.env.example index 49a7dc62..c10ed1a3 100644 --- a/.devcontainer/.env.example +++ b/.devcontainer/.env.example @@ -1,4 +1,2 @@ -CRYPTER_FORK="" -CRYPTER_FORK_TOKEN="" CRYPTER_GIT_EMAIL="" CRYPTER_GIT_NAME="" diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile index 2c159d91..63ba2156 100644 --- a/.devcontainer/Dockerfile +++ b/.devcontainer/Dockerfile @@ -12,8 +12,7 @@ ENV DOTNET_CLI_TELEMETRY_OPTOUT=1 \ DOTNET_TOOLS=/usr/local/share/dotnet-tools ENV PATH="${PATH}:${DOTNET_TOOLS}" -# Claude Code refuses --dangerously-skip-permissions when running as root on Linux, -# so the agents need an unprivileged user to run as. +# The agents run unattended, so they run as an unprivileged user rather than root. RUN groupadd --gid $USER_GID $USERNAME \ && useradd --uid $USER_UID --gid $USER_GID --create-home --shell /bin/bash $USERNAME @@ -27,15 +26,6 @@ RUN apt-get update \ less \ && rm -rf /var/lib/apt/lists/* -RUN curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg \ - -o /usr/share/keyrings/githubcli-archive-keyring.gpg \ - && chmod go+r /usr/share/keyrings/githubcli-archive-keyring.gpg \ - && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" \ - > /etc/apt/sources.list.d/github-cli.list \ - && apt-get update \ - && apt-get install --yes --no-install-recommends gh \ - && rm -rf /var/lib/apt/lists/* - RUN curl -fsSL "https://deb.nodesource.com/setup_${NODE_MAJOR}.x" | bash - \ && apt-get install --yes --no-install-recommends nodejs \ && rm -rf /var/lib/apt/lists/* @@ -50,8 +40,8 @@ RUN dotnet workload install wasm-tools RUN dotnet tool install dotnet-ef --version '10.0.*' --tool-path "${DOTNET_TOOLS}" -COPY .devcontainer/clone-fork.sh /usr/local/bin/crypter-clone-fork -RUN chmod +x /usr/local/bin/crypter-clone-fork +COPY .devcontainer/clone-upstream.sh /usr/local/bin/crypter-clone-upstream +RUN chmod +x /usr/local/bin/crypter-clone-upstream # The workspace and the agent's Claude Code state are both named volumes. Docker creates a # mount point that the image does not already contain as root, so creating these here is diff --git a/.devcontainer/clone-fork.sh b/.devcontainer/clone-fork.sh deleted file mode 100644 index 82aee0c0..00000000 --- a/.devcontainer/clone-fork.sh +++ /dev/null @@ -1,43 +0,0 @@ -#!/usr/bin/env bash -# Prepare the pipeline workspace: a clone of your fork, with the org repository added as a -# read-only upstream. The container runs this on every start; an existing workspace is left alone. -# -# The workspace is a named volume rather than a bind mount of a host checkout. The agents -# get their own clone, so they cannot touch uncommitted work on the host, and `origin` is -# the fork that the container's fork-scoped token can actually push to. -set -euo pipefail - -: "${CRYPTER_FORK:?Set CRYPTER_FORK in .devcontainer/.env to / of your fork}" -: "${GH_TOKEN:?Set CRYPTER_FORK_TOKEN in .devcontainer/.env so it reaches the container as GH_TOKEN}" -: "${CRYPTER_GIT_NAME:?Set CRYPTER_GIT_NAME in .devcontainer/.env to the author name on the commits}" -: "${CRYPTER_GIT_EMAIL:?Set CRYPTER_GIT_EMAIL in .devcontainer/.env to the author email on the commits}" - -upstream_repo="${CRYPTER_UPSTREAM:-Crypter-File-Transfer/Crypter}" - -# Has to match the workspace path the pipeline skill and docker-compose.yml use. -workspace="/work/Crypter" - -if [[ "${CRYPTER_FORK}" == "${upstream_repo}" ]]; then - echo "CRYPTER_FORK is the upstream repository. Point it at your fork instead." >&2 - exit 1 -fi - -git config --global user.name "${CRYPTER_GIT_NAME}" -git config --global user.email "${CRYPTER_GIT_EMAIL}" -gh auth setup-git - -if [[ -d "${workspace}/.git" ]]; then - echo "Workspace already present at ${workspace}" -else - git clone "https://github.com/${CRYPTER_FORK}.git" "${workspace}" -fi - -if ! git -C "${workspace}" remote get-url upstream >/dev/null 2>&1; then - git -C "${workspace}" remote add upstream "https://github.com/${upstream_repo}.git" -fi - -git -C "${workspace}" fetch --quiet origin -git -C "${workspace}" fetch --quiet upstream - -echo "Workspace ready at ${workspace}" -git -C "${workspace}" remote -v diff --git a/.devcontainer/clone-upstream.sh b/.devcontainer/clone-upstream.sh new file mode 100644 index 00000000..8bd0f8d1 --- /dev/null +++ b/.devcontainer/clone-upstream.sh @@ -0,0 +1,34 @@ +#!/usr/bin/env bash +# Prepare the pipeline workspace: a clone of the org repository. The container runs this on +# every start; an existing workspace is left alone. +# +# The workspace is a named volume rather than a bind mount of a host checkout. The agents get +# their own clone, so they cannot touch uncommitted work on the host. The clone is anonymous +# and the remote has no push url, so the agents read public code and commit locally. Pushing +# and opening pull requests happen on the host. +set -euo pipefail + +: "${CRYPTER_GIT_NAME:?Set CRYPTER_GIT_NAME in .devcontainer/.env to the author name on the commits}" +: "${CRYPTER_GIT_EMAIL:?Set CRYPTER_GIT_EMAIL in .devcontainer/.env to the author email on the commits}" + +upstream_repo="${CRYPTER_UPSTREAM:-Crypter-File-Transfer/Crypter}" + +# Has to match the workspace path the container skills and docker-compose.yml use. +workspace="/work/Crypter" + +git config --global user.name "${CRYPTER_GIT_NAME}" +git config --global user.email "${CRYPTER_GIT_EMAIL}" + +if [[ -d "${workspace}/.git" ]]; then + echo "Workspace already present at ${workspace}" +else + git clone --origin upstream "https://github.com/${upstream_repo}.git" "${workspace}" +fi + +# A push from the container fails here rather than at a credential prompt. +git -C "${workspace}" remote set-url --push upstream no-push + +git -C "${workspace}" fetch --quiet upstream + +echo "Workspace ready at ${workspace}" +git -C "${workspace}" remote -v diff --git a/.devcontainer/docker-compose.yml b/.devcontainer/docker-compose.yml index 1178ebfa..b0cfd68d 100644 --- a/.devcontainer/docker-compose.yml +++ b/.devcontainer/docker-compose.yml @@ -9,20 +9,22 @@ services: context: .. dockerfile: .devcontainer/Dockerfile environment: - GH_TOKEN: ${CRYPTER_FORK_TOKEN} - CRYPTER_FORK: ${CRYPTER_FORK} CRYPTER_UPSTREAM: Crypter-File-Transfer/Crypter CRYPTER_GIT_NAME: ${CRYPTER_GIT_NAME} CRYPTER_GIT_EMAIL: ${CRYPTER_GIT_EMAIL} volumes: - workspace:/work - claude:/home/agent/.claude - # /work/Crypter does not exist until crypter-clone-fork has run, so the container starts + # Plans are authored on the host and read from /plans. + - ../.claude/plans:/plans:ro + # Findings, conformance and triage are artifacts on the host, written from /runs. + - ../.claude/runs:/runs + # /work/Crypter does not exist until crypter-clone-upstream has run, so the container starts # one level up. Open a shell with `exec -w /work/Crypter`. working_dir: /work - # crypter-clone-fork leaves an existing workspace alone and only refetches, so running it - # on every start is safe. - command: bash -lc "crypter-clone-fork && sleep infinity" + # crypter-clone-upstream leaves an existing workspace alone and only refetches, so running + # it on every start is safe. + command: bash -lc "crypter-clone-upstream && sleep infinity" volumes: workspace: diff --git a/.gitignore b/.gitignore index f4262e8e..cf594157 100644 --- a/.gitignore +++ b/.gitignore @@ -462,8 +462,11 @@ Crypter.Web/pnpm-lock.yaml # Claude Code worktrees .claude/worktrees/ -# Agentic pipeline run state -.claude/pipeline/ +# Plans authored on the host and mounted into the pipeline container +.claude/plans/ + +# Findings and triage written back from the pipeline container +.claude/runs/ # Devcontainer configuration, copied from .devcontainer/.env.example .devcontainer/.env diff --git a/Documentation/Development/Agentic Development Pipeline.md b/Documentation/Development/Agentic Development Pipeline.md index d00c5f96..a10cc22d 100644 --- a/Documentation/Development/Agentic Development Pipeline.md +++ b/Documentation/Development/Agentic Development Pipeline.md @@ -1,17 +1,107 @@ # Agentic Development Pipeline -The `/pipeline` skill takes a requirement from a plan to a draft pull request with green checks, -using a chain of subagents that each start with their own context. It runs inside a devcontainer -built from `.devcontainer/Dockerfile`. +Three orchestrators compose a set of task skills. You invoke an orchestrator in your own session, +and it invokes the rest. -Everything it does happens on **your fork**. The container's token cannot reach -`Crypter-File-Transfer/Crypter`, and the workspace is a named Docker volume rather than a bind -mount of your checkout, so the agents cannot touch uncommitted work on your machine. When the -pipeline finishes you have a fork pull request to read; opening one against the org repository is -something you do by hand afterwards. +| Orchestrator | Does | +|---|---| +| `/crypter-change ""` | Carries a requirement to a green draft pull request | +| `/crypter-review {pr-number}` | Puts an existing pull request through the reviewer lenses | +| `/crypter-triage-review {pr-number}` | Rules on the findings left on a pull request and fixes the ones that hold | + +| Task skill | Executes in | Does | +|---|---|---| +| `/crypter-step-plan` | Your session | Drafts the plan interactively, with the web, your tooling and you available to it | +| `/crypter-devcontainer-implement` | Container | Builds the plan into commits on a new branch | +| `/crypter-devcontainer-examine` | Container | Reviews a diff for plan adherence and code quality | +| `/crypter-devcontainer-verify` | Container | Rules on each finding in a report against the code | +| `/crypter-devcontainer-remediate` | Container | Applies triaged findings or a CI failure to an existing branch | +| `/crypter-step-open-pull-request` | Your session | Pushes the branch and opens or updates the draft pull request | + +Every skill that reads or writes code runs in the container, against the container's own clone. +Your session plans, decides what to act on, and talks to GitHub. `/crypter-review` reviews +nothing itself: it fetches the pull request into the container and runs +`/crypter-devcontainer-examine` there. + +Both prefixes say the same thing: an orchestrator invokes this, you do not. `crypter-step-` runs +in your session, and `crypter-devcontainer-` runs in the container, which expects `/work/Crypter`, +`/plans` and `/runs` — none of which your session has. The three skills without a prefix are the +ones to invoke. + +`/crypter-step-plan` is the one worth borrowing when you want a plan and nothing else, and +`/crypter-step-open-pull-request` is safe to run repeatedly, which is how the CI loop uses it. + +**Run the orchestrators from the root of your main checkout.** The container's mounts are +relative to `.devcontainer/`, so `.claude/plans` and `.claude/runs` resolve against that one +directory. Started from a worktree, a run writes its plan somewhere the container cannot read. + +**The container holds no GitHub credential.** Its workspace is an anonymous clone of the org +repository with one remote, `upstream`, which has no push url, so the agents read public code +and commit locally. Every authenticated GitHub operation happens in your session with your own +access, and `/crypter-change` pushes and re-pushes without stopping to ask. The workspace is a +named Docker volume rather than a bind mount of your checkout, so the agents cannot touch +uncommitted work on your machine. + +The container does hold your Claude Code credential, in the `crypter-pipeline-claude` volume, +and its network egress is open. Treat it as a trust boundary rather than a sandbox. + +When `/crypter-change` finishes you have a fork pull request to read; opening one against the org +repository is something you do by hand afterwards. This document covers the setup you need before the container will start. +## The two mounts + +Everything crossing the container boundary goes through one of two directories, both gitignored +and both on your disk: + +| Host | Container | Direction | Holds | +|---|---|---|---| +| `.claude/plans` | `/plans` | Read-only | `{run-id}/plan.md` | +| `.claude/runs` | `/runs` | Writable | `{run-id}/conformance.md`, `{run-id}/findings/{lens}.md`, `{run-id}/review.md`, `{run-id}/verification/{id}.md`, `{run-id}/triage.md`, `{run-id}/ci-{n}.md` | + +The plan goes in and cannot be rewritten by the agents. Findings come back out as files you can +open, grep and keep, rather than as text in a transcript, and each is written by the agent that +found it. `triage.md` is what `/crypter-change` decided to act on, and reading it is how you +check that judgement. + +The container's `agent` user is uid 1001, because the base image already has a user on 1000. A +bind mount keeps host ownership, so the orchestrators create every directory under `.claude/runs` +themselves and give it mode 777. Directories made on the host stay deletable from the host; a +directory the container creates is one you need `docker exec` to remove. + +The branch itself travels differently. It never passes through a mount: + +```bash +git -c protocol.ext.allow=user fetch \ + "ext::docker exec -i crypter-pipeline git upload-pack /work/Crypter" {branch}:{branch} +``` + +`protocol.ext.allow` is passed per command, so it stays out of your git config. + +A container created before these mounts existed picks them up on +`docker compose -f .devcontainer/docker-compose.yml up -d --force-recreate`. + +## Running a change + +```bash +/crypter-change "" +``` + +It plans, stops for your approval, then builds, examines, triages, remediates, opens the draft +pull request, and holds it against CI for at most three fix attempts. The approval is the only +stop, and the pull request stays a draft until you take it out of one. + +`/crypter-review {pr-number}` is the second entry point. It fetches a pull request's head into +the container, runs the lenses against it with no plan to audit, triages what they raise, and +posts one review that comments. It never approves and never requests changes. + +`/crypter-triage-review {pr-number}` is the third. It reads the findings already on a pull +request, whoever left them, and gives one verifier per finding a worktree and nothing else to +judge it by. A finding that does not survive that gets a reply on its thread saying what the +code does instead. A finding that does becomes a commit, where the head branch is one you can +push to. Nothing is fixed on the strength of the finding alone. + ## Configuration `.devcontainer/.env` holds everything Compose substitutes when it creates the container. It is @@ -23,13 +113,11 @@ cp .devcontainer/.env.example .devcontainer/.env | Variable | Value | |---|---| -| `CRYPTER_FORK` | Your fork, as `/`. Startup fails if this is the upstream repository. | -| `CRYPTER_FORK_TOKEN` | A fine-grained personal access token. Reaches the container as `GH_TOKEN`. | | `CRYPTER_GIT_NAME` | Author name on the agents' commits. | | `CRYPTER_GIT_EMAIL` | Author email on the agents' commits. | -All four are required. Leaving one empty fails the container's startup script with a message -naming the variable. +Both are required. Leaving one empty fails the container's startup script with a message naming +the variable. ## Launching the container @@ -38,32 +126,22 @@ Compose project from the application stack at the repository root, so `docker co `docker compose down` there never touch it, and the two share no network. ```bash +mkdir -p .claude/plans .claude/runs docker compose -f .devcontainer/docker-compose.yml up -d docker compose -f .devcontainer/docker-compose.yml exec -w /work/Crypter pipeline bash ``` +Create the two mount sources first. They are gitignored, so a fresh clone has neither, and +Docker creates a missing bind-mount source as root — which the orchestrators then cannot write +into. + Swap `up -d` for `down` to stop it. The named volumes outlive the container, so the next `up` reuses the workspace and your Claude Code credentials. -## The token - -Create a fine-grained personal access token with access to **your fork only**. That restriction -is what makes the rest of the design hold: the agents push branches, open pull requests, and read -check results without any path to the org repository. - -Grant it these repository permissions: - -| Permission | Access | Needed for | -|---|---|---| -| Contents | Read and write | Pushing the branch | -| Pull requests | Read and write | Opening the draft pull request | -| Actions | Read | Reading check runs and failed job logs | -| Metadata | Read | Mandatory on every fine-grained token | - ## Enable Actions on your fork GitHub disables workflows on new forks. Until you turn them on, pushing a branch runs nothing, -and the pipeline stops at the CI stage reporting that no run ever appeared. +and `/crypter-change` stops at the CI stage reporting that no run ever appeared. Open the **Actions** tab on your fork and use the button confirming you want to run workflows. You only do this once. @@ -75,11 +153,11 @@ the container pulls it for you. There is nothing to build unless you are changin itself. Built on `mcr.microsoft.com/dotnet/sdk:10.0`, running as an unprivileged user named `agent` -because Claude Code refuses `--dangerously-skip-permissions` as root: +rather than as root: - The .NET 10 SDK, the `wasm-tools` workload, and `dotnet-ef` - Node 22 and pnpm 11.18.0, which `Crypter.Web`'s PreBuild target needs -- The GitHub CLI and Claude Code +- Claude Code There is **no Docker in the container**, so `Crypter.Test` cannot run there — it needs Testcontainers to start PostgreSQL. The agents build but never test locally; the test suite runs @@ -90,10 +168,10 @@ Two named volumes survive rebuilds: `crypter-pipeline-workspace` holds the works ## First start -Every `up` runs `crypter-clone-fork`, which clones your fork to `/work/Crypter`, adds the org -repository as a read-only `upstream`, and fetches both. If it already finds a workspace there it -leaves it alone and only refetches, so restarting the container does not discard work in -progress. +Every `up` runs `crypter-clone-upstream`, which clones the org repository to `/work/Crypter` as +the `upstream` remote, clears that remote's push url, and fetches. If it already finds a +workspace there it leaves it alone and only refetches, so restarting the container does not +discard work in progress. To start over from nothing, take the container down and remove the volumes: @@ -111,10 +189,9 @@ your host and a code to paste back. Credentials live in `/home/agent/.claude`, which is the `crypter-pipeline-claude` volume, so they survive container rebuilds. You only do this again after removing that volume. -Run the agents with `--dangerously-skip-permissions`. A pipeline that stops to approve every -file write is not a pipeline, and the fork-scoped token is what bounds the blast radius rather -than the permission prompts. That flag is also why the container runs as the unprivileged -`agent` user; Claude Code refuses it as root. +Run the agents with `--permission-mode auto`. They work unattended, so a prompt they cannot +answer is a run that stalls. What bounds the blast radius is the container itself: a workspace +in a named volume, a remote with no push url, and no GitHub credential to push with. ## Changing the image