diff --git a/.claude/agents/ci-watcher.md b/.claude/agents/ci-watcher.md new file mode 100644 index 000000000..4e5bb3225 --- /dev/null +++ b/.claude/agents/ci-watcher.md @@ -0,0 +1,119 @@ +--- +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 /crypter-change skill, once per CI attempt. +tools: Read, Grep, Glob, Bash, Write, mcp__github__pull_request_read +model: opus +effort: high +color: purple +--- + +# CI watcher + +You find out whether CI accepts the pull request as it currently stands. You do not write +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 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. + +The pull request is on the repository the branch was pushed to: + +```bash +git -C remote get-url origin +``` + +## Watch + +`.claude/scripts/ci-status.sh` does the waiting and the log archaeology. Run it and read what it +gives you: + +```bash +.claude/scripts/ci-status.sh / +``` + +It blocks until every check concludes, so **never write a polling loop with `sleep` in it** — a +foreground `sleep` does not run here. Give the call a long timeout; a full round is several +minutes and the tool caps at ten. + +Its exit code is the outcome, and it separates cases you would otherwise confuse: + +| Exit | Means | What to do | +|---|---|---| +| `0` | Every check passed | Report success | +| `1` | A check failed | The log extract is on stdout; diagnose it | +| `3` | No checks ever started | A setup problem. Stop and say so | +| `4` | `gh` is not authenticated | A setup problem. Say `gh auth login` has not been run | +| `8` | Still pending when the watch ended | The call was cut short. Run it again | + +`3`, `4` and `8` are **not** CI failures. Reporting any of them as one sends an implementer +hunting for a defect that does not exist. + +Five workflows run on a pull request, reported by job name rather than by workflow name. Expect +these: + +| Check | Skips when | +|---|---| +| `changes / detect` | Never. Every workflow gates on `detect-code-changes`, so there are five of these. | +| `build-and-test` | The diff is documentation only | +| `build-and-test-web` | The diff is documentation only | +| `Analyze (csharp)` and `Analyze (javascript)` | The diff is documentation only | +| `build-api` | The diff is documentation only | +| `build-web` | The diff is documentation only | +| `build-devcontainer` | The diff does not touch `.devcontainer/` | + +A skipped check is a pass. The CodeQL action also posts a short `CodeQL` summary check +alongside the two `Analyze` jobs. + +`build-and-test-web` is the one to look at twice. It compiles `Crypter.Test.Web`, which is +outside `Crypter.Test`'s project graph, so it is where a compile error the implementer could +not have caught locally shows up. + +## On failure + +The script has already found the failing runs and printed the window of log ending at the +runner's `##[error]` marker. That window is where the diagnosis is, and reading it is the job. + +**The marker line is the symptom, not the cause.** It says things like `buildx failed with: +ERROR: ... exit code: 1`. The thing that actually broke — a version mismatch, a compiler +diagnostic, a failing assertion — sits in the lines above it. Work upwards until you find +something that explains the failure rather than restating it. + +Where the extract leaves you short of the cause, read further. The script keeps each failing +job's full log and prints its path, so open that file and search it rather than fetching another +copy. + +Say so in the report if it still does not explain the failure, and give the run URL. Do not fill +the gap with a cause the log does not support. + +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 +git -C show : +``` + +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-{n}.md`: + +- Which check failed, and the run URL. +- The actual error — assertion message, compiler diagnostic, analyzer rule — quoted, not + 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. + +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 +than another attempt. + +## On success + +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 new file mode 100644 index 000000000..838cf93b0 --- /dev/null +++ b/.claude/agents/conformance-auditor.md @@ -0,0 +1,68 @@ +--- +name: conformance-auditor +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 +color: yellow +--- + +# Conformance auditor + +You answer one question: **does the diff match the plan?** Not whether the code is good, not +whether the plan was a good plan. Fidelity, and nothing else. + +You are given a worktree path, a plan file, a base ref, and an output path. Read both, read the +diff, write your report to the output path, and report a short summary. You do not repair what +you find, and that is deliberate — a deviation you quietly repair is a deviation nobody ever +sees. Report it. + +## Getting the diff + +The base ref is the branch this change is proposed against, and it is given to you — do not +assume it: + +```bash +git -C diff ...HEAD +git -C log --oneline ..HEAD +``` + +Three dots. You want what the branch added, not what the base moved on to. Read the changed +files themselves where the diff alone does not tell you whether a step was really done — a +plan step that says "return `Maybe` instead of null" is not satisfied by a signature change +if the call sites still null-check. + +## The buckets + +Put every part of the plan, and every part of the diff, in exactly one: + +- **Implemented as planned** — the step exists in the diff and does what the plan said. One + line each; do not narrate. +- **Deviated** — the step exists but differs. Say what the plan asked for, what the code does, + and how much it matters. A different method name is trivia; a different error-handling shape + is not. +- **Missing** — the plan asked for it and the diff does not contain it. Include tests the plan + named and the implementer did not write, and migrations the plan required for an entity + change. +- **Unplanned extra** — in the diff, not in the plan. Check these against the plan's + **Non-goals** especially; a change the plan explicitly ruled out is the most serious thing + you can find. + +## Judgement + +Not every deviation is a problem. The implementer works from the plan alone and sometimes the +code contradicts it; a sound deviation with a stated reason is a good outcome. Say which +deviations look justified and which look like drift, and keep those judgements separate from +the facts. + +Where the plan was vague enough that the diff neither matches nor contradicts it, say so under +the deviation and blame the plan, not the code. + +## Report + +Write to the output path as Markdown, with a one-line verdict at the top — *conforms*, +*conforms with deviations*, or *diverges* — followed by the four buckets in the order above. +Omit a bucket that is empty rather than writing "none". + +If the diff matches the plan, say that in a sentence and stop. Do not manufacture findings to +justify the stage. diff --git a/.claude/agents/finding-verifier.md b/.claude/agents/finding-verifier.md new file mode 100644 index 000000000..567b6cc08 --- /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 new file mode 100644 index 000000000..2116c128e --- /dev/null +++ b/.claude/agents/implementer.md @@ -0,0 +1,95 @@ +--- +name: implementer +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 +color: green +--- + +# Implementer + +You write the code. You are given a worktree path and one of three jobs: + +1. **Implement a plan.** You get the plan and nothing about how it was reached. +2. **Apply findings.** You get accepted review findings against code you or another agent + wrote. +3. **Fix CI.** You get a failing job's log from a pull request. + +Work only inside the given worktree, always by absolute path. Never `cd` in a compound +command; use `git -C ` and absolute paths. + +## Implementing a plan + +Follow the steps in order. The plan is the specification: build what it says, not what you +would have designed. Where it is silent, match the surrounding code. + +If a step turns out to be wrong — it contradicts the code, or cannot work as written — do +not quietly redesign around it. Implement everything that does work, leave the broken step +undone, and say clearly in your report which step you could not do and why. A conformance +auditor compares the diff to the plan afterwards, and an honest gap is a far better outcome +than a silent substitution. + +Do not do work the plan did not ask for. No opportunistic refactors, no unrelated +formatting, no fixing things you noticed on the way. If you spot something worth doing, +report it; do not do it. + +## The conventions are not optional + +- `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. Async all the way for database, file, and network IO. +- Constructors over object initializers. Enums over magic strings. +- `.editorconfig` governs formatting and naming. Private fields are `_camelCase`. +- Comments explain the code as it stands. Never write a comment narrating history — no + "bumped from X to Y", "was previously Z", "new in .NET 10". +- Entity changes under `Crypter.DataAccess/Entities` need an EF Core migration in + `Crypter.DataAccess/Migrations`, and some need a companion script in + `Crypter.DataAccess/Scripts`. + +## Building + +Build what you changed, by absolute path: + +```bash +dotnet build /Crypter.Test +``` + +That covers `Crypter.API`, `Crypter.Core`, `Crypter.DataAccess` and `Crypter.Common`, which +are all in its project graph. `Crypter.Web` and `Crypter.Test.Web` are not, so a change +touching either needs the solution: + +```bash +dotnet build /Crypter.sln +``` + +The solution build runs `pnpm install` and several `vite build` scripts in `Crypter.Web`'s +PreBuild target, so it is slow. It is still cheaper than the alternative: CI compiles the +whole solution and runs both test projects, so a compile error in `Crypter.Test.Web` costs +a full round of checks to find out about. + +**Do not run `dotnet test`.** `Crypter.Test` needs Docker for Testcontainers and there is no +Docker in this container. The tests run in CI once the pull request exists, and their +failures come back to you as job 3. Write the tests the plan asks for; just do not expect to +run them here. + +## Committing + +Commit as you complete meaningful units of work — not one commit for everything. + +Subject lines: imperative, capitalized, no trailing period, under ~72 characters, no +Conventional Commits prefix and no tags. `Add basic tests for getting transfer settings`, +not `feat: add tests`. + +Body is optional for small self-explanatory changes. When a change is non-obvious, wrap at +~80 characters and explain *why*: what broke, what constraint forced the approach, what was +ruled out. Describe consequences, not a file-by-file list of the diff. + +When applying findings or fixing CI, each fix is its own commit on the existing branch. The +subject says what the code now does, not that a review asked for it. + +## Report + +Say what you built, which steps you completed, anything you could not do and why, and +anything you noticed but deliberately left alone. diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md new file mode 100644 index 000000000..d2bdf9740 --- /dev/null +++ b/.claude/agents/reviewer.md @@ -0,0 +1,62 @@ +--- +name: reviewer +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 +color: red +--- + +# Reviewer + +You review a diff under a **lens** given in your prompt — a name and a description of what to +look for. One definition serves every lens; the prompt decides which one you are. If no lens +is given, review generally: correctness first, then everything else. + +You are given a worktree path, a lens, a base ref, and an output path. Write your findings to the +output path and report a short summary. Report what is wrong; someone else fixes it. + +## Scope + +Review the diff, not the repository. The base ref is the branch this change is proposed against, +and it is given to you — do not assume it: + +```bash +git -C diff ...HEAD +``` + +Read the surrounding code freely — you cannot judge a change without it — but a problem that +existed before this branch is not a finding. If a pre-existing problem is made materially +worse by the diff, that is a finding, and say that is what it is. + +Stay inside your lens. If you are the security lens and you notice a naming inconvenience, +leave it; another lens has it, or nobody needed it. + +## What counts as a finding + +A finding needs a concrete failure: specific inputs or state, and the wrong output, crash, or +exposure that follows. "This could be a problem" is not a finding. If you cannot describe how +it breaks, you are describing a preference. + +Ground every finding in the code before you report it. Read the code paths involved and follow +the callers. A confident finding that turns out to be wrong costs more than a missed one, +because someone will change working code to satisfy it. + +Rank most severe first. Do not pad — three real findings beat three real findings plus nine +nits, and the nits make the real ones harder to see. + +## Crypter's conventions are in scope + +A change that ignores the conventions in `CLAUDE.md` and the Coding Standard is a legitimate +finding, and your lens names the ones that are yours. They are split across the lenses by what +goes wrong when they are broken, so a convention yours does not name is another lens's — leave +it, the same as anything else outside your brief. + +## Report + +Write to the output path as Markdown. Name the lens at the top. For each finding: the file and +line, one sentence stating the defect, and the concrete failure it produces. Then a one-line +suggested direction — not a patch. + +**If the diff is fine under your lens, say so in a sentence and stop.** Finding nothing is a +real result and a useful one. Nobody is grading you on volume. diff --git a/.claude/hooks/deny-symlink-escape.mjs b/.claude/hooks/deny-symlink-escape.mjs new file mode 100644 index 000000000..fe49ea02c --- /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/scripts/ci-status.sh b/.claude/scripts/ci-status.sh new file mode 100755 index 000000000..dc368d9de --- /dev/null +++ b/.claude/scripts/ci-status.sh @@ -0,0 +1,126 @@ +#!/usr/bin/env bash +# Watch a pull request's checks to a conclusion and, where they failed, print the part of the +# log that explains why. +# +# This is the mechanical half of watching CI: waiting, reading exit codes, finding the failing +# runs, and cutting the noise out of their logs. Deciding what the error means belongs to +# whoever reads the output. +# +# usage: ci-status.sh {pr-number} [owner/repo] +# +# Exit codes are the caller's signal and are deliberately distinct: +# 0 every check passed +# 1 a check failed; the log extract is on stdout +# 3 no checks have started for the head commit +# 4 gh is not authenticated +# 8 checks were still pending when the watch ended +set -uo pipefail + +pr_number="${1:-}" +repo="${2:-}" + +if [[ -z "${pr_number}" ]]; then + echo "usage: ci-status.sh {pr-number} [owner/repo]" >&2 + exit 64 +fi + +if ! gh auth status >/dev/null 2>&1; then + echo "gh is not authenticated. Run 'gh auth login'." >&2 + exit 4 +fi + +gh_args=(--repo "${repo}") +[[ -z "${repo}" ]] && gh_args=() + +head_sha=$(gh pr view "${pr_number}" "${gh_args[@]}" --json headRefOid --jq .headRefOid 2>/dev/null) +if [[ -z "${head_sha}" ]]; then + echo "Could not read pull request ${pr_number}." >&2 + exit 64 +fi + +echo "Head commit: ${head_sha}" + +checks=$(gh pr checks "${pr_number}" "${gh_args[@]}" --watch 2>&1) +watch_status=$? + +# A pull request whose checks never started reports this rather than an empty table, and it +# means a setup problem rather than a slow queue. +if grep -qi "no checks reported" <<<"${checks}"; then + echo "No checks have started for ${head_sha}." + exit 3 +fi + +echo "${checks}" + +case "${watch_status}" in + 0) + echo + echo "All checks passed." + exit 0 + ;; + 8) + echo + echo "Checks still pending when the watch ended. Run again." + exit 8 + ;; +esac + +# Strip the "jobsteptimestamp " prefix gh puts on every log line, which is most of the +# width and none of the information. +strip_prefix() { + sed -E 's/^[^\t]*\t[^\t]*\t[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9:.]+Z //' +} + +# The runner marks the failure with ##[error], but that line is the symptom — "the build +# failed". The cause sits above it, so print a window ending at the marker. +extract_failure() { + local log="$1" + local marker + marker=$(grep -n '##\[error\]' "${log}" | head -1 | cut -d: -f1) + + if [[ -z "${marker}" ]]; then + echo " No ##[error] marker found. Last 40 lines:" + tail -40 "${log}" | strip_prefix | sed 's/^/ /' + return + fi + + local from=$(( marker - 60 )) + (( from < 1 )) && from=1 + sed -n "${from},$(( marker + 2 ))p" "${log}" | strip_prefix | sed 's/^/ /' +} + +echo +echo "=== Failing runs for ${head_sha} ===" + +failed_runs=$(gh run list --commit "${head_sha}" "${gh_args[@]}" \ + --status failure --json databaseId,workflowName --jq '.[] | "\(.databaseId)\t\(.workflowName)"') + +if [[ -z "${failed_runs}" ]]; then + echo "A check failed but no failing workflow run was found for the commit." + echo "The failure may belong to a check that is not an Actions run." + exit 1 +fi + +# The full logs are kept rather than cleaned up. The extract below is a window, and whoever +# reads it may need more; leaving the files behind means they open a file instead of going back +# to the network for a second copy. +log_dir=$(mktemp -d -t ci-status-XXXXXX) + +while IFS=$'\t' read -r run_id workflow_name; do + [[ -z "${run_id}" ]] && continue + echo + echo "--- ${workflow_name} (run ${run_id}) ---" + echo " https://github.com/${repo:-$(gh repo view --json nameWithOwner --jq .nameWithOwner)}/actions/runs/${run_id}" + echo + + log="${log_dir}/${run_id}.log" + if gh run view "${run_id}" "${gh_args[@]}" --log-failed >"${log}" 2>/dev/null && [[ -s "${log}" ]]; then + extract_failure "${log}" + echo + echo " Full log: ${log}" + else + echo " Could not read the failed log for run ${run_id}." + fi +done <<<"${failed_runs}" + +exit 1 diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 000000000..01ea193cd --- /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 000000000..80f4172b6 --- /dev/null +++ b/.claude/skills/crypter-change/SKILL.md @@ -0,0 +1,176 @@ +--- +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 a checkout — a worktree does as well as a main one. `/plans` and `/runs` +resolve against it, and the container is named after it, so the one you reach is always the one +reading the plan you wrote. + +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, the run directory has to be writable from inside it, and the +image has to carry the current tooling. Confirm before starting: + +```bash +.devcontainer/pipeline.sh exec -- test -d /plans/{run-id} && \ + .devcontainer/pipeline.sh exec -- test -w /runs/{run-id}/findings && \ + .devcontainer/pipeline.sh exec -- test -x /usr/local/bin/crypter-workspace +``` + +`pipeline.sh` resolves the container from the checkout it sits in, so which container you get is +settled by where you are rather than by anything you check. The mount checks prove `/plans` and +`/runs` are both there and that uid 1001 can write the run directory. The last check is separate +because an older image passes the first two and then fails at workspace creation with nothing but +a missing executable to go on. All three are `test` because `docker exec` runs a binary and not a +shell, so a builtin like `command -v` exits 127 whether or not the thing it was looking for is +there. + +**Do not `docker start` an exited container to fix any of this.** The image is fixed when a +container is created, so starting an old one brings back the old tooling. Bring it up from here +instead: + +```bash +.devcontainer/pipeline.sh up +``` + +That rebuilds the image and recreates this checkout's container. It cannot touch another +checkout's. What it does destroy is every workspace under `/work` in *this* container, because +`/work` is the container's own filesystem and not a volume — so **ask the user before running it +if another run may be live here.** + +Then make the workspace the container builds in. It is a clone of the repository taken from +GitHub, and it lasts exactly as long as this run: + +```bash +.devcontainer/pipeline.sh exec -- crypter-workspace create {run-id} +``` + +A change of your own targets `stable`, which is what `create` uses when no `--base` is given. +**If it fails, stop and say so.** + +The workspace takes `stable` as the repository holds it, so nothing about your checkout — what +it is on, how stale it is, what is uncommitted in it — reaches the branch. + +## 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 +.devcontainer/pipeline.sh exec -w /work/{run-id} -- \ + 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 +.devcontainer/pipeline.sh exec -w /work/{run-id} -- \ + claude --permission-mode auto -p "/crypter-devcontainer-examine {run-id} {branch} origin/stable /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 +.devcontainer/pipeline.sh exec -w /work/{run-id} -- \ + 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. Nothing to fix has been established yet, and a push that starts no checks is a +setup problem rather than a code one. + +## 8. Tear down and report + +The branch is on the fork and the artifacts are on your disk, so the workspace has nothing left +to hold: + +```bash +.devcontainer/pipeline.sh exec -- crypter-workspace remove {run-id} +``` + +Remove it on every exit path, including the ones where you stopped early. Nothing under `/runs` +or `.claude/plans` is touched by this — those are the record of the run and they stay. + +Then report: + +- The 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. diff --git a/.claude/skills/crypter-devcontainer-examine/SKILL.md b/.claude/skills/crypter-devcontainer-examine/SKILL.md new file mode 100644 index 000000000..c22924ea4 --- /dev/null +++ b/.claude/skills/crypter-devcontainer-examine/SKILL.md @@ -0,0 +1,112 @@ +--- +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} {base-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 the run's workspace at `/work/{run-id}`. + +## Setup + +You are given a run id, a ref, a base ref, and optionally a plan path: +`/crypter-devcontainer-examine {run-id} {ref} {base-ref} [plan-path]`. + +The base ref is the branch the change is proposed against, named as the workspace knows it — +`origin/stable` for work built here, `origin/main` for a pull request that targets `main`. Every +agent below diffs against it. **Pass it on as given; never substitute a default.** A base that +does not match the pull request produces a diff nobody asked about, and the emptiest version of +that failure — a base identical to the ref — reads as four lenses finding nothing wrong. + +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. Put the workspace on the ref + +The workspace at `/work/{run-id}` already exists; the host created it. **If it is missing, stop +and say so** rather than creating one. + +```bash +git -C /work/{run-id} checkout --detach {ref} +``` + +`--detach` because you only read. Leaving the branch unclaimed keeps a later stage free to check +it out and commit to it. **If this fails, stop and say so.** + +Every agent gets the workspace path and works by absolute path inside it. Never `cd`. + +## 2. Plan adherence + +Given a plan path, invoke `conformance-auditor` with it, the workspace, the base ref, 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 workspace, the +base ref, `/runs/{run-id}/findings/{lens}.md`, and its brief below, in full. + +A brief is the whole of what its lens covers, conventions included. The conventions are split by +what goes wrong when they are broken rather than kept as one list, so that a lens is told the +ones it can judge the consequences of and left ignorant of the rest. + +**correctness** — Bugs, boundary conditions, error paths, and what happens when inputs are +hostile or absent. Including: + +- Nulls or exceptions where `Maybe` or `Either` from `Crypter.Common/Monads` + belongs, and the crash or swallowed failure that follows. +- Sync IO on a database, file, or network path. +- Object initializers where a constructor belongs, leaving an object usable before it is whole. +- Magic strings where an enum belongs. +- An entity change under `Crypter.DataAccess/Entities` with no migration in + `Crypter.DataAccess/Migrations`, and whether it needs a companion script in + `Crypter.DataAccess/Scripts`. + +**maintainability** — Readability, scope creep, and the conventions in `CLAUDE.md` and the Coding +Standard that no other lens claims. Including: + +- Comments narrating history rather than explaining the code as it stands. +- A missing `Async` suffix on an async method. The naming is yours; sync IO on a path that should + be async belongs to correctness. + +**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. Including: + +- Raw strings where a validated type from `Crypter.Common/Primitives` exists, and the unchecked + value that reaches past a boundary as a result. + +Adding a lens means adding a brief 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. + +Leave the workspace as it is. The host removes it when the run ends. diff --git a/.claude/skills/crypter-devcontainer-implement/SKILL.md b/.claude/skills/crypter-devcontainer-implement/SKILL.md new file mode 100644 index 000000000..53090e79b --- /dev/null +++ b/.claude/skills/crypter-devcontainer-implement/SKILL.md @@ -0,0 +1,62 @@ +--- +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 at `/work/{run-id}` is a clone of the host repository, taken from a read-only +mount. It has no push url and no credential. 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. + +`/work/{run-id}` already exists; the host created it. **If it is missing, stop and say so** +rather than creating one — the host owns the workspace for the whole run and removes it at the +end. + +## 1. Branch + +The workspace is checked out at `origin/stable`, so build from there: + +```bash +git -C /work/{run-id} checkout -b {branch} refs/remotes/origin/stable +``` + +**If this fails, stop and say so.** A branch cut from the wrong base leaves the diff and the +eventual pull request on the wrong base, and nothing downstream will notice. + +Work by absolute path inside the workspace. Never `cd`. + +## 2. Implement + +Invoke `implementer` with `/plans/{run-id}/plan.md` and the workspace 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 + +Leave the workspace as it is, with `{branch}` checked out and its commits on it. The host fetches +the branch out of it and removes it when the run ends. + +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 000000000..e71b5b9a4 --- /dev/null +++ b/.claude/skills/crypter-devcontainer-remediate/SKILL.md @@ -0,0 +1,54 @@ +--- +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 the run's workspace at `/work/{run-id}`. 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. Claim the existing branch + +The workspace at `/work/{run-id}` already exists; the host created it. **If it is missing, stop +and say so** rather than creating one. + +```bash +git -C /work/{run-id} checkout {branch} +``` + +No `-b` — the branch is already there, carrying the commits an earlier stage put on it. **If +this fails, stop and say so.** + +## 2. Fix + +Invoke `implementer` with the report path and the workspace 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 + +Leave the workspace as it is, with the new commits on `{branch}`. The host fetches them out and +removes the workspace when the run ends. + +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 000000000..db52c5fb3 --- /dev/null +++ b/.claude/skills/crypter-devcontainer-verify/SKILL.md @@ -0,0 +1,68 @@ +--- +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 and crypter-review skills. +--- + +# 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 the run's workspace at `/work/{run-id}`. 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 path lives under `/runs/{run-id}/` and is either a file or a directory. **If it is +absent, stop and say so.** + +**A file** is a report someone collected, and each finding in it already carries an id. Use those +ids. + +**A directory** is the lenses' own output, one report per lens and no ids in it. Every `.md` in +it is a lens report named for its lens. Read each one, split it into its individual findings, and +give each an id of `{lens}-{n}` numbered from 1 in the order the lens reported them — the lenses +rank most severe first, so that order is information worth keeping. + +A lens that found nothing still writes its file saying so. It contributes no findings and no ids, +which is a result rather than a problem. Where every lens reported that way there is nothing to +verify, and that is the answer — say so and stop. + +**A directory holding no files at all is a different thing:** whatever should have filled it did +not run. Stop and say so, and do not report it as lenses finding nothing. + +`/runs/{run-id}/verification/` already exists; the caller creates it. **If it is missing, stop +and say so** rather than creating it. + +## 1. Put the workspace on the ref + +The workspace at `/work/{run-id}` already exists; the host created it. **If it is missing, stop +and say so** rather than creating one. + +```bash +git -C /work/{run-id} checkout --detach {ref} +``` + +`--detach` because you only read. **If this fails, stop and say so.** + +Every agent gets the workspace 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 workspace +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. + +Leave the workspace as it is. The host removes it when the run ends. diff --git a/.claude/skills/crypter-review/SKILL.md b/.claude/skills/crypter-review/SKILL.md new file mode 100644 index 000000000..d90365757 --- /dev/null +++ b/.claude/skills/crypter-review/SKILL.md @@ -0,0 +1,215 @@ +--- +name: crypter-review +description: Review an existing pull request with the container's reviewer lenses, verify what they found, and post the findings that hold. 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. + +Every finding lands on disk. The ones that survive verification also land on the pull request, as +one review that comments and neither approves nor requests changes. + +The lenses do the reviewing and the verifiers rule on what they found. Both run in the container. +You start them, carry what survives to the pull request, and report the rest — **you never review +the diff yourself, and you never decide whether a finding is true.** + +Everything that reaches the author came from a lens that read the code in the container and a +verifier that checked it there. A review posted from here is attributed to them, so anything of +your own inside it is a claim made in someone else's name. + +## Setup + +You are given a pull request number: `/crypter-review {pr-number}`. + +Run from the root of a checkout. `/plans` and `/runs` resolve against it, and the container is +named after it, so the one you reach is always the one whose artifacts you are reading. + +Use `pr-{number}` as the run id. + +```bash +mkdir -p .claude/runs/pr-{number}/findings .claude/runs/pr-{number}/verification +chmod 777 .claude/runs/pr-{number} .claude/runs/pr-{number}/findings \ + .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. Creating them here also keeps you able to delete what +they wrote. + +Then confirm the container is up and current, before anything depends on it: + +```bash +.devcontainer/pipeline.sh exec -- test -w /runs/pr-{number}/findings && \ + .devcontainer/pipeline.sh exec -- test -w /runs/pr-{number}/verification && \ + .devcontainer/pipeline.sh exec -- test -x /usr/local/bin/crypter-workspace +``` + +`pipeline.sh` resolves the container from the checkout it sits in, so which container you get is +settled by where you are rather than by anything you check. What the probes are for is the rest: +the first two prove `/runs` is mounted and that uid 1001 can write the directories you just +made, and the third proves the image carries the current tooling. Without them a later step +fails in a way that reads like something else — a missing executable, findings written somewhere +you never look. + +All three are `test` because `docker exec` runs a binary and not a shell, so a builtin like +`command -v` exits 127 whether or not the thing it was looking for is there. + +**Do not `docker start` an exited container to fix this.** The image is fixed when a container is +created, so starting an old one brings back the old tooling. Bring it up from here instead: + +```bash +.devcontainer/pipeline.sh up +``` + +That rebuilds the image and recreates this checkout's container. It cannot touch another +checkout's. What it does destroy is every workspace under `/work` in *this* container, because +`/work` is the container's own filesystem and not a volume — so **ask the user before running it +if another run may be live here.** + +## 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. + +Read for orientation and for the base branch, not for defects. This read is how you follow the +lenses later, not a first pass at the review. Whatever you notice here is not a finding, and +noticing it is not a reason to go looking for more. + +Take its **base branch** from the same read — `base.ref` from `pull_request_read` with method +`get`, or `.baseRefName` from `gh pr view`. Most pull requests here target `stable`, but a +release targets `main`, and nothing about the number tells you which. Everything below diffs +against the branch the pull request actually names. + +## 2. Fetch it into a workspace + +The container clones from the repository itself, so the pull request head comes straight from +GitHub and nothing has to be staged in your checkout first: + +```bash +.devcontainer/pipeline.sh exec -- crypter-workspace create pr-{number} \ + --base {base-branch} '+refs/pull/{number}/head:refs/heads/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. The base branch needs no fetching of its own — +the clone brings every branch the repository has. + +Whatever your checkout is on, and however stale it is, does not reach the review. + +**If it fails, stop and say so.** + +The workspace lasts for this review and no longer. + +## 3. Examine + +```bash +.devcontainer/pipeline.sh exec -w /work/pr-{number} -- \ + claude --permission-mode auto -p "/crypter-devcontainer-examine pr-{number} pr-{number} origin/{base-branch}" +``` + +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`. + +`claude -p` exits 0 whether or not the run worked. An unknown command, an expired session and a +clean review all come back as success, so the exit code tells you nothing. Look at what it wrote +instead: + +```bash +ls .claude/runs/pr-{number}/findings/ +``` + +An empty directory is a failed run, not four quiet lenses — a lens with nothing to say still +writes its file. **Stop and say so.** + +Do not stand in for the lenses that did not run. Reading the diff and writing up what you would +have found produces a review with nothing behind it, wearing their name, at exactly the moment +there is nothing to post and the pull request looks unreviewed. The run failing is the result; +report that instead. + +## 4. Verify + +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. So every finding is ruled on against the code before it goes +anywhere — by a verifier in the container, one per finding, not by you. + +```bash +.devcontainer/pipeline.sh exec -w /work/pr-{number} -- \ + claude --permission-mode auto -p "/crypter-devcontainer-verify pr-{number} pr-{number} /runs/pr-{number}/findings/" +``` + +Given the findings directory, verify treats every file in it as a lens report, assigns each +finding an id, and gives one verifier the finding and nothing else. + +Verdicts land in `.claude/runs/pr-{number}/verification/{id}.md`, each with the evidence behind +it. + +An empty `verification/` means one of two things and they are not the same. Either every lens +found nothing, which verify reports and which ends in no review — a real and welcome result — or +the run failed the silent way step 3 describes. Verify's own report distinguishes them. **If it +failed, stop and say so**; do not read the absence of verdicts as a clean diff. + +**Do not read the findings before the verdicts exist.** A finding you have already formed a view +on is one you will post or bury on your own authority, which is the whole thing this step moves +into the container. Wait for the verdict and route by it. + +## 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`. + +**Only findings that held go up.** A finding the verifier ruled against is the process working, +and the pull request is not where that belongs — it would cost the author a read to reach the +same conclusion the verifier already reached with the code in front of it. Unsettled findings do +not go up either; nothing unverified reaches the author. + +They are not lost. The verdicts and their evidence stay under `.claude/runs/pr-{number}/`, which +is where a later pass over this pipeline reads what the lenses claimed and how it turned out. + +The review body carries: + +- Which lenses ran, and which found nothing. A quiet lens is a result worth stating. +- Every held finding 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 and the verifiers as what ruled +on them, so the person reading knows what produced it. **Nothing in the review is yours.** + +**If nothing held, post no review.** Say so to the user instead. A review that reports only that +it found nothing still costs everyone subscribed a notification. + +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. Tear down and report + +```bash +.devcontainer/pipeline.sh exec -- crypter-workspace remove pr-{number} +``` + +Remove it on every exit path, including the ones where you stopped early. The findings and +verdicts under `.claude/runs/pr-{number}` are the record and they stay. + +Then report: + +- The review URL, and what went up. +- The counts: how many findings each lens raised, how many held, how many did not, how many are + unsettled. +- The unsettled ones in full. They reached nobody else, so this is the only place they surface. +- Which lenses found nothing. +- Where the artifacts are. + +A lens that raised plenty and had none of it hold is worth a sentence of its own. That is the +pipeline telling you something about the lens rather than about the pull request. 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 000000000..529e0f8bd --- /dev/null +++ b/.claude/skills/crypter-step-open-pull-request/SKILL.md @@ -0,0 +1,64 @@ +--- +name: crypter-step-open-pull-request +description: Push a branch the pipeline built in the container to the repository 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 repository, 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 run's workspace. `git` reaches it over `docker exec`: + +```bash +git -c protocol.ext.allow=user fetch \ + "ext::docker exec -i $(.devcontainer/pipeline.sh name) git upload-pack /work/{run-id}" {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 repository + +`origin` is the org repository, the same one the container cloned and the same one the pull +request opens against. Confirm that before pushing anything: + +```bash +git remote get-url origin +``` + +**If it is not `Crypter-File-Transfer/Crypter`, stop and say so.** A checkout wired up +differently — a fork on `origin`, or the org on some other remote — pushes the branch somewhere +the pull request will not find it. + +```bash +git fetch origin +git push origin {branch} +``` + +## 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 `Crypter-File-Transfer/Crypter`, 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 reviewers who will read it on that pull request. + +## 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 000000000..e7298fb14 --- /dev/null +++ b/.claude/skills/crypter-step-plan/SKILL.md @@ -0,0 +1,76 @@ +--- +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 origin +``` + +Read the code at `origin/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 000000000..d2a02cb4e --- /dev/null +++ b/.claude/skills/crypter-triage-review/SKILL.md @@ -0,0 +1,156 @@ +--- +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 a checkout. `/plans` and `/runs` resolve against it, and the container is +named after it, so the one you reach is always the one whose artifacts you are reading. + +## 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. + +Then confirm the container is up and current, before anything depends on it: + +```bash +.devcontainer/pipeline.sh exec -- test -w /runs/pr-{number}/verification && \ + .devcontainer/pipeline.sh exec -- test -x /usr/local/bin/crypter-workspace +``` + +`pipeline.sh` resolves the container from the checkout it sits in, so which container you get is +settled by where you are rather than by anything you check. What the probes are for is the rest: +the first proves `/runs` is mounted and that uid 1001 can write the directory you just made, and +the second proves the image carries the current tooling. Without them a later step fails in a way +that reads like something else — a missing executable, verification written somewhere you never +look. + +Both are `test` because `docker exec` runs a binary and not a shell, so a builtin like +`command -v` exits 127 whether or not the thing it was looking for is there. + +**Do not `docker start` an exited container to fix this.** The image is fixed when a container is +created, so starting an old one brings back the old tooling. Bring it up from here instead: + +```bash +.devcontainer/pipeline.sh up +``` + +That rebuilds the image and recreates this checkout's container. It cannot touch another +checkout's. What it does destroy is every workspace under `/work` in *this* container, because +`/work` is the container's own filesystem and not a volume — so **ask the user before running it +if another run may be live here.** + +## 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 a workspace + +The container clones from the repository itself, so the head comes straight from GitHub and +nothing has to be staged in your checkout first: + +```bash +.devcontainer/pipeline.sh exec -- crypter-workspace create pr-{number} \ + '+refs/pull/{number}/head:refs/heads/{head-branch}' +``` + +The branch in the workspace takes the pull request's own branch name, so the commits go back to +the branch they came from. + +**If it fails, stop and say so.** + +## 3. Verify + +```bash +.devcontainer/pipeline.sh exec -w /work/pr-{number} -- \ + 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`. Where the +finding has no thread to reply on, comment on the pull request itself with `add_issue_comment`, +quoting enough of the finding that the reply stands on its own. + +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 +.devcontainer/pipeline.sh exec -w /work/pr-{number} -- \ + 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. Tear down and report + +```bash +.devcontainer/pipeline.sh exec -- crypter-workspace remove pr-{number} +``` + +Remove it on every exit path, including the ones where you stopped early. The verdicts under +`.claude/runs/pr-{number}` are the record and they stay. + +Then 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/.devcontainer/.env.example b/.devcontainer/.env.example new file mode 100644 index 000000000..321f8b28c --- /dev/null +++ b/.devcontainer/.env.example @@ -0,0 +1,8 @@ +CRYPTER_GIT_EMAIL="" +CRYPTER_GIT_NAME="" + +# Claude Code's credential in the container. Generate one on the host with `claude setup-token`. +CLAUDE_CODE_OAUTH_TOKEN="" + +# The repository workspaces are cloned from. Set it to work against a fork. +#CRYPTER_REPO_URL="https://github.com/Crypter-File-Transfer/Crypter.git" diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile new file mode 100644 index 000000000..e9cf0728b --- /dev/null +++ b/.devcontainer/Dockerfile @@ -0,0 +1,58 @@ +FROM mcr.microsoft.com/dotnet/sdk:10.0 + +ARG USERNAME=agent +ARG USER_UID=1001 +ARG USER_GID=$USER_UID +ARG NODE_MAJOR=22 +ARG PNPM_VERSION=11.18.0 +ARG CLAUDE_CODE_VERSION=latest + +ENV DOTNET_CLI_TELEMETRY_OPTOUT=1 \ + DOTNET_NOLOGO=1 \ + DOTNET_TOOLS=/usr/local/share/dotnet-tools +ENV PATH="${PATH}:${DOTNET_TOOLS}" + +# Workspaces are ephemeral, so the package caches have to live outside them or every run pays a +# full restore. Both paths are named volumes in docker-compose.yml. +ENV NUGET_PACKAGES=/caches/nuget \ + npm_config_store_dir=/caches/pnpm + +# 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 + +RUN apt-get update \ + && apt-get install --yes --no-install-recommends \ + ca-certificates \ + curl \ + git \ + gnupg \ + jq \ + less \ + && 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/* + +# Crypter.Web runs pnpm install and several vite build scripts in a PreBuild target, +# so a solution build fails without pnpm. Pinned to the version CI uses. +RUN npm install --global "pnpm@${PNPM_VERSION}" \ + && npm install --global "@anthropic-ai/claude-code@${CLAUDE_CODE_VERSION}" \ + && npm cache clean --force + +RUN dotnet workload install wasm-tools + +RUN dotnet tool install dotnet-ef --version '10.0.*' --tool-path "${DOTNET_TOOLS}" + +COPY .devcontainer/workspace.sh /usr/local/bin/crypter-workspace +RUN chmod +x /usr/local/bin/crypter-workspace + +# The caches and the agent's Claude Code state are named volumes. Docker creates a mount point +# that the image does not already contain as root, so creating these here is what gives the +# volumes the right ownership. /work holds the ephemeral workspaces and is not a volume. +RUN mkdir -p /work /caches/nuget /caches/pnpm /home/$USERNAME/.claude \ + && chown -R $USER_UID:$USER_GID /work /caches /home/$USERNAME/.claude + +USER $USERNAME +WORKDIR /work diff --git a/.devcontainer/docker-compose.yml b/.devcontainer/docker-compose.yml new file mode 100644 index 000000000..4f6bf3410 --- /dev/null +++ b/.devcontainer/docker-compose.yml @@ -0,0 +1,50 @@ +services: + pipeline: + # Named per checkout by pipeline.sh, so two checkouts get two containers instead of fighting + # over one. Required rather than defaulted: a bare `docker compose` here would create a + # container whose name says nothing about which checkout's mounts it holds. + container_name: ${CRYPTER_PIPELINE_CONTAINER:?run .devcontainer/pipeline.sh up instead} + # Built here rather than pulled. The image carries tooling and nothing else, so a change to + # it is a local rebuild instead of a publish someone has to approve. + image: crypter-devcontainer:local + build: + context: .. + dockerfile: .devcontainer/Dockerfile + labels: + # What `pipeline.sh list` reads to say which checkout a container belongs to. + com.crypter.pipeline.checkout: ${CRYPTER_PIPELINE_CHECKOUT:?run .devcontainer/pipeline.sh up instead} + environment: + CRYPTER_GIT_NAME: ${CRYPTER_GIT_NAME} + CRYPTER_GIT_EMAIL: ${CRYPTER_GIT_EMAIL} + # Workspaces are cloned from here rather than from the host, so a run sees the branch as + # the repository holds it. Override for a fork. + CRYPTER_REPO_URL: ${CRYPTER_REPO_URL:-https://github.com/Crypter-File-Transfer/Crypter.git} + CLAUDE_CODE_OAUTH_TOKEN: ${CLAUDE_CODE_OAUTH_TOKEN:-} + volumes: + - claude:/home/agent/.claude + # 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 + # Package caches are the one thing that outlives a run. Workspaces are ephemeral, so + # without these every run restores NuGet and pnpm from scratch. + - nuget:/caches/nuget + - pnpm:/caches/pnpm + # Workspaces are created per run at /work/{run-id}. Open a shell on one with + # `exec -w /work/{run-id}`. + working_dir: /work + command: sleep infinity + +# External so every instance on the machine shares them: Claude Code is authenticated once and +# the caches are warmed once. A Compose-owned volume carries the project it was created for, and +# a second project mounting it fails on the mismatch. `pipeline.sh up` creates them. +volumes: + claude: + external: true + name: crypter-pipeline-claude + nuget: + external: true + name: crypter-pipeline-nuget + pnpm: + external: true + name: crypter-pipeline-pnpm diff --git a/.devcontainer/pipeline.sh b/.devcontainer/pipeline.sh new file mode 100755 index 000000000..f2b017148 --- /dev/null +++ b/.devcontainer/pipeline.sh @@ -0,0 +1,119 @@ +#!/usr/bin/env bash +# Resolve which pipeline container belongs to this checkout, and drive its lifecycle. +# +# /plans and /runs are relative mounts, so a container is welded to the checkout it was created +# from. Naming the container after that checkout is what lets several exist at once: the name a +# checkout resolves to is its own, so an orchestrator can never reach another checkout's mounts. +# +# The name is derived rather than configured. There is nothing to set, and nothing that can drift +# out of step with where the checkout actually is. +# +# Parallel runs within one checkout need none of this. They are already separated by the per-run +# workspaces at /work/{run-id}. +set -euo pipefail + +script_dir="$(cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")" && pwd)" +checkout="$(dirname "${script_dir}")" +compose_file="${script_dir}/docker-compose.yml" +env_file="${script_dir}/.env" + +# Shared by every instance on the machine, which is why Claude Code is authenticated once and the +# package caches are warmed once. Declared external, so nothing creates them but this script. +volumes=(crypter-pipeline-claude crypter-pipeline-nuget crypter-pipeline-pnpm) + +usage() { + cat >&2 <<'EOF' +usage: pipeline.sh name print this checkout's container name + pipeline.sh exec [flags] -- {cmd} run a command in this checkout's container + pipeline.sh up create or recreate it, rebuilding the image + pipeline.sh login authenticate Claude Code interactively + pipeline.sh down stop and remove it + pipeline.sh list every pipeline container, and its checkout +EOF + exit 64 +} + +# The name is both a container name and a Compose project name. Compose is the stricter of the +# two: lowercase, and no dots. The slug keeps `docker ps` readable and the hash of the real path +# separates two checkouts that share a basename. +container_name() { + local slug hash + slug="$(basename "${checkout}" | tr '[:upper:]' '[:lower:]' | tr -c 'a-z0-9' '-')" + slug="${slug#-}" + slug="${slug%-}" + slug="${slug:0:20}" + slug="${slug:-checkout}" + + hash="$(printf '%s' "${checkout}" | sha256sum | cut -c1-8)" + + printf 'crypter-pipeline-%s-%s\n' "${slug}" "${hash}" +} + +compose() { + local name + name="$(container_name)" + CRYPTER_PIPELINE_CONTAINER="${name}" \ + CRYPTER_PIPELINE_CHECKOUT="${checkout}" \ + docker compose --project-name "${name}" --file "${compose_file}" "$@" +} + +case "${1:-}" in + name) + container_name + ;; + + exec) + shift + # Flags for docker exec come first, then `--`, then the command. The separator is what keeps + # a command's own flags from being read as docker's. + flags=() + while [[ $# -gt 0 && "${1}" != "--" ]]; do + flags+=("${1}") + shift + done + [[ "${1:-}" == "--" ]] || usage + shift + [[ $# -gt 0 ]] || usage + + docker exec "${flags[@]}" "$(container_name)" "$@" + ;; + + up) + token="${CLAUDE_CODE_OAUTH_TOKEN:-}" + if [[ -z "${token}" && -f "${env_file}" ]]; then + token="$(. "${env_file}" >/dev/null 2>&1; printf '%s' "${CLAUDE_CODE_OAUTH_TOKEN:-}")" + fi + if [[ -z "${token}" ]]; then + echo "No CLAUDE_CODE_OAUTH_TOKEN in .devcontainer/.env." >&2 + echo "Claude Code will use the login in the volume; 'pipeline.sh login' renews it." >&2 + fi + + # Compose will not create an external volume, and a missing one fails the `up` rather than + # being made on the fly. Creating is idempotent, so this is safe on every run. + for volume in "${volumes[@]}"; do + docker volume create "${volume}" >/dev/null + done + + compose up --detach --build + ;; + + login) + # Credentials land in /home/agent/.claude, which is a volume, so the login outlives the + # container and is shared by every checkout on the machine. + docker exec -it "$(container_name)" claude + ;; + + down) + compose down + ;; + + list) + docker ps --all \ + --filter 'label=com.crypter.pipeline.checkout' \ + --format 'table {{.Names}}\t{{.Status}}\t{{.Label "com.crypter.pipeline.checkout"}}' + ;; + + *) + usage + ;; +esac diff --git a/.devcontainer/workspace.sh b/.devcontainer/workspace.sh new file mode 100755 index 000000000..98590295e --- /dev/null +++ b/.devcontainer/workspace.sh @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +# Create and remove the per-run workspaces the agents build and review in. +# +# A workspace is a clone of the repository taken from CRYPTER_REPO_URL. It belongs to one run and +# is removed with it, so no copy of the repository outlives the state it was made from. +# +# Cloning from the remote rather than from the host means a run sees the branch as the repository +# holds it, not as some checkout happens to have fetched it. Nothing on the host is mounted here, +# so neither uncommitted work nor a stale checkout can reach a run. +# +# The clone is anonymous and read-only. Branches leave a workspace by the host fetching from it +# over `docker exec ... git upload-pack`, so no credential is needed in here. +set -euo pipefail + +usage() { + echo "usage: crypter-workspace create {run-id} [--base {branch}] [refspec]" >&2 + echo " crypter-workspace remove {run-id}" >&2 + exit 64 +} + +subcommand="${1:-}" +run_id="${2:-}" +[[ -n "${subcommand}" && -n "${run_id}" ]] || usage +shift 2 + +# The run id becomes a path under /work that `remove` deletes recursively, so it has to be a +# plain name before it is used as one. +if [[ ! "${run_id}" =~ ^[A-Za-z0-9][A-Za-z0-9._-]*$ ]]; then + echo "Run id '${run_id}' is not a plain name" >&2 + exit 64 +fi + +# The branch the run is built or reviewed against. A pull request states its own, so the caller +# passes what it read rather than letting this default stand in for it. +base="stable" +if [[ "${1:-}" == "--base" ]]; then + base="${2:-}" + [[ -n "${base}" ]] || usage + shift 2 +fi + +# The base reaches git as a ref, where a leading dash would be read as an option instead. +if [[ ! "${base}" =~ ^[A-Za-z0-9][A-Za-z0-9._/-]*$ ]]; then + echo "Base branch '${base}' is not a plain branch name" >&2 + exit 64 +fi + +refspec="${1:-}" +workspace="/work/${run_id}" + +case "${subcommand}" in + create) + : "${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}" + : "${CRYPTER_REPO_URL:?Set CRYPTER_REPO_URL to the repository the workspaces are cloned from}" + + if [[ -e "${workspace}" ]]; then + echo "A workspace already exists at ${workspace}. Remove it or use another run id." >&2 + exit 1 + fi + + git clone --quiet "${CRYPTER_REPO_URL}" "${workspace}" + + # The base has to exist before the run is measured against it, and the clone is the first + # place that can be checked. A workspace without its base is no use, so it goes. + if ! git -C "${workspace}" rev-parse --verify --quiet "refs/remotes/origin/${base}" >/dev/null + then + echo "${CRYPTER_REPO_URL} has no ${base} branch." >&2 + rm -rf "${workspace}" + exit 1 + fi + + if [[ -n "${refspec}" ]]; then + git -C "${workspace}" fetch --quiet origin "${refspec}" + fi + + git -C "${workspace}" checkout --quiet -B "${base}" "refs/remotes/origin/${base}" + + git -C "${workspace}" config user.name "${CRYPTER_GIT_NAME}" + git -C "${workspace}" config user.email "${CRYPTER_GIT_EMAIL}" + + echo "Workspace ready at ${workspace}, based on ${base}" + git -C "${workspace}" log --oneline -1 "refs/remotes/origin/${base}" + ;; + + remove) + if [[ ! -d "${workspace}" ]]; then + echo "No workspace at ${workspace}" + exit 0 + fi + + rm -rf "${workspace}" + echo "Removed ${workspace}" + ;; + + *) + usage + ;; +esac diff --git a/.github/workflows/detect-code-changes.yml b/.github/workflows/detect-code-changes.yml index 4aa4dbdfb..aa688bb07 100644 --- a/.github/workflows/detect-code-changes.yml +++ b/.github/workflows/detect-code-changes.yml @@ -6,6 +6,9 @@ on: code: description: 'true when the event touches anything other than documentation' value: ${{ jobs.detect.outputs.code }} + devcontainer: + description: 'true when the event touches the devcontainer' + value: ${{ jobs.detect.outputs.devcontainer }} jobs: detect: @@ -15,6 +18,7 @@ jobs: outputs: code: ${{ steps.detect.outputs.code }} + devcontainer: ${{ steps.detect.outputs.devcontainer }} steps: - name: Checkout repository @@ -29,6 +33,7 @@ jobs: base_sha: ${{ github.event.pull_request.base.sha }} head_sha: ${{ github.event.pull_request.head.sha }} documentation: '(\.md$|^Documentation/|^\.github/ISSUE_TEMPLATE/)' + devcontainer: '(^\.devcontainer/|^\.github/workflows/pr-build-devcontainer\.yml$)' run: | set -euo pipefail @@ -36,6 +41,7 @@ jobs: # the scheduled CodeQL analysis, has to assume code is in scope. if [ "$event_name" != 'pull_request' ]; then echo "code=true" >> "$GITHUB_OUTPUT" + echo "devcontainer=true" >> "$GITHUB_OUTPUT" exit 0 fi @@ -51,3 +57,10 @@ jobs: else echo "code=true" >> "$GITHUB_OUTPUT" fi + + if [ -n "$changed_files" ] && ! echo "$changed_files" | grep -qE "$devcontainer"; then + echo "The devcontainer is untouched." + echo "devcontainer=false" >> "$GITHUB_OUTPUT" + else + echo "devcontainer=true" >> "$GITHUB_OUTPUT" + fi diff --git a/.github/workflows/pr-build-devcontainer.yml b/.github/workflows/pr-build-devcontainer.yml new file mode 100644 index 000000000..c187ec626 --- /dev/null +++ b/.github/workflows/pr-build-devcontainer.yml @@ -0,0 +1,30 @@ +name: Build devcontainer image + +on: + pull_request: + branches: [ main, stable ] + +jobs: + changes: + uses: ./.github/workflows/detect-code-changes.yml + + build-devcontainer: + + needs: changes + if: needs.changes.outputs.devcontainer == 'true' + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0 + + - name: Build image + uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0 + with: + context: . + file: ./.devcontainer/Dockerfile + platforms: linux/amd64 + push: false diff --git a/.gitignore b/.gitignore index d9def341b..cf5941575 100644 --- a/.gitignore +++ b/.gitignore @@ -460,4 +460,13 @@ Crypter.Web/wwwroot/js/dist Crypter.Web/pnpm-lock.yaml # Claude Code worktrees -.claude/worktrees/ \ No newline at end of file +.claude/worktrees/ + +# 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 new file mode 100644 index 000000000..93b710c4b --- /dev/null +++ b/Documentation/Development/Agentic Development Pipeline.md @@ -0,0 +1,324 @@ +# Agentic Development Pipeline + +Three orchestrators compose a set of task skills. You invoke an orchestrator in your own session, +and it invokes the rest. + +| 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. It does not read code to form a +view on it — reviewing a diff and ruling on a finding are both judgements made in the container, +by an agent with the code in front of it. `/crypter-review` reviews nothing itself and settles +nothing itself: it fetches the pull request into the container, runs +`/crypter-devcontainer-examine` there, has `/crypter-devcontainer-verify` rule on what came back, +and carries the survivors to GitHub. + +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 a workspace, +`/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 a checkout**, main or worktree. `.claude/plans` and +`.claude/runs` are mounts relative to `.devcontainer/`, so they resolve against whichever checkout +you launch from, and the container is named after that checkout — so the one you reach is always +the one holding the artifacts you are reading. + +**The container holds no GitHub credential.** It clones anonymously over https and can only read +a public repository. Every authenticated GitHub operation happens in your session with your own +access, and `/crypter-change` pushes and re-pushes without stopping to ask. + +The branch is pushed to the org repository and the pull request opens against it, base `stable`, +the same route a branch of your own takes. `/crypter-change` leaves you a draft pull request to +read. + +The container does hold your Claude Code credential, in the `crypter-pipeline-claude` volume or as +`CLAUDE_CODE_OAUTH_TOKEN` in its environment, and its network egress is open. Treat it as a trust +boundary rather than a sandbox. + +## Workspaces + +The agents build and review in a **workspace**: a clone of the repository at `/work/{run-id}`, +made when a run starts and deleted when it ends. Nothing that holds a copy of the code outlives +the run that made it, so there is no second checkout drifting away from yours. + +Workspaces are cloned from `CRYPTER_REPO_URL` — the org repository on GitHub, not your checkout. +Nothing on the host is mounted for them to read. **Your checkout is therefore irrelevant to what +a run builds or reviews**: it sees the branch as the repository holds it, whatever yours is on, +however stale it is, and whatever is uncommitted in it. A pull request head is fetched straight +from `refs/pull/{number}/head`, so nothing has to be staged on your side first. + +Because each workspace is a full clone taken when it is created, and nothing re-fetches +afterwards, a run is pinned to the commit it started from. Merge to `stable` while a run is going +and it keeps building against what it cloned; the next run gets the new commit. Two runs at +different bases are no trouble. + +The orchestrator owns the lifecycle. It creates the workspace in its setup and removes it when +the run ends; the container skills use it and never create or destroy one. + +```bash +.devcontainer/pipeline.sh exec -- crypter-workspace create {run-id} [--base {branch}] [refspec] +.devcontainer/pipeline.sh exec -- crypter-workspace remove {run-id} +``` + +`--base` is the branch the run is built or reviewed against, `stable` when it is not given. A +pull request states its own base, and a release states `main`, so the review skills pass what +they read rather than assuming. + +This document covers the setup you need before the container will start. + +## The mounts + +Everything crossing the container boundary goes through one of these: + +| 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` | + +Both are gitignored and live on your disk. Only `/runs` is writable; `/plans` the container can +read and nothing more. + +Source is not among them. Nothing of your repository is mounted, so a run cannot read your +working tree, your local branches, or a ref you have not pushed — it clones from GitHub instead. + +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. + +A branch the agents built travels back out the same way it would from any remote, over a git +transport that runs `docker exec` instead of opening a socket: + +```bash +git -c protocol.ext.allow=user fetch \ + "ext::docker exec -i $(.devcontainer/pipeline.sh name) git upload-pack /work/{run-id}" {branch}:{branch} +``` + +`protocol.ext.allow` is passed per command, so it stays out of your git config. Pushing is then +yours, with your credentials — which is what keeps a write token out of a container running +unattended agents. + +## One container per checkout + +`/plans` and `/runs` are relative paths in the Compose file, so they resolve against the checkout +you launch from and a container is stuck with whatever they resolved to when it was created. Each +checkout therefore gets its own container, named after it: + +```bash +.devcontainer/pipeline.sh name # crypter-pipeline-crypter-4f3a9c21 +``` + +The name is derived from the checkout's real path — a readable slug, and a hash to separate two +clones that share a basename. Nothing to configure, and nothing that can drift out of step with +where the checkout actually is. Because a checkout resolves only to its own name, a run can never +reach another checkout's mounts, and recreating one container leaves the others alone. + +```bash +.devcontainer/pipeline.sh list # every instance, and the checkout it belongs to +``` + +This is not what makes runs parallel. Several runs share one container quite happily — they are +separated by their workspaces at `/work/{run-id}` and their artifacts at `/runs/{run-id}`, and +concurrent `docker exec` calls do not queue. Per-checkout naming is about which host directories +a container is wired to, nothing more. + +Starting an exited container does not pick up a newer image — `docker start` reuses what the +container was created with. Recreate it instead: + +```bash +.devcontainer/pipeline.sh up +``` + +That rebuilds the image and recreates this checkout's container. `/work` is the container's own +filesystem rather than a volume, so this destroys any workspace a run in this checkout is still +using. + +## 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, then gives one verifier per +finding the same treatment `/crypter-triage-review` gives findings from anywhere else. Only the +findings that survive that are posted, as one review that comments. It never approves and never +requests changes. + +What a lens raised and a verifier then ruled against stays in `.claude/runs`. It is a record of +the pipeline checking itself, and not something the pull request has to carry. + +`/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 +ignored by git. Copy the template and fill it in before the first `up`. + +```bash +cp .devcontainer/.env.example .devcontainer/.env +``` + +| Variable | Value | | +|---|---|---| +| `CRYPTER_GIT_NAME` | Author name on the agents' commits. | Required | +| `CRYPTER_GIT_EMAIL` | Author email on the agents' commits. | Required | +| `CLAUDE_CODE_OAUTH_TOKEN` | Claude Code's credential. | Optional; an alternative to `pipeline.sh login` | +| `CRYPTER_REPO_URL` | The repository workspaces are cloned from. | Defaults to the org repository; set it for a fork | + +The two git variables fail workspace creation with a message naming the variable when left empty. + +The container's name needs no configuration. It is derived from where the checkout is. + +## Launching the container + +The container is a Compose service in `.devcontainer/docker-compose.yml`, driven through +`pipeline.sh` — which supplies the per-checkout name Compose needs. That is a separate Compose +project from the application stack at the repository root, so `docker compose up` and +`docker compose down` there never touch it, and the two share no network. + +```bash +mkdir -p .claude/plans .claude/runs +.devcontainer/pipeline.sh up +.devcontainer/pipeline.sh exec -- bash # add -it for an interactive shell +``` + +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. + +Running `docker compose` against this file directly fails, on purpose: the container name is a +required variable, and a container created without it would say nothing about which checkout's +mounts it holds. + +The first `up` takes a few minutes, mostly installing the `wasm-tools` workload. After that +Docker's layer cache makes it quick, and a change to `workspace.sh` rebuilds only the last couple +of layers. + +`pipeline.sh down` stops it. The named volumes outlive the container, so the next `up` keeps your +Claude Code credentials and your package caches. + +## What is in the container + +The image is built locally from `.devcontainer/Dockerfile` and tagged `crypter-devcontainer:local`. +It carries tooling and no source, so it only changes when the tooling does. + +Built on `mcr.microsoft.com/dotnet/sdk:10.0`, running as an unprivileged user named `agent` +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 +- 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 +in CI once the pull request exists, and failures come back to the implementer from there. + +It does need **outbound network**, both for the Anthropic API and now for cloning workspaces. +The pipeline does not work offline. + +Three named volumes survive rebuilds, and none of them holds source: + +| Volume | Holds | +|---|---| +| `crypter-pipeline-claude` | The agent's Claude Code state and credentials | +| `crypter-pipeline-nuget` | The NuGet package cache | +| `crypter-pipeline-pnpm` | The pnpm store | + +**Every instance on the machine shares all three**, which is why Claude Code is authenticated +once rather than once per checkout, and why a second checkout's first build is not a cold +restore. They are declared external so that several Compose projects can mount them; `pipeline.sh +up` creates them. + +The two caches exist because workspaces are ephemeral. Without them every run would restore +NuGet and pnpm from nothing, which is most of a build. + +`/work` is the container's own filesystem rather than a volume, so live workspaces do not +survive a `down`. That is the intent: a run that was interrupted leaves nothing behind to +collide with the next one. + +To start over from nothing, take the container down and remove the volumes: + +```bash +.devcontainer/pipeline.sh down +docker volume rm crypter-pipeline-claude crypter-pipeline-nuget crypter-pipeline-pnpm +``` + +The `down` is per-checkout, but removing the volumes is not — it takes the credentials and caches +away from **every** instance. Use `pipeline.sh list` to see what else is on the machine first, +including containers left behind by checkouts that no longer exist. + +## Authenticate Claude Code + +The image ships Claude Code but no credential. Log in once: + +```bash +.devcontainer/pipeline.sh login +``` + +Type `/login` and follow the prompt. The container has no browser, so the flow gives you a URL to +open on your host and a code to paste back. Credentials live in `/home/agent/.claude`, which is +the `crypter-pipeline-claude` volume, so they survive rebuilds and are shared by every checkout +on the machine. Run this again when the login lapses. + +A machine that would rather configure the credential than open a browser can set +`CLAUDE_CODE_OAUTH_TOKEN` in `.devcontainer/.env` instead, generated on the host with `claude +setup-token`. Compose reads it into the container's environment and Claude Code uses it in place +of the stored login. Replacing an expired one means editing `.env` and running `up` again, since +the environment is fixed when the container is created — a stored login renews without that. + +`up` warns when neither is set. Either one on its own is enough. + +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 +that is thrown away at the end of the run, no access to your repository at all, and no GitHub +credential to push with — its only reach into the repository is an anonymous read of what is +already public. + +## Changing the image + +Needed when the tooling changes — a new tool the agents need, a runtime version bump. Source +changes never require it, because the image carries no source. + +```bash +.devcontainer/pipeline.sh up +``` + +That is the whole loop. The image is local to your machine — it is never published, and nobody +else consumes it — so a change to `workspace.sh` or the Dockerfile takes effect on your next +`up`. + +The image tag is shared, so the rebuild is machine-wide; other instances pick the new image up +when they are next recreated, not before. + +`pr-build-devcontainer` builds the image on a pull request that touches `.devcontainer/`. It +pushes nothing; it is there to catch a Dockerfile that does not build. diff --git a/README.md b/README.md index 06e27569b..ec19ba978 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,7 @@ Check out these documents to get started working on Crypter: * [Contribution Guide](./CONTRIBUTING.md) * [Coding Standard](<./Documentation/Development/Coding Standard.md>) * [Development Environment Setup](<./Documentation/Development/Development Environment Setup.md>) +* [Agentic Development Pipeline](<./Documentation/Development/Agentic Development Pipeline.md>) Also take a look at some of the articles that have come in handy while working on the project: