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:
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.
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.
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.
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 namedacknowledge_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
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.
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.
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.
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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 touchbcc_core.py,tests/test_core.py,.github/workflows/ci.yml,bcc.py, ordata/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:
ReviewWindow._on_signhardcoded"main"as the TOCTOU comparison ref, while_on_loadpins 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.ReviewSessionnow carriesloaded_ref(the exact ref that was reviewed).sign_precondition(session, resolve_blob_sha: Callable[[str], str])resolves the TOCTOU SHA fromsession.loaded_ref, never a hardcoded"main"._on_signnow calls this instead of hand-rolling the hardcoded-main resolution._on_load()(full re-fetch + re-diff), not a re-pin of the stale SHA, so a blob mismatch can't loop._on_loadsetold_catalog = new_catalog, sodiff_catalogs(x, x) → [], andcan_sign()treated an empty changeset as vacuously satisfied (set() <= set()isTrue). This is exactly the path commitb08cf21used 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.data/catalog.json's git history until a version verifies against the currentdata/catalog.json.sig(catalog_review.find_last_signed_catalog_raw/catalog_console.last_signed_catalog_raw) — not against itself.can_sign()now itself checkshas_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.acknowledge_all, ever).Tests
test_no_acknowledge_all_function_exists(asserted only that no function was namedacknowledge_all, and passed the entire time the gate was bypassable) is replaced withtest_no_acknowledge_all_shortcut_and_gate_is_real, which also exercises the actual gate.sign_preconditionresolving against the loaded ref (not"main") with a fake resolver that would fail the test if the ref were hardcoded,sign_preconditionrefusing on a moved blob, andfind_last_signed_catalog_raw(positive + negative).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 intosign_preconditionand confirmedtest_sign_precondition_resolves_against_loaded_ref_not_hardcoded_maincatches 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.pygets its ownRELEASE_PUBKEYS: list[bytes](separate frombcc_core.CATALOG_PUBKEYS, which this PR does not touch) and averify_checksums_against_any()helper (rotation-friendly, same "any key in the list" semantics asbcc_core.verify_catalog_signature). Domain-separation prefixb"bcc-release-v1|"is unchanged..github/workflows/release.yml'ssigning-smoke-testnow verifiesRELEASE_SIGNING_KEYagainstRELEASE_PUBKEYSonly — it no longer importsbcc_core/CATALOG_PUBKEYSat all, so this workflow structurally cannot compare a CI secret against the catalog's root of trust. Fails closed (loud, explicit message) ifRELEASE_PUBKEYSis still empty.catalog_console.py:keygenandshow-seed-b64both 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-b64refuses 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.What the maintainer needs to do
show-seed-b64/RELEASE_SIGNING_KEYflow) and must be treated as burned for catalog use:python catalog_console.py keygen(no--release) to generate a fresh catalog key.bcc_core.pyto add toCATALOG_PUBKEYS(out of scope for this PR — that file is owned by a concurrent fix).python catalog_console.py keygen --release.RELEASE_PUBKEYSinscripts/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 theRELEASE_SIGNING_KEYGitea Actions secret (replacing whatever is there now, which was the catalog seed).signing-smoke-testjob manually (workflow_dispatch) to confirm the secret andRELEASE_PUBKEYSmatch.README.mdplaceholder 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/(excludingtests/test_catalog_console_packaging.py's PySide6-free static checks, which also pass) — all green, including the pre-existingtests/test_checksums.py(unaffected by theRELEASE_PUBKEYSaddition).python3 -m py_compile catalog_console.py catalog_review.py scripts/sign_checksums.py— clean. PySide6 does not import in this sandbox (no libEGL), socatalog_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), andcatalog_console.py's GUI-facing changes arepy_compile+ruff-clean only, not test-covered directly. The GUI's_on_sign/_on_loadare now thin call-throughs to the tested pure functions..github/workflows/release.ymlvalidated withyaml.safe_load(structure) and the embedded Python heredoc block extracted andcompile()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.