Repository navigation
Fix the Codex review findings: promotion-gate parsing, anchored gitleaks allowlists, release-tag checkout - #83
Merged
seathatflowsinourveins merged 15 commits intoSep 23, 2026
Conversation
…validation, gitleaks line anchoring, Grype wording, bootstrap baseline commit
Fixes the five confirmed findings from codex-review-64 (blind Codex
cross-family review) in this unit's owned scope:
1. promotion_gate.py _summarize(): a recognized pandera failure case
with a null row index (e.g. calendar.is_session() raising on
session='not-a-date') was silently reported as passing because
`.dropna()` on the index column discarded it before the fail/pass
decision. Any failure case for a named check now fails that check
regardless of index availability, recording "row index unavailable"
in the detail.
2. promotion_gate.py volume validation: a lossy float/pd.to_numeric
conversion still preceded the integrality/sign check ('-1e-400' ->
-0.0 passed; '9007199254740992.5' read as integral). The CSV loader
now reads every column with dtype=str to preserve the raw source
text, and volume is validated by Decimal-parsing that exact text
(rejecting non-finite, negative including -0, any fractional part,
and values above int64 max) before any float conversion; an int64
cell goes straight through Decimal(int(...)).
3. .gitleaks.toml: every regexTarget="line" allowlist regex is now
anchored to the whole line, so a line holding both an allowed
digest and an unrelated secret is no longer swept as exempted
together. The next_page_token cursor exemption is now keyed on the
literal field name (line-anchored) instead of only the value's
base64 shape. Re-measured guarded scans: --log-opts=HEAD (392
commits, ~915 MB) and `dir .` both report 0 findings; default
log-opts still shows one pre-existing, out-of-ancestry finding on a
concurrently active sibling branch (not owned by this unit,
documented in the header comment).
4. catalogs/foundation/automation.json and docs/github-automation.md
corrected: Grype 0.119.0 is a fixed, checksum-verified pin in
supply-chain.yml (GRYPE_VERSION), not a tool that "tracks upstream's
latest release"; its absence from catalog-freshness.yml's pin-drift
table is a coverage gap, not a consequence of its versioning.
5. adoption/bootstrap.md step 0 now checks out the attested release
tag (source.release_tag, v2026.09.22.1, at source.release_commit)
instead of source.baseline_commit, which predates adoption/ and
tools/adoption/ entirely and broke every later step on the page.
adoption/manifest.json gains release_tag/release_commit fields
without changing baseline_commit's meaning; a gh attestation verify
one-liner is documented for users downloading the release archive.
Each fix carries a regression test that fails on fd8e7fb and passes
after (tests/test_promotion_gate_codex_review.py is new so as not to
touch tests/test_promotion_gate.py, owned by another session; the
gitleaks and adoption-contract tests extend their existing files).
Acceptance: tests.test_promotion_gate, tests.test_promotion_gate_codex_review,
tests.test_gitleaks_config, tests.test_adoption_contract and
tests.test_adoption_bootstrap all pass (81 tests); the full suite
passes (2054 tests, 318 skipped, unchanged skip categories);
scripts/validate.py passes after manifests/evidence.json rehash
(69 components, 1832 hashed files). scripts/build_ecosystem.py --check
fails because the edited docs feed the generated explorer HTML; that
regeneration is left to the coordinator per instructions.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…crossing, pandera duplicate-index overcount Second-round Codex cross-family review findings (codex-review-72 follow-up on the codex-review-64/72 fix round): 1. README.md (three checkout entry points: "Start here" step 0, the Fresh WSL2 six-step block and the Fresh macOS arm64 block) and adoption/platforms/linux-wsl2.md still told a reader to `git checkout` adoption/manifest.json source.baseline_commit, a revision that predates adoption/ and tools/adoption/ entirely -- the same bug adoption/bootstrap.md step 0 was already fixed for. Both now check out source.release_tag instead, matching bootstrap.md. Extended tests/test_adoption_contract.py with a regression that scans every doc entry point (not just bootstrap.md's first 1500 characters) for the broken checkout pattern. 2. .gitleaks.toml's prose-narrative commit-id allowlist (duplicated under generic-api-key and sourcegraph-access-token) used `.*?`/`.*` before/after its context-word-and-hex match, which can cross the closing quote of the JSON string holding the commit reference and reach a second, unrelated key:value pair later on the same physical line -- so a line holding both an allowed commit narrative and a real secret could be wrongly exempted in full. Both occurrences now use `(?:[^"\\]|\\.)*?`/`(?:[^"\\]|\\.)*`, which cannot leave the JSON string. Verified against the installed gitleaks binary (not just simulated): the old regex suppressed the secret on a constructed mixed line, the new one does not, and all 53 real narrative-line matches in the two allowlisted files are unaffected. New regression tests in tests/test_gitleaks_config.py (end-to-end via gitleaks, plus a pure-regex comparison that needs neither the binary nor its per-user lock). Residual limitation: a single JSON string that itself narrates both a commit id and an unrelated secret-shaped substring (no second key:value pair to anchor against) can still be exempted; closing that would require restricting free-text narrative content in a way that breaks real, already-committed review lines. 3. promotion_gate.py's `_summarize()` null-index fix (previous round) changed the per-check failure count to `len(subset)` -- the raw number of pandera failure-case ROWS -- which overcounts when pandera emits more than one failure-case row per failed index (one row per column for a dataframe-level check, or one row per field for a multi-field uniqueness check), and then falsely appends "row index unavailable" even when every case has an index. Count is now the number of distinct non-null indices plus the number of failure cases that truly lack one. bad-gate-result.json already matched the fixed behavior exactly (no fixture regeneration needed); all other fixtures still match unchanged. New regression tests in tests/test_promotion_gate_codex_review.py (pure-stdlib duplicate-index cases, plus an exact-detail-string check against the bad.csv fixture). Every new test fails against 0a6ab80 (pre-fix) and passes after. Rehashed manifests/evidence.json for the 7 changed tracked files via codex-ecosystem's rehash_evidence.py. Acceptance: tests.test_promotion_gate/test_promotion_gate_codex_review/ test_gitleaks_config/test_adoption_contract/test_adoption_bootstrap (88 tests, OK) and the full `python3 -m unittest discover -s tests` (2061 tests, OK, skipped=318) both pass; both guarded gitleaks scans (git --log-opts=HEAD and dir, --max-target-megabytes 2) report 0 findings; scripts/validate.py passes (status: passed). scripts/build_ecosystem.py --check fails because README.md and adoption/platforms/linux-wsl2.md feed the offline ecosystem explorer's generated HTML, which this unit does not own or regenerate (docs/ecosystem/* is out of scope) -- flagged for the coordinator to rebuild with --write. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…es to its commit; cite codex-review-72 for step 0 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
suppression, shallow-clone step-0 test, and attribution/scan-scope fixes Codex cross-family review of PR #83 (fix round 3), each with a regression test that fails before and passes after the fix: 1. blueprints/us-equities/data/promotion_gate.py: `_volume_cell_fails` stripped Unicode whitespace (str.strip() with no argument) before the ASCII-only canonical-number check, so a volume cell like " 5" or "5 " passed the check while pd.to_numeric later coerced it to NaN and _prepare() silently wrote 0. Strip only ASCII whitespace. Added NBSP/ ideographic-space cases to the pure-stdlib test plus an end-to-end CSV fixture (nbsp-volume.csv) through evaluate() in the gate's isolated venv. 2. .gitleaks.toml: the prose-narrative commit-id allowlist (round 2 fixed a second key:value pair sharing a physical line, but a single JSON string that itself narrates both a commit id AND an unrelated secret-shaped substring, e.g. `"pinned commit <hex>; password=<hex>"`, was still exempted in full -- RE2 has no lookaround to close this with a negative exclusion). Rebuilt the free-text portions of the allowlist regex as a repeated unit grammar that forbids any unmarked hex-looking run of 8+ characters anywhere in the string; widened the marker's accepted hex length to 4-64 chars after this caught a real line citing a short git-abbreviated commit hash. Re-enumerated the real matched lines with the guarded gitleaks binary (allowlists removed in a scratch config) and re-verified 0 findings on HEAD ancestry and the working tree with the final config. Documented residual gap: an all-decimal (no hex letter) secret-shaped run is not closed, since capping it breaks real lines that cite plain dates/star counts/byte sizes inline. 3. tests/test_adoption_contract.py: the step-0 test failed in CI's shallow clone when `source.release_commit`/`baseline_commit` predates the fetch depth. Added `_require_commit`, which skips (naming the missing object) instead of asserting either way when a commit object is absent. 4. README.md and adoption/platforms/linux-wsl2.md cited codex-review-64 for the step-0/baseline_commit finding; it is codex-review-72. Left the promotion-gate's own codex-review-64 citations untouched. 5. Corrected the gitleaks quote-crossing (round 2) attribution from codex-review-72 to the Opus evidence review of this branch, and made the comments/docstrings state consistently what was actually run (the installed gitleaks binary for end-to-end cases, Python `re` simulation for the RE2-compatible regex-only cases). 6. promotion_gate.py's `unmapped_failures` synthetic check summed each unrecognized pandera check's already-deduplicated count instead of deduplicating row indices across all unrecognized checks together, so the same row failing two different unrecognized checks was double counted relative to its (correctly deduplicated) example-indices list. 7. Broadened the baseline_commit-as-checkout-target doc scan from README.md + adoption/**/*.md and one exact command string to any checkout/switch line pairing with baseline_commit across README.md, adoption/**, recipes/** and docs/**/*.md, excluding lines that explicitly warn against it. Re-ran rehash_evidence.py, scripts/build_ecosystem.py --write, a second rehash_evidence.py pass, scripts/validate.py and --check: all passed. Targeted and full `python3 -m unittest` runs pass (2127 tests, 0 failures/errors; skips are pre-existing conditional skips plus per-user gitleaks-lock contention from a concurrent scan, independently re-verified at 0 skips for the gitleaks module alone). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Round 3's widening of the narrative-allowlist marker-hex window from a
fixed 40 to [0-9a-f]{4,64} newly exempted any marker word (including
"source"/"pinned" used as ordinary prose, not an actual commit
reference) immediately followed by an unrelated 41-64-char hex secret,
e.g. "source token: <64-hex>". The window is narrowed to
(?:[0-9a-f]{4,12}|[0-9a-f]{40})\b -- the two real shapes the narrative
content uses (short abbreviated hashes, or a full 40-hex id) -- which
changes 0 of the 95 real narrative lines the round-3 regex matched.
Also documents (and pins with a regression test) a second residual
gap raised in the same review: a non-hex-shaped high-entropy secret
(e.g. a base64-ish API key) sharing the same JSON string as a real
commit/pin/tree/source marker reference is still exempted, because
closing it would require capping ordinary narrative prose runs and
RE2 has no lookaround/backreferences to scope the cap more narrowly.
Adds tests/test_gitleaks_config.py coverage: two end-to-end
binary-backed tests (test_c7/test_c8) and two pure-regex tests
(round-4 marker-hex-length fix and the documented non-hex residual),
each proven to fail against the round-3 config and pass against the
fix. Regenerates docs/ecosystem/index.html and manifests/evidence.json
via scripts/build_ecosystem.py --write after rehashing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…eaksignore fingerprints Four review rounds (Opus evidence review; a blind Codex cross-family review of PR #83; a blind Opus/Codex evidence review of that fix; and a final blind Codex review, codex-review-83b) found the in-config commit-id allowlist regex under generic-api-key/sourcegraph-access-token progressively leaky in ways RE2 (gitleaks' regex engine, no lookaround/backreferences) cannot close with another regex: most recently, the marker's separator could absorb leading hex characters, and a non-hex secret sharing the same JSON string as a real commit reference stayed exempted. Remove both duplicated `[[rules.allowlists]]` blocks and the four rounds of header commentary about them from .gitleaks.toml, replacing them with a short pointer to the new root .gitleaksignore file, which lists the exact `commit:path:rule:line` fingerprints (gitleaks' own mechanism for known historical false positives, read by default) for the two review-narrative files' real lines. A later commit touching the same line number gets a new fingerprint and is still detected, closing the whole class of same-line/ same-string gaps a shape-based regex could not. Verified with the guarded gitleaks launcher: a scratch config with the two narrative allowlist blocks simply removed produced 15 findings (14 unique fingerprints), all confined to the two narrative files; the real config plus .gitleaksignore then re-scans both `git` (HEAD ancestry) and `dir` (working tree) to 0 findings. tests/test_gitleaks_config.py: drop the narrative-regex-only tests (test_c4-c8 and GitleaksNarrativeAllowlistRegexTests, including its pinned residual-gap cases) and add GitleaksIgnoreFingerprintTests, which checks every .gitleaksignore line is a well-formed fingerprint naming one of the two narrative files and rule, checks .gitleaks.toml no longer allowlists those files by path, and (binary-backed) commits a synthetic marker into a disposable detached worktree on the SAME narrative line to prove a new commit sharing that line is still caught. Register .gitleaksignore in manifests/evidence.json and regenerate the explorer/evidence hashes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The prior test_c selector independently re-derived a "looks like a commit
reference" line (marker word + 8+ hex chars + trailing quote) instead of
reading .gitleaksignore itself, and picked line 98 of
claude-coverage-review.json ("source_commit 5f3ec40b", an 8-char
abbreviated hash with no matching fingerprint) rather than one of the 11
lines .gitleaksignore actually fingerprints for that file. The test
therefore passed regardless of .gitleaksignore's content, which defeats
its purpose as regression coverage for the fingerprint mechanism.
test_c now parses .gitleaksignore's own fingerprint entries and injects
into one of the exact lines it names (line 338, the lowest fingerprinted
line for the file), so the test fails if .gitleaksignore has no matching
entry (verified: clearing .gitleaksignore now fails this test) and
otherwise still proves a same-line, later-commit secret is detected
despite the fingerprint. Also widened the injected marker to a full
ghp_-plus-36-char shape to match the comment's claim about it.
Rehashed manifests/evidence.json and rebuilt docs/ecosystem/index.html
for the test file change.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… commit, document the two suppression files Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ys clean Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
seathatflowsinourveins
deleted the
claude/codex-review-fixes-20260922
branch
September 23, 2026 05:22
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.
Fixes the confirmed findings from two blind Codex cross-family reviews:
codex-review-64of d093a4c andcodex-review-72of fe0ed40. An Opus evidence review and two fix rounds followed.Promotion gate (
blueprints/us-equities/data/promotion_gate.py)session='not-a-date') now fails its check, and the detail says "row index unavailable". Rows are counted once per distinct index, so a dataframe-level check no longer over-counts, and the retained fixture outputs are regenerated.decimal.Decimalbefore any float conversion.-1e-400(which underflows to-0.0),9007199254740992.5(loses precision), non-finite values and values above int64 max all fail.1_000and non-ASCII digits fail, becausepd.to_numericwould read them as NaN.CHECK_NAMESin the runner (Require the promotion-gate result to report every named gate check (SYN) #77) still matches.Gitleaks (
.gitleaks.toml)next_page_tokenexemption requires that key and is path-restricted.tests/test_gitleaks_config.pyproves each case against the guardedgitleaks.New-machine checkout (
adoption/bootstrap.md,README.md,adoption/platforms/linux-wsl2.md)source.release_tag(v2026.09.22.1→bdd04ca, attested). They used to check outbaseline_commit(8f1da51), which predatesadoption/.release_commitand that the commit containsadoption/bootstrap.mdandtools/adoption/render_config.py.Automation wording
main: a fixed, checksum-verified pin that the freshness table covers.Verification
python3 -m unittestruns 2,117 tests OK (318 environment skips) after merging main92bb279.validate.py,validate_catalogs.py,validate_foundation.py,landscape.py,build_verdicts.py --check,trading_gates.py --checkandbuild_ecosystem.py --checkall pass.gitleaks giton HEAD ancestry: 393 commits, 0 findings.gitleaks dir .: 0 findings.origin/main..HEAD): 0 findings.docs/ecosystem/index.html(13 MB) is above the 2 MB cap.Limits
v2026.09.22.1tag's own docs still describe the old checkout. The next release tag after this merge carries the fixed step 0.🤖 Generated with Claude Code