Skip to content

build: share the Go build cache across worktrees with -trimpath - #793

Merged
EricAndrechek merged 1 commit into
mainfrom
build/trimpath-v2
Oct 9, 2026
Merged

EricAndrechek merged 1 commit into
mainfrom
build/trimpath-v2

Conversation

@EricAndrechek

Copy link
Copy Markdown
Member

Go keys this module's own packages in the build cache by absolute directory, so every worktree compiled and cached its own copy of each variant (normal, test, -race, -cover -coverpkg=./...), and Go only evicts entries unused for five days. -trimpath makes the entries path-independent.

  • Makefile. Exports GOFLAGS=-trimpath, keeping any GOFLAGS the caller passes (also when given on the make command line). A GOFLAGS that already names -trimpath is left alone, so sub-makes don't prepend it again and GOFLAGS=-trimpath=false make … opts out. CI runs the same targets and inherits it. No workflow or .goreleaser.yaml change: they have no direct go build/go test calls that matter here, and release-build paths are build: add -trimpath to release, container, and goreleaser builds #537.
  • Path anchors. natstest.repoFile walks up from the working directory to go.mod, because tests in several packages import it. The roles integration test builds the binary from two levels above its package directory, which is where go test runs it. Nothing else uses runtime.Caller; debug.Stack in internal/api/router.go only feeds a log field.
  • Debugging. make build's debug binary is exempt (scripts/build.sh passes -trimpath=false) and keeps absolute source paths for Delve. Other binaries are trimmed and need a source-path substitution; development.md has the dlv example.

Test plan

This branch's diff is identical to #765's (same 6 files, +40/−11, compared line by line), which CodeRabbit approved. Re-run here against main: the pre-commit make verify and a full make ci (unit suite, integration suite including TestRoles, and the builds). The three measurement items below were measured on #765 and not repeated.

Related Issues

Closes #763. Related: #537 (-trimpath for release builds). Part of #740. Supersedes #765 (rebuilt on main, no longer stacked on #760).

🤖 Generated with Claude Code

https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH

Go keys this module's own packages in the build cache by absolute
directory, so every worktree compiled and cached its own copy of each
variant (normal, test, -race, -cover). The Makefile now exports
GOFLAGS=-trimpath, keeping any GOFLAGS the caller passes; CI runs the
same targets and inherits it.

The two tests that located repository files with runtime.Caller, whose
path is module-relative under -trimpath, now find the root from the
working directory: natstest walks up to go.mod, and the roles test builds
from two levels above its package directory.

make build's debug binary is exempt (scripts/build.sh passes
-trimpath=false) and keeps absolute source paths for debuggers;
development.md gives a runnable Delve substitute-path example for the
trimmed binaries.

Closes #763. Part of #740.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 69dcf875-fe36-4304-8b2c-91c3835da19e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added documentation Improvements or additions to documentation go Pull requests that update go code area/docs Documentation, site/, README area/infra CI, build, deploy, Docker, release labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

📚 Docs preview is live → https://231f9f01-wavehouse-docs.wave-rf.workers.dev

  • Commit — 17d1d42: build: share the Go build cache across worktrees with -trimpath
  • Author — @EricAndrechek, Claude Opus 5.5
  • Committed — 2026-10-09 10:49 (UTC-04:00)
  • Deployed — 2026-10-09 11:31 EDT

@github-code-quality

github-code-quality Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: Go

Go

The overall line coverage in commit 17d1d42 in the build/trimpath-v2 branch remains at 93%, unchanged from commit 6c80008 in the main branch.

Show a line coverage summary of the most impacted files.
File main 6c80008 build/trimpath-v2 17d1d42 +/-
internal/mq/nats_topology.go 92% 91% -1%
internal/mq/external.go 85% 85% 0%
internal/mq/nat...est/natstest.go 75% 75% 0%

Updated October 09, 2026 15:35 UTC

@EricAndrechek
EricAndrechek marked this pull request as ready for review October 9, 2026 15:29
@EricAndrechek
EricAndrechek requested review from a team and taitelee October 9, 2026 15:29
@EricAndrechek
EricAndrechek merged commit d26b2b6 into main Oct 9, 2026
@EricAndrechek
EricAndrechek deleted the build/trimpath-v2 branch October 9, 2026 15:35
EricAndrechek added a commit that referenced this pull request Oct 9, 2026
Brings in #793 (the squash of this branch's parent, identical tree) and #788.
This PR's own diff against main is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
EricAndrechek added a commit that referenced this pull request Oct 9, 2026
)

