Skip to content

fix(hook): consult the credential floor when the python scanner cannot run - #433

Merged
KbWen merged 2 commits into
mainfrom
fix/precommit-credential-failopen
Sep 5, 2026
Merged

fix(hook): consult the credential floor when the python scanner cannot run#433
KbWen merged 2 commits into
mainfrom
fix/precommit-credential-failopen

Conversation

@KbWen

@KbWen KbWen commented Sep 5, 2026

Copy link
Copy Markdown
Owner

What this fixes

The opt-in pre-commit credential pre-screen could let a staged secret reach object
history while reporting success. Two independent states, both reproduced end-to-end
on a fresh deploy.sh target with a staged PEM private-key header:

host state pre-fix post-fix
real python blocked blocked
no python at all blocked (floor) blocked (floor)
python on PATH that cannot start could not run (exit 49); continuingcommit landed blocked, via the probe
python fine, scanner returns rc=3 could not run (exit 3); continuing, validator green → commit landed blocked, via the floor fallback

In both landing cases git show HEAD:leak.txt returned the header afterwards.

Root cause is a fix that was applied to two of three call sites. Backlog #144 replaced
existence-only interpreter selection with a startability probe in validate.sh and
validate.ps1; the hook is the third site and was never swept. validate.sh:292-301
names the hazard in its own words — the stock-Windows App Execution Alias exists on
PATH with no Python installed. Separately, the hook turned every non-0/1 scanner exit
into "continuing", so the deployed python-free floor — present, executable, and the
control ADR-008 designated canonical — was never consulted.

Why this is conformance, not a contract change

ADR-008's Alternatives Considered records (A) shell+PS regex canonical + .py optional [CHOSEN], and its tier ledger reads "the shell/PS regex floor blocks before object
history with no Python dependency; .py enriches when present." AC-S4 says the
floor "Replaces the Python-gated screen". The shipped hook inverted that: python
gated, and on failure the canonical control was skipped entirely. dev-flow-hardening
AC-8 — titled "Credential floor cannot fail open silently" — allows a WARN "only when
the native credential floor still ran". It did not. No ADR or spec amendment is needed;
this delivers what both already say. The same repo already applies the correct policy in
CI: security.yml:164-171, "an execution error is NOT a clean scan — fail closed".

A live downstream break, found on the way

While reproducing the fallback's blast radius, a pre-existing adopter break surfaced.
A no-Python adopter following the deploy banner's own instructions verbatim — install
the hook, then the banner's own git add .agentcortex-manifest AGENTS.md ... .agentcortex/ ...
had their first framework commit blocked and was told to rotate a secret out of
.agentcortex/tools/scan_credentials.py, a core-tier file the framework had just
force-written. The flagged line is the docstring that documents the allowlist escape
hatch, citing AWS's public example key. Exactly one line matched across the 169-file
staged set. The file's own fixtures already avoid this by concatenation; the prose never
applied the file's own convention. Fixed by abbreviating the shape in the two prose
lines. Measured on the identical path: COMMIT_EXIT=1 before, COMMIT_EXIT=0 after.

The deeper asymmetry behind it is not fixed here and is filed as #195: the floor
reads whole staged blobs while the scanner reads only added lines, the scanner
self-excludes two files and the floor excludes none, and their allowlist checks differ
in case sensitivity. Narrowing the floor would contradict AC-S4's per-staged-file and
the deliberate anti-false-negative reason recorded for it, so it needs a spec decision,
not a patch. The hook's header comment now states the fallback is broader, not a drop-in.

What the review caught

A fresh-context reviewer returned NOT READY on the first round and proved each
finding with a mutant rather than asserting it: with the probe deleted but the fallback
kept, the suite still passed 6/6; with the fallback hardcoded to block, it also passed
6/6. Neither half was pinned. The tests now assert the discriminators the hook already
prints, and each half is independently mutation-verified:

mutant result
probe deleted, fallback kept 1 failed
fallback deleted, probe kept 2 failed
fallback always blocks 2 failed
reduced-assurance line removed 1 failed
example key reintroduced at scan_credentials.py:79 1 failed
clean 8 + 6 passed

A second round found a regression this change introduced, and it is the most useful
thing the review produced. The floor screens 3 credential shapes; the scanner screens 7.
Measured on one staged file: scanner exit 1 with 4 findings (google / slack / openai /
github-pat), floor exit 0. So on the fallback path a clean floor is not a clean
scan -- and the first version of this fix printed "falling back to the no-python floor"
and then went silent, which reads as clean. The old code said "could not run; continuing",
which was honest. On the very path this diff adds, the fix was quieter than what it
replaced, inverting the AC-8 invariant its own new header cites. It now prints an explicit
REDUCED ASSURANCE line when the fallback's floor comes back clean.

A guard was also missing under all of this. test_credential_floor_shell.py's own
docstring promised "no full literal sits in the repo" and had no verifier -- and the
promise was already broken, which is exactly what blocked the adopter above. A new test
runs the floor's own parsed patterns (never a second matcher -- #165 forbids that) over
every tracked file. Reintroducing the shape turns it red; removing it turns it green.

A roundtable seat proposed a smaller remedy — delete the interpreter gate, run the floor
unconditionally — on the strength of AC-S4's "canonical". It was refuted by
measurement
: staging the two files the scanner self-excludes gives floor exit 1 and
scanner exit 0, so floor-first-always would block every commit touching the credential
controls on a working-python host, including this PR.

Adopter delta

