fix(#3631): exclude only __pycache__-resident bytecode from the consent digest (#3650)

* test(3631): failing-first coverage for bytecode-cache in the consent hash

bundleContentHash digests a walk with no exclusion, so a routine 'python3 -m unittest'
inside a Python-backed capability bundle writes __pycache__ under the bundle, the
recomputed hash stops matching the consent record, and the capability silently goes
inactive — no error, no warning, and loop render-hooks then omits its step and gate.

Two distinct triggers, and the second is the sharper one: collectBundleEntries pushes a
{kind:'dir'} entry for EVERY directory and the digest emits a TAG_DIR marker for it, so an
EMPTY __pycache__/ flips the hash before a single .pyc is written. A fix filtering only
*.pyc would leave that live. Verified by execution against the built lib: 5 of 7 probe
rows diverge from intent today, including the empty-directory row.

The anti-regression rows are the point of the shape: editing a real scripts/m.py and
adding node_modules/pkg/index.js must BOTH still change the hash. node_modules is
deliberately not excludable — its contents are required at runtime, so dropping it from
the digest would stop consent binding executable content. The symlink row pins ordering:
exclusion must apply after the lstat fail-closed rejection, never before.

Refs #3631

* fix(3631): exclude derived bytecode caches from the consent digest

RED proven at e5ba8f1fe on the remote runner: 8 failures, exactly the rows predicted to
fail, with the four anti-regression rows already green.

collectBundleEntries now skips a hardcoded, gitignore-independent set from the DIGEST:
basenames __pycache__, .pytest_cache, .DS_Store, and any .pyc/.pyo file. Matching is
byte-exact on the raw Buffer name (the walk never utf8-decodes) and case-sensitive, so the
digest does not vary with how a name happens to be spelled on a case-insensitive volume.

Three properties were preserved deliberately, each pinned by a test:

  - The filter runs AFTER the lstat symlink/non-regular fail-closed rejection. Filtering
    first would have turned the exclusion into a way to smuggle a symlink past the check;
    a symlink named x.pyc still throws.
  - Excluded entries still count toward BUNDLE_MAX_FILES and BUNDLE_MAX_TOTAL_BYTES. The
    caps guard the WALK; the digest answers a different question, and exclusion must not
    become an unbounded-bytes hole.
  - An excluded DIRECTORY is neither emitted as a TAG_DIR marker nor recursed into. The
    directory marker was the sharper half of this bug: an empty __pycache__ flipped the
    hash before any .pyc existed, so a *.pyc-only filter would have left it live.

The issue proposed either a gitignore-aware walk or a list including node_modules. Both
are rejected. A consent binding must not delegate its scope to a .gitignore the bundle
author does not control — one line there would drop arbitrary executable content out of
the hash. And node_modules holds code that is required at runtime; excluding it would stop
consent binding executable content, turning a usability bug into a supply-chain hole. What
makes __pycache__ different is that CPython validates each .pyc against its sibling
source, which remains hashed, so a real code change still invalidates consent.

Docs: CONTEXT.md's 'EVERY regular file AND directory' claim is corrected in place.
ADR-2363's residual-gap section said the walk had 'no exclusions' — per
docs/adr/README.md ('ADRs are append-only') that is corrected by a dated amendment rather
than an in-place edit. Its D4 argument is unaffected: skill bodies are .md and stay bound.

Fixes #3631

* fix(3631): narrow the digest exclusion after two isolated security reviews

The first cut of this fix passed the full suite and was still wrong. Both orthogonal
reviews rejected it, and the second one found a hole that has nothing to do with Python.

HIGH — an excluded DIRECTORY was 'continue'd before recursion, so its whole subtree was
permanently outside the digest. Declared hook script paths allow '_', '.' and '/' with no
directory or extension rule, so hooks:[{script:'__pycache__/run.js'}] installed, executed
via node, and its bytes could be rewritten forever without moving the hash. Ship benign
v1, collect consent, then own the machine. No Python involved.

FALSE RATIONALE — the justification I wrote into the code, CONTEXT.md, the ADR amendment
and the changeset claimed CPython validates a cached .pyc against its sibling source, so
the source staying hashed kept consent honest. That is not true, and I proved it by
execution rather than argument: default timestamp invalidation compares only the source's
mtime and size, both settable by anyone who can write the bundle. A forged pyc ran while
the .py was byte-identical.

Also wrong: '*.pyc' matched anywhere, but a legacy sourceless scripts/x.pyc IS importable,
so excluding it was a live vector.

Narrowed to what is actually defensible:
  - a DIRECTORY named __pycache__/.pytest_cache has only its TAG_DIR marker suppressed;
    the walk still recurses and hashes every non-excluded child.
  - .pyc/.pyo are excluded ONLY when the parent basename is exactly __pycache__.
  - a regular FILE named __pycache__, and a DIRECTORY named x.pyc, stay bound.
  - declared hook paths containing a __pycache__/.pytest_cache segment or a .pyc/.pyo
    basename are now rejected in both validator copies — a file named .pyc can contain
    perfectly valid JavaScript, so the exclusion must not be reachable from a declared
    surface.

Accepted residual risk, stated plainly in ADR-2363 and CONTEXT.md instead of explained
away: a forged __pycache__/mod.pyc matching an unmodified, still-hashed mod.py executes
without moving the digest. Before this change that write was detected. It is accepted to
stop routine bytecode caching from silently deactivating capabilities, and it is bounded —
the attacker needs post-consent write access, everything outside __pycache__/*.pyc stays
hashed, and no declared surface can point into the excluded space.

Known limitation, not papered over: .pytest_cache CONTENTS still move the digest. Only the
directory marker is suppressed. Excluding that subtree would reopen the HIGH finding.

Refs #3631

* fix(3631): drop the .DS_Store exclusion and pin what the caps actually bind

Second round of isolated review findings. The hardening closed the two original holes —
both re-reviews confirmed that by execution — but it introduced a new one of the same
shape, and left three claims unbacked.

HIGH, self-inflicted: .DS_Store was excluded from the digest at any depth, but the hook
path validator was hardened only for __pycache__/.pytest_cache/.pyc/.pyo. So
script:'hooks/.DS_Store' was ACCEPTED, runnableHookCommand emits the bare quoted path for
a non-.js name (the branch .sh hooks already use), and capability-source copies it with
its mode bit intact. Ship it +x with a benign shebang, take consent, then rewrite it
forever — the digest never moves. Fixed by DELETING the .DS_Store exclusion rather than
teaching the validator about it: .DS_Store has nothing to do with this issue's Python
bytecode symptom, and an excluded filename is a permanently unhashed name. The narrower
the exclusion, the smaller the hole.

The residual-risk bound in ADR-2363 and CONTEXT.md claimed declared surfaces cannot reach
excluded space. That is false and is now stated correctly: node resolves an unregistered
extension through the default .js handler, so a hashed, consent-covered hooks/run.js that
requires '../__pycache__/mod.pyc' reaches it in one hop. The validator guard raises the
bar for DECLARED surfaces; it does not contain the risk. The two bounds that are real —
post-consent write access required, everything outside __pycache__/*.pyc still hashed —
are kept.

The BUNDLE_MAX_FILES boundary test had gone vacuous: it padded with root-level *.pyc,
which the hardening made non-excluded, so it no longer proved anything about excluded
entries while the ADR claimed the caps were test-pinned. It now pads __pycache__/f{i}.pyc,
with the arithmetic re-derived by execution (capability.json + the still-counted
__pycache__ dir + N). BUNDLE_MAX_TOTAL_BYTES had zero coverage at all and is now pinned by
a sparse 32 MiB __pycache__/big.pyc that must still trip the size cap — the test that
proves exclusion did not become an unbounded-bytes hole.

Added the parity assertion CLAUDE.md's Generative Fix Divergence rule requires for the two
isSafeHookScriptPath copies, and proved it can fail: mutating one BUILT copy to drop .pyo
made the parity check report the divergence. Also pinned semantics that were correct but
untested and would have survived mutation — __pycache__/sub/x.pyc stays hashed (the parent
resets to sub, which is the recursion threading itself), .pytest_cache/y.pyc stays hashed,
and .pyo in both directions, which was a free surviving mutant.

Changeset rewritten: it still described the rejected wholesale-exclusion semantics.

Refs #3631

* chore(3631): backfill changeset PR number (#3650)

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-18 22:35:54 -04:00
committed by GitHub
parent 02a36d3db9
commit 9e4f0e99ad
8 changed files with 697 additions and 17 deletions

View File

@@ -1918,6 +1918,82 @@ describe('C4: description and hooks validation', () => {
assert.deepEqual(hookErrors, [], 'Expected a normal nested relative script to be accepted, got: ' + JSON.stringify(hookErrors));
});
// ─── #3631 (defense-in-depth): a declared hook script path must not point into the space
// bundleContentHash excludes from the consent digest. A file named `x.pyc` can contain valid
// JavaScript and would be executed by `node` regardless of extension, and a __pycache__/
// .pytest_cache path segment marks digest-excluded space — so a manifest-declared executable
// surface must never be able to reach either. ───
for (const [label, script] of [
['__pycache__ path segment', '__pycache__/run.js'],
['.pytest_cache path segment', '.pytest_cache/run.js'],
['.pyc basename suffix', 'hooks/x.pyc'],
]) {
test(`hook script pointing into digest-excluded space is rejected (${label})`, () => {
const cap = { ...UI_CAP, hooks: [{ event: 'PostToolUse', script }] };
const errors = validateCapability(cap, 'ui');
const hookErrors = errors.filter((e) => e.includes('hooks[0].script'));
assert.ok(
hookErrors.length > 0,
`Expected a hooks[0].script rejection for ${label} (script=${JSON.stringify(script)}), got: ` + JSON.stringify(errors),
);
});
}
test('hook script not pointing into digest-excluded space is still accepted (hooks/check.js)', () => {
const cap = { ...UI_CAP, hooks: [{ event: 'PostToolUse', script: 'hooks/check.js' }] };
const errors = validateCapability(cap, 'ui');
const hookErrors = errors.filter((e) => e.includes('hooks[0].script'));
assert.deepEqual(hookErrors, [], 'Expected a normal .js hook script to be accepted, got: ' + JSON.stringify(hookErrors));
});
// ─── Generative-fix-divergence parity: `isSafeHookScriptPath` is duplicated hand-maintained
// logic in src/capability-lifecycle.cts (via `confinedBundleScript`, the nearest exported
// consumer) and gsd-core/bin/lib/capability-validator.cjs (via `validateCapability`, the
// nearest exported consumer). There is no src/capability-validator.cts — the .cjs is hand
// -maintained — so this drives BOTH real copies behaviorally (never source-greps either file)
// and asserts they agree on every path, catching the two implementations drifting apart. #3631
// finding 1 removed the `.DS_Store` DIGEST exclusion, but the validator never rejected
// `.DS_Store` on either side — `hooks/.DS_Store` is expected to be ACCEPTED by both.
test('isSafeHookScriptPath parity: capability-lifecycle.cts and capability-validator.cjs agree on every path', () => {
const { confinedBundleScript } = require('../gsd-core/bin/lib/capability-lifecycle.cjs');
// A capDir that does not exist on disk: confinedBundleScript falls into its lexical
// (does-not-exist-yet) branch, so the verdict reflects ONLY isSafeHookScriptPath — never a
// realpath/confinement side effect unrelated to what is under test here.
const fakeCapDir = path.join(os.tmpdir(), 'gsd-parity-probe-nonexistent-cap-dir');
const cases = [
['hooks/check.js', true],
['__pycache__/run.js', false],
['hooks/__pycache__/run.js', false],
['.pytest_cache/run.js', false],
['hooks/x.pyc', false],
['x.pyo', false],
['hooks/x.PYC', false],
['hooks/.DS_Store', true],
['hooks/../__pycache__/run.js', false],
['hooks/__pycache__./run.js', true],
['__PYCACHE__/run.js', true],
['hooks\\__pycache__\\run.js', false],
];
for (const [script, expectedAccept] of cases) {
const cap = { ...UI_CAP, hooks: [{ event: 'PostToolUse', script }] };
const errors = validateCapability(cap, 'ui');
const cjsAccept = errors.filter((e) => e.includes('hooks[0].script')).length === 0;
const ctsAccept = confinedBundleScript(fakeCapDir, script) !== null;
assert.strictEqual(
cjsAccept,
ctsAccept,
`capability-validator.cjs (accept=${cjsAccept}) and capability-lifecycle.cts (accept=${ctsAccept}) disagree on ${JSON.stringify(script)}`,
);
assert.strictEqual(
cjsAccept,
expectedAccept,
`expected accept=${expectedAccept} for ${JSON.stringify(script)}, both copies returned accept=${cjsAccept}`,
);
}
});
test('description present in UI_CAP passes validation', () => {
const errors = validateCapability(UI_CAP, 'ui');
const descErrors = errors.filter((e) => e.includes('description'));