Repeated pushes to a PR reused no Go build objects. Measured from CI
logs (2026-10-06 to 10-08), that only matters on a PR that changes
`go.mod`/`go.sum`, so only that case is cached, and only for the two
long-pole jobs.

- **Measured, typical PR.** The integration suite's `app` part compiles
in 5.8–8.5 s from `main`'s entry, against 2.3–3.2 s from a same-tree
one. e2e's 4–7 s cover build is hidden behind its SDK build. A PR-scoped
cache would buy about 4–5 s per push, which is inside the `app` part's
run-to-run spread, so these PRs still save nothing.
- **Measured, dependency-changing PR (#730, #742, #753).** Every push
recompiles the dependency delta from `main`'s generation: 35–50 s in the
integration suite and a 37–49 s cover build in e2e, against about 3 s
and 4 s from the PR's own entry. That is about 30–45 s per push on the
critical path. Of 129 PR runs since 2026-09-25, 34 came from 9 such PRs,
and 25 of those were second or later pushes.
- **Classifier.** `scripts/classify-paths.sh` also emits `go-deps`: any
`go.mod`/`go.sum` in the change set, in any module, which are the files
the cache keys hash. `classify-changes.sh` passes it through only for a
cleanly classified pull request and sets it false everywhere it fails
closed. The `changes` job exposes it as an output.
- **setup-env.** A new `go-cache-pr-save` input, which the `app` part
and e2e set only when `go-deps` is true. So a PR that changes no
dependency never saves, even while `main` holds no entry for its key:
just after a bump lands on `main`, after a failed `main` run, or for a
newly opted-in job. With the input set, the job looks up the exact key
first, from the PR's scope or `main`'s, so a PR whose dependency change
`main` already has hits `main`'s entry and saves nothing. Only on a miss
does it restore the newest entry of its flavor (the PR's own, then
`main`'s) and save the exact key into the PR's scope. The key is per
dependency state, so a PR saves once per dependency change. Merge-queue
refs and branches stay restore-only, and nothing is added to `ci.yml`
beyond the two inputs.
- **Pruning.** `pr-cache-cleanup.yml` gains a `workflow_run` job that
runs after every PR's CI run. It runs the new
`scripts/ci/prune-pr-build-cache.sh` from the default branch with
`actions: write` and deletes every PR-scoped `gobuild-v3` entry that
`main` also holds or that a newer entry of the same flavor supersedes.
It deduplicates the paged listing before grouping, since a row returned
twice would otherwise delete the PR's newest entry, and a fixture test
(`make test-prune-pr-build-cache`, a new `make verify` leaf) pins the
selection and the failure handling. No PR code holds the write token.
The job needs no PR number, so it covers fork PRs too. A PR holds at
most about 0.48 GiB of build cache (double that for the minutes before a
prune).
- **Budget.** The README sizes this for four such PRs open at once, the
most open at once in the last two weeks. The worst case at rest is about
11.7 GiB, about 1.7 GiB over the cap, and eviction by last access takes
it from `main`'s superseded generation, which nothing reads (inferred).
At two such PRs everything fits. `main`'s live entries stay safe up to
six: 3.9 + N × (0.5 CodeQL + 0.48 build) ≤ 10 GiB, assuming the
superseded generation is the size of the live one. The six build flavors
together are 1.02 GiB on `main`; the issue's "about 1.3 GiB per suffix"
was the total for all six of a PR's flavors.

## Test plan

This branch's diff matches #783's (same 13 files, +450/−67), which
CodeRabbit approved; the only line differences are the verify-leaf list
and the `make verify` table row, which differ because the base did.
Re-run here: the pre-commit `make verify` (which runs both fixture
suites below) and a full `make ci`. Items marked "measured on #783" were
not repeated; the CI items are observable once this PR targets `main`.

- [x] `make lint-gha` (actionlint, which also checks the new input and
the `go-deps` output) and `make lint-sh` clean; `make verify` green in
the pre-commit hook, including the new leaf.
- [x] `make test-classify-paths`: new `go-deps` cases (root and nested
modules, look-alike names, mixed with docs, empty, large input); a
mutant that matches only the root `go.mod`/`go.sum` fails it (mutant
measured on #783). `classify-changes.sh` against a stub `gh`:
`go-deps=true` only for a pull request whose file list holds a `go.sum`;
false on push, merge group, dispatch and a failed listing.
- [ ] (measured on #783) The setup-env step conditions, evaluated for
push to `main`, `pull_request` with and without the input, `merge_group`
and `workflow_dispatch`: exactly one restore path runs, and only a PR
miss with the input set is save-capable.
- [ ] (measured on #783) `prune-pr-build-cache.sh --dry-run` against the
live cache list selects 6 entries (1058 MiB): copies of `main`'s keys
under #731, #754 and #756. It keeps #753's own entries (one per flavor,
not on `main`) and never touches `main` or merge-queue scopes.
- [x] `make test-prune-pr-build-cache`: deletes copies of `main`'s key
and version and keeps a same key with another version; deletes
superseded entries and the older of a same-second pair; keeps
`-integration-` apart from `-integration-app-`; ignores other families
and `main`/merge-queue scopes; keeps a PR's only entry listed twice.
Through a stub `gh`: a 404 reads as already gone, a 403 fails after the
rest are attempted, and a failed listing fails. Mutants caught (measured
on #783): no early dedup (the pre-fix script), key-only match, merged
flavors, no prefix filter, `main` scope included, listing failure
ignored, 404 treated as an error, 403 swallowed.
- [ ] This PR's own run, which changes no dependency: `Detect changes`
logs `go-deps=false`, and every Go job, the `app` part and e2e included,
uses the plain restore step, with no `Cache saved` for `gobuild-v3` in
any post step.
- [ ] After merge, the first `PR cache cleanup` run triggered by a PR's
CI (`Prune PR build caches`): passes and deletes the entries the dry run
listed, if they are still there.
- [ ] The first PR that changes `go.mod`/`go.sum` (the next Dependabot
go-deps PR): `Detect changes` logs `go-deps=true`; the first push
restores `main`'s generation through restore-keys and saves
`gobuild-v3-Linux-go-integration-app-<its hash>` and `-e2e-cov-<its
hash>` under `refs/pull/<n>/merge`; the second push exact-hits them, its
integration compile is about 3 s and e2e reaches its containers about 20
s after `make` starts; the prune leaves one entry per flavor; the close
cleanup deletes them. A fork PR's save is unverified: if GitHub refuses
it, the job only logs a warning.
- [x] `make ci` passes on this branch, after merging `main` (#793 and
#788).

## Related Issues

Closes #781. Refs #535 (two generations on `main` still leave thin
headroom). Part of #740.

Supersedes #783. Now based on `main`: its parent, the `-trimpath` PR
#793, has merged. It does not itself use `-trimpath` or `make
ci-remote`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
EricAndrechek added a commit that referenced this pull request Oct 9, 2026
This PR has two parts:

1. **`make ci-remote`** runs the pre-push gate's `make ci` on another
machine.
2. **The repo's hooks now fail closed.** These are git's `.githooks/`
and the Claude Code hooks in `.claude/`. Each one failed the wrong way
when a file it runs was missing, or when an action came in a form the
hook didn't recognise (#678, #787, #790).

They share the pre-push hook, the verify leaves and the hook docs, so
they ship together and their overlaps are resolved once.

### Part 1: `make ci-remote`

A code push needs `tmp/ci-passed-tree-<tree>`, and only a `make ci` in
the same checkout wrote it. So every push paid for a full local run
(race detector, coverage, a Docker daemon carrying every suite's
containers), and the gate only ever exercised the contributor's own OS
and architecture.

`make ci-remote HOST=<host>` runs that `make ci` on another machine and
keeps the gate's meaning.

- **Transports.** `HOST` is any ssh destination (a `~/.ssh/config` alias
works), or `docker:<container>` for a local container or devcontainer.
Over ssh the commands run in a login shell; in a container, in its own
environment.
- **What travels.** The worktree must be clean. `HEAD` is streamed as a
`git bundle` on the transport's stdin, right after the host-side script,
so nothing is pushed. The host checks out that commit in a fresh
directory under `REMOTE_DIR`, runs `make tools && make ci`, streams the
output back with its `uname -sm` first, and removes the directory unless
`KEEP=1`. Stopping the command, by Ctrl-C or a signal to the script
alone, stops the run on the host: the host's stdin stays open only while
the script runs, and a watcher stops the run's process group when it
closes.
- **When the marker is written.** Only if all of these hold: the host
exits 0; the tree it reports having checked out is `HEAD^{tree}`; `make
ci` left its checkout clean; and the local worktree is still clean and
on that tree at the end. The host's report lines carry a per-run nonce,
so `make ci`'s own output can't forge them. The verify marker is written
too, as `make ci` does.
- **Provenance.** `tmp/ci-passed-tree-<tree>.provenance` records the
host, OS/arch, start and end times, and the run's own local copy of the
log (`tmp/ci-remote-<tree>-<run>.log`, so concurrent runs of one tree
don't overwrite each other). The pre-push hook prints `make ci passed on
<host> (<os>/<arch>)` from it. Marker semantics and local `make ci` are
unchanged.
- **Docs.** A `make ci-remote` section in the development guide (usage,
host requirements, how the gate treats it), the AGENTS.md pre-push
rules, the hook table, and CONTRIBUTING.md's pre-push sentence.

### Part 2: hooks that fail closed

- **#678:** git's `pre-commit` failed loudly in a worktree with no code,
so nothing could be committed there.
- **#787:** the Claude Code hooks failed silently when their scripts
were gone, so the push gate stopped gating and nothing said so.
- **#790:** the agent gate checked only the `gh pr` spelling of each
action it guards, so the alias and API forms went through. Its `gh`
checks also blocked commands that only *mentioned* a guarded action in a
heredoc or a comment.

Each now fails closed and says why.

#### #678: git hooks in a worktree with no Makefile

With an absolute `core.hooksPath`, git runs one checkout's `.githooks/`
in every worktree, including one on an orphan branch that keeps a tool's
state and no code. There, `pre-commit` couldn't find
`scripts/ci-marker.sh`, hid that behind `2>/dev/null`, and ran `make
verify` with no Makefile (`No rule to make target 'verify'`).

- **`pre-commit`:** no `Makefile` in the worktree means nothing to
verify, so it exits 0 with a note. A missing `scripts/ci-marker.sh`, or
one that fails with anything but "no marker" (exit 1), is now reported,
and then `make verify` runs, which is the fail-closed path.
- **`pre-push`** had the same failure, reported as a misleading "can't
resolve tree". It now decides per pushed commit, not per worktree. A
pushed commit with no `Makefile` holds no code to validate, so it goes
through. One with a `Makefile`, pushed from a worktree that lacks
`scripts/ci-marker.sh`, is blocked with a message naming the script.
Deciding by worktree would have failed open: `git push origin
<code-branch>` from a worktree with no code would have skipped the
check.
- **The `make ci-remote` test needed one fix for this.** Its scratch
repository had no `Makefile`. So once the pre-push hook passed a commit
without one, the test's two pre-push cases ("blocks a code push with no
marker" and "accepts the marker and names the host") stopped exercising
the marker check and failed. A code repository has a Makefile, so the
scratch one now does too, in its own commit.

**On `core.hooksPath`.** This PR doesn't change it. `make tools` sets
the relative `.githooks`, and git resolves a relative `core.hooksPath`
in each worktree (checked with git 2.56). So each worktree runs its own
branch's hooks, and a worktree whose branch has no `.githooks/` runs no
hooks at all. That fails open, the class of bug #787 is about. #678 was
found with an absolute path, which runs one checkout's hooks everywhere.
Switching to a relative path would have "fixed" #678 by turning a loud
failure into a silent one. So the fix is in the hooks, which now behave
correctly under either setting.

#### #787: Claude Code hooks whose scripts are missing

Each hook in `.claude/settings.json` was a bare
`$CLAUDE_PROJECT_DIR/.claude/hooks/<script>.sh`. `CLAUDE_PROJECT_DIR` is
fixed when the session starts. Once that checkout is removed, or a
script loses its executable bit, the shell exits 127 or 126. Claude Code
[treats that as a non-blocking
error](https://code.claude.com/docs/en/hooks#other-exit-codes): "When
the script path doesn't exist or isn't executable, the shell exits with
a code like 127 … For most hook events, the action proceeds. … a
mistyped path in `settings.json` leaves the gate silently disabled."

The scripts may be the thing that's missing, so the fix has to live in
the commands. Each is now a one-line POSIX `sh` guard; `sh -c` is how
Claude Code [runs a shell-form
hook](https://code.claude.com/docs/en/hooks#exec-form-and-shell-form) on
macOS and Linux.

When the script is there and executable, the guard `exec`s it, with the
same process, stdin, argv and environment as before. The only visible
change is that the path is now quoted, so a project directory with a
space in it works.

When the script isn't there, the guard acts by event:

| Event | Script missing or not executable → | Why this code |
| ----- | ---------------------------------- | ------------- |
| PreToolUse (Bash) | **exit 2** for any command containing the words
`git` and `push`; `gh` and `pr` with one of `create`, `new`, `edit`,
`ready`, `review` or `merge`; or `gh` and `api` with `pulls` or
`graphql`. That last rule blocks every call to the pull-request or
GraphQL API, reads included. Also **exit 2 for every command** when `jq`
is missing or the payload can't be parsed. Every other command: exit 0.
| "Exit 2 means a blocking error … `PreToolUse` blocks the tool call",
and exit 2 is "the only exit code that blocks through the code alone"
([Exit code 2](https://code.claude.com/docs/en/hooks#exit-code-2)). |
| SubagentStop | **exit 1**, with a one-line stderr message | For
SubagentStop, exit 2 "Prevents the subagent from stopping" ([per-event
table](https://code.claude.com/docs/en/hooks#exit-code-2-behavior-per-event)),
which risks a loop. A non-blocking error shows "a `<hook name> hook
error` notice followed by the first line of stderr" ([Other exit
codes](https://code.claude.com/docs/en/hooks#other-exit-codes)), so the
message is one line. No marker gets written, so a push is still blocked,
by the push gate or by the PreToolUse fallback above. |
| PostToolUse (both formatters) | **exit 2**, with a stderr message |
For PostToolUse, exit 2 "Shows stderr to Claude; the tool already ran"
([per-event
table](https://code.claude.com/docs/en/hooks#exit-code-2-behavior-per-event)).
The docs recommend exit 2 for exactly this case: "To surface a warning
to Claude from a `PostToolUse` … hook, exit 2 instead" ([Exit code
0](https://code.claude.com/docs/en/hooks#exit-code-0)). Stderr from exit
0 "goes to the debug log only … Claude never sees it", and exit 1 only
reaches the transcript notice. |

Each message names the missing script and says how to recover: restart
Claude Code from a live checkout.

Design notes:

- **Why not block every Bash call?** A session whose launch checkout has
gone can still be working in other worktrees, and it needs Bash to
notice the problem and recover. Blocking only what the gate guards keeps
the gate's guarantee. The exception is a missing `jq`: then the command
can't be read, so every command is blocked, which is what
`agent-bash-gate.sh` already does without `jq`.
- **The fallback's detection is crude on purpose.** It matches whole
words anywhere in the command text, ignoring quoting, so it over-blocks:
`git stash push`, `git push` mentioned in a commit message, and a
read-only `gh api …/pulls/N/comments` are all blocked. It never runs
while the real gate can run, and like the gate it guards against
accidents, not deliberate workarounds. Only `.tool_input.command`
decides; the Bash tool's `description` field is ignored.
- **`jq` isn't guaranteed.** It's a documented prerequisite (`make
verify` and the hooks need it), not a given, so its absence fails
closed.
- **Every fallback reads its payload to the end** before exiting, so
Claude Code isn't left half-way through writing a large `Write` payload
to a hook that never reads it.
- **A script that exists but can't start** (a broken shebang, say) still
fails open. The guard checks that the script exists and is executable;
catching a failed `exec` would mean giving up the `exec`.

#### #790: the agent gate missed the alias and API forms of what it
guards

`agent-bash-gate.sh` matched only the `gh pr` spellings, so each guarded
action went through in another form:

- `gh pr new`, an alias of `gh pr create`, skipped the `--draft` check
and the title lint.
- `gh api …/pulls/N/reviews -f event=APPROVE` approved a PR.
- `gh api …/pulls -f head=… -f title=…` opened a non-draft PR.
- `gh api …/requested_reviewers -f 'reviewers[]=…'` requested a
reviewer. The check wanted `-X POST` or a literal `reviewers=`, though a
field already makes `gh api` POST.
- `gh api -X PUT …/pulls/N/merge` merged.
- GraphQL `markPullRequestReadyForReview` marked a draft ready.

Each of these now gets the same answer as its `gh pr` form:

- **`gh pr new`** is checked like `gh pr create`.
- **REST.** A `gh api` call counts as a write when it names a method
other than GET or, naming none, sends fields.
  - A write to `…/requested_reviewers` (`DELETE` included) is blocked.
  - A write to `…/pulls/N/merge` is blocked.
- A write that opens a PR (`…/pulls`) needs `draft=true`. Its `title=`
goes through the PR-title lint, as does a retitle (`PATCH …/pulls/N`).
  - A review with `event=APPROVE` or `event=REQUEST_CHANGES` is blocked.
- **GraphQL.** These are blocked:
  - `markPullRequestReadyForReview`;
  - `mergePullRequest` and `enablePullRequestAutoMerge`;
  - `requestReviews`;
- an `addPullRequestReview` or `submitPullRequestReview` that approves
or requests changes;
  - a `createPullRequest` without `draft: true`.
- **`gh pr merge` stays with the `deny` rule** in
`.claude/settings.json` and out of the gate. A person merges with it
from Claude Code's shell mode (`! gh pr merge …`), and the docs don't
say whether PreToolUse hooks see shell-mode commands. The [shell-mode
section](https://code.claude.com/docs/en/interactive-mode#shell-mode-with-prefix)
says only that they run "without going through Claude" and don't
"require Claude to interpret or approve the command"; the [hooks
reference](https://code.claude.com/docs/en/hooks) doesn't mention them.
Blocking it in the gate could break that workflow. The API and GraphQL
merge forms don't overlap with it, so the gate blocks them.

**The checks now read what the line runs, not what it mentions.** The
`gh` checks used to read the line with quoted strings removed and
everything else kept. That made them wrong both ways:

- They blocked a command that only mentioned a guarded action in a
heredoc body or a comment, such as a `python3 - <<'EOF'` script with `#
gh pr ready` in it.
- They missed a guarded action inside a `$(…)` or handed to `bash -c`.

They now read the commands the line runs, through the gate's own
tokenizer, the same view the push check already uses:

- A mention in a quoted string, a heredoc body or a comment doesn't
count.
- Code a shell runs is queued, tokenized and judged the same way. That
covers a `$(…)`, `eval`'s words, a heredoc a shell reads directly or
through a pipe, and the code a shell is handed with `-c`. Options before
`-c` are stepped over, including the value of `-o`/`+o`: `bash -euo
pipefail -c '…'` and `bash --noprofile --norc -c '…'` both count. A
short-option cluster containing `c` counts as `-c`; a long option such
as `--norc` doesn't.
- The code inside a `$(…)` is read as written, quotes and heredoc bodies
included. So `url=$(gh pr create --draft --title "feat(x): …")` is
judged on its real title, and a heredoc a shell reads inside a `$(…)`
counts too. (The push check still doesn't follow that last form.)
- A GraphQL `query=` computed by a command (`-f query="$(cat <<'EOF' …
EOF)"`) has its guarded mutation names matched against the code that
computes it, heredoc bodies included.
- The gate follows 64 pieces of nested code. Past that, a piece left
that mentions a `gh pr` or `gh api` command is refused, unjudged.
- A line the tokenizer can't parse is refused when it mentions a `gh pr`
or `gh api` command, because what it runs can't be told.

**Each `gh` command is judged on its own words.**

- A `gh api` call is judged by its endpoint (its first argument that
isn't a flag) and by its fields. `draft=`, `title=` and `event=` are
read only from `-f`, `-F`, `--field` or `--raw-field` values, and
GraphQL mutation names only from the `query=` field. So a review-thread
reply or comment whose body names an endpoint, a field or a mutation
doesn't count, and neither does another call on the same line.
- gh's own `-R`/`--repo` before the command is stepped over (`gh -R o/r
pr ready 12`, `--repo=o/r`, `-Ro/r`), for `pr` and `api` both.
- A boolean flag set with `=` is read as gh reads it: `--draft=true`
counts as `--draft`, and `--approve=false` doesn't approve.
- Titles are read from the tokens, so an unquoted `--title` is linted
too.

The gate still reads the command line only, so fields sent through
`--input` or `-F field=@file` go unseen.

**It runs under macOS's bash 3.2.** On a Mac with no newer bash on
`PATH`, `#!/usr/bin/env bash` finds `/bin/bash` 3.2. An earlier revision
of this branch stopped there with "unbound variable" whenever a gh line
had no substitution, because it expanded an empty array under `set -u`.
The gate then exited 1, which Claude Code doesn't treat as a block. The
scan now walks its arrays by index. The three hook tests run a second
time with `/bin/bash` first on `PATH` when it is a different bash, so a
construct 3.2 rejects fails a test.

**Timing.** The tokenizer used to step through a double-quoted string
one character at a time; it now takes runs of plain text in one step.
Measured on an earlier revision of this change (a 100 KB inline `--body
"…"`: 11 s with the old character loop, under 1 s after); not
re-measured after the per-call rewrite.

## Test plan

**On this branch at its head, 9b8c30c (measured):**

- [x] `make verify` passes with all 23 leaves.
- [x] `make build-docs` passes.
- [x] shellcheck v0.11.0 is clean on all 48 tracked `*.sh` and
`.githooks/*` files.
- [x] The pre-commit `make verify` passed on each commit that went
through it.
- [ ] `make ci`: not run.

**Part 1, `make ci-remote`.** Carried over from the stacked version of
this PR (#766) unless marked as re-run here.

- [x] `scripts/ci/remote-ci.test.sh`, a new `make verify` leaf
(`test-remote-ci`): **31/31 on this branch (re-run here)**. It runs in a
scratch repository, which now has a Makefile, with stand-in
`ssh`/`docker` commands that run the host side for real, in its own
process group as sshd would, against a stub `make`, `uname` and `git`.
It covers:
  - a passing run: markers, provenance fields, the log, host cleanup;
  - `KEEP` and `REMOTE_DIR`;
  - a failing `make ci`;
- two tree mismatches: HEAD moved during the run, and the host checked
out another commit;
- a worktree dirtied during the run, and `make ci` dirtying the host's
checkout;
- stopping the local process group, or only the script, mid-run; either
way the host's `make ci` stops and its directory goes;
  - a report line forged in `make ci`'s output;
- dirty-worktree and bad-`HOST` refusals with
`status.showUntrackedFiles=no` set;
  - the `docker exec` command line;
  - two runs of one tree keeping separate logs;
  - the pre-push hook with and without provenance.
- [ ] Each check in the script and the hook was deleted in turn, and
every deletion fails at least one case. The exception was the
empty-report check, which was then folded into the tree comparison.
(Carried over.)
- [ ] Against a real Linux x86_64 host over ssh (carried over):
- What worked: the bundle checked out at the right tree, the report
lines came first, and phase 1 of `make ci` passed there, `remote-ci
test` included.
- What didn't: phase 2 needed host-side Docker networking setup. `make
ci` ran inside a container on the host's network, so testcontainers
dialed the bridge gateway and timed out;
`TESTCONTAINERS_HOST_OVERRIDE=localhost` is the documented fix.
- No marker was written for that run, and none when the local side was
killed.
  - A full green `make ci` on a remote host has not been run yet.
- [ ] `ssh -T` and `docker exec -i` both return within a second of the
remote command exiting while local stdin is still open, which the
interrupt watcher relies on. (Carried over.)

**Part 2, hooks that fail closed (measured):**

- [x] `make test-githooks` (`scripts/githooks.test.sh`, new verify
leaf): 11/11 pass, under bash 5.3 and again with `/bin/bash` 3.2 first
on `PATH`.
- It builds a scratch repository whose `core.hooksPath` names this
checkout's `.githooks/` by absolute path, then commits and pushes from a
code worktree and from an orphan worktree with no Makefile.
- The test clears git's repository variables, uses `git -C` throughout,
and refuses to write unless each scratch repository and worktree is
where it asked.
- Against the old hooks, **6 of 11 fail**, including `make: *** No rule
to make target 'verify'` for the commit with no Makefile.
- [x] `make test-hook-commands` (`.claude/hooks/hook-commands.test.sh`,
new verify leaf): 171/171 pass under `sh`, `dash` and `bash`, and
171/171 again with `/bin/bash` 3.2 first on `PATH`.
- It reads every hook command from the real `settings.json` and runs it
under each shell.
- It covers an empty `CLAUDE_PROJECT_DIR`, a non-executable copy of
every script, the variable unset, and no `jq`.
- It runs each command against stub scripts that record their stdin byte
for byte, their argv and their exit code, including with a path
containing a space.
  - It also runs this checkout's real scripts.
- A catch-all case fails any hook command, present or future, that exits
0, 126 or 127 when its script is missing or not executable. Dropping `[
-x "$h" ] &&` from the SubagentStop guard fails 6 cases.
- Against the old `settings.json`, **141 of 171 fail**. The 30 that pass
are exactly the stub-script and real-checkout cases, which shows the
commands behave as before when the scripts exist.
- [x] `make test-review-gate`: 230/230 pass under bash 5.3, and 230/230
with `/bin/bash` 3.2.57 first on `PATH`.
  - The 93 new cases cover every alias, REST and GraphQL form above.
  - **Code inside `$(…)`, read as written:**
- Blocked: a GraphQL ready mutation captured by `$(…)`; a bad title is
still linted there; a `gh pr ready` in a heredoc read by `bash` inside
`$(…)`.
- Allowed: a draft PR's URL captured with a two-word title and with a
scoped title; a comment body with an apostrophe; a `grep -c 'gh pr
ready'`.
- **Look-alikes that must go through:** GET on a PR, its reviews, its
requested reviewers and its merge status; `-X GET` with fields; a
COMMENT review; a GraphQL query; a GraphQL draft create; an issue
comment that mentions approving and merging.
- **Mentions, allowed:** a Python heredoc's comment, a `cat` heredoc, a
shell comment, a quoted string, a PR-body heredoc.
- **A guarded command a shell runs, blocked:** in a `$(…)`, `bash -c`,
`bash -lc`, `sh -c` or `eval`, in a heredoc read by `bash` or piped into
`sh`, and behind `timeout`.
- **`=`-flags in both directions:** `--draft=true` allowed,
`--draft=false` blocked, `--approve=true` and `--request-changes=true`
blocked, `--approve=false --comment` allowed.
- **Per-call judgment:** a review-thread reply quoting a merge endpoint,
a comment naming `requested_reviewers` or `event=APPROVE`, and a GraphQL
comment naming a mutation are allowed. A reviewer read followed by a
comment is allowed; a comment followed by a reviewer write is blocked. A
non-draft create whose body says `draft=true` is blocked.
- **`-R`/`--repo` forms:** blocked for ready, review `--approve`, create
without `--draft`, edit `--add-reviewer` and an API merge. `gh -R o/r pr
view` is allowed.
- **Nesting cap:** a `gh pr ready` in the 63rd `$(…)` is judged and
blocked, one past the 64 pieces the gate follows is refused, and a gh
read beside 70 substitutions with no gh in them is allowed.
- **Heredoc GraphQL query:** ready-for-review is blocked; a read is
allowed.
- **Shell options before `-c`:** `-e -c`, `-euo pipefail -c`, `-o
pipefail -c`, `+o posix -c` and `--noprofile --norc -c` are all
followed, by the gh check and the push check alike. `bash --norc`
running a script and a read under `bash -o pipefail -c` are allowed.
  - **Against main's gate:** 49 of the 93 fail, under both bashes.
  - **Against earlier revisions of this branch:**
- The revision before the last round failed 6 of the 7 newest cases, the
`$(…)` ones above, under both bashes. Only "a bad title is still linted"
passed there.
- The round before that failed 12 of its 21 newest cases (cap, `-R`,
heredoc query, shell options), under both bashes.
- The one before that, under `/bin/bash` 3.2, gave 135 ok / 67 FAIL,
every failure "TK_SUBS[@]: unbound variable".
- [x] `make test-hook-commands` also caught the bash 3.2 bug: "the real
gate blocks gh pr ready" exited 1 instead of 2, in all three shells.
- [x] The inline guards pass shellcheck as `sh` and `dash`. `make
lint-sh` now also checks the extensionless `.githooks/` files.
- [x] Timing: measured on an earlier revision of this change (a 100 KB
double-quoted `--body`: 11 s with the old character loop, under 1 s
after); not re-measured after the per-call rewrite.

## Related Issues

Closes #764
Closes #678
Closes #787
Closes #790
Supersedes #766 (rebuilt on `main` after #785).
Part of #740

Based on `main`. The two PRs it was stacked on, #793 (`-trimpath`) and
#794 (the per-PR build cache), have merged. Gate forms review found but
left out of this PR, as less common than the ones it covers: #797.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation, site/, README area/infra CI, build, deploy, Docker, release documentation Improvements or additions to documentation go Pull requests that update go code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

build: worktrees don't share the Go build cache

1 participant