Repository navigation
chore: Commit the Sandcastle setup - #228
Conversation
Track .sandcastle/ (Dockerfile, main.mts, prompts, coding standards and .env.example) and the package.json and pnpm-lock.yaml that pull in @ai-hero/sandcastle, so a fresh clone can run it. The nested .sandcastle/.gitignore still keeps .env, logs/ and worktrees/ out. The prompts use a top-level heading per section and inline tags, so a nested .markdownlint-cli2.jsonc turns off MD025 and MD033 for them, as in Blazor-Server. One long line in plan-prompt.md is wrapped for MD013. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #228 +/- ##
=======================================
Coverage 85.63% 85.63%
=======================================
Files 75 75
Lines 1566 1566
Branches 186 186
=======================================
Hits 1341 1341
Misses 190 190
Partials 35 35 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
The sandbox lacks .NET tooling, verification scripts fail, and generated branches violate enforced repository conventions.
7 open findings
What changed in this PR
Adds Sandcastle orchestration for autonomous issue planning, implementation, review, and merging.
Changes:
- Adds Docker-based Sandcastle configuration and prompts.
- Adds Node dependencies and pnpm lockfile.
- Adds environment, ignore, and Markdown lint configuration.
| File | Description |
|---|---|
.sandcastle/.env.example |
Documents required tokens. |
.sandcastle/.gitignore |
Excludes generated and secret files. |
.sandcastle/.markdownlint-cli2.jsonc |
Configures prompt linting exceptions. |
.sandcastle/CODING_STANDARDS.md |
Adds reviewer standards placeholder. |
.sandcastle/Dockerfile |
Defines the agent container. |
.sandcastle/implement-prompt.md |
Directs issue implementation. |
.sandcastle/main.mts |
Orchestrates planning, execution, review, and merging. |
.sandcastle/merge-prompt.md |
Directs branch merging and issue closure. |
.sandcastle/plan-prompt.md |
Plans issue dependencies and branches. |
.sandcastle/review-prompt.md |
Directs automated code review. |
package.json |
Declares Sandcastle dependencies and scripts. |
pnpm-lock.yaml |
Locks Node dependency versions. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The planner named branches sandcastle/issue-{id}, which
scripts/check-branch-name.sh rejects. The host now names them in
.sandcastle/lib/branches.mts, as Blazor-Server does: hotfix/{n}-{slug}
for a bug and feature/{n}-{slug} otherwise, with the slug taken from the
issue's GitHub title. An issue's existing feature/{n}-* or hotfix/{n}-*
branch is reused, so a renamed issue keeps its earlier work. The planner
now returns only issue ids and titles.
pnpm test:sandcastle runs the branch tests with Node's built-in test
runner. One test checks generated names against check-branch-name.sh.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The image had only Node, so an agent couldn't build or test the solution. It now installs the latest .NET 10 SDK (global.json pins 10.0.401 with rollForward latestMinor) and libicu72 for it, plus python3, yamllint and shellcheck for scripts/gate.sh, and enables pnpm through corepack at package.json's pinned version. This follows Blazor-Server's image. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sandbox hook ran npm install, which ignores pnpm-lock.yaml and can leave an untracked package-lock.json. It now runs pnpm install --frozen-lockfile, and package.json pins pnpm 12.6.0 in packageManager (recorded in the lockfile) and refuses npm. package.json said ISC; the repo is MIT. It is now private, drops the npm init placeholders, and adds pnpm sandcastle to start the loop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mmits The prompts ran npm run typecheck and npm run test, which don't exist for this .NET repo. The implement, review and merge prompts now run the build and test commands from CLAUDE.md, and skip the three test projects that need Docker, which the sandbox lacks; CI runs them. The implementer was told to prefix commits with RALPH:. Its commits, and the reviewer's and merger's, now follow git-commit-instructions.md: <type>(<scope>): <Summary>. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ules CODING_STANDARDS.md held only the template's placeholder examples. It now names the repo's sources of truth (CLAUDE.md, .editorconfig, the instructions files and docs/PROCESS.md) and lists the rules reviews most often catch, without restating them in full. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Automated release blog posts for #228, opened by the release workflow. It holds every Release whose post isn't on main yet, rebuilt from main on each run. The [skip-release] title marker keeps its merge from starting another release. Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mpaulosky
added
the
review:claude
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Review
This PR adds Sandcastle, an agent loop under .sandcastle/ that runs on the host with pnpm and Node. A planner agent picks open issues labelled Sandcastle. For each one, an implementer agent and then a reviewer agent work on a branch the host names in lib/branches.mts. A merger agent merges those branches into the checked-out branch and closes the issues. The PR also adds a sandbox Dockerfile with the .NET SDK, the prompts, branch-name tests, package.json and pnpm-lock.yaml. Verdict: hold for changes. (1) The merge step counts only commits made in the current run. That breaks the new branch reuse, and it lets unfinished work be merged and its issue closed. (2) The merger merges into the local checked-out branch (main, per PROCESS.md) and closes issues before anything is pushed or goes through a PR. (3) Text that any GitHub user can write (issue comments, and issue bodies the author can still edit) reaches agents that hold GH_TOKEN and Claude tokens. (4) Bug issues get hotfix/ branches instead of the fix/ that PROCESS.md calls for. (5) CI never runs the new tests. Bash was disabled and the @ai-hero/sandcastle package isn't installed here, so I ran nothing and couldn't read its source. Findings 1 and 2 rely on my understanding of how it behaves: sandcastle.run() with no branch works on the host's checked-out branch, and RunResult.commits lists only that run's commits.
| .filter( | ||
| (entry) => | ||
| entry.outcome.status === "fulfilled" && | ||
| entry.outcome.value.commits.length > 0, |
There was a problem hiding this comment.
A branch is merged only if commits.length > 0. As I understand Sandcastle, that counts only commits made in this run, and it conflicts with the branch reuse in branchFor. It goes wrong both ways.
- Earlier work is never merged. Round 1 commits to
feature/8-…, then the reviewer or merger fails or crashes, so issue 8 stays open. In round 2 the planner picks it again andbranchForreuses the branch. The implementer sees the work is done and makes no new commits. The branch is skipped every round untilMAX_ITERATIONSruns out, and the reviewer never runs (line 152). - Unfinished work is merged. If the implementer stops at 100 iterations without
<promise>COMPLETE</promise>, any commits it made still go to the merger, which closes the issue as "Completed by Sandcastle".
Fix: decide on what the branch holds compared with the current branch (for example git rev-list --count HEAD..<branch> > 0) rather than this run's commits. Run the reviewer on the same condition, and require the completion signal before an issue counts as done.
|
|
||
| For each branch that was merged, close its issue using the following command: | ||
|
|
||
| `gh issue close <ID> --comment "Completed by Sandcastle"` |
There was a problem hiding this comment.
Issues are closed before any of the work reaches GitHub.
- The merger runs
sandcastle.run()without abranch(main.mts:222). As I understand Sandcastle, that means it merges into whatever the host clone has checked out. By docs/PROCESS.md that ismain, since the clone stays on main. - It then runs
gh issue close <ID> --comment "Completed by Sandcastle". No branch has been pushed, no PR opened, and no required checks or Copilot review have run. - The pre-push hook refuses pushes to
main, so the work can't be pushed frommain, but the issues are already closed. .github/hooks/pre-commitalso refuses commits in the primary checkout. So the "single commit summarizing the merge", or committing a merge that had conflicts, fails there.
Fix: push each finished branch and open a PR with Fixes #n, or merge into an integration branch that goes through a PR. Let the PR's merge close the issue instead of having the agent close it.
|
|
||
| <issues-json> | ||
|
|
||
| !`gh issue list --state open --label Sandcastle --limit 100 --json number,title,body,labels,comments --jq '[.[] | {number, title, body, labels: [.labels[].name], comments: [.comments[].body]}]'` |
There was a problem hiding this comment.
This puts every comment on a Sandcastle-labelled issue into the planner's prompt. The implementer also reads the issue with gh issue view (implement-prompt.md:3).
- On a public repo any GitHub user can comment on a labelled issue. An issue's author can also edit its body at any time after a maintainer labels it.
- These agents run with
GH_TOKEN(Issues: write) and the Claude OAuth token/API key. An injected instruction such as "add the Sandcastle label to issue #N" makes the planner queue an attacker's issue for the implementer, whose code is then merged. It could also send the tokens out of the sandbox.
Fix: keep only comments whose authorAssociation is OWNER, MEMBER or COLLABORATOR (for example [.comments[] | select(.authorAssociation | IN("OWNER","MEMBER","COLLABORATOR")) | .body]). Treat issues from untrusted authors the same way. Otherwise, accept the risk explicitly.
| .sort()[0]; | ||
| if (existing) return existing; | ||
|
|
||
| const prefix = issue.labels.includes("bug") ? "hotfix" : "feature"; |
There was a problem hiding this comment.
Bug issues get hotfix/{n}-… branches, but docs/PROCESS.md says fix/{issue}-{slug} is for "A bug fix" and hotfix/ is for "An urgent fix". So every Sandcastle bug fix is filed as urgent.
isIssueBranch (line 32) and localIssueBranches (line 54, refs/heads/feature/ and refs/heads/hotfix/ only) also ignore fix/ branches. If someone already started fix/7-… the standard way, Sandcastle creates a second branch for issue 7 rather than building on it.
Fix: use fix for the bug label. Match and list fix/{n}- branches as well, and update the hotfix/7-stop-the-crash test in branches.test.mts.
| "description": "Sandcastle, the agent loop in .sandcastle/. The app itself is .NET: see CLAUDE.md.", | ||
| "scripts": { | ||
| "sandcastle": "node .sandcastle/main.mts", | ||
| "test:sandcastle": "node --test \".sandcastle/**/*.test.mts\"" |
There was a problem hiding this comment.
test:sandcastle is defined only here. No job in ci.yml runs .sandcastle/**/*.test.mts, so the branch-naming tests (including the check against scripts/check-branch-name.sh) never run on a PR. A change that breaks branch naming would merge without notice.
Fix: add a step like the existing automerge-tests job (setup-node 22, then node --test ".sandcastle/**/*.test.mts").
Also, running .mts files without flags needs Node 22.18 or later. Consider adding "node": ">=22.18" to engines, so an older host Node fails clearly instead of hitting a syntax error.
…ocally (#236) ## Why This addresses the Claude Review findings on the merged #228, which committed the Sandcastle setup. Three of them are design problems with the old plan, implement, merge loop: - **Done was decided by this run's commits.** A branch went to the merger only when `commits.length > 0` for this run. A re-planned issue whose branch already held finished work made no new commits, so it was never merged. An implementer that ran out of iterations without `<promise>COMPLETE</promise>` still had its commits merged and its issue closed. - **The merger closed issues before anything reached GitHub.** It merged into the host checkout (which stays on `main`, where the hooks refuse commits and pushes) and then ran `gh issue close`, with no PR, checks or review. - **Untrusted text reached agents holding `GH_TOKEN`.** Every comment and issue body on a `Sandcastle` issue went into the planner's prompt, and the implementer fetched the issue with `gh`. On a public repo anyone can comment, so injected instructions could steer agents that had an Issues-write token. ## What changed This ports the host-side design from mpaulosky/Articles#308, without the problems that design's own review found. - **One draft PR per issue, no merger.** `merge-prompt.md` is gone. `lib/build.mts` runs each issue in its own sandbox: implement, merge `origin/main`, run `.sandcastle/check.sh`, review, then push and open a draft PR whose body ends `Fixes #n`. The issue closes when that PR merges through the normal checks. Anything that stops (unfinished, merge conflict, failed check, rejection, failed push) gets a comment on the issue and isn't pushed. - **Done means finished and ahead of main.** The implementer must signal completion. After that the branch is checked, reviewed and published whenever `git rev-list --count origin/main..HEAD` is above 0, so earlier finished work still gets its PR. - **The reviewer returns a verdict.** It ends with `<verdict>{"approved", "summary"}</verdict>` (`lib/verdict.mts`). A missing or malformed verdict counts as a rejection. - **Only the host talks to GitHub.** `lib/github.mts` reads issues with the host's `gh` auth and keeps only issues opened by, and comments from, an `OWNER`, `MEMBER` or `COLLABORATOR`. The agents get the issue through prompt arguments. `.env.example` no longer has `GH_TOKEN`, and `main.mts` refuses to start if `.sandcastle/.env` sets `GH_TOKEN` or `GITHUB_TOKEN`, since Sandcastle passes every key in that file into the sandbox. - **Fixes for what Articles' review found:** - The host runs git with `-c core.hooksPath=/dev/null`. `core.hooksPath` is `.github/hooks`, a path inside the worktree the agents edit, so running its pre-push hook on the host would run their code outside the sandbox. CI runs the full suite, the Docker-backed tests included. - Each branch merges a freshly fetched `origin/main` in the sandbox before the check. On a conflict the merge is aborted and the issue gets a comment. - Open PRs from forks (`isCrossRepository`) neither hold an issue back nor pass for its PR. - Repeated planner picks are dropped (`lib/plan.mts`). - A PR title like `Add search .` becomes `feat: Add search`, with no trailing space. - After the reviewer, the host compares HEAD with the commit that passed the check, not the commits the run reported, so an amend or reset is caught. If HEAD moved, the check runs again. The host pushes that exact commit by its id, without force. - **Branches:** existing local branches are reused alongside origin's. When origin has the branch, the local branch is created from it, or fast-forwarded to it if it's behind. - **Host hardening (from this PR's review):** - Sandcastle mounts the clone's `.git` into each sandbox, writable, so an agent could write commands into `.git/config` for the host's git to run. `main.mts` records `git config --local --list` before any sandbox starts, and the host refuses to run git or `gh` once it has changed (`lib/shell.mts`). `gh` takes `--repo` from the name read at the start, not from the remotes. - Issue comments and PR bodies have the sandbox's secrets removed: the values `.sandcastle/.env` gives it, and anything shaped like a Claude or GitHub token. - "fixes #12"-style references in the reviewer's summary are rewritten as "fixes issue 12", so only the host's `Fixes #n` line closes an issue. - A branch that changes the files deciding what `check.sh` runs or skips (`check.sh`, `needs-docker.sh`, `discover_tests.py`, `package.json`, or a `RequiresDocker` flag) is named in a WARNING at the top of its PR. - The planner gets the in-review issues as open blockers, since nothing reaches main between rounds, and an issue that stopped isn't rebuilt later in the same run. - **`.sandcastle/check.sh` (from #231) is now enforced.** The host runs it in the sandbox through `lib/check.mts`, and a passing check over uncommitted changes still fails. `check.sh`, `CODING_STANDARDS.md` and `CLAUDE.md` no longer call it advisory, and they say CI runs the Docker-backed suites. The implement and review prompts keep #231's Docker guidance. - **`test:sandcastle`** runs `tsc --noEmit -p .sandcastle` before the tests (new `.sandcastle/tsconfig.json`, with `typescript` and `@types/node` as dev dependencies). `engines` requires Node 22.18 or later, which strips the `.mts` types. The tests now import `@ai-hero/sandcastle` and `zod`, so `.github/ci/sandcastle-tests.sh` runs `pnpm install --frozen-lockfile` first. It uses pnpm from the PATH, or corepack's when there isn't one. ### Findings checked but not changed here - **`branches.mts:45`, bug issues on `hotfix/`:** already fixed by #232. - **`test:sandcastle` not run in CI:** #231 runs it from the gate and from CI's Build Solution job, through `.github/ci/sandcastle-tests.sh`. - **Shell blocks from issue titles:** in `@ai-hero/sandcastle` 0.12.0, `substitutePromptArgs` marks the template's own `` !`…` `` blocks with `\x01` before it fills in `{{KEY}}` arguments and strips that marker from argument values. `preprocessPrompt` runs only marked blocks, so a block written in an issue title or body isn't run. A new test in `prompts.test.mts` checks that only host-made values (`BRANCH`, `BASE_BRANCH`, `TASK_ID`) appear inside the prompts' shell blocks. ## Verification - `bash .github/ci/sandcastle-tests.sh ""`: the type check is clean and 89 of 89 tests pass, including #231's. New tests cover publishing earlier finished work, refusing unfinished work, merge conflicts, a reviewer that amends or resets, fork PRs, repeated picks, the trailing space in titles, the git config guard (against a real temp repo), redaction, closing keywords and the check-file warning. - The pre-push gate passed (lints, Release build, every test project). - No live Sandcastle run: one needs labelled issues and opens real PRs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: T <t@e.com>
Why
.sandcastle/and thepackage.jsonthat installs@ai-hero/sandcastlewere never committed, so a fresh clone can't run Sandcastle and prompt changes go unreviewed. Sandcastle expects the folder to be committed, with only its generated files ignored. Blazor-Server already works this way.As generated, the setup also didn't fit this repo. The image had no .NET SDK. The prompts verified with npm scripts that don't exist. Branch names failed the branch standard, and commits used a
RALPH:prefix instead of Conventional Commits. This PR fixes those problems too.What changed
.sandcastle/holds theDockerfile,main.mts, the*-prompt.mdfiles,CODING_STANDARDS.md,.env.exampleand Sandcastle's.gitignore(which ignores.env,logs/andworktrees/).package.jsonandpnpm-lock.yamlpull in@ai-hero/sandcastleandzod..sandcastle/.markdownlint-cli2.jsonc, copied from Blazor-Server, turns off MD025 and MD033 for the prompts.rollForward: latestMinor) and libicu72. It also adds python3, yamllint and shellcheck forscripts/gate.sh, and enables pnpm through corepack.pnpm install --frozen-lockfile.package.jsonpinspnpm@12.6.0inpackageManager(pnpm records this in the lockfile) and refuses npm.pnpm sandcastlestarts the loop..sandcastle/lib/branches.mts, never the model. A bug getshotfix/{n}-{slug}and every other issue getsfeature/{n}-{slug}, with the slug taken from the issue title. An issue's existing branch is reused.pnpm test:sandcastleruns the tests, and one of them checks names againstscripts/check-branch-name.sh.dotnet build IssueTracker.slnx -c Release -warnaserroranddotnet testonce per test project. They skip the three test projects that need Docker, because the sandbox has none; CI still runs those.<type>(<scope>): <Summary>instead of using theRALPH:prefix.CODING_STANDARDS.mdused to hold placeholders. It now points the reviewer atCLAUDE.md,.editorconfig,.github/instructions/anddocs/PROCESS.md, and lists the rules reviewers most often have to flag.package.json, matching the repo. It was ISC.Verification
.env,logs/andworktrees/are not committed, and every value in.env.exampleis empty..sandcastle/Markdown files.pnpm test:sandcastle: 11 tests pass.pnpm install --frozen-lockfilepasses against the committed lockfile.dotnet --versionreports 10.0.401 in the workspace, and pnpm 12.6.0, python3, yamllint, shellcheck and gh all run.scripts/gate.sh) passed on5df8e82.🤖 Generated with Claude Code