chore: tool-neutral verification harness — scripts/verify.sh, AGENTS.md, + Claude Code wiring - #39
Merged
Merged
Conversation
… hooks Adds .claude/ so Claude Code sessions in this repo (a) stop prompting for routine read-only and build commands, and (b) surface CI failures at edit time instead of after a red run. The hook dispatches by path, mirroring the `paths:` filters in .github/workflows/: crates/** -> cargo fmt --all (rust.yml fmt lane) web/** -> npm run typecheck (web.yml web job) frontend/** -> node frontend/_verify.js (web.yml prototype job) Kept deliberately narrow: - The heavy rust.yml lanes (cargo test across the three feature lanes, clippy) are permitted but NOT hooked — too slow for every edit. - cargo fmt rewrites rather than --check, and never fails the hook; formatting isn't worth interrupting an edit over. - git/gh allow rules are enumerated read-only verbs rather than `git *` / `gh *`, which would have silently allowed push and reset --hard. Writes still prompt. Uses python3 rather than jq to parse the hook payload; jq is not present on all dev machines here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three additions to the project harness, all grounded in files already in
the repo rather than in generic advice:
/ci — runs every lane from .github/workflows/ locally: the three-way
rust test matrix, both clippy lanes, check-apply, the web
typecheck+build, the prototype verify, and mkdocs --strict.
Covers what the edit hook deliberately skips (the hook only runs
the fast per-file checks). Carries forward the workflow's own
warning: a default-features run cannot see code behind
datafusion/serve, so `cargo test --workspace` alone is not green.
/dst — runs the deterministic-simulation suite and interprets it against
ADR-001. States the constraint the ADR is emphatic about: DST
proves orchestration, scheduling and the timing model, never
absolute GB/s. DST + calibration, never DST instead of it.
dst-reviewer — reviews Rust changes for sans-I/O and four-seam violations.
The agent encodes a layer distinction that a naive grep would get wrong.
`SystemTime::now`, `Instant::now` and `thread_rng` currently appear 9 times
under crates/ — every one of them legitimately, either in the shell
(crates/vdg main.rs/serve.rs: CLI parsing, HTTP handlers, request timing) or
in the seam definitions themselves (core/src/clock.rs, core/src/rng.rs,
which *are* the Clock and Rng seams). Flagging those would make the agent
noise. It flags them only in control-plane code, or where control-plane
logic has leaked into the shell.
Verified before committing: frontmatter parses on all three; the DST suite
passes (4 tests, 0.38s); clippy is clean on the default lane; and both
--features vdg/serve and vdg/datafusion resolve.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The layer guidance had the core/seam relationship backwards. It told the reviewer that `SystemTime::now()` in core/src/clock.rs "is the point, not a violation" — treating the seam-defining files as exempt. That is not how this codebase is structured. core/src/clock.rs holds the `Clock` trait and `SimClock`; core/src/rng.rs holds the `Rng` trait and `SeededRng`. Both are deterministic and neither touches a real time source or real entropy. The production `RealClock` lives in the shell, at crates/vdg/src/realclock.rs, whose doc comment says why: "Lives in the shell (not core) so the core stays free of any real time source." So the invariant is the sharper one: core holds seam traits and their deterministic impls only; every real, nondeterministic impl lives in the shell. A real SystemTime::now() in core/src/clock.rs would be a genuine violation — and the previous wording trained the agent to wave through exactly that. Also adds a verified table of the known-legitimate hits, which the earlier pass asserted but had not fully checked (a `head -3` had truncated one of four SystemTime hits, and two cited "seam" hits were doc comments rather than calls). Notably serve.rs:1132 `gen_secret()` must keep using OS entropy — drawing a 256-bit auth token from the injected seeded Rng would make tokens reproducible from a seed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The verification commands and the DST review rules were encoded only in
.claude/, which no other agent or contributor reads. Someone using Codex,
MiniMax, or no agent at all had no way to know what to run or which
architectural rules matter. The knowledge was portable; the format wasn't.
Extracts both into tool-neutral files and reduces .claude/ to thin wiring:
scripts/verify.sh The single source of truth for what must pass. Callable
by a human, any agent, a git hook, or CI.
verify.sh <path>... per-path, sub-3s (edit-time)
verify.sh --all [group] full gate set (rust|web|docs)
Exit 0 pass / 1 fail. Degrades gracefully when a tool
isn't installed rather than failing.
AGENTS.md Root, cross-tool convention (the repo already used it at
frontend/AGENTS.md). Covers how to verify, the DST
discipline, the core/shell layering rule, the
known-legitimate-hits table, and what a green DST run
does and does not prove.
.claude/ now delegates rather than duplicates:
hooks/on-edit.sh Reduced to a payload adapter: parse Claude Code's JSON,
call scripts/verify.sh, map exit 1 -> 2. No check
knowledge of its own.
commands/ci.md Calls `scripts/verify.sh --all $ARGUMENTS`, with an
explicit instruction not to expand the command list
inline (that is how it drifts from CI).
commands/dst.md Points at AGENTS.md §2.4 for interpretation.
agents/dst-reviewer.md
Keeps only the review *procedure*; the rules now live in
AGENTS.md §2 so there is one place to update. Adds the
"do not truncate grep output" note, since that mistake
already produced a wrong conclusion on this codebase.
CLAUDE.md gains a pointer at the top: it is the product brief (what Verdigris
is), AGENTS.md is how to work on it.
Verified: verify.sh passes from an unrelated cwd; every per-path branch exits
0 on pass; web and frontend failure paths exit 1 through the script and 2
through the Claude adapter (proven with a real TS2322 error and a thrown
exception, then reverted); malformed stdin, node_modules, dist and unrelated
paths no-op; missing mkdocs skips gracefully; bash -n clean on both scripts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
explise
changed the title
Code review of this branch found three separate paths where verify.sh printed "verify: OK" and exited 0 without running a check. For a script whose whole job is gating a push, that is the worst possible failure mode — and AGENTS.md tells people to trust it. Fixed, each reproduced before and after: 1. Missing toolchain reported success. `have cargo || return` skipped the check but left fail=0, so on a fresh clone or slim container `verify.sh --all` printed four SKIP lines and then "verify: OK", exit 0. Skips are now tracked separately and surface as exit 2 (INCOMPLETE) with a SKIP line each. An unrun check is not a passed check. 2. Relative paths from a subdirectory silently matched nothing. Routing glob-matched the raw argument, so `cd web && ../scripts/verify.sh src/lib/api.ts` returned instantly with OK — the typecheck never ran. Arguments are now resolved against the caller's original cwd (captured before the cd) and made repo-relative before matching. Paths outside the repo warn instead of being silently dropped. 3. An empty argument printed help and exited 0. `verify.sh ""` — what a caller interpolating an unset variable produces — reported success having checked nothing. Zero-args (help) is now distinguished from an empty first arg (error), `--` is supported, and unknown `-*` options are rejected rather than treated as paths. Also fixed: 4. `cargo fmt --all` in per-path mode reformatted the entire workspace, so editing one file silently rewrote unrelated crates — demonstrated by dirtying crates/ingest and then verifying a file in crates/query. Now runs `rustfmt` on the named files only, with the edition read from Cargo.toml. 5. CI parity was overstated. The header claimed workflows "run the same commands" while CI pins node 20 and this box runs node 26. The claim is now scoped to commands, not toolchain versions, and a detectable node major mismatch prints a warning. 6. Docs built into a hardcoded /tmp/verdigris-docs-verify — a collision between users on a shared host, never cleaned up, and divergent from CI's _site. Now mktemp -d, removed after. 7. `npm ci` wiped the developer's node_modules on every --all run, discarding npm-linked or patched deps. Now only installs when node_modules is missing or older than package-lock.json. `--all web` drops from a cold reinstall to 4.3s. 8. AGENTS.md §2.3 pinned the seam-exception table to line numbers that drift. Now keyed by symbol (RealClock::now_millis, track_metrics, h_query, now_millis, gen_anchored, gen_secret), with an explicit note that a hit in an unlisted symbol is a candidate finding. 20-case regression suite run, all passing: the three false-green paths, the fmt blast radius (unrelated crate untouched, named file still formatted), temp-dir cleanup, option parsing, and the previously-verified behaviour (failure paths exit 1 through the script and 2 through the Claude adapter, node_modules/dist/target skipped, malformed stdin no-op). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
verify.sh gates every push and AGENTS.md tells contributors to trust it, but it sat outside every workflow's `paths:` filter, so changing the gate ran no CI at all. It had already shipped three separate paths that printed "verify: OK" and exited 0 having checked nothing. The thing that verifies everything else was the one thing unverified. Adds .github/workflows/harness.yml, triggering on scripts/**, .claude/**, and AGENTS.md. It runs shellcheck, then scripts/verify_selftest.sh, then two guards: the self-test must not SKIP any case in CI (a skip there means a toolchain is missing and the case covers nothing), and it must leave the working tree clean. scripts/verify_selftest.sh is the 27-case suite, previously existing only as ad-hoc commands in a terminal. Every case that fixed a real bug is marked REGRESSION so it doesn't get deleted as redundant. The suite is mutation-tested — a suite that cannot fail is worthless. Each of the three fixed regressions was deliberately reintroduced into verify.sh and the suite caught all three: skip-accounting (4 cases fail), format scoping (1), empty-arg handling (1). That exercise found a bug in the suite itself. The format-scoping case originally used scratch files under crates/ that belonged to no crate, so `cargo fmt --all` never reached them — the assertion passed vacuously and could not detect the blast-radius regression it existed for. It now dirties two real files in two different crates and restores them via an EXIT trap that is safe on a dirty tree. Re-mutated to confirm the correct assertion now fires. Also fixes shellcheck findings in the new suite (SC2015 A && B || C rewritten as if/then/else, SC2181, SC2329 on the trap-invoked cleanup). verify.sh and the hook were already clean. Verified with shellcheck v0.11.0 — without this the new CI step would have failed on its own first run. Unlike the rest of this branch, this workflow does trigger on this PR: nine changed files match its path filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The harness lane failed on its first real run. Two causes, both version drift between my machine and the runner: 1. The runner's preinstalled shellcheck is older than the v0.11.0 I tested against, and the two disagree about a trap-invoked function: older releases flag the body as unreachable (SC2317), newer ones flag the function as uncalled (SC2329). I had suppressed only SC2329, so the lint passed locally and failed in CI. Both codes are now suppressed, with a comment explaining why both are needed. 2. The step used the runner's copy at all. Now pinned to v0.11.0 and downloaded explicitly, matching how this repo pins node 20 and mkdocs==1.6.1 — otherwise a runner image upgrade silently breaks the lane again. Extracted into RUNNER_TEMP rather than /usr/local/bin, which is root-owned on the runner and would have needed sudo. Verified against BOTH v0.9.0 (reproducing the version that failed CI) and v0.11.0: clean on all three scripts. The exact curl|tar extraction and lint invocation from the workflow were run locally first. This is the same class of problem the branch already documents for node (local v26 vs CI's pinned v20) — a green local run is not a green CI run unless the toolchain is pinned. Now it is, for this lane. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a verification harness any contributor or coding agent can use, plus thin Claude Code wiring on top. No product code changes.
Structure
The substance is tool-neutral. Nothing in it assumes a particular agent, editor, or model.
scripts/verify.shAGENTS.md.claude/verify.sh.Per-path routing mirrors the
paths:filters in.github/workflows/:crates/**cargo fmt --allweb/**npm run typecheckfrontend/**node frontend/_verify.js--alladds the three-lane test matrix, both clippy lanes,check-apply, the production web build, andmkdocs --strict.Why not just leave it in
.claude/That was the first draft, and it was wrong. The verification commands and the DST review rules were in a format only one agent reads — anyone on Codex, MiniMax, or no agent had no way to know what to run or which architectural rules matter. The knowledge is portable; the format wasn't.
.claude/now delegates rather than duplicates.hooks/on-edit.shis a payload adapter (parse JSON, callverify.sh, map exit 1→2) with no check knowledge of its own./cicallsverify.sh --alland is explicitly instructed not to expand the command list inline, since that's how it drifts from CI.If you add a check, add it to
scripts/verify.shand.github/workflows/together.Deliberate asymmetries
Documented in
AGENTS.md§1 so nobody "fixes" them:cargo fmtrewrites rather than--checks in per-path mode, and never fails. Formatting isn't worth interrupting an edit over. CI's--checklane is the gate.--all. The test matrix and clippy are too slow for every edit.git/ghallow rules are enumerated read-only verbs, notgit */gh *— the wildcard would silently allowpushandreset --hard.python3, notjq, to parse the hook payload —jqisn't on all dev machines here.The part worth reviewing carefully
AGENTS.md§2.1 states the DST layering rule:Grepping ADR-001's discipline rules against the tree surfaces 9 hits for
SystemTime::now/Instant::now/thread_rng— all 9 legitimate (§2.3 tabulates each). An agent that flags them is noise within a day.An earlier commit in this branch had this rule inverted, telling reviewers that a real
SystemTime::now()incore/src/clock.rswas expected rather than a violation — a blind spot for exactly the regression the rule exists to catch.crates/vdg/src/realclock.rssettles it in its own doc comment: "Lives in the shell (not core) so the core stays free of any real time source."Second opinion welcome on where that line is drawn.
Also flagged never-fix:
serve.rs:1132gen_secret()mints a 256-bit auth token and must use OS entropy. Routing it through the seededRngwould make auth tokens reproducible from a seed.Verification
cargo check --workspace --features vdg/servecargo check --workspace --features vdg/datafusioncargo check -p vdg --features applycargo clippy --workspace --all-targets -- -D warningsverify.shfrom an unrelated cwdverify.shevery per-path branch, passingverify.shweb + frontend, deliberately brokennode_modules/dist/ unrelated pathsmkdocsbash -non both scriptsFailure paths were proven with a real TS2322 type error and a thrown exception, then reverted.
Notes for reviewers
crates/**,web/**,frontend/**,docs/**; this diff touches none of them. So the harness ships without CI signal — the table above is from local runs. Worth knowing this PR can't be gated by the gates it configures..claude/didn't exist when these sessions started, so the settings watcher won't load it until you open/hooksonce or restart. Verified directly — an edit made after writing the hook did not trigger it. The config is correct; it just needs one reload..claude/entirely and callscripts/verify.sh.🤖 Generated with Claude Code