Skip to content

Monitor release identities and published signed digests - #168

Draft
gontzess wants to merge 4 commits into
mainfrom
steve.gontzes/rg22-rekor-monitor
Draft

gontzess wants to merge 4 commits into
mainfrom
steve.gontzes/rg22-rekor-monitor

Conversation

@gontzess

@gontzess gontzess commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Why

Watch new signatures under the three release-workflow identities continuously, and distinguish a real workflow run from authorization of the bytes that were signed.

What

  • Add an hourly, read-only, main-only cron with a serialized checkpoint/reconciliation artifact and an explicit first-run bootstrap. Missing state fails closed rather than silently restarting coverage.
  • Install upstream rekor_monitor directly at an exact Go pseudo-version; the reusable workflow exports only a checkpoint artifact, not matched entries. Use the actual YAML output keys (outputIdentities, outputIdentitiesFormat) and the explicit v1 endpoint.
  • Upstream validates signed checkpoints and consistency, not inclusion of matched entries. Add a small isolated Go helper using upstream Sigstore libraries to authenticate each fetched entry's RFC 6962 inclusion, UUID/body binding, signed timestamp, proof checkpoint and consistency against the saved view. Proof checkpoints can be newer than the saved root; a plain Python root comparison would not suffice.
  • Supply reviewed public Fulcio root/intermediate PEMs to upstream and the helper. Non-chaining certificates are informational UNAUTHENTICATED-SAN diagnostics, never identity matches.
  • Keep the reconciliation glue stdlib-only Python: certificate repository/tag → public catalog index → connector index → exact release manifest. Compare literal signed-byte SHA256 to archive/checksum hashes or the published manifest bytes, not to an unrelated payload hash.
  • Emit separate run-identity-match and artifact-authorized verdicts. Check the real concluded run, pinned reusable workflow, caller SHA and plausible signature time. Pending runs/publication reconcile for six hours across invocations.
  • Fail if an unchanged checkpoint origin/size/root is observed for more than two hours. A new note signature alone does not reset its first-observed clock.
  • Alert delivery is intentionally a placeholder naming #ops-testing-new-alerts. No webhook, production routing, live signing or canary entry is added.

Bounded UNKNOWN semantics

DSSE/intoto payloads, hidden catalogs (including HTTP 403), unknown storage aliases and missing/non-joinable extensions stay UNKNOWN. A private run that is inaccessible with this repository token also stays UNKNOWN; no access expansion is attempted.

Hashedrekord OCI signatures contain a simple-signing-payload hash, not the image digest. On an image-capable release, at most N distinct unrecorded hashes remain UNKNOWN after the six-hour window; N is the count of public-ECR image indexes (isIndex: true). Private Lambda images do not grant budget. The next distinct unrecorded hash is an anomaly. The manifest has no explicit signed flag today; this predicate encodes the current signed-image shape and must be revisited if that shape/schema changes. Artifact-only releases have zero budget, so a valid-run/unrecorded digest is an anomaly. The budget persists across runs.

Tool and trust-material provenance

  • Upstream commit: 47017b2379cb3452271ce0cc1e33d05f61c5e275.
  • Module: github.com/sigstore/rekor-monitor v0.0.0-20261019132539-47017b2379cb.
  • Module sum: h1:+ICdEtSt8tWgvUR8YdwoMOavuNf2yWEqH5FXSpxVdhI=; go.mod sum: h1:b8wU/k2HXf4mHROlRsUh/Mdd5+7bfSosG67J4GSRHLw=. Installation uses GOTOOLCHAIN=local, GOFLAGS=-mod=readonly, GOSUMDB=sum.golang.org; the helper has its own checked-in dependency sums.
  • Fulcio PEMs extracted after a verified public-good Sigstore Go TUF refresh: snapshot 166, targets 14, root 16.
  • Verified trusted_root.json SHA256: 6494e21ea73fa7ee769f85f57d5a3e6a08725eae1e38c755fc3517c9e6bc0b66.
  • Committed roots (2 certs) SHA256: d543eb6e067c069c6b2fd2c22258452db2f923b221597303a6bce4c664209ee6.
  • Committed intermediates (1 cert) SHA256: ac5a60ca80f473f8b7742111f9eb49bf7dbd8f03f41a5b39d58b62b6f0de4d0f.

