scripts/ci/check_agents_md_size.py (in scripts/check and lint.yml): root <= 12k, every
AGENTS.md plus its ancestors <= 30k (the joined root->cwd block on a 128k window, minus
the per-file labels), every area file <= _MAX_HINT_CHARS read from the source so the two
cannot drift. On origin/main it reports the root and all 11 root-to-area chains over.
On a case-insensitive filesystem (Windows, macOS) `scripts/check --staged`/`--commit`
and the hooks' judge() aborted with "checkout-index -a failed: pkg/Notes.md already
exists" when the judged tree held two paths that differ only by case, so the
case-collision guard never got to report them (exit 2 instead of FAILED:
case-collisions). Both snapshots now check out with -f into their empty temp dir; the
guards read the private index, which keeps both paths. Found on the Windows runner by
the case-collision test once that test staged its premise as blobs.
The pre-push hook judges each pushed branch tip, as CI judges the PR as a whole, not
every intermediate commit; CONTRIBUTING.md and the hook comments said "each commit you
push". CONTRIBUTING.md also said a moved function you lightly edit keeps its cap; that
holds only when no function of that name exists at the destination, and a rename plus
an edit is new code. Both limits are now stated.
The hooks' judge() built its private index and tree under `mktemp -d`, a POSIX /c/...
path in Git Bash, and passed it to native git.exe and python.exe through GIT_INDEX_FILE,
--prefix and the checker path. Hermes' Windows terminal exports MSYS_NO_PATHCONV=1 and
MSYS2_ARG_CONV_EXCL=*, so those paths arrived unconverted and the hook died
("Unable to create '/c/.../index.lock'") before any check ran. judge() now hands native
tools the `cygpath -m` spelling when cygpath exists; elsewhere nothing changes.
No behaviour change: line wrapping in the hook text, parse_switch docstring,
commit_or_none and the new tests; the test helpers lose an unused env parameter.
JoaoMarcos44 (inline scripts/check:40): the hooks began with
`[ -f "$repo/scripts/check" ] || exit 0`, before reading the pushed SHAs, so the
checked-out branch decided whether another revision was checked. Pushing an
invalid checker-containing branch while an older branch was checked out succeeded
without judging it.
Eligibility is now decided from the candidate tree: pre-push runs
`git cat-file -e <local_sha>:scripts/check` per pushed ref (a candidate that
predates the checker is skipped, deletions are still skipped), pre-commit does the
same for the index tree. An eligible candidate is checked out alone into a temp
dir (private index + checkout-index; the worktree, index and stash are untouched)
and judged by its own scripts/check with GIT_DIR pointing at the repo - with the
previous commit's snapshot-bound health job, implementation, job table, config and
policy all come from the judged artifact, whatever is checked out. pre-commit
exports an absolute GIT_INDEX_FILE because git may hand hooks a relative one.
unsupportedpastels M5: both hooks ran whatever python3 came first on PATH, but
scripts/check imports ruff_runner, which imports tomllib (3.11+, the project
minimum). With Ubuntu 22.04's python3 3.10 every hooked commit or push died with
"ModuleNotFoundError: No module named 'tomllib'".
The hooks now use the repo's virtualenv interpreter when there is one (.venv/ or
venv/, bin/python or Scripts/python.exe; this worktree first, then the main
checkout) and fall back to python3, then python. scripts/check checks
sys.version_info before importing the engine and exits 2 with a one-line message
naming the virtualenv instead of a traceback. Verified with a real CPython 3.10.21:
exit 2 and the message, where 96bb97a crashed in ruff_runner.
F20 (andrexibiza): in --staged/--commit mode the guard jobs ran inside the
snapshot root with the snapshot's git env, but the health job ran in the real
worktree. Its source was pinned to the index tree, yet its engine and policy were
not: Measurer reads scripts/ci/profile_scope_patterns.json from the repo it runs
in, so with an identical `git write-tree`, editing only the unstaged P05
path_regex turned PS-P05 / exit 1 into "ok" / exit 0 and back.
The health job now runs like every other job: cwd = the snapshot root with the
snapshot env, so `python -m scripts.code_health` imports the snapshot's own engine
and reads the snapshot's policy files and ruff pin. The ruff job resolves its pin
from the same root. Unstaged edits to the policy file or the engine no longer move
the verdict, and they stay untouched in the worktree; plain runs are unchanged
(root is the worktree).
unsupportedpastels m4: scripts/check says it runs the lint checks CI runs, but CI
also runs scripts/check-case-collisions.py, scripts/ci/check_lazy_deps_imports.py
and scripts/ci/check_profile_archive_boundary.py in workflows of their own
(case-collision-check.yml, lazy-deps-guard.yml, profile-artifact-check.yml). A
case-colliding pair passed scripts/check and failed CI.
All three are deterministic and local (git ls-files / an os.walk of the checkout;
measured here: 0.2 s, ~7 s, 0.1 s on the full tree), so they run as ordinary guard
jobs - in the snapshot root for --staged/--commit like the other guards - and are
selectable with --only case-collisions / lazy-deps / profile-archives.
unsupportedpastels m3: (a) the switch was found by a regex that accepted only the
exact `ENFORCEMENT = "..."` spelling, so `ENFORCEMENT: str = "off"` silently stayed
blocking; (b) local runs and both hooks read it from the merge-base with
origin/main, so a flip on main reached a branch only after a rebase, contrary to
"CI and local hooks alike".
(a) parse_switch() reads the module-level plain or annotated assignment with ast
(last one wins, as at runtime); missing, unparseable or unknown values are
blocking. (b) Without --base, the base is still the merge-base of the head (a
--head commit, else HEAD) but the switch comes from origin/main when it resolves,
falling back to the base. An explicit --base pins both (CI passes HEAD^1, the
target tip). scripts/check --commit now hands the commit itself to the ratchet so
it derives that commit's merge-base. The head under test never supplies the switch.
(c) A test pins enforcement() on this tree to config.ENFORCEMENT.
F22 (andrexibiza): `scripts/check --only heath`, `--only ,` and `--only heath,rof`
filtered the job table down to nothing and reported "0 checks, ok" / exit 0, and
`--only health,rof` silently dropped the unknown name. A typo in a developer or
automation command could claim success without running the requested check.
The whole selector set is now validated before any job is built or dispatched:
an empty selection or any unknown name is a usage error (exit 2) that lists the
valid names. A valid selector still runs its check and keeps its status.
- `ENFORCEMENT` in scripts/code_health/config.py (blocking / advisory / off)
turns the ratchet down or off with a one-line commit to main. It is read
from the BASE revision, so it applies to fork PRs (repo variables are not
passed to fork-triggered workflows), CI and local hooks alike, and a PR
cannot relax its own check.
- `scripts/check --install-hook pre-push` judges each pushed commit the way
CI judges the PR (base = that commit's merge-base); `--commit REV` is the
underlying mode, `--uninstall-hook` removes our hooks and leaves foreign
ones. Pre-commit and pre-push share one tree snapshot that never touches
the user's worktree or index.
- A failing report names the local commands; CONTRIBUTING, AGENTS.md and the
PR template point at `python scripts/check`.
Fourth review round (F11-F18, N1-N4, kvnloo, Adolanium):
- Hits are old only where the base occurrence survives: a line the comment- and
whitespace-blind line diff keeps (blame semantics), or anything inside a unit whose
body is unchanged. An identical violation moved past unrelated code is new; removing
an allow comment or re-indenting is not. Hit fingerprints exclude comments (N4).
- Inline allows cover their own line; only a comment-only line covers the next (N3).
- Rules read a copy of the AST with import aliases resolved, so `sp.run`,
`from subprocess import run as execute`, aliased threading/asyncio/load_config are
seen (F12); deadline helpers must resolve to the real asyncio API, and
communicate()/wait() are judged only on receivers holding a spawned process (F13);
infinite deadlines are unbounded (F14).
- HERMES_* writes introduce a name too (N1); bool(os.environ[...]) is HX010 (N2);
HX009 needs the isinstance subject to come from gather(return_exceptions=True).
- Recursive self-references are normalised out of body hashes, so a recursive rename
keeps its cap, py and ts (F15). Decorators belong to their function's scope and
classes are matchable units, so their hits follow proven moves (F16).
- An over-cap file may not grow (F17, Teknium: strict improvement-only).
- A file the checker cannot parse or measure (ast/ruff/TS parse error) fails.
- scripts/check: --staged always lints an index snapshot (ignored files are invisible
to git status, not to guards) (F18); create the --prefix dir for Git for Windows
(F11); --only builds just the selected jobs, so it needs no ruff for non-ruff checks.
- CODEOWNERS covers scripts/check + scripts/code_health; frontend-only PRs skip the
Python-only lint jobs again and run only the ratchet.
Second-round review on #132646 (F1-F10), each reproduced on 2531fb2 first:
- Moves are matched one-to-one: unchanged units are reserved first, then
same-file body, then cross-file body only when the origin's name is gone,
then same name. A copy of unchanged debt is new debt; a function moved
over a same-name stub keeps its own cap (F1, F3).
- Every base hit is owned by exactly one head scope, so a file rename no
longer counts each old hit twice (F2).
- Ruff runs when either side has Python, so a delete-only .py diff with a
TS edit no longer crashes on a missing ruff result (F4).
- Deadlines must be finite: timeout=None, **{} and asyncio.timeout(None)
are no deadline; awaited calls inside `async with asyncio.timeout(x)`
are bounded (F5, F8).
- HX005 walks only what runs at import: the import-time branch of a
`__name__` guard (either operator), loops, match, try*, decorators,
class bases; a lambda value is deferred (F9).
- `health: allow` counts only in real comment tokens (Python tokenize, TS
comment trivia), never in string literals (F7).
- TS function lines are owned by the innermost function, so siblings on
one line give it up once (F10).
- The pre-commit hook runs `scripts/check --staged`: health compares the
index tree, and when the working tree differs from the index the other
checkers run on a checkout-index copy with a copied index, so commits are
judged on the staged bytes and unstaged work is never touched (F6).
Independent review of #132646 found five defects, each reproduced and fixed:
- Pattern hits from any vanished scope went into a shared (rule, text) pool, so deleting one
`except Exception:` paid for an unrelated new one elsewhere, and class scopes (never units)
always looked vanished. Hits now transfer only along a unit matched by body hash.
- A unit's prior is now looked up by body hash before qualname, so a TS anonymous callback
whose source-order ordinal shifted keeps its cap.
- The TS body hash included the function name; it is now parameters + body, so a rename keeps
its cap (as in Python).
- HX006 flagged `await asyncio.wait_for(proc.communicate(), timeout=...)`; calls passed to
wait_for/timeout are bounded. The fix text now names the asyncio form.
- scripts/check printed output only on failure, hiding advisories and comment allows; the
health report is now always shown.
Docs now say growth of a file already over 2,000 lines is a warning for now.
scripts/code_health measures the files a change touches on both sides
(base revision and head) and fails when:
- a function or file over target grows past its own value on the base
(CC 20, 300 own lines, nesting 6, file 2,000 lines);
- a new function or file is over target;
- a pattern rule gains a fingerprint (rule + enclosing function + line
text), so fixing one violation never buys another.
Code that moves keeps its cap: functions match by name-independent body
hash, pattern hits by (rule, line text) once their function has left the
old place. Nested functions are their own units, so a closure edit never
counts against the hook or function around it. Python CC is ruff's C901
minus the direct nested defs; TypeScript is parsed with the lockfile's
typescript (no type check).
Rules: swallowed exceptions (S110, BLE001, E722), correctness (F821/F823/
F811/F841, RUF006), hangs (ASYNC230/240, S113, subprocess/urlopen without
timeout, sync config I/O in async def, get_event_loop, gather results
checked against Exception), profile scope (hardcoded ~/.hermes, a new
HERMES_* env var, UnscopedSecretError -> os.getenv, import-time capture,
raw threading.Thread, P05/P06 from profile_scope_patterns.json), argv
process identity, bool() of an env string, and if/elif ladders.
Growth of a file already over 2,000 lines is advisory: replaying 300
merged PRs, a blocking file-size ratchet would have stopped 46 of them
before the 119 over-target files are split. Everything else blocks; the
same replay finds 18 PRs that would have needed a change, every finding
a real hit for its rule.
No baseline file: base is measured on the fly, so an improvement that
lands on main is the new cap at once and parallel PRs never conflict on
ratchet state. `# health: allow <RULE> -- <why>` is the reviewed escape.