deploy.sh:283-290 updates an unmodified .sample and sidecars a modified one to
.acx-incoming with merge instructions. But .githooks/pre-commit is a copy the user
made; deploy.sh never writes it. Protection arrives only after an adopter re-runs the
INSTALL copy
— filed as #197, since the population this fix exists for is exactly the
population that does not automatically receive it. The scan_credentials.py prose fix is
core tier and force-updates, so the first-commit break above is closed on next deploy
regardless.

Also filed: #196, credential_floor.ps1 ships core-tier with zero runtime callers —
AC-S6 parity-tests the two floors against each other, which proves the twins agree but
never that either is reached.

Not done, deliberately

No CHANGELOG entry: every section is a released version with a release narrative and
there is no Unreleased convention; inventing one is a governance-surface change needing
its own justification. The record goes to SSoT Ship History at ship time.

Evidence

  • pytest tests/ci/ tests/guard/ .agentcortex/tests/ -q (CI's own paths): 947 collected, 946 passed, 1 skipped, exit 0.
  • bash .agentcortex/bin/validate.sh un-piped: pass=115 warn=7 fail=0 skip=2, exit 0.
  • Downstream simulation on fresh deploy.sh targets throughout — the two landing commits above, the four post-fix arms, the benign controls on all four PATH states, and the no-Python adopter's first framework commit before and after.

One judgement call worth flagging

The REDUCED ASSURANCE line quotes concrete counts ("3 credential shapes" / "7"). The
simpler option was vaguer prose with no number to keep honest. The numbers were kept
because "3 vs 7" tells an operator how much the fallback actually gives up, and a test
now pins both to the two tools' own sources — an enforced claim rather than a decaying
one. Reviewers who disagree should say so; dropping the numbers also drops that test.

🤖 Generated with Claude Code

KbWen and others added 2 commits September 5, 2026 19:33
…t run

The opt-in pre-commit pre-screen picked its interpreter by existence only and
treated any non-0/1 scanner exit as pass-through, so two host states let a
staged PEM private-key header reach object history with the hook exiting 0:
a python that resolves on PATH but cannot start (the stock-Windows App
Execution Alias stub), and a startable python whose scanner run errors.
Both reproduced end-to-end on fresh deploy targets before and after.

Backlog #144 replaced existence-only selection with a startability probe in
validate.sh and validate.ps1; the hook is the third call site and was never
swept. Separately, ADR-008 chose the regex floor as the canonical control
with the .py path as enrichment, and dev-flow-hardening AC-8 allows a WARN
"only when the native credential floor still ran" - the hook took the WARN
arm without its precondition. This delivers what both already say.

Also fixes a live, pre-existing downstream break found while measuring the
fallback's blast radius: a no-Python adopter following the deploy banner's
own instructions had their first framework commit blocked and was told to
rotate a secret out of scan_credentials.py, whose docstring cited AWS's
public example key in full. Abbreviated at source, and the promise that no
full literal sits in the repo now has the verifier it never had.

Files: the hook, both credential floors' stale backstop claim, the two
prose lines carrying the shape, three hook test arms, two floor guards.
Backlog #195-#197 record what was deliberately left out of scope.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ve its Work Log

SSoT sequence 167->168 with the completion record at the top of Ship History;
rotated at cap 10 (Ship-fix-validator-twin-parity-176-175-2026-08-23 ->
archive/ship-history-2026.md, verbatim, 150 -> 151 entries, no relative links
to re-depth). Work Log moved to archive/ with its date suffix and appended to
the hash-chained INDEX.jsonl via append_chain_entry.py (prev_sha dc48eafa);
check_audit_chain.py reports the chain intact.

One L2 entry in docs/architecture/ci-security.log.md records the constraint
that cost this unit two review rounds: the python scanner and the no-python
floor do NOT share a detection surface - added lines vs whole staged blob,
two self-exclusions vs none, case-insensitive vs case-sensitive allowlist,
7 shapes vs 3 - so routing between them is never a drop-in in either
direction. Decisions D-1/D-4 dispose to that entry; D-2/D-3 are local.

validate.sh after these writes: pass=99 warn=4 fail=0 skip=3, exit 0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@KbWen
KbWen merged commit 0b08cb5 into main Sep 5, 2026
19 checks passed
@KbWen
KbWen deleted the fix/precommit-credential-failopen branch September 5, 2026 12:15
@KbWen KbWen mentioned this pull request Sep 5, 2026
KbWen added a commit that referenced this pull request Sep 5, 2026
Bumps the seven canonical version surfaces plus CITATION.cff date-released
(1.8.25 -> 1.8.26, 2026-08-27 -> 2026-09-05) and adds the CHANGELOG entry for
the three units merged since v1.8.25: the pre-commit credential fail-open fix
(#433), the workflow job-graph guard (#183), and the skill trigger-accuracy
eval (#398).

The release notes lead with the one thing an adopter must act on: re-run the
INSTALL copy. .githooks/pre-commit is a copy the user made and deploy.sh never
rewrites it, so the fix does not reach an already-installed hook on its own.
Without that line the release would claim a fix most existing adopters do not
have - the same over-promise class the release itself fixes.

Downstream delta measured rather than described: of the 26 files changed since
v1.8.25, 6 are in the deploy set. The three credential tools are core tier and
force-update; the eval suite deliberately does not ship.

Records: SSoT sequence 168->169, Ship History rotated at cap 10, Work Log
archived with a hash-chained INDEX.jsonl entry (chain verified intact).

No engine, gate order, deployed file set, or configuration change.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant