Code review tracking¶
This documents the conventions for tracking whole-codebase human
review: periodically reading a repository file by file to catch the
inconsistencies in style, architecture, and approach that creep in
over years of incremental change. It is complementary to both PR
review (which only ever examines deltas) and the consistency audits
(which catch mechanical drift like missing files -- see docs/audits/).
The full design rationale is in
docs/plans/PLAN-code-review-tracking.md.
The automation lives in this repository:
scripts/review-tracking.py (subcommands stamp, prune, regen,
next, status, scope-orphans, and import), with tests in
scripts/tests/test_review_tracking.py. In a developer's clone it is
run by hand -- deliberately not from git hooks. An earlier iteration
wired stamp and prune into the pre-commit, post-merge,
post-checkout, and post-rewrite hooks, but review state silently
changing in the middle of unrelated git operations proved more
confusing than helpful. That objection does not apply to CI running
against a repository's own default branch, so in steady state four
subcommands also run automatically: prune then import, in that
order, from an adopting repo's prune-reviews workflow -- copied
from templates/review-tracking/, see "Adopting a repository"
below -- on every push to the default branch and once a day, and
status and scope-orphans from the consistency audit's
review-coverage and review-scope-completeness checks (see
"Steady state" below).
Target repositories carry a thin wrapper (for example ryll's
tools/review-tracking.sh) that locates a local clone of this
repository and passes through to the script.
The pieces¶
- weAudit (
trailofbits.weauditon the VSCode marketplace) provides the in-editor workflow: mark files or regions as reviewed, attach notes and findings, see progress. Its state lives in.vscode/<username>.weaudit, one JSON file per reviewer, committed to the repository being reviewed. - Signed git commits of that state file provide attestation. The signature covers the commit tree, which contains both the review mark and the exact content of the reviewed file -- so "who reviewed what, when, at which content version" needs nothing beyond git.
- A sidecar file (
.vscode/<username>.weaudit-shas.json, written by thestampsubcommand, never by weAudit) records the blob SHA and date of each review so staleness is a mechanical check, and a generatedREVIEWS.mdsurfaces the review state to people who do not know to look in.vscode/. - An imports file (
.vscode/imports.weaudit-shas.json, written by theimportsubcommand, never by weAudit or a human) records full-file reviews carried in from this repository's own review history rather than made in this clone -- see "Importing reviews from this repository" under "Steady state". - A scope config (
.vscode/review-scope.toml) defines which files count as reviewable:includeandexcludelists of fnmatch patterns matched against repo-relative paths (*matches across directory separators; an empty or absentincludemeans all tracked files). Anexcludeentry beginning with!is a re-include, so a directory can be excluded except for one file without naming every other file by hand; it cannot put the tracking machinery (.vscode/*,REVIEWS.md) back, which is always excluded, since those files describe the reviews and can never attest to themselves. Scope should cover the executable artifacts (source code in every language the repo uses, plus shell scripts), the declarative configuration that is executable in practice (CI workflows, container and deployment manifests, Ansible playbooks, tool and packaging config -- the YAML that decides what runs, with which permissions, against which secrets, and which is where a supply-chain change is most likely to hide), and the prose that documents them (READMEs,ARCHITECTURE/AGENTS, anddocs/guides): prose drifts out of sync just as quietly as code and benefits from the same periodic re-reading. Generated and vendored code should be excluded, as should ephemeral working documents like adocs/plans/archive -- point-in-time records of intended work, not living artifacts, and numerous enough to swamp the queue. Whether unit tests are in scope is a per-repo decision. An optionalimport-excludelist names files that must never be marked reviewed by import (see "Importing reviews from this repository" under "Steady state").
Adopting a repository¶
- Ensure the review state files are committable. If
.gitignoreexcludes.vscode/, add exceptions:
The ignore rule must then be .vscode/* rather than .vscode/:
git cannot re-include a file whose parent directory is excluded,
so exceptions under a bare .vscode/ do nothing. The
prune-reviews workflow fails the run if the imports file it
writes is ignored, rather than importing the same reviews every
day and committing none of them.
Exceptions rather than un-ignoring .vscode/ wholesale, because
weAudit also writes .vscode/.weauditdaylog, a log of which files
each session opened. Nothing reads it, it is not attestation, and
it churns every session -- so if the repository does not ignore
.vscode/ it needs the opposite entry, or the day log rides along
in every review commit and triggers the expensive CI lane that
step 8's paths-ignore block exists to skip:
Then, if the repository runs a pre-commit hook that rewrites the
files it is given -- end-of-file-fixer, trailing-whitespace
and friends -- exempt the review marks from those hooks:
- id: end-of-file-fixer
exclude: ^\.vscode/.*\.weaudit
- id: trailing-whitespace
exclude: ^\.vscode/.*\.weaudit
Without it, end-of-file-fixer rewrites the weAudit file on every
pre-commit run --all-files, because the generator emits no
trailing newline -- and reports a failure that cannot usefully be
fixed, since committing the newline only means the next regen drops
it again. The pattern deliberately covers the .weaudit-shas.json
sidecar as well as the weAudit file itself.
Scope the exclude to those hooks rather than putting it at the top
level of .pre-commit-config.yaml. A top-level exclude is shorter,
but it hides the review marks from every hook, including content
scanners -- and review notes are prose, so a blanket exclude stops
gitleaks and the bidi/zero-width check from reading exactly the
kind of human-written text a secret or a smuggled character would
land in. This is the same reasoning that keeps content scanners out
of the paths-ignore block in step 8; it applies just as much to a
local hook as to a CI workflow. A repository that runs no rewriting
hook at all (ryll, today) needs no exclude and should not add one.
The consistency audit's review-marks-pre-commit check enforces
this, and reports not applicable where no rewriting hook is
configured.
-
Ensure commit signing is configured for the clone(s) reviews will be made from (see below).
-
Protect the branch that will carry review state (normally the default branch) from history rewrites: a repository ruleset with the
non_fast_forwardanddeletionrules is the minimum. For example:
gh api -X POST repos/shakenfist/<repo>/rulesets --input - <<'EOF'
{
"name": "Protect default branch history",
"target": "branch",
"enforcement": "active",
"conditions": {"ref_name": {"include": ["~DEFAULT_BRANCH"],
"exclude": []}},
"rules": [{"type": "deletion"}, {"type": "non_fast_forward"}]
}
EOF
(~DEFAULT_BRANCH tracks whatever the default branch is, so the
same command works on every repo.)
(shakenfist/shakenfist's "Develop branch" ruleset is a stricter superset -- PRs, merge queue, status checks -- and also satisfies this.)
- Write
.vscode/review-scope.toml. Cover code, YAML configuration, and docs across the languages the repo uses, and exclude generated, vendored, and ephemeral files. For example (ryll's config; adjust the language patterns to the repo):
# Source in every language the repo uses, plus shell scripts,
# protobuf definitions, the YAML that configures CI and
# deployment, and the Markdown documentation.
include = ['*.rs', '*.sh', '*.py', '*.pyi', '*.js', '*.proto', '*.md', '*.yml', '*.yaml']
# docs/plans/ is a point-in-time archive, not living artifacts;
# protoc output is generated, so reviewing it would attest to the
# generator rather than to anything a human wrote.
exclude = [
'docs/plans/*',
'*_pb2.py',
'*_pb2.pyi',
'*_pb2_grpc.py',
'*_pb2_grpc.pyi',
]
An empty include means all tracked files, which is a fine
starting point for a small repo -- but then lean on exclude to
keep generated code (*_pb2.py and friends), vendored trees
(vendor/*, minified third-party JavaScript), and ephemeral
archives out of the queue.
Whichever you choose, every tracked file has to end up either in
scope or named by an exclude entry: the
review-scope-completeness audit fails on a file that is out of
scope only because include does not name it. Enumerating
extensions and leaving include empty are both compliant, and
they differ in which way a new file type fails. With an empty
include it silently joins the review queue and somebody excludes
it if that was wrong; with a list it fails the audit and somebody
decides. Run review-tracking.py scope-orphans after writing the
config to see which files it leaves out.
Prefix an exclude entry with ! to re-include something the
pattern above it takes away:
Reach for this when a directory is excluded because its files are machine-rewritten and one file in it is not. Prefer it to naming the keepers individually, which is a list that has to be edited on the day a file changes category and therefore will not be. Do not reach for it to carve out several files from a directory that is excluded for a reason those files will grow into -- a spec awaiting its check has no generated table today and will have one the day the check lands, so re-including it buys a review that the next audit run invalidates.
Widening scope in a repo that has already been adopted needs no
migration -- the newly in-scope files simply have no review mark,
so they enter the next queue like any other unreviewed file.
Expect the review-coverage audit to open an issue until the
backlog has been worked off, since a repo's worth of workflow
files arrives at once.
- Copy
tools/review-tracking.shbyte-for-byte fromtemplates/review-tracking/review-tracking.sh-- verify withgit hash-objectagainst the template rather than trusting the copy. It is the thin wrapper: it locates a local clone of this repository --$SHAKENFIST_DEVELOPMENT, then a sibling../development, then~/src/shakenfist/development-- and passes its arguments through toscripts/review-tracking.py. Changes to the wrapper go to the template, not to a repository's copy of it.
This repository is the exception: its own wrapper does not go
looking, because scripts/review-tracking.py is in the tree
beside it. The search order above would find a sibling clone
and run that one's copy of the script, which is the wrong answer
in the one repository where the right answer is certain.
-
Bootstrap existing review marks, if any were made before the tooling was adopted. A stale pre-existing mark must not be blessed:
stamprecords whatever content the file has now, whether or not that is what was reviewed. So: -
For each pre-existing mark, check the file is unchanged since its signed review commit; unmark any that changed (they are stale, and will come back around via
next). -
Run the stamp by hand, and correct the recorded dates to the true review dates (a fresh stamp records today):
-
Delete any hand-maintained REVIEWS.md; the generated file replaces it.
-
Commit the wrapper, corrected weAudit state, sidecar, and REVIEWS.md together (signed).
-
Copy
.github/workflows/prune-reviews.ymlandtools/ci-prune-reviews.shbyte-for-byte fromtemplates/review-tracking/-- again verify withgit hash-object. This is the steady-state prune-and-import automation (see "Steady state" below): it runsprunethenimporton every push to the default branch, once a day, and on manual dispatch, and lands the result as one commit. See templates/review-tracking/README.md for what else the workflow needs (the.gitignoreexception,.vscode/review-scope.toml, and thepaths-ignoreblock from step 8) and for gitsign and the development clone it makes. A repository whose default-branch ruleset requires a pull request needs aDEPENDENCIES_TOKENsecret, since GitHub will not accept the built-in Actions app as a bypass actor and shakenfist-bot has to push with a token instead; a repository without that requirement pushes withGITHUB_TOKEN. Changes to the workflow or script go to the template, not to a repository's copy of them. The consistency audit'sreview-coveragecheck needs no per-repo setup -- it notices the scope config and starts checking the repo automatically. -
Teach the repository's build and analysis workflows to ignore review-only changes, so a review session (or a bot prune) does not burn a CI run on files no build reads. Apply it to the code-shaped workflows (unit tests, lint, CodeQL, functional lanes) but not to content scanners like gitleaks or the bidi/zero-width check: review notes are prose, and prose is a place secrets or Unicode smuggling could land.
For a workflow whose checks are not required, a trigger-level
paths-ignore is the simple answer:
on:
pull_request:
branches: [develop]
paths-ignore:
- 'REVIEWS.md'
- '.vscode/*.weaudit'
- '.vscode/*.weaudit-shas.json'
- '.vscode/review-scope.toml'
That form is unsafe once a workflow backs a required status
check, and the two failure modes look similar but are not the
same thing. A paths-ignore'd workflow whose trigger conditions
are not met never starts at all, so it never reports any of its
jobs -- a required check that was waiting on one of those jobs
simply never arrives, and GitHub blocks the merge forever waiting
for it. A job skipped by an if: condition is different: the
workflow still runs, the job still reports a result (skipped),
and a required check satisfied by a "skipped" report unblocks the
merge normally. So a workflow that provides a required check
needs the skip pushed down to the job level instead of left on
the trigger.
The job-level form uses a check_paths job running
dorny/paths-filter to compute whether anything outside the
review paths changed, and gates the real job on that output with
needs: and if:. The fleet's CodeQL template,
templates/codeql/codeql-analysis.yml,
is the worked example, and hunkydory runs it unchanged:
Analyze is a required check on hunkydory's default branch, so
pull requests skip it at the job level rather than with a
trigger-level filter, while push carries no required check and
keeps paths-ignore directly. Besides the review paths it skips
docs/**, and it does not analyse fork pull requests at all
(trimmed; the template's comments say why each condition is
there):
on:
push:
branches: [main, develop]
paths-ignore:
- 'REVIEWS.md'
- '.vscode/*.weaudit'
- '.vscode/*.weaudit-shas.json'
- '.vscode/review-scope.toml'
- 'docs/**'
pull_request:
branches: [main, develop]
# ... and a weekly schedule
jobs:
check_paths:
# ... name, runner and timeout
permissions:
pull-requests: read
outputs:
code_changed: ${{ steps.filter.outputs.code || 'true' }}
steps:
- uses: dorny/paths-filter@v4
id: filter
if: github.event_name == 'pull_request'
with:
predicate-quantifier: 'every'
filters: |
code:
- '**'
- '!REVIEWS.md'
- '!.vscode/*.weaudit'
- '!.vscode/*.weaudit-shas.json'
- '!.vscode/review-scope.toml'
- '!docs/**'
analyze:
name: Analyze
needs: [check_paths]
if: |
!cancelled()
&& needs.check_paths.outputs.code_changed != 'false'
&& (github.event_name != 'pull_request'
|| github.event.pull_request.head.repo.full_name == github.repository)
# ... runner, concurrency and permissions
steps:
- uses: actions/checkout@v7
# ... the rest of the analysis
A few details in that shape are load-bearing:
check_pathshas no checkout step.dorny/paths-filterreads the pull request's changed-file list from the GitHub API rather than from a working tree, so a fork's content never reaches the runner -- and the job needs onlypull-requests: read, notcontents: read.- The filter step runs only
if: github.event_name == 'pull_request'. Onpushorscheduleit is skipped, and its output falls back through|| 'true'to "code changed", so those events always run the gated job. predicate-quantifier: 'every'is what makes the!exclusions work at all:dorny/paths-filter's default is ANY-match, under which the bare'**'entry would already match and every!exclusion would be a no-op.- The output fails open (
${{ steps.filter.outputs.code || 'true' }}): if the filter step never ran or produced nothing, the gated job still runs rather than being silently skipped. - The gated job's condition needs
!cancelled(), not just the path check. Without it, ifcheck_pathsitself failed or was cancelled, GitHub Actions' default propagation would skipanalyzetoo -- and a required check reporting "skipped" because its own dependency failed still reads as green to the merge gate.!cancelled()forcesanalyze's ownif:to be evaluated instead of inheriting that failure as a silent pass. - The fork guard is specific to CodeQL's autobuild, which runs the
pull request's own build on the shared static runners. A fork's
pull request skips
analyze, which satisfies the required check, so its change is scanned by the push after it merges rather than before. A workflow whose job runs nothing from the pull request does not need the clause.
Which mechanism a given workflow needs is a property of whether
it backs a required status check, not of what kind of workflow it
is, so verifying this step is judgment work rather than a
deterministic audit: the review-tracking-adoption Claude skill
(in this repository's .claude/skills/) carries the verification
procedure. Re-run it when new workflows land in an adopted repo.
docs/audits/expensive-lane-path-filter.md checks the same
required-checks-need-the-job-level-form rule for a different set
of expensive lanes (VM-runner jobs, with kerbside's check_paths
jobs as its worked example), and endorses the same
dorny/paths-filter shape used above.
The review account¶
Reviews are performed from a dedicated user account on the review machine, with its own clones of the repositories under review. This is deliberate isolation: the review clones never contain in-flight development changes, so the clean-tree rule below holds structurally rather than by discipline, and there is no risk of attesting to code mid-edit. Development happens in the primary account's clones; review marks are only ever made and committed from the review account.
Commit signing¶
Review-state commits must be signed; the simplest way to guarantee
that is to sign all commits made from the review account. The
convention is gitsign (Sigstore keyless signing, matching the
Sigstore use in our release automation): the signing certificate
is issued for mikal@stillhq.com via GitHub OAuth at commit time,
and every signature is recorded in the Rekor transparency log --
which gives each review attestation an independent public
timestamp as a side effect.
Setup in the review account:
git config --global commit.gpgsign true
git config --global gpg.format x509
git config --global gpg.x509.program gitsign
Verification, from any account with gitsign installed:
gitsign verify \
--certificate-identity=mikal@stillhq.com \
--certificate-oidc-issuer=https://github.com/login/oauth \
<sha>
git log --show-signature -- .vscode/ # with the config above
import performs the same check, and needs to know which identity
each reviewer signs as: REVIEWER_IDENTITIES in
scripts/review-tracking.py maps a reviewer name (the <username>
of .vscode/<username>.weaudit) to their certificate identity.
When a new reviewer starts signing review-state commits here, add
an entry for them; until then import cannot verify their reviews
and skips each one with a warning naming them.
Note that GitHub's web UI shows gitsign commits as "Unverified"
(reason bad_cert): GitHub cannot validate Fulcio's short-lived
certificates. This is expected -- the trust path is gitsign
verification against Fulcio and Rekor, not GitHub's badge. An
example review attestation: shakenfist/ryll commit 755a3cc
("review: glz.rs").
Session discipline¶
The signed-commit attestation is only as good as the tree it covers, which imposes three rules:
- Mark reviews only on a clean working tree. If a file has uncommitted edits when it is marked reviewed, the committed tree will not match what was actually read. The dedicated review account makes this structural: its clones never carry development edits.
- Commit review state at the end of every session, before pulling or rebasing, so the marking commit's tree reflects the reviewed state:
- Never rewrite history on the branch carrying review state (enforced by the ruleset above).
A session therefore looks like:
git pullon a clean tree, then:
discards marks for files changed since their review and
regenerates REVIEWS.md. Anything it pruned is a good
candidate work queue for the session. If VSCode was already
open, reload the window (or toggle the weAudit tree view) so
the ticks refresh -- weAudit does not watch its state file for
external changes. Note the pull is load-bearing, not just
hygiene: the prune-reviews workflow commits prunes to the
default branch between sessions, and marking reviews on a
clone that has not picked those up risks a review-state
commit that conflicts on push. The local prune usually finds
nothing left to do.
2. Pick a file:
picks a random unreviewed in-scope file and opens it in VSCode
(--no-open to just print it). A file imported from this
repository's own review history (see "Importing reviews from
this repository" under "Steady state") shows no tick in weAudit
and is not offered here; reviewing it by hand supersedes the
import.
Name one or more paths to pick only from those subtrees --
next src/agent for the next unread file there. Relative paths
are repo-relative, not relative to the shell's directory,
because the wrapper runs the script from the repo root; an
absolute path is converted, so next $PWD means the subtree
you are standing in. A subtree with nothing in scope (a typo,
or a directory the scope config leaves out) exits non-zero
rather than reporting it all reviewed.
next picks at random by default, which suits an audit: it
spreads attention over the backlog instead of always starting
at its front. A scope config with order = 'listed' makes it
offer the first unread file in include order instead, for
reading a codebase to learn it, where the order is the point.
Each file takes the position of the first pattern that names
it, and files sharing a pattern go in path order -- so name a
file ahead of the glob that would otherwise cover it to read it
sooner than its neighbours. No adopted repository sets it.
3. Read it. weAudit's explorer ticks show what is already done;
Claude Code in the integrated terminal for questions.
4. Mark it reviewed (weAudit: Mark File as Reviewed), attach
findings or notes as needed.
5. Repeat from 2. At the end of the session:
The stamp records each newly reviewed file's blob SHA and date
in the sidecar and regenerates REVIEWS.md, printing exactly
what to git add. The (signed) commit that lands contains the
marks, the stamps, and the regenerated REVIEWS.md together.
Reviewing a file that is out of scope¶
stamp ends with a banner naming any marked file the scope config
excludes, and exits non-zero. It is deliberately the loudest thing
the tool prints, because the mistake is otherwise close to
undetectable from the outside: an out-of-scope review does get a
row in the REVIEWS.md table, so it looks recorded, but the
coverage count above that table only counts in-scope files and does
not move. status cannot see it either, so the review-coverage
audit goes on reporting the file as outstanding, and next never
offered it in the first place. The reviewer reads a file carefully
and the number they are trying to move stays where it was.
The banner repeats on every run, not just the one that first stamps the file. A mark noticed once and left alone is precisely the case that needs saying again, and the second run is when the reviewer is looking for confirmation that the count moved.
Two ways out, and which one is right is a judgement call:
- If reviewing the file was a mistake, un-mark it in weAudit and
re-run
stamp, which drops the stamp along with the mark. - If the file should have been in scope, widen the include or
exclude patterns in
.vscode/review-scope.tomland say why in the commit message. The exclusions in that file are argued rather than incidental -- each one carries a comment explaining what would go wrong if the file were in the queue -- so an addition that does not engage with the argument is likely to be reverted by whoever wrote it.
Staleness¶
A review applies to the file content that was read, not the path: once the file changes, that review is stale and the file should be treated as unreviewed. weAudit does not track this -- a stale tick looks identical to a fresh one.
The tooling makes staleness a mechanical check: stamp records
each reviewed file's blob SHA in the sidecar, and prune discards
any mark -- whole-file or region -- whose stamped SHA no longer
matches HEAD, regenerating REVIEWS.md to match. Region marks
are pruned wholesale with the file: line ranges shift as files
change, so a partial review of a changed file is not trusted
either. On the default branch the prune-reviews workflow does
this automatically after every push (see "Steady state" below); in
clones, pruning remains part of the session discipline: run it
after every pull (and after any merge or rebase in a clone
carrying review state). See the plan for the full design,
including why the stamps live in a sidecar rather than in
weAudit's own JSON.
Four behaviours worth knowing about:
- Prune compares against whatever
HEADcurrently is -- run it with an old branch checked out and it will (correctly, but perhaps surprisingly) discard reviews of files that differ there. If that was not what you meant,git restore .vscode/ REVIEWS.mdputs the state back. - A stamped entry is never re-stamped while it exists: if a
reviewed file changes, the only path forward is prune then
re-review. This is what prevents a stale review being silently
refreshed at the file's current content.
stampreports such a file and exits non-zero rather than passing over it, which is the last chance anything gets to say so:stampruns by hand and not from a hook, so nothing downstream will stop the commit. Until this was checked it skipped the file in silence, and because a review-only commit is exempt from CI, the mark then survived to the default branch whereprune-reviewsdeleted it -- discarding the review rather than the staleness, which is the wrong half. - A stamp going stale is not a CI failure. Editing a reviewed
file is the ordinary case rather than a mistake, and the
change that does it is often not a change that can
re-review anything -- a Renovate action bump touching
stamped workflow files least of all. Staleness is handled
after the merge instead of gated before it:
prune-reviewsdiscards the mark on the next push to the default branch, and thereview-coverageaudit recomputes coverage againstHEADand raises an issue once the backlog reaches five files. This repository briefly asserted the opposite at commit time, and every dependency bump touching a stamped workflow failed CI until somebody re-stamped by hand. - When every file in a directory is reviewed, weAudit adds a
derived directory entry to
auditedFilesalongside the per-file entries. The tooling treats these as pure UI state: they are never stamped or listed inREVIEWS.md, and prune removes them when a file inside stops being reviewed (mirroring what weAudit itself does when a file is unmarked in its UI).
Steady state¶
Once a repository approaches full coverage, the interesting questions change: reviews go stale as PRs merge, and someone needs to notice when enough staleness has accumulated to be worth a session. Two pieces of automation cover this.
One consequence of REVIEWS.md being generated is worth stating
before either of them: the header count is a property of the whole
tree, so adding or removing any in-scope file changes it -- on
whatever branch happens to do so. That is why a pull request does
not regenerate it. A generated file that every branch has to
rewrite is a merge-conflict hot spot, and the conflicts are
semantic rather than textual: two branches each adding an in-scope
file regenerate to the same header text, merge cleanly, and leave
a count that is wrong by one. Resolving that costs a rebase and a
full CI run for a change that is entirely prose, which on a small
CI cluster is a real tax on forward progress.
Nothing is needed to make this safe. prune regenerates
REVIEWS.md whether or not it pruned anything, so the
prune-reviews workflow corrects a moved count on the next push
to the default branch, in the same commit it would have made
anyway. The review-coverage audit does not read the count at
all: review-tracking.py status recomputes coverage against
HEAD, precisely so that a missed regeneration cannot inflate it.
This repository does assert at commit time
(review-tracking-tests) that REVIEWS.md is reproducible from
the committed state, because a file that is not is how a missing
stamp sidecar hides -- the count trusts marks rather than stamps,
so a commit that lands the marks and forgets the sidecar reports
the right number while every Date and Blob SHA cell renders as
-. That assertion compares every row but skips the count line,
so it catches the sidecar and hand-edited rows without making the
file a hot spot. An adopting repository that copies that hook
inherits the check; one that does not, does not.
Automatic pruning and importing. Each adopting repository carries
a prune-reviews workflow, copied byte-for-byte from
templates/review-tracking/ (step 7 of "Adopting a repository"
above), that runs on every push to its default branch, once a day, and
on manual dispatch -- the push is the only event that can create
staleness there, and the daily and dispatch runs are what pick up new
reviews imported from this repository even when the target repository
itself is quiet. It clones this repository for the script, runs
prune then import, and commits the updated review state and
regenerated REVIEWS.md whenever either changed -- including a
regeneration that pruned nothing, which is what corrects a moved
header count -- directly back to that branch as shakenfist-bot, in one
commit, Prune and import review marks.. It lands that commit by
regenerating rather than rebasing: each of up to three attempts
fetches the branch, resets to its tip, runs prune and import
there, and pushes. Everything in the commit is derived from the tree
beneath it, so a merge that lands mid-run costs a retry, never a
conflict, and the retry also prunes the marks that merge made stale.
If import fails, the run discards whatever it wrote, lands prune's
result alone, and only then fails: prune is what stops a changed
file reading as reviewed, so an import bug must not hold it up, but it
must not pass unnoticed either. A concurrency group serialises the
workflow's own runs, and the loop terminates either way: a repository
that can push with the default GITHUB_TOKEN never retriggers the
workflow at all, while one whose ruleset forces a PAT instead -- as
ryll's does, since develop requires a pull request before merging and
GitHub will not accept the built-in Actions app as a bypass actor, so
shakenfist-bot pushes with DEPENDENCIES_TOKEN -- retriggers exactly
once, into a run that finds nothing left to do (observed 2026-09-11
04:57:20 UTC, before ryll carried the merged workflow, Prune stale
review marks., which committed nothing).
The job runs on shared, long-lived static runners that also run pull
request code, so it trusts as little as it can of what an earlier job
could have left there. It never reads the reused workspace: the script
comes from a fresh clone in RUNNER_TEMP, and lands from another; it
ignores global and system git configuration, gives gitsign a fresh TUF
cache on every run, keeps no credentials in either clone, and hands
the push token to its one git push alone, through the environment
rather than argv or .git/config. "Runner isolation" in
templates/review-tracking/README.md
has the detail.
The bot's commit is not signed, and does not need to be for the
prune half of it: prune can only remove marks, never add or
refresh them ('a stamped entry is never re-stamped while it
exists', above, is enforced by stamp, which the automation never
runs). The attestations live in the signed review-state commits
already in history, and verifying a native mark means verifying the
signed commit that introduced its stamp -- an unsigned later
commit that deletes marks weakens nothing. Removing a mark is
always safe; it merely queues the file for re-review.
import also runs unsigned from the bot, in the same commit as
prune, and it adds marks, so it needs its own argument rather
than inheriting prune's. An imported
entry is not itself an attestation: it is a pointer to one --
the repository, path, and introducing commit of a signed
review-state commit already made in this repository (see
"Importing reviews from this repository" below). Trusting the entry
means verifying that commit, which import does at the moment it
writes the entry, not later and not by anyone downstream. This is a
genuinely new trust surface -- for the first time, an unsigned bot
commit can cause a file to read as reviewed -- and verification is
what bounds it: the bot can only relay an attestation a human has
already signed, and an entry it cannot verify is skipped and
reported rather than written.
That bound has a limit worth stating plainly. The code that does the
verifying, and REVIEWER_IDENTITIES, the map saying whose signature
counts for which reviewer, both come from this repository's main
branch, the clone every adopted repository's workflow makes. gitsign
therefore protects against forged review data: a stamp or a mark
nobody signed. It does not protect against a change to the code or
to the identity map, which could, say, add an identity, weaken the
check, or skip it, and every adopted repository would run the
changed version on its next import. A pull request that changes
scripts/review-tracking.py's import path or REVIEWER_IDENTITIES
therefore deserves the same scrutiny as a review attestation, since
in effect it decides what counts as one.
Importing reviews from this repository. Much of what an adopted
repository has to review is not its own code: workflow templates,
helper scripts, and exported GitHub configuration are written and
reviewed here, then copied out byte-for-byte by
standards-alignment rollouts and reviewed again, file by file, in
every repository that received them. A review mark attests to a
blob SHA, not a path, and two files with the same blob SHA have the
same bytes -- so a review of a blob here is equally a review of an
identical blob anywhere else. review-tracking.py import, run in an
adopted repository, marks as reviewed every in-scope file whose
HEAD blob matches one this repository has fully reviewed.
Only full-file marks count, and only from this repository's own
sidecar history. stamp also stamps files carrying partial (region)
marks, so import walks every .vscode/*.weaudit-shas.json in this
repository's history, oldest commit first, and credits a
(reviewer, blob) pair only where the same commit's
.vscode/<reviewer>.weaudit has a full-file auditedFiles entry for
that path. Where a blob was reviewed more than once, the earliest
qualifying commit wins, since that is the attestation to point at.
The history walked is that of origin/main in the clone import
runs from, not whatever branch the clone has checked out: a review
committed on a branch that has not merged is not yet an attestation,
and may never become one. A clone with no origin/main falls back to
its HEAD, with a warning saying so.
Any path is eligible, not only templates/: identical bytes need no
context the file does not carry, and a whole-file review never
attested to context either -- a native mark does not go stale when a
script the file calls changes. History matters because a target's
copy often lags a template this repository has since changed and
re-reviewed: the copy's blob then matches an older stamp, which only
the sidecar's history still records, not HEAD.
The imported marks live only in .vscode/imports.weaudit-shas.json,
a sidecar-shaped file with no .weaudit file beside it, and
import never writes a .weaudit file or a human reviewer's own
sidecar. This is deliberately narrower than it could be: weAudit
loads every .vscode/*.weaudit file and decorates a file as audited
from its path alone, whatever file or author the entry came from
(isAudited() in src/codeMarker.ts of trailofbits/vscode-weaudit), so a
.weaudit imports file would show ticks in VSCode for files nobody
read in this clone. Worse, weAudit writes an auditedFiles change
back to <author>.weaudit (toggleAudited() and updateSavedData()), so an
imported entry authored by the original reviewer would, once
un-ticked, be written into that reviewer's own state file in this
repository, mixing imported reviews into a human's record. weAudit only globs *.weaudit, so it never reads the
sidecar-shaped imports file, and the name is already covered by the
.gitignore exception and the paths-ignore entry
(.vscode/*.weaudit-shas.json) that step 8 of "Adopting a
repository" puts in every adopted repository, so an import commit
needs no new exception and triggers none of the expensive CI lanes.
Each entry records where the attestation lives, not the attestation itself:
{
"version": 1,
"files": {
"<target path>": {
"sha": "<blob sha>",
"date": "<source stamp date>",
"imported": {
"repo": "shakenfist/development",
"reviewer": "<reviewer>",
"path": "<source path>",
"commit": "<introducing commit>",
"verified": true
}
}
}
}
date is the source stamp's date, not the day of the import, because
it records when the content was actually read. verified records
whether the introducing commit's signature was checked -- see the
attestation argument above for why import checks it before trusting
the entry, rather than leaving verification to a later reader. A
commit that fails verification is skipped with a warning naming it, as
is a review by a reviewer with no entry in REVIEWER_IDENTITIES (see
"Commit signing"). A verification that times out is skipped the same
way, and after three timeouts in one run (VERIFY_TIMEOUT_BUDGET)
import stops verifying altogether: it skips every remaining
candidate with a single warning giving the count, so that a Sigstore
outage costs a run minutes rather than running it into its job
timeout, and a later run picks them up. status does not verify an
imported entry's provenance, any more than it verifies a native mark's
signature: it only checks that the blob SHA still matches HEAD, the
same as it does for a native mark.
--no-verify is the only way to import without verification. It
disables the check outright, every run under it says so on stderr,
and what it imports is recorded with verified: false and shown in
REVIEWS.md with an (unverified) suffix on its Source cell,
rather than passed off as checked. A run without gitsign on PATH
does not fall back to that: it imports nothing, leaves the entries
already recorded alone, ends with a warning banner on stderr saying
so and pointing at --no-verify, and still exits zero, so that a
CI job on a runner without gitsign neither fails nor marks anything
reviewed. It still removes entries, which needs no verification.
Verifying an imported entry by hand. Checking one imported
mark without running review-tracking.py takes four steps. Run
git fetch in both clones first: a stale clone of this repository
may not have the commit yet, and steps 2 and 4 compare against
each repository's origin/<default branch>.
- Read the entry from the target repository's
.vscode/imports.weaudit-shas.jsonon its default branch. You need the targetpathandsha, plusimported.reviewer,imported.path(the source path here) andimported.commit. - In a clone of this repository, verify the commit's signature:
gitsign verify \
--certificate-identity=<identity> \
--certificate-oidc-issuer=https://github.com/login/oauth \
<commit>
<identity> comes from REVIEWER_IDENTITIES in
scripts/review-tracking.py, which maps a reviewer name to a
signing identity; that map is the authority, and the example
here (mikal signs as mikal@stillhq.com) may not list every
reviewer. Run this in a full clone, not a linked worktree:
gitsign reads the repository through go-git, which cannot
follow a worktree's .git file, and reports every commit as
"reference not found". See "Commit signing" for the trust path
this checks.
Then confirm the commit is on this repository's default branch:
A signed commit that was never merged, or was rebased away, is
not an attestation: nobody accepted it into the record, and the
review it carries may have been rewritten since. import walks
only origin/main for this reason, so an entry whose commit is
not on it did not come from import.
3. Confirm the commit records a full-file review of that blob:
git show <commit>:.vscode/<reviewer>.weaudit-shas.json \
| jq -c --arg p '<source path>' '.files[$p]'
git show <commit>:.vscode/<reviewer>.weaudit \
| jq -c --arg p '<source path>' \
'{full: [.auditedFiles[]? | select(.path == $p)],
partial: [.partiallyAuditedFiles[]? | select(.path == $p)]}'
The first must print the same sha and date as the imported
entry. The second must show one full entry and an empty
partial list: a full-file mark is a plain
{"path": ..., "author": ...} entry in the top-level
auditedFiles array, and region marks in
partiallyAuditedFiles never count for import. (Both files are
whole-repository state, hundreds of entries long; jq keeps the
output to the one path being checked.)
4. In the target repository, confirm the entry is still current:
must equal the entry's sha.
A file whose native mark has gone stale is not imported until
prune has removed the mark, which is why the workflow will run
prune first; until then the file's own review history says it
needs a human. An imported entry that has itself gone stale -- its
blob is no longer the file's content at HEAD -- is replaced if the
new content was reviewed here too and can be imported, and
otherwise removed by import itself, with the same message prune
gives, so that REVIEWS.md never shows it as reviewed in the
meantime. import regenerates REVIEWS.md when it changes
anything, as stamp and prune do.
Every run also checks the entries already in the imports file against
this repository's history, not only the ones it adds. An attestation
is introduced at a commit whose state holds the review and none of
whose parents' state does, so the same review can be introduced more
than once -- on a review branch merged after main made it too, or by
re-stamping a blob after a prune -- and an entry pointing at any of
those commits is supported. An entry whose sha, imported.reviewer,
imported.path and imported.commit are not one of them -- one
written by hand, or one pointing at a commit that is not on
origin/main or merely carries a review forward, such as a prune or
the merge of a review branch, or one naming a repository other than
shakenfist/development -- is replaced by a real import of the same
content if there is one, and otherwise removed, with a warning on
stderr either way. Entries imported under --no-verify are checked
the same way and kept when history supports them. A run that cannot
judge an entry keeps it and says so: one without gitsign, one that has
spent its timeout budget, and one walking a shallow clone or a clone
with no origin/main, whose history may simply lack the attestation.
An entry not shaped as import writes it -- not an object, or missing
a string sha or a string repo, reviewer, path or commit
under imported -- is ignored with a warning by every command that
reads the imports file, and dropped from it the next time import or
prune writes it. A file that is not valid JSON, or not a JSON
object, is treated as empty the same way.
A native review always supersedes an import: if a reviewer marks a
file that already carries an imported entry, import removes its
own entry on the next run, so REVIEWS.md shows one row per file and
the human's review is the one kept.
An import can be rejected the same way scope itself is narrowed.
.vscode/review-scope.toml gains an import-exclude list of
fnmatch patterns with the same semantics as exclude (!
re-includes), and, like the other exclusions in that file, an entry
should say why:
# Byte-identical to the template today, but this file is genuinely
# per-repository and a shared review would hide that it needs its
# own read.
import-exclude = ['config/local-overrides.yml']
import never adds an entry matching import-exclude, and removes
any existing imported entry that starts matching one.
REVIEWS.md's reviewed-files table gains a Source column: - for
a native review, development@<12-char commit> for an imported one
(with (unverified) appended if it was imported under --no-verify),
with the Reviewer cell taken from the entry's own provenance rather
than from a .weaudit file in this clone.
This repository does not import from itself. A template and this
repository's own instance of it could in principle cross-import, but
the case is rare enough that import run against this repository,
whether this clone or a worktree of it, is a no-op: it prints
review-import: nothing to do; shakenfist/development is the source
of imported reviews, writes nothing, and exits zero, so that the
shared prune-reviews workflow can run it here unchanged.
Backlog alerting. The weekly consistency audit runs a
review-coverage check (docs/audits/review-coverage.md) against
every repository in its matrix. Repositories without a
.vscode/review-scope.toml are reported as not applicable, so
adopting the tooling automatically opts a repository in. The check
runs review-tracking.py status, which recomputes coverage against
HEAD -- which marks are still valid, which files are stale or never
reviewed -- rather than trusting the committed REVIEWS.md, so
alerting stays honest even if the prune automation breaks. When 5 or
more in-scope files need review (REVIEW_BACKLOG_THRESHOLD in
scripts/audit/checks/review.py), the audit files a Consistency:
Human review coverage issue on the repository listing the files
needing review, and closes it once a session brings the backlog back
under the threshold. That list is a snapshot, not a queue that stays
current: it is written once, when the issue is filed, and
scripts/audit-manage-issues.py creates, dedupes and closes such
issues but never edits one that is already open, so a long-lived
issue understates a growing backlog and any link it carries to a
criterion spec rots if that spec moves (development#138).
Expect the issue to open and close routinely: a single feature PR
can touch five in-scope files, and the issue is a standing nudge
rather than an alarm.
Scope alerting. The same audit runs a
review-scope-completeness check
(docs/audits/review-scope-completeness.md), which measures the
scope config rather than the backlog. It fails when a tracked file
is out of scope only because no include pattern names it, as
opposed to because an exclude entry says it should not be
reviewed. The two checks fail in opposite directions and the gap
between them matters: narrowing include is the cheapest way to
make a review-coverage issue close, and without this check
nothing notices a repository that reaches full coverage by
shrinking what counts. It runs review-tracking.py scope-orphans,
and the issue it files lists the unnamed files.
The case it was written for was not adversarial.
templates/renovate/renovate.json in this repository is a template
copied across the fleet -- by this repository's own scope config the
most consequential kind of file in it -- and it sat outside review
for as long as the include list had no JSON pattern, because JSON
had simply never come up. No issue could have been filed about it:
from review-coverage's perspective the repository was fully
measured.
status and scope-orphans are also useful interactively: run
./tools/review-tracking.sh status in any clone to see effective
coverage at that clone's HEAD, or
./tools/review-tracking.sh scope-orphans to see what the scope
config is silently leaving out. Neither touches any state.