fix(catalog-console): close the vacuous review gate; split catalog/release signing keys (#68 findings 1, 5) #69

Merged

the_og merged 2 commits from fix/68-console-gate into main 2026-07-12 21:30:13 -04:00

Conversation 0 Commits 2 Files Changed 6

+628 -98

the_og commented 2026-07-12 21:26:11 -04:00

Owner

Copy Link

Copy Source

Fixes #68 findings 1 and 5 (see #62 for the Console's original spec).

Scope, per the task: catalog_review.py, catalog_console.py, tests/test_catalog_review.py, .github/workflows/release.yml, scripts/sign_checksums.py, README.md. Does not touch bcc_core.py, tests/test_core.py, .github/workflows/ci.yml, bcc.py, or data/catalog.json (owned by a concurrent fix for the other findings in #68).

Finding 1 — the review gate signed without reviewing anything

Two compounding bugs, both fixed:

1. ReviewWindow._on_sign hardcoded "main" as the TOCTOU comparison ref, while _on_load pins the PR head's blob SHA when a PR is selected — for any PR that changes the catalog those SHAs differ by definition, so the PR review path could never sign, and the retry path re-pinned the same wrong value forever.
   • ReviewSession now carries loaded_ref (the exact ref that was reviewed).
   • New pure sign_precondition(session, resolve_blob_sha: Callable[[str], str]) resolves the TOCTOU SHA from session.loaded_ref, never a hardcoded "main". _on_sign now calls this instead of hand-rolling the hardcoded-main resolution.
   • The retry path calls _on_load() (full re-fetch + re-diff), not a re-pin of the stale SHA, so a blob mismatch can't loop.
2. Source = "main" made _on_load set old_catalog = new_catalog, so diff_catalogs(x, x) → [], and can_sign() treated an empty changeset as vacuously satisfied (set() <= set() is True). This is exactly the path commit b08cf21 used to sign all 19 catalog entries with zero of them reviewed.
   • can_sign() now explicitly refuses an empty changeset, with a comment explaining why a vacuously-satisfied gate is worse than no gate.
   • Source = "main" now diffs against the last catalog a maintainer actually signed — found by walking data/catalog.json's git history until a version verifies against the current data/catalog.json.sig (catalog_review.find_last_signed_catalog_raw / catalog_console.last_signed_catalog_raw) — not against itself.
3. can_sign() now itself checks has_blocking_risk() across every changed entry, so a blocking finding is enforced by the pure gate, not only by the GUI having disabled a checkbox.
4. The acknowledge-gating and the no-bulk-shortcut comment are unchanged (still no acknowledge_all, ever).

Tests

• test_no_acknowledge_all_function_exists (asserted only that no function was named acknowledge_all, and passed the entire time the gate was bypassable) is replaced with test_no_acknowledge_all_shortcut_and_gate_is_real, which also exercises the actual gate.
• Added: empty-changeset refusal, blocking-risk refusal even when acknowledged, sign_precondition resolving against the loaded ref (not "main") with a fake resolver that would fail the test if the ref were hardcoded, sign_precondition refusing on a moved blob, and find_last_signed_catalog_raw (positive + negative).
• Verified every new/changed guard fails when removed: I disabled each of the four can_sign() guards (empty-diff, TOCTOU, blocking-risk, acknowledge) one at a time and re-ran the targeted tests — each failed as expected — then restored and confirmed the full suite goes green again. I also reintroduced the literal hardcoded-"main" bug into sign_precondition and confirmed test_sign_precondition_resolves_against_loaded_ref_not_hardcoded_main catches it (asserts the resolver was never called with "main").

69 tests in tests/test_catalog_review.py (up from 62), all green.

Finding 5 — catalog key and release key were the same CI-resident key

• scripts/sign_checksums.py gets its own RELEASE_PUBKEYS: list[bytes] (separate from bcc_core.CATALOG_PUBKEYS, which this PR does not touch) and a verify_checksums_against_any() helper (rotation-friendly, same "any key in the list" semantics as bcc_core.verify_catalog_signature). Domain-separation prefix b"bcc-release-v1|" is unchanged.
• .github/workflows/release.yml's signing-smoke-test now verifies RELEASE_SIGNING_KEY against RELEASE_PUBKEYS only — it no longer imports bcc_core/CATALOG_PUBKEYS at all, so this workflow structurally cannot compare a CI secret against the catalog's root of trust. Fails closed (loud, explicit message) if RELEASE_PUBKEYS is still empty.
• catalog_console.py: keygen and show-seed-b64 both gain --release. Catalog and release keys are stored under separate keychain entries / filenames (signing_key_catalog.enc / signing_key_release.enc) so they can never be loaded interchangeably. show-seed-b64 refuses to run without --release — this is a structural guard, not just a warning: there is no code path left in this tool that can print the catalog seed for pasting into CI.
• README gets a new "Signing keys" section documenting both keys' trust properties plainly: the catalog key is offline/Console-only and signs what users execute; the release key is CI-resident and signs only checksums; a CI compromise burns the release key, never the catalog key.

What the maintainer needs to do

1. Rotate the catalog key — the current one has been in CI (via the old show-seed-b64/RELEASE_SIGNING_KEY flow) and must be treated as burned for catalog use:
   • python catalog_console.py keygen (no --release) to generate a fresh catalog key.
   • Hand the printed public key to whoever owns bcc_core.py to add to CATALOG_PUBKEYS (out of scope for this PR — that file is owned by a concurrent fix).
   • Re-sign the catalog through the Console once the new key is trusted.
2. Generate a separate release key:
   • python catalog_console.py keygen --release.
   • Paste the printed public key into RELEASE_PUBKEYS in scripts/sign_checksums.py (currently [] — the smoke test fails closed until this is populated) and commit that change.
   • python catalog_console.py show-seed-b64 --release, then set that value as the RELEASE_SIGNING_KEY Gitea Actions secret (replacing whatever is there now, which was the catalog seed).
   • Run the signing-smoke-test job manually (workflow_dispatch) to confirm the secret and RELEASE_PUBKEYS match.
3. Paste the release public key into the README.md placeholder under "Signing keys" (still a placeholder — I did not fabricate a key).

No key was generated or committed by this PR.

Verification

• ruff check . — all checks passed.
• ruff format --check . — all files already formatted.
• pytest tests/ (excluding tests/test_catalog_console_packaging.py's PySide6-free static checks, which also pass) — all green, including the pre-existing tests/test_checksums.py (unaffected by the RELEASE_PUBKEYS addition).
• python3 -m py_compile catalog_console.py catalog_review.py scripts/sign_checksums.py — clean. PySide6 does not import in this sandbox (no libEGL), so catalog_console.py's GUI classes could not be exercised at runtime; the fix therefore lives in the pure, tested seam (catalog_review.sign_precondition/can_sign), and catalog_console.py's GUI-facing changes are py_compile + ruff-clean only, not test-covered directly. The GUI's _on_sign/_on_load are now thin call-throughs to the tested pure functions.
• .github/workflows/release.yml validated with yaml.safe_load (structure) and the embedded Python heredoc block extracted and compile()d separately (syntax).

What I disagreed with / flagged

Nothing in the two findings themselves — both reproduced exactly as described. One adjacent bug I found and fixed while touching _on_sign: the reload-on-failure check used to test for the substring "changed" in the refusal reason, which also matches "Not every changed entry has been acknowledged yet" — so an incomplete-acknowledgement refusal would have incorrectly triggered a full reload (discarding in-progress acknowledgements) under the original code too. Tightened to match only "mismatch", which appears solely in the TOCTOU refusal reason.

Fixes #68 findings 1 and 5 (see #62 for the Console's original spec). Scope, per the task: `catalog_review.py`, `catalog_console.py`, `tests/test_catalog_review.py`, `.github/workflows/release.yml`, `scripts/sign_checksums.py`, `README.md`. Does not touch `bcc_core.py`, `tests/test_core.py`, `.github/workflows/ci.yml`, `bcc.py`, or `data/catalog.json` (owned by a concurrent fix for the other findings in #68). ## Finding 1 — the review gate signed without reviewing anything Two compounding bugs, both fixed: 1. **`ReviewWindow._on_sign` hardcoded `"main"`** as the TOCTOU comparison ref, while `_on_load` pins the **PR head**'s blob SHA when a PR is selected — for any PR that changes the catalog those SHAs differ by definition, so the PR review path could never sign, and the retry path re-pinned the same wrong value forever. - `ReviewSession` now carries `loaded_ref` (the exact ref that was reviewed). - New pure `sign_precondition(session, resolve_blob_sha: Callable[[str], str])` resolves the TOCTOU SHA from `session.loaded_ref`, never a hardcoded `"main"`. `_on_sign` now calls this instead of hand-rolling the hardcoded-main resolution. - The retry path calls `_on_load()` (full re-fetch + re-diff), not a re-pin of the stale SHA, so a blob mismatch can't loop. 2. Source = "main" made `_on_load` set `old_catalog = new_catalog`, so `diff_catalogs(x, x) → []`, and `can_sign()` treated an empty changeset as vacuously satisfied (`set() <= set()` is `True`). This is exactly the path commit `b08cf21` used to sign all 19 catalog entries with zero of them reviewed. - `can_sign()` now explicitly refuses an empty changeset, with a comment explaining why a vacuously-satisfied gate is worse than no gate. - Source = "main" now diffs against **the last catalog a maintainer actually signed** — found by walking `data/catalog.json`'s git history until a version verifies against the current `data/catalog.json.sig` (`catalog_review.find_last_signed_catalog_raw` / `catalog_console.last_signed_catalog_raw`) — not against itself. 3. **`can_sign()` now itself checks `has_blocking_risk()`** across every changed entry, so a blocking finding is enforced by the pure gate, not only by the GUI having disabled a checkbox. 4. The acknowledge-gating and the no-bulk-shortcut comment are unchanged (still no `acknowledge_all`, ever). ### Tests - `test_no_acknowledge_all_function_exists` (asserted only that no function was *named* `acknowledge_all`, and passed the entire time the gate was bypassable) is replaced with `test_no_acknowledge_all_shortcut_and_gate_is_real`, which also exercises the actual gate. - Added: empty-changeset refusal, blocking-risk refusal even when acknowledged, `sign_precondition` resolving against the loaded ref (not `"main"`) with a fake resolver that would fail the test if the ref were hardcoded, `sign_precondition` refusing on a moved blob, and `find_last_signed_catalog_raw` (positive + negative). - **Verified every new/changed guard fails when removed:** I disabled each of the four `can_sign()` guards (empty-diff, TOCTOU, blocking-risk, acknowledge) one at a time and re-ran the targeted tests — each failed as expected — then restored and confirmed the full suite goes green again. I also reintroduced the literal hardcoded-`"main"` bug into `sign_precondition` and confirmed `test_sign_precondition_resolves_against_loaded_ref_not_hardcoded_main` catches it (asserts the resolver was never called with `"main"`). 69 tests in `tests/test_catalog_review.py` (up from 62), all green. ## Finding 5 — catalog key and release key were the same CI-resident key - `scripts/sign_checksums.py` gets its own `RELEASE_PUBKEYS: list[bytes]` (separate from `bcc_core.CATALOG_PUBKEYS`, which this PR does not touch) and a `verify_checksums_against_any()` helper (rotation-friendly, same "any key in the list" semantics as `bcc_core.verify_catalog_signature`). Domain-separation prefix `b"bcc-release-v1|"` is unchanged. - `.github/workflows/release.yml`'s `signing-smoke-test` now verifies `RELEASE_SIGNING_KEY` against `RELEASE_PUBKEYS` only — it no longer imports `bcc_core`/`CATALOG_PUBKEYS` at all, so this workflow structurally cannot compare a CI secret against the catalog's root of trust. Fails closed (loud, explicit message) if `RELEASE_PUBKEYS` is still empty. - `catalog_console.py`: `keygen` and `show-seed-b64` both gain `--release`. Catalog and release keys are stored under separate keychain entries / filenames (`signing_key_catalog.enc` / `signing_key_release.enc`) so they can never be loaded interchangeably. **`show-seed-b64` refuses to run without `--release`** — this is a structural guard, not just a warning: there is no code path left in this tool that can print the catalog seed for pasting into CI. - README gets a new "Signing keys" section documenting both keys' trust properties plainly: the catalog key is offline/Console-only and signs what users execute; the release key is CI-resident and signs only checksums; a CI compromise burns the release key, never the catalog key. ### What the maintainer needs to do 1. **Rotate the catalog key** — the current one has been in CI (via the old `show-seed-b64`/`RELEASE_SIGNING_KEY` flow) and must be treated as burned for catalog use: - `python catalog_console.py keygen` (no `--release`) to generate a fresh catalog key. - Hand the printed public key to whoever owns `bcc_core.py` to add to `CATALOG_PUBKEYS` (out of scope for this PR — that file is owned by a concurrent fix). - Re-sign the catalog through the Console once the new key is trusted. 2. **Generate a separate release key**: - `python catalog_console.py keygen --release`. - Paste the printed public key into `RELEASE_PUBKEYS` in `scripts/sign_checksums.py` (currently `[]` — the smoke test fails closed until this is populated) and commit that change. - `python catalog_console.py show-seed-b64 --release`, then set that value as the `RELEASE_SIGNING_KEY` Gitea Actions secret (replacing whatever is there now, which was the catalog seed). - Run the `signing-smoke-test` job manually (`workflow_dispatch`) to confirm the secret and `RELEASE_PUBKEYS` match. 3. Paste the release public key into the `README.md` placeholder under "Signing keys" (still a placeholder — I did not fabricate a key). No key was generated or committed by this PR. ## Verification - `ruff check .` — all checks passed. - `ruff format --check .` — all files already formatted. - `pytest tests/` (excluding `tests/test_catalog_console_packaging.py`'s PySide6-free static checks, which also pass) — all green, including the pre-existing `tests/test_checksums.py` (unaffected by the `RELEASE_PUBKEYS` addition). - `python3 -m py_compile catalog_console.py catalog_review.py scripts/sign_checksums.py` — clean. **PySide6 does not import in this sandbox (no libEGL)**, so `catalog_console.py`'s GUI classes could not be exercised at runtime; the fix therefore lives in the pure, tested seam (`catalog_review.sign_precondition`/`can_sign`), and `catalog_console.py`'s GUI-facing changes are `py_compile` + `ruff`-clean only, not test-covered directly. The GUI's `_on_sign`/`_on_load` are now thin call-throughs to the tested pure functions. - `.github/workflows/release.yml` validated with `yaml.safe_load` (structure) and the embedded Python heredoc block extracted and `compile()`d separately (syntax). ## What I disagreed with / flagged Nothing in the two findings themselves — both reproduced exactly as described. One adjacent bug I found and fixed while touching `_on_sign`: the reload-on-failure check used to test for the substring `"changed"` in the refusal reason, which also matches "Not every **changed** entry has been acknowledged yet" — so an incomplete-acknowledgement refusal would have incorrectly triggered a full reload (discarding in-progress acknowledgements) under the original code too. Tightened to match only `"mismatch"`, which appears solely in the TOCTOU refusal reason.

the_og added 1 commit 2026-07-12 21:26:12 -04:00

fix(catalog-console): close the vacuous review gate; split catalog/release signing keys ... 82483e693d

Fixes #68 findings 1 and 5.

Finding 1 -- the review gate signed without reviewing anything:
- ReviewWindow._on_sign hardcoded "main" as the TOCTOU comparison ref, so
  any PR review (where _on_load pins the PR head's blob SHA) could never
  sign; the only working path was main-vs-itself, whose empty diff made
  can_sign() vacuously True (set() <= set()). Commit b08cf21 signed 19
  entries through exactly that path with zero of them reviewed.
- can_sign() now refuses an empty changeset outright, and itself checks
  has_blocking_risk() across every changed entry rather than trusting the
  GUI to have disabled a checkbox.
- ReviewSession now carries loaded_ref (the exact ref reviewed); a new pure
  sign_precondition(session, resolve_blob_sha) resolves the TOCTOU SHA from
  that ref, never a hardcoded "main". _on_load's retry path re-diffs
  instead of re-pinning the same stale SHA, so a blob-mismatch refusal
  can't loop forever.
- source="main" now diffs against the last catalog a maintainer actually
  SIGNED (walking catalog.json's git history until a version verifies
  against the current .sig), not against itself.
- Replaced the theatre-only test_no_acknowledge_all_function_exists (only
  asserted no function was *named* acknowledge_all) with a test that also
  exercises the real gate. Added can_sign/sign_precondition coverage for
  the empty-diff, blocking-risk, and ref-resolution seams -- each verified
  to fail when its guard is removed.

Finding 5 -- the catalog key and release key were the same CI-resident key:
- scripts/sign_checksums.py gets its own RELEASE_PUBKEYS (separate from
  bcc_core.CATALOG_PUBKEYS) and a verify_checksums_against_any() helper.
- release.yml's signing-smoke-test now verifies RELEASE_SIGNING_KEY against
  RELEASE_PUBKEYS only -- it no longer imports bcc_core/CATALOG_PUBKEYS at
  all, so this workflow can never compare a CI secret against the
  catalog's root of trust.
- catalog_console.py: keygen/show-seed-b64 gain --release, with separate
  keychain/file storage per key kind. show-seed-b64 refuses to run without
  --release, so the catalog seed can't be exported to a CI secret by habit.
- README documents both keys' trust properties and the asymmetry: a CI
  compromise burns the release key, never the catalog key.

The maintainer must rotate the catalog key (it was CI-resident, so treat it
as burned for catalog use) and generate a fresh release key -- see the PR
description for the exact steps. No key is generated or committed here.

the_og added 1 commit 2026-07-12 21:28:29 -04:00

Merge branch 'main' into fix/68-console-gate 26c66b7db1

the_og merged commit cd2ac2f6f8 into main 2026-07-12 21:30:13 -04:00

the_og deleted branch fix/68-console-gate 2026-07-12 21:30:13 -04:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0

P1

P2

blocked

Waiting on an external thing before it can move.

decision

A decision record (D-007 style) — settled call, referenceable by number from any repo.

needs-decision

Waiting on AJ's call.

next

Current focus / next actionable item.

status

The single pinned STATUS issue for this repo — live state, next action, open items, decisions needed.

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

the_og

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: the_og/better-claude-config#69