Evidence

  • Artifact download failures fail closed immediately without restoring or uploading older history; the next scheduled run retries the newest artifact. Only successfully downloaded artifacts with missing or empty required files emit a warning and fall through to an older candidate. Fixtures verify a single attempted download on transient failure, no upload-ready state, and warned fallback for incomplete snapshots.
  • Candidate artifacts are staged in isolated temporary directories and accepted only when checkpoint.txt and reconciliation.json are both non-empty; incomplete candidates fall through to older artifacts. An always-run completeness step gates uploads on the same non-empty-file requirement, so partial initial state cannot perpetuate an incomplete artifact. Synthetic fixtures cover missing-checkpoint and missing-reconciliation fallback plus the guarded upload condition.
  • State restore selects the newest non-expired rekor-monitor-state artifact from a main-branch run, not the newest completed run, so artifact-less (cancelled/early-failed) runs cannot wedge the monitor. The selected run is also checked against this workflow's ID before download.
  • make test-scripts: PASS (native PowerShell fixture unavailable/skipped).
  • go -C tools/rekor-entry test ./...: PASS; synthetic signed checkpoint/inclusion/SET and certificate-chain controls, including altered body/index/root/checkpoint/timestamp/UUID, rejected consistency and untrusted SAN.
  • go -C tools/rekor-entry vet ./..., go -C tools/rekor-entry mod verify and helper build: PASS.
  • Synthetic Python fixtures: missing run, valid-run unexpected digest, pending→resolved/expired, repeated old checkpoint, bounded image-hash budget (N UNKNOWN / N+1 anomaly), hidden data, opaque payloads, token-host/redirect boundary, persisted state/helper failure and mutable-pin negative controls.
  • Exact upstream go install and rekor_monitor --help: PASS.
  • Read-only real smoke: pinned upstream binary with the committed Fulcio configuration fetched/verified its first public Rekor checkpoint and explicitly reported no historical scan. No release/signing or alert was executed.
  • Read-only real helper smoke: fetched one existing public DSSE entry, verified its body/inclusion/SET and consistency from the saved checkpoint to the newer proof checkpoint, then emitted kind: dsse with no artifact digest or identity authorization (correctly opaque/UNKNOWN). This did not create a signature or claim a watched-release match.
  • Offline openssl verify of committed intermediate against committed roots: PASS.
  • YAML parse and git diff --check: PASS.
  • Repository ShellCheck invocation unavailable: shellcheck is not installed locally. CI retains its ShellCheck job.

Rollout stops and low-priority follow-ups

Draft only. Before rollout, review the pins/roots, initialize on main, observe real fleet and axiomatic canary-release entries, then wire and exercise the test-channel delivery before considering production alerts. No deployed behavior or live run→digest match is claimed by synthetic tests.

Follow-ups: (1) a dedicated-identity scheduled canary to close the initial-view/liveness gap; (2) compare DSSE payload hashes against payload bytes in published provenance/SBOM bundles; (3) scheduled reviewed TUF root refresh; (4) index OCI simple-signing payload hashes. Until (4), an attacker replay inside an image release's N budget remains undetected. An initial checkpoint's currency is unproven; a quiet log and a stalled feed are indistinguishable at first observation. A replay over already-recorded bytes remains indistinguishable. This monitor is a detection aid over published release data, not registry authorization enforcement.

TUF metadata recheck

The published timestamp metadata advertises snapshot 166; 166.snapshot.json has signed version 166 and targets version 14; 16.root.json has signed root-role version 16. These confirm the extraction provenance above; no version correction or trust-material change is needed. The root-role version is read from the root metadata itself, not the snapshot meta entry named root.json.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit d2022ba6459e

General PR Review: Monitor release identities and published signed digests

Blocking Issues: 1 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ad85b5b02764.
Review mode: full
View review run

Review Summary

This PR adds a new hourly workflow on main that runs the pinned upstream rekor_monitor against three release-workflow identities. A new Go helper (tools/rekor-entry) then re-checks each matched entry's UUID/body binding, inclusion proof, signed timestamp and consistency with the saved checkpoint. Its certificate check uses Fulcio PEMs committed to the repo. A stdlib-only Python reconciler checks the GitHub run and matches each signed SHA256 to the published dist.conductorone.com manifest. State is carried between runs in a workflow artifact. CI and the Makefile now run the helper's vet/test and the new Python fixtures. I scanned the full diff for security and correctness. Workflow permissions are read-only, the token is only sent to api.github.com, redirects are refused, and lookups are limited to two allowed hosts. No repo-local criteria loaded, so only the base criteria were applied. Coverage gap: the sigstore module sources were not in the local module cache, so I could not confirm that verify.VerifyLogEntry (rekor v1.5.3) checks the proof checkpoint's signature itself. The helper verifies saved.Verify(verifier) directly but not checkpoint.Verify(...), and relies on ProveConsistency for the newer checkpoint. go.sum was not audited line by line.

Security Issues

None found.

Correctness Issues

  • New .github/workflows/rekor-monitor.yaml:56-60: state restore picks the latest run with --status completed, whatever its conclusion. Some completed runs never upload rekor-monitor-state: runs that fail before monitor-state/ has files (if-no-files-found: error), runs where gh run download fails transiently, and queued runs cancelled by the concurrency group. The next run then fails to download, uploads nothing itself, and every later run fails permanently.

Suggestions

  • New (medium confidence) scripts/rekor_digest_check.py:206-210: MONITOR-ERROR rows are retried with no time limit. A deterministic helper error (for example a sha512 hashedrekord, or an identity mismatch) will fail the job every hour until someone edits the state artifact by hand.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `.github/workflows/rekor-monitor.yaml`:
- Around line 56-60: `gh run list --status completed --limit 1` can select a cancelled, skipped or early-failed run that has no `rekor-monitor-state` artifact. `gh run download` then fails, the run uploads nothing, and the monitor stays wedged. Instead, find the newest non-expired `rekor-monitor-state` artifact from a main-branch run of this workflow (for example `gh api repos/$GITHUB_REPOSITORY/actions/artifacts?name=rekor-monitor-state`, filtered on `workflow_run.head_branch == "main"` and `expired == false`, sorted by `created_at`), and download that run's artifact. Keep the fail-closed bootstrap behavior when none exists.

## Suggestions

In `scripts/rekor_digest_check.py`:
- Around line 206-210: MONITOR-ERROR rows are added to `retry` without the `WINDOW` bound used for UNKNOWN rows. Track first_seen for MONITOR-ERROR too, and stop retrying once it has been alerted for longer than WINDOW (or record it as a terminal anomaly), so a deterministic helper error does not fail every hourly run forever.

Comment thread .github/workflows/rekor-monitor.yaml Outdated
Comment on lines +56 to +60
run_id="$(gh run list --repo "$GITHUB_REPOSITORY" --workflow rekor-monitor.yaml \
--branch main --status completed --limit 1 --json databaseId --jq '.[0].databaseId // empty')"
if [ -n "$run_id" ]; then
gh run download "$run_id" --repo "$GITHUB_REPOSITORY" \
--name rekor-monitor-state --dir monitor-state

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Bug (high confidence): --status completed also matches cancelled, skipped and failed runs, including runs that never uploaded rekor-monitor-state. That happens if a run fails before monitor-state/ has files (for example in ca-trust-guard, locked-tools, or a transient gh run download error, where if-no-files-found: error drops the upload), or if a queued run is cancelled by the concurrency group. The next run then picks that run, gh run download fails, and it uploads nothing either, so every later run fails the same way. Select the most recent run that actually has the artifact instead, for example gh api repos/$REPO/actions/artifacts?name=rekor-monitor-state filtered to workflow_run.head_branch == "main" and not expired.

Comment on lines +206 to +210
retry = {row["uuid"] for row in rows if row["status"] in {"PENDING", "MONITOR-ERROR"}
or (row["status"] == "UNKNOWN" and "unavailable" in row["reason"]
and now - state.get("first_seen", {}).get(row["uuid"], now) < WINDOW)}
state["pending"] = [item for uuid, item in observations.items() if uuid in retry]
state["first_seen"] = {key: value for key, value in state.get("first_seen", {}).items() if key in retry}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion (medium confidence): MONITOR-ERROR rows are retried with no window, unlike PENDING/UNKNOWN. A deterministic helper error, such as a watched-identity hashedrekord using sha512 (unsupported signed digest) or certificate identity differs from observation, will be re-fetched and fail the job every hour. It can only be cleared by hand-editing the artifact state. Consider alerting once and then expiring it, or recording it as a terminal anomaly, after WINDOW.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit 8f6e8616deed

General PR Review: Monitor release identities and published signed digests

Blocking Issues: 1 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ad85b5b02764.
Review mode: full
View review run

Review Summary

This PR adds a new hourly workflow on main that runs the pinned upstream rekor_monitor against three release-workflow identities. A new Go helper (tools/rekor-entry) then re-checks each matched entry's UUID/body binding, inclusion proof, signed timestamp and consistency with the saved checkpoint. Its certificate check uses Fulcio PEMs committed to the repo. A stdlib-only Python reconciler checks the GitHub run and matches each signed SHA256 to the published dist.conductorone.com manifest. State is carried between runs in a workflow artifact. CI and the Makefile now run the helper's vet/test and the new Python fixtures. I scanned the full diff for security and correctness. Workflow permissions are read-only, the token is only sent to api.github.com, redirects are refused, and lookups are limited to two allowed hosts. No repo-local criteria loaded, so only the base criteria were applied. Coverage gap: the sigstore module sources were not in the local module cache, so I could not confirm that verify.VerifyLogEntry (rekor v1.5.3) checks the proof checkpoint's signature itself. The helper verifies saved.Verify(verifier) directly but not checkpoint.Verify(...), and relies on ProveConsistency for the newer checkpoint. go.sum was not audited line by line.

Security Issues

None found.

Correctness Issues

  • New .github/workflows/rekor-monitor.yaml:56-60: state restore picks the latest run with --status completed, whatever its conclusion. Some completed runs never upload rekor-monitor-state: runs that fail before monitor-state/ has files (if-no-files-found: error), runs where gh run download fails transiently, and queued runs cancelled by the concurrency group. The next run then fails to download, uploads nothing itself, and every later run fails permanently.

Suggestions

  • New (medium confidence) scripts/rekor_digest_check.py:206-210: MONITOR-ERROR rows are retried with no time limit. A deterministic helper error (for example a sha512 hashedrekord, or an identity mismatch) will fail the job every hour until someone edits the state artifact by hand.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `.github/workflows/rekor-monitor.yaml`:
- Around line 56-60: `gh run list --status completed --limit 1` can select a cancelled, skipped or early-failed run that has no `rekor-monitor-state` artifact. `gh run download` then fails, the run uploads nothing, and the monitor stays wedged. Instead, find the newest non-expired `rekor-monitor-state` artifact from a main-branch run of this workflow (for example `gh api repos/$GITHUB_REPOSITORY/actions/artifacts?name=rekor-monitor-state`, filtered on `workflow_run.head_branch == "main"` and `expired == false`, sorted by `created_at`), and download that run's artifact. Keep the fail-closed bootstrap behavior when none exists.

## Suggestions

In `scripts/rekor_digest_check.py`:
- Around line 206-210: MONITOR-ERROR rows are added to `retry` without the `WINDOW` bound used for UNKNOWN rows. Track first_seen for MONITOR-ERROR too, and stop retrying once it has been alerted for longer than WINDOW (or record it as a terminal anomaly), so a deterministic helper error does not fail every hourly run forever.

Reviewed commit: d2022ba6459e

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see the full review report

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit 8f6e8616deed

General PR Review: Monitor release identities and published signed digests

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ad85b5b02764.
Review mode: incremental since d2022ba
View review run

Review Summary

The new commit changes how state is restored. Before, the workflow took the latest completed run from gh run list. Now it lists every non-expired rekor-monitor-state artifact from main, sorts them newest first, and takes the first one whose run belongs to this workflow (checked by workflow_id). The bootstrap path still fails closed when no state exists. The commit also updates the README and adds a fake-gh fixture test that covers cancelled and early-failed runs, expired artifacts, other branches, other workflows, and pagination. I scanned the full PR diff for security and correctness, not just the new commit. No repo-local criteria loaded, so I applied only the base criteria.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • New (medium confidence) .github/workflows/rekor-monitor.yaml:61-73: the step is named "Restore newest valid monitor state", but it never checks the artifact's contents before choosing it. The upload step runs on always(), so a failed bootstrap can upload an artifact with only diagnostics.log, or one without reconciliation.json. After that, every run downloads it, fails test -s, and uploads the same incomplete files again. The monitor stays stuck until someone deletes the artifact by hand.
  • Prior — still present (medium confidence) scripts/rekor_digest_check.py:206: MONITOR-ERROR rows still go into retry with no WINDOW limit; only UNKNOWN/unavailable rows are time-bounded. A deterministic helper error will fail the job every hour indefinitely.

Resolved prior findings

  • Fixed: state restore picking an artifact-less completed run (old lines 56-60). .github/workflows/rekor-monitor.yaml:56-68 now selects only non-expired rekor-monitor-state artifacts from main runs of this workflow, so cancelled or early-failed runs without an artifact are skipped. scripts/test-rekor-monitor.py covers this, including a negative control that runs the old selection logic.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/workflows/rekor-monitor.yaml`:
- Around line 61-73: The restore loop picks the newest artifact without checking that it has both checkpoint.txt and reconciliation.json. A failed bootstrap run uploads an incomplete artifact (the upload step runs on always()), and the monitor then stays stuck permanently. Fix: for each candidate, download into a temp dir and accept it only if both files are non-empty; otherwise move on to the next older candidate. Alternatively, guard the upload step so it runs only when both files exist in monitor-state/.

In `scripts/rekor_digest_check.py`:
- Around line 206-210: MONITOR-ERROR rows are retried with no time limit. Record first_seen for them and stop retrying once now - first_seen >= WINDOW (or record a terminal anomaly), so a deterministic helper error does not fail every hourly run forever.

Comment thread .github/workflows/rekor-monitor.yaml Outdated
Comment on lines +61 to +73
while IFS= read -r candidate; do
[ -n "$candidate" ] || continue
candidate_workflow="$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/$candidate" --jq '.workflow_id')"
if [ "$candidate_workflow" = "$workflow_id" ]; then
run_id="$candidate"
break
fi
done <<< "$candidates"
if [ -n "$run_id" ]; then
gh run download "$run_id" --repo "$GITHUB_REPOSITORY" \
--name rekor-monitor-state --dir monitor-state
test -s monitor-state/checkpoint.txt
test -s monitor-state/reconciliation.json

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion (medium confidence): this picks the newest artifact, but nothing checks that its contents are complete before it wins. A failed bootstrap can upload an artifact with only diagnostics.log (rekor_monitor failed) or with no reconciliation.json (Python failed), because the upload step runs on always(). Every later run downloads it, fails test -s, and uploads the same incomplete files again, so the monitor stays stuck until someone deletes the artifact by hand. Either download each candidate into a temp dir and fall back to the next one when either file is missing, or skip the upload unless both files exist.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit be16700e7755

General PR Review: Monitor release identities and published signed digests

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ad85b5b02764.
Review mode: incremental since d2022ba
View review run

Review Summary

The new commit changes how state is restored. Before, the workflow took the latest completed run from gh run list. Now it lists every non-expired rekor-monitor-state artifact from main, sorts them newest first, and takes the first one whose run belongs to this workflow (checked by workflow_id). The bootstrap path still fails closed when no state exists. The commit also updates the README and adds a fake-gh fixture test that covers cancelled and early-failed runs, expired artifacts, other branches, other workflows, and pagination. I scanned the full PR diff for security and correctness, not just the new commit. No repo-local criteria loaded, so I applied only the base criteria.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • New (medium confidence) .github/workflows/rekor-monitor.yaml:61-73: the step is named "Restore newest valid monitor state", but it never checks the artifact's contents before choosing it. The upload step runs on always(), so a failed bootstrap can upload an artifact with only diagnostics.log, or one without reconciliation.json. After that, every run downloads it, fails test -s, and uploads the same incomplete files again. The monitor stays stuck until someone deletes the artifact by hand.
  • Prior — still present (medium confidence) scripts/rekor_digest_check.py:206: MONITOR-ERROR rows still go into retry with no WINDOW limit; only UNKNOWN/unavailable rows are time-bounded. A deterministic helper error will fail the job every hour indefinitely.

Resolved prior findings

  • Fixed: state restore picking an artifact-less completed run (old lines 56-60). .github/workflows/rekor-monitor.yaml:56-68 now selects only non-expired rekor-monitor-state artifacts from main runs of this workflow, so cancelled or early-failed runs without an artifact are skipped. scripts/test-rekor-monitor.py covers this, including a negative control that runs the old selection logic.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/workflows/rekor-monitor.yaml`:
- Around line 61-73: The restore loop picks the newest artifact without checking that it has both checkpoint.txt and reconciliation.json. A failed bootstrap run uploads an incomplete artifact (the upload step runs on always()), and the monitor then stays stuck permanently. Fix: for each candidate, download into a temp dir and accept it only if both files are non-empty; otherwise move on to the next older candidate. Alternatively, guard the upload step so it runs only when both files exist in monitor-state/.

In `scripts/rekor_digest_check.py`:
- Around line 206-210: MONITOR-ERROR rows are retried with no time limit. Record first_seen for them and stop retrying once now - first_seen >= WINDOW (or record a terminal anomaly), so a deterministic helper error does not fail every hourly run forever.

Reviewed commit: 8f6e8616deed

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found — see the full review report

@github-actions
github-actions Bot dismissed their stale review October 22, 2026 15:34

Superseded by a completed review with no blocking issues; current audit evidence persisted in CI.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit be16700e7755

General PR Review: Monitor release identities and published signed digests

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ad85b5b02764.
Review mode: incremental since 8f6e861
View review run

Review Summary

The new commit makes state restore more robust. Each candidate rekor-monitor-state artifact is now downloaded into its own mktemp directory, and it is used only if both checkpoint.txt and reconciliation.json are non-empty. Otherwise the step moves on to the next older artifact, and an EXIT trap cleans up the temporary directory. The commit also adds an always-run completeness check, so state is uploaded only when both files exist. The README and the Python fixture tests were updated to match, including negative controls. I scanned the full PR diff for security and correctness and found nothing new that blocks merge. No repo-local criteria loaded, so only the base criteria were applied. The incremental artifact was complete, with no dropped paths and no truncation.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • Prior — still present (medium confidence) scripts/rekor_digest_check.py:206-208: MONITOR-ERROR rows are still added to retry with no WINDOW limit. Only UNKNOWN/unavailable rows have a time limit. If the helper fails the same way every time, the job will fail every hour with no end.
  • New (medium confidence) .github/workflows/rekor-monitor.yaml:68-79: if the newest candidate fails because gh run download hit a temporary error (not because its files are incomplete), the step quietly falls back to an older artifact and writes no log line. The older reconciliation state (pending entries, first_seen, the persisted image-hash budget, checkpoint first_observed) is then restored and uploaded again as the newest. That can undo budget consumption without anyone noticing. A ::warning:: for each skipped candidate would help, and so would failing outright on download errors instead of only on incomplete contents.

Resolved prior findings

  • Fixed: restore choosing an incomplete artifact and perpetuating it. Restore now requires both files to be non-empty before it accepts a candidate (.github/workflows/rekor-monitor.yaml:67-79). Upload now depends on steps.state.outputs.state_complete == 'true' (.github/workflows/rekor-monitor.yaml:114-123). Both are covered by scripts/test-rekor-monitor.py (missing_checkpoint/missing_reconciliation cases and test_upload_requires_both_nonempty_state_files).
  • Fixed (previously confirmed): artifact-less completed runs could wedge restore. Selection still filters on non-expired main artifacts from this workflow (.github/workflows/rekor-monitor.yaml:56-66).
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `scripts/rekor_digest_check.py`:
- Around line 206-208: MONITOR-ERROR rows are retried with no time limit. Apply the same `now - first_seen < WINDOW` bound used for UNKNOWN/unavailable rows, and make sure `first_seen` is recorded for MONITOR-ERROR rows, so a helper that keeps failing the same way expires instead of failing the job every hour forever.

In `.github/workflows/rekor-monitor.yaml`:
- Around line 68-79: When a candidate artifact is skipped, emit `::warning::` with the run ID and the reason (download failure vs. missing/empty checkpoint.txt or reconciliation.json). Consider failing the step when `gh run download` itself fails, so a temporary error cannot roll the monitor back to older reconciliation state (budget, pending, first_seen) that is then re-uploaded as the newest.

Comment thread .github/workflows/rekor-monitor.yaml Outdated
Comment on lines +68 to +79
if gh run download "$candidate" --repo "$GITHUB_REPOSITORY" \
--name rekor-monitor-state --dir "$candidate_dir" &&
test -s "$candidate_dir/checkpoint.txt" &&
test -s "$candidate_dir/reconciliation.json"; then
rmdir monitor-state
mv "$candidate_dir" monitor-state
candidate_dir=""
run_id="$candidate"
break
fi
rm -rf "$candidate_dir"
candidate_dir=""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion (medium confidence): a temporary gh run download failure on the newest artifact now silently falls back to an older one with no log line. That restores an older reconciliation.json (pending entries, first_seen, digest budget, checkpoint first_observed), and that older state is then uploaded as the newest. Consider emitting ::warning:: with the skipped run ID, and failing outright when the download errors, as opposed to when it succeeds but the files are incomplete.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit 2d4ffcee1a69

General PR Review: Monitor release identities and published signed digests

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ad85b5b02764.
Review mode: incremental since 8f6e861
View review run

Review Summary

The new commit makes state restore more robust. Each candidate rekor-monitor-state artifact is now downloaded into its own mktemp directory, and it is used only if both checkpoint.txt and reconciliation.json are non-empty. Otherwise the step moves on to the next older artifact, and an EXIT trap cleans up the temporary directory. The commit also adds an always-run completeness check, so state is uploaded only when both files exist. The README and the Python fixture tests were updated to match, including negative controls. I scanned the full PR diff for security and correctness and found nothing new that blocks merge. No repo-local criteria loaded, so only the base criteria were applied. The incremental artifact was complete, with no dropped paths and no truncation.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • Prior — still present (medium confidence) scripts/rekor_digest_check.py:206-208: MONITOR-ERROR rows are still added to retry with no WINDOW limit. Only UNKNOWN/unavailable rows have a time limit. If the helper fails the same way every time, the job will fail every hour with no end.
  • New (medium confidence) .github/workflows/rekor-monitor.yaml:68-79: if the newest candidate fails because gh run download hit a temporary error (not because its files are incomplete), the step quietly falls back to an older artifact and writes no log line. The older reconciliation state (pending entries, first_seen, the persisted image-hash budget, checkpoint first_observed) is then restored and uploaded again as the newest. That can undo budget consumption without anyone noticing. A ::warning:: for each skipped candidate would help, and so would failing outright on download errors instead of only on incomplete contents.

Resolved prior findings

  • Fixed: restore choosing an incomplete artifact and perpetuating it. Restore now requires both files to be non-empty before it accepts a candidate (.github/workflows/rekor-monitor.yaml:67-79). Upload now depends on steps.state.outputs.state_complete == 'true' (.github/workflows/rekor-monitor.yaml:114-123). Both are covered by scripts/test-rekor-monitor.py (missing_checkpoint/missing_reconciliation cases and test_upload_requires_both_nonempty_state_files).
  • Fixed (previously confirmed): artifact-less completed runs could wedge restore. Selection still filters on non-expired main artifacts from this workflow (.github/workflows/rekor-monitor.yaml:56-66).
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `scripts/rekor_digest_check.py`:
- Around line 206-208: MONITOR-ERROR rows are retried with no time limit. Apply the same `now - first_seen < WINDOW` bound used for UNKNOWN/unavailable rows, and make sure `first_seen` is recorded for MONITOR-ERROR rows, so a helper that keeps failing the same way expires instead of failing the job every hour forever.

In `.github/workflows/rekor-monitor.yaml`:
- Around line 68-79: When a candidate artifact is skipped, emit `::warning::` with the run ID and the reason (download failure vs. missing/empty checkpoint.txt or reconciliation.json). Consider failing the step when `gh run download` itself fails, so a temporary error cannot roll the monitor back to older reconciliation state (budget, pending, first_seen) that is then re-uploaded as the newest.

Reviewed commit: be16700e7755

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found — see the full review report

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit 2d4ffcee1a69

General PR Review: Monitor release identities and published signed digests

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ad85b5b02764.
Review mode: incremental since be16700
View review run

Review Summary

The new commit makes state restore fail closed. If gh run download fails, the step now logs ::error:: and exits instead of quietly falling back to an older artifact. A downloaded artifact that is missing a file is still skipped, but now with a ::warning::. The README and the restore fixtures were updated to match: the fixtures check that exactly one download is attempted, that no temp dir is left behind, and that the completeness step reports no upload-ready state. I scanned the full PR diff (workflow, Python and shell glue, Go helper, go.mod/go.sum, PEMs, CI/test changes) for security and correctness. No repo-local criteria loaded, so only the base criteria were applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • Prior — still present (medium confidence) scripts/rekor_digest_check.py:206-208: MONITOR-ERROR rows are still retried with no WINDOW limit. evaluate() returns at lines 135-136 before it sets first_seen for them. If one entry keeps failing in the helper, the job fails every hour and never stops.

Resolved prior findings

  • Fixed: a temporary download failure no longer quietly rolls the monitor back to an older artifact. .github/workflows/rekor-monitor.yaml:68-72 now exits with ::error::download failed for run …, and the EXIT trap at line 62 removes the temp dir. Nothing gets uploaded afterward, because monitor-state/ stays empty and the gate at lines 118-127 never sets state_complete. Incomplete artifacts now log a warning (line 81). Both paths are covered by scripts/test-rekor-monitor.py (download-error-* cases and the warning assertion).
  • Fixed (previously confirmed): restore now requires both state files to be non-empty and upload is gated on completeness (rekor-monitor.yaml:73-80, 118-127). Artifact-less runs can't wedge selection (rekor-monitor.yaml:56-66).
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `scripts/rekor_digest_check.py`:
- Around line 134-136 and 206-208: MONITOR-ERROR rows return from evaluate() before first_seen is recorded, and the retry set keeps them with no WINDOW limit. Record first_seen for error rows, for example with state.setdefault("first_seen", {}).setdefault(entry["uuid"], now) before the early return. Then apply the same `now - first_seen < WINDOW` check to MONITOR-ERROR rows in the retry comprehension, so an entry that keeps failing expires or escalates instead of failing the job every hour forever.

Comment on lines +206 to +208
retry = {row["uuid"] for row in rows if row["status"] in {"PENDING", "MONITOR-ERROR"}
or (row["status"] == "UNKNOWN" and "unavailable" in row["reason"]
and now - state.get("first_seen", {}).get(row["uuid"], now) < WINDOW)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: MONITOR-ERROR rows go into retry without any WINDOW limit. evaluate() also returns before first_seen is set for them (line 135-136), so the age check could not apply even if it were added. If one entry keeps failing the helper the same way, it stays in pending forever and the job fails every hour. Consider recording first_seen for error rows and expiring them after WINDOW (or escalating them to a terminal ANOMALY).

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

General PR Review: Monitor release identities and published signed digests

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ad85b5b02764.
Review mode: incremental since be16700
View review run

Review Summary

The new commit makes state restore fail closed. If gh run download fails, the step now logs ::error:: and exits instead of quietly falling back to an older artifact. A downloaded artifact that is missing a file is still skipped, but now with a ::warning::. The README and the restore fixtures were updated to match: the fixtures check that exactly one download is attempted, that no temp dir is left behind, and that the completeness step reports no upload-ready state. I scanned the full PR diff (workflow, Python and shell glue, Go helper, go.mod/go.sum, PEMs, CI/test changes) for security and correctness. No repo-local criteria loaded, so only the base criteria were applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • Prior — still present (medium confidence) scripts/rekor_digest_check.py:206-208: MONITOR-ERROR rows are still retried with no WINDOW limit. evaluate() returns at lines 135-136 before it sets first_seen for them. If one entry keeps failing in the helper, the job fails every hour and never stops.

Resolved prior findings

  • Fixed: a temporary download failure no longer quietly rolls the monitor back to an older artifact. .github/workflows/rekor-monitor.yaml:68-72 now exits with ::error::download failed for run …, and the EXIT trap at line 62 removes the temp dir. Nothing gets uploaded afterward, because monitor-state/ stays empty and the gate at lines 118-127 never sets state_complete. Incomplete artifacts now log a warning (line 81). Both paths are covered by scripts/test-rekor-monitor.py (download-error-* cases and the warning assertion).
  • Fixed (previously confirmed): restore now requires both state files to be non-empty and upload is gated on completeness (rekor-monitor.yaml:73-80, 118-127). Artifact-less runs can't wedge selection (rekor-monitor.yaml:56-66).
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `scripts/rekor_digest_check.py`:
- Around line 134-136 and 206-208: MONITOR-ERROR rows return from evaluate() before first_seen is recorded, and the retry set keeps them with no WINDOW limit. Record first_seen for error rows, for example with state.setdefault("first_seen", {}).setdefault(entry["uuid"], now) before the early return. Then apply the same `now - first_seen < WINDOW` check to MONITOR-ERROR rows in the retry comprehension, so an entry that keeps failing expires or escalates instead of failing the job every hour forever.

Reviewed commit: 2d4ffcee1a69

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found — see the full review report

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