Skip to content

Publish durable completed-batch audit receipts#203

Merged
justin808 merged 4 commits into
mainfrom
jg-codex/184-audit-receipt-formatting
Jul 19, 2026
Merged

Publish durable completed-batch audit receipts#203
justin808 merged 4 commits into
mainfrom
jg-codex/184-audit-receipt-formatting

Conversation

@justin808

@justin808 justin808 commented Jul 18, 2026

Copy link
Copy Markdown
Member

Summary

  • publish the full completed-batch audit receipt as a verified durable GitHub comment
  • keep final chat output to a compact hash, actor, and timestamp-bound reference plus canonical conversation status
  • fail closed on target, actor, mutation ambiguity, blocker, parser, and replay mismatches

Validation

  • independent adversarial and type-state review: CLEAN, P1 0 / P2 0
  • receipt helper: 37 runs, 327 assertions
  • post-merge policy: 15 runs, 291 assertions
  • goal contract: 69 runs, 1,021 assertions
  • model routing: 18 runs, 957 assertions
  • fake GitHub matrix: 16 runs, 149 assertions
  • prompt-size checker: PASS, Codex headroom 319 to 320
  • full bin/validate: PASS at exact HEAD e8ca5be, including 85-file RuboCop
  • GitHub Markdown rendering: PASS through non-persistent POST /markdown
  • Codex Desktop render probe emitted in the coordinator task; programmatic self-window inspection is safety-blocked

Closes #184

Summary by CodeRabbit

  • New Features
    • Completed-batch audit v1 receipts are now published as verified durable GitHub comments, with the final chat output using compact hash-bound references.
    • Replay now rebinds and revalidates the receipt against the manifest-bound target and the exact comment ID before reporting status.
  • Documentation
    • Updated coordination/publishing and “Conversation status” formatting rules (compact receipt line must directly precede the final status line).
  • Tests
    • Expanded receipt publish/replay validation tests, including stricter failure-closed behavior and permission/target checks.
    • Added additional post-merge audit flow coverage to the validation script.

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: db4921a8-ba5f-4387-ab17-24eb22f0d1e1

📥 Commits

Reviewing files that changed from the base of the PR and between f35f8a7 and 452e902.

📒 Files selected for processing (3)
  • skills/post-merge-audit/bin/completed-batch-audit-receipt
  • skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb
  • skills/pr-batch/bin/goal-completion-contract-test.rb

Walkthrough

Completed-batch audits now publish verified durable GitHub comments through a shared Ruby receipt module. Coordinator ownership, compact final output, manifest-bound replay, marker validation, shared audit integration, and comprehensive tests were added or tightened.

Changes

Completed-batch receipt flow

Layer / File(s) Summary
Receipt ownership and output contracts
skills/post-merge-audit/SKILL.md, skills/pr-batch/SKILL.md, workflows/*.md, CHANGELOG.md, skills/post-merge-audit/bin/post-merge-audit-policy-test.rb
Skills, workflows, changelog, and policy tests define coordinator-only durable publication, compact final-chat references, fixed receipt formatting, companion-pack requirements, and exact manifest-bound replay.
Receipt parsing, publication, and replay
skills/post-merge-audit/bin/completed-batch-audit-receipt
Adds marker parsing, readiness and blocker computation, deterministic target authorization, verified comment publication/readback, compact reference generation, timeout handling, and exact-ID replay.
Shared audit integration and validation wiring
skills/pr-batch/bin/goal-completion-contract-test.rb, bin/validate
Existing completed-batch audit checks delegate to the shared receipt module, and validation runs the receipt test suite.
Receipt behavior and failure coverage
skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb
Adds coverage for successful and failed publish/replay flows, binding integrity, permissions, authorship, canonicalization, blockers, mutation states, timeouts, and API harness behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Coordinator
  participant CompletedBatchAuditReceipt
  participant GitHubAPI
  participant DurableComment
  Coordinator->>CompletedBatchAuditReceipt: publish completed-batch receipt and targets
  CompletedBatchAuditReceipt->>GitHubAPI: validate target and write permission
  CompletedBatchAuditReceipt->>DurableComment: POST canonical receipt
  DurableComment-->>CompletedBatchAuditReceipt: return comment ID and metadata
  CompletedBatchAuditReceipt->>GitHubAPI: read back and verify exact comment
  Coordinator->>CompletedBatchAuditReceipt: replay compact reference
  CompletedBatchAuditReceipt->>GitHubAPI: fetch manifest-bound comment ID
  GitHubAPI-->>CompletedBatchAuditReceipt: return comment bindings and body
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: publishing durable completed-batch audit receipts.
Linked Issues check ✅ Passed The changes align with issue #184 by making the receipt durable, replayable, and more human-readable while preserving auditability.
Out of Scope Changes check ✅ Passed The modified scripts, tests, and workflow docs all support the durable receipt formatting and replayability goals.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jg-codex/184-audit-receipt-formatting

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review: Publish durable completed-batch audit receipts

Overview

This PR adds a completed-batch-audit-receipt helper (Ruby, ~840 lines + ~1450 lines of tests) that:

  • Publishes the full completed-batch-audit v1 wrapper as a durable GitHub comment (publish), verifying the target, authenticated actor, and write permission before posting, then reading the comment back to confirm it landed unmodified.
  • Emits a compact, hash-bound chat reference (replay) instead of the full wrapper in final chat output.
  • Wires the new binary into bin/validate and rewires skills/pr-batch/bin/goal-completion-contract-test.rb to delegate to the shared module instead of duplicating ~300 lines of parsing logic (nice DRY cleanup).

Overall this is a very thoroughly tested, defensively written module (fail-closed everywhere, extensive adversarial-style tests for bot actors, timestamp edits, mismatched URLs, enterprise ports, etc.). A couple of things stood out on closer read:

Potential issues

  1. Timeout.timeout around Open3.capture3 doesn't stop the underlying gh process (gh_api, around line 401-410). Ruby's Timeout module raises an exception in the calling thread when the deadline hits, but it does not kill the spawned subprocess — gh keeps running (and could still complete a POST after the script has already reported comment-post-outcome-unknown/timed out), and the process can be left orphaned/zombied since nothing calls Process.kill/reaps it. The PostOutcomeUnknownError path already treats this as "ambiguous" from a correctness standpoint, so no duplicate-comment risk from the code's own logic, but it's worth killing the process group explicitly (e.g. Open3.popen3 + Process.kill on timeout, or Timeout.timeout(..., &block) with an ensure that kills wait_thr.pid) to avoid leaking processes on every timeout, especially since batches can retry/loop.

  2. target_base_url uses the manifest's literal repo casing, not GitHub's canonical casing (target_base_url, verify_comment's expected_url around line 297/343). validated_target only checks the owner/repo shape via regex — it never normalizes to GitHub's canonical case. If the trusted target manifest names a repo with different casing than GitHub's actual stored casing (repo renamed, or just a typo that still resolves), html_url returned by the API will use GitHub's canonical case while expected_url is built from the manifest's case, so verify_comment will always fail with "comment URL mismatch" even though the target is otherwise correct. This fails closed (safe), but the error message won't point at the real cause — worth a comment or a case-insensitive compare on this specific field, or normalizing repo to the case returned by the target-payload lookup in select_verified_anchor before using it to build the expected URL.

  3. Minor: select_verified_anchor issues up to 3 gh api calls (user, issue lookup, permission lookup) per manifest target until one verifies successfully. For larger multi-target manifests this is a few sequential API round-trips (not parallelized) — fine for the typical single/dual-target batch case this is designed for, but worth keeping in mind if manifests grow.

Nits

  • The same three paragraphs of prose ("Parse and bind the local receipt...", "Use completed-batch-audit-receipt for both publish and replay...", "Replay parses the compact reference...") are duplicated verbatim across SKILL.md, workflows/post-merge-audit.md, workflows/pr-processing.md, and skills/pr-batch/SKILL.md. This matches the pre-existing pattern in these docs (the surrounding rules are already duplicated the same way), so not a new problem, but a fourth copy is a bit more to keep in sync on future edits.

Security

No command-injection risk found — gh_api always builds the command as an array passed to Open3.capture3 (never shell-interpolated), and host/repo/number are validated by regex before use. Bot-account and non-human-actor exclusion, association/permission checks, and body/timestamp/SHA-256 binding on replay all look sound.

Test coverage

Very strong — the new test file covers the failure matrix (wrong author, edited comment, mismatched URL, bot actors on user/permission/comment payloads, ambiguous POST outcomes, enterprise port canonicalization, empty manifests, malformed external blockers) exhaustively via a fake gh binary.

Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt Outdated
Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/post-merge-audit/bin/completed-batch-audit-receipt`:
- Around line 169-180: Update the replay path around select_verified_anchor so
it does not reselect the anchor using the current GitHub actor’s permissions or
lock state. Resolve and validate the canonical target from the durable receipt’s
author/manifest binding, then keep the existing target equality check and
comment verification flow using that replay-stable anchor.
- Around line 812-828: Update the rescue handling around the generic exception
path so INVALID_MARKER_BLOCKER is added only for parser or batch-binding
failures. Map permission, API, unknown POST outcome, and readback exception
classes to their exact operational blockers, and build blockers and final_status
from those mapped values so mutation-recovery actions remain visible; preserve
the existing mutation_state and comment_id fields for PostOutcomeUnknownError
and PostReadbackError.
- Around line 206-208: Update the author capture in parse_reference to accept
underscores alongside the existing alphanumeric and hyphen characters, matching
the unchanged authenticated GitHub login emitted by compact_reference while
preserving the rest of the reference validation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 036db5aa-6b42-45fc-a9e0-dcd188d2b95d

📥 Commits

Reviewing files that changed from the base of the PR and between c485f35 and e8ca5be.

📒 Files selected for processing (10)
  • CHANGELOG.md
  • bin/validate
  • skills/post-merge-audit/SKILL.md
  • skills/post-merge-audit/bin/completed-batch-audit-receipt
  • skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb
  • skills/post-merge-audit/bin/post-merge-audit-policy-test.rb
  • skills/pr-batch/SKILL.md
  • skills/pr-batch/bin/goal-completion-contract-test.rb
  • workflows/post-merge-audit.md
  • workflows/pr-processing.md

Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt Outdated
Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt Outdated
Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f52b61e3a9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt Outdated
Comment thread skills/pr-batch/bin/goal-completion-contract-test.rb Outdated
@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review: Publish durable completed-batch audit receipts (PR 203)

Overview
This PR replaces dumping the full completed-batch-audit v1 marker into final chat output with a two-step "receipt" model:

  • skills/post-merge-audit/bin/completed-batch-audit-receipt (new, ~930 lines) posts the full marker as a durable GitHub comment (publish), and can later re-fetch/re-verify it by exact comment ID (replay).
  • Chat output is reduced to a compact, hash-bound reference line (Completed-batch audit: <verdict> — [durable v1 receipt](<url>); SHA-256 ...; author ...; version ...).
  • SKILL.md / workflow docs are updated across post-merge-audit, pr-batch, and the two workflows/*.md files to match the new contract.
  • skills/pr-batch/bin/goal-completion-contract-test.rb was refactored to reuse the new module instead of duplicating ~300 lines of marker-parsing logic.

Code quality / correctness
The implementation is unusually thorough for a shell-out helper:

  • capture_process implements a manual thread-based timeout (write/read via separate threads, deadline-based joins, TERM->grace->KILL) instead of the stdlib Timeout module, a good choice since Timeout.timeout can leave child processes/state in an inconsistent spot. The "ambiguous outcome" states (comment-post-outcome-unknown vs comment-created-readback-unknown) are a sensible way to fail closed without ever retrying a POST whose success is unknown (avoids duplicate comments).
  • publish/replay re-verify essentially everything after the fact: target identity via html_url, comment issue_url, author (rejecting bots and type-mismatched actors), trusted association, unedited timestamps, and a SHA-256 of the body, good defense against a mismatched/edited/wrong-target comment being trusted.
  • URL/host parsing (canonical_host, verify_issue_url!, parse_reference) correctly handles GitHub Enterprise ports and the api.github.com vs github.com host split, with tests covering explicit :443 canonicalization and non-default enterprise ports.
  • All gh invocations use Open3.popen3 with an argv array (not a shell string), so there is no shell-injection risk even though repo/host values are only loosely validated.
  • Test coverage is extensive (1800+ lines) and includes a believable fake-gh harness covering the POST-timeout/ambiguous-outcome paths, bot/EMU actors, casing normalization, and tampered-reference rejection.

Portability concern (see inline comment)
skills/pr-batch/bin/goal-completion-contract-test.rb now does load File.expand_path("../../post-merge-audit/bin/completed-batch-audit-receipt", __dir__), a hard runtime dependency from one skill folder test suite onto another skill folder implementation file. The README documents skills/* as independently "copy or symlink"-able folders; this is the first case in the repo of one skill code reaching into another skill bin/ directory rather than just reading shared markdown/docs. Worth documenting explicitly or restructuring so a partial skill copy/install does not silently break this test.

Minor / lower-confidence notes

  • In CompletedBatchAuditReceipt.run top-level rescue, malformed target-manifest schema errors (e.g. a target object missing number, raising KeyError/ArgumentError from validated_target) fall through to the generic INVALID_MARKER_BLOCKER ("completed-batch-audit marker invalid") label, a slightly misleading message for what is actually a manifest problem rather than a marker-parsing problem. Not a functional bug (still fails closed with exit 1), just a diagnostics/UX nit.
  • select_verified_anchor only ever tries the first deterministic target from the manifest and never falls back to a second target if verification fails (explicitly covered by test_publish_anchor_selection_never_falls_through_first_deterministic_target). This looks intentional (fail-closed, deterministic, no ambiguity about which target wins), just flagging it as a deliberate design choice worth double-checking against expectations for multi-target batches.

Security
No injection or auth-bypass issues found. Bot-account and wrong-actor-type rejections, HTTPS-only URL checks, and the "never re-open the receipt URL, only refetch via authenticated gh api" replay design all look sound.

Overall: solid, well-tested change. The one thing worth addressing (or consciously accepting) before merge is the new cross-skill file dependency noted above.

Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt
@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review: Publish durable completed-batch audit receipts (#203)

Overview

This PR replaces the old approach of emitting the full completed-batch-audit v1 marker directly into chat with a new flow: the batch coordinator posts the full marker as a durable GitHub comment (completed-batch-audit-receipt publish), and chat output only shows a compact, verified, hash-bound reference to that comment plus the final Conversation status line. replay re-fetches and re-validates the comment by exact ID (never by following the URL) before trusting it. The bulk of the change is a new ~940-line Ruby script (skills/post-merge-audit/bin/completed-batch-audit-receipt) plus an extensive test suite (~1880 lines), with skills/pr-batch/bin/goal-completion-contract-test.rb refactored to load and reuse the shared module instead of duplicating the marker-parsing logic — a good de-duplication.

Code quality / correctness

  • The core parsing/validation logic (marker grammar, UNKNOWN handling, NFKC canonicalization, terminal/nonterminal disposition rules) is unchanged from the prior implementation and was already well covered; good call factoring it into a single module (CompletedBatchAuditReceipt) that both pr-batch and post-merge-audit now load instead of maintaining two copies.
  • select_verified_anchor → POST → readback → re-verify chain is fail-closed throughout: anchor target/actor/permission verification happens before the POST, the POST result is only trusted after Integer(id, exception: false)&.positive?, and a distinct mutation_state (comment-post-outcome-unknown vs comment-created-readback-unknown) is surfaced so callers never blindly retry into a potential duplicate comment. This is a subtle area to get right and it's handled carefully, backed by dedicated tests (test_ambiguous_post_outcome_has_distinct_state_without_comment_id_or_retry, test_post_success_readback_failure_reports_ambiguous_mutation_without_retry).
  • capture_process/terminate_process implement a bounded subprocess timeout using separate reader/writer threads plus a monotonic deadline rather than Timeout.timeout around the whole call — this avoids the classic Timeout thread-interruption hazards and correctly reaps the child (TERM, grace period, then KILL) so a hung gh process can't leak past the configured timeout. test_post_timeout_is_bounded_and_reaps_child_before_delayed_side_effect specifically checks this.
  • Left one minor inline nit: the options ? options["other_blockers"] : [] guard in run's rescue block is dead code since options is always assigned before any raise that lands there.

Security

  • All gh api invocations use the array form of Open3.popen3(*command) (no shell interpolation), so there's no command-injection surface from repo names, hosts, or batch IDs flowing into the command line.
  • Host/repo/target identity is revalidated end-to-end: canonical_host restricts hosts to a strict label grammar, verify_target_payload! cross-checks the API's html_url against the manifest-supplied target (rejecting host/repo/type/number mismatches), and verify_issue_url!/verify_comment similarly bind the comment back to the expected target and comment ID before anything is trusted.
  • Author trust is enforced with both a login/type check (verified_human_login!, rejecting [bot] logins and non-User types) and author_association in TRUSTED_ASSOCIATIONS (OWNER/MEMBER/COLLABORATOR), and select_verified_anchor additionally checks the current actor has write/maintain/admin permission before posting — good defense in depth against an untrusted actor or bot forging a "clean" receipt.
  • One inherent (not blocking) design note: there's a small TOCTOU window between the permission check in select_verified_anchor and the actual comment POST — permissions could theoretically change in between. Given this runs as part of a single coordinator invocation immediately after the check, this is a reasonable tradeoff and not something I'd block on.

Performance

  • Nothing concerning — this is a low-frequency, per-batch operation (a handful of gh api calls), and the subprocess timeout (default 60s, configurable via COMPLETED_BATCH_AUDIT_GH_TIMEOUT_SECONDS) bounds worst-case latency per call.

Test coverage

  • Very thorough: the new test file covers happy path publish/replay, GitHub Enterprise host/port canonicalization, EMU-style logins, bot-actor/bot-comment rejection, hostile html_url payloads, malformed/edited/wrong-author comments, ambiguous POST/readback outcomes, and CLI usage/error-exit-code contracts, using a fake gh binary. The pr-batch contract test was updated to load the shared module and assert both SKILL.md files document the companion-skill dependency, which is a nice touch for catching skew between the two skills.

Overall this is a well-scoped, carefully fail-closed implementation with strong test coverage. Only a trivial dead-code nit found (left as an inline comment); no correctness or security blockers identified.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f35f8a733b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread skills/pr-batch/bin/goal-completion-contract-test.rb Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/post-merge-audit/bin/completed-batch-audit-receipt`:
- Line 23: Move timeout parsing from the top-level GH_TIMEOUT_SECONDS assignment
into the run command’s structured error boundary. In the completed-batch audit
execution flow, parse COMPLETED_BATCH_AUDIT_GH_TIMEOUT_SECONDS and validate that
it is a positive integer before any API operation; ensure invalid or
non-positive values produce run’s JSON failure response instead of failing
during module load.
- Around line 396-405: Update canonical_comment_body to preserve the supplied
marker/receipt wrapper byte-for-byte: remove the newline-appending
transformation in canonical_marker, or reject input that is not already
canonical. Ensure marker_state validation and the returned comment use the
unchanged marker.

In `@skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb`:
- Around line 931-971: Remove the fragile elapsed wall-clock assertion and its
associated timing setup from
test_post_timeout_is_bounded_and_reaps_child_before_delayed_side_effect. Keep
the child reaping check, delayed-side-effect absence check, mutation-state
assertions, and POST call count unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 60c7b37d-a262-40ed-a455-3380587b2862

📥 Commits

Reviewing files that changed from the base of the PR and between e8ca5be and f35f8a7.

📒 Files selected for processing (5)
  • skills/post-merge-audit/SKILL.md
  • skills/post-merge-audit/bin/completed-batch-audit-receipt
  • skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb
  • skills/pr-batch/SKILL.md
  • skills/pr-batch/bin/goal-completion-contract-test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
  • skills/pr-batch/bin/goal-completion-contract-test.rb

Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt Outdated
Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt
Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb
Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt
Comment thread skills/post-merge-audit/bin/completed-batch-audit-receipt
@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown

Review: Publish durable completed-batch audit receipts (#203)

Overview

This PR changes how the completed-batch-audit v1 marker is surfaced: instead of dumping the full wrapper into final chat output, the batch coordinator now publishes it as a durable GitHub comment via a new skills/post-merge-audit/bin/completed-batch-audit-receipt helper, and chat gets only a compact, hash/actor/timestamp-bound reference line. replay re-fetches and re-verifies that comment (target, comment id, author, trust association, unedited timestamps, SHA-256, batch id) before trusting it. skills/pr-batch's contract test now loads this same production parser instead of keeping a forked copy, with an explicit fail-closed "missing companion" guard if the sibling skill isn't present. Docs (SKILL.md x2, workflows/*.md x2) and bin/validate are updated in lockstep, and a large new test suite (completed-batch-audit-receipt-test.rb, ~2000 lines) backs the new helper.

Code quality / correctness

  • The marker-parsing/validation logic (marker_fields, disposition_records, finding_refs, etc.) is a straight extraction of the previously duplicated, already-battle-tested logic from pr-batch's contract test into a shared module — the break-based control flow in the refactored each_with_object blocks preserves the original "any malformed record invalidates the whole marker" fail-closed semantics.
  • The new networking/verification code (gh_api, capture_process, select_verified_anchor, verify_comment, verify_target_payload!, parse_reference) is careful and fail-closed throughout: gh is invoked via an argv array (no shell interpolation), timeouts are enforced with a real deadline and SIGTERM→grace→SIGKILL escalation, actor/host/URL identity is cross-checked from multiple independent GitHub responses rather than trusted from a single field, bot/non-human actors are rejected, and POST-vs-readback failure is distinguished into typed errors (PostOutcomeUnknownError vs PostReadbackError, the latter preserving the comment_id for recovery) so a network hiccup after a successful POST doesn't silently look like "nothing happened."
  • Left two minor inline suggestions: the repo field's regex validation in validated_target is looser than it needs to be for a string that ends up interpolated into the gh api endpoint path, and a small dead ternary in the top-level rescue in run.

Security

  • Nothing blocking found. The design intentionally never lets the coordinator open the receipt's URL during replay, always re-derives the target/comment via authenticated gh api from the trusted manifest, and rejects edited comments (created_at != updated_at) and bot/non-write-permission actors before trusting a receipt as evidence.

Test coverage

  • Very thorough — the new test file exercises the CLI end-to-end against a fake gh binary (timeouts, non-JSON/array/missing-id responses, POST failure vs POST-then-readback-failure, edited/relocated/wrong-author/wrong-association comments, case-normalized repo/host, and the malformed-manifest/missing-file paths), plus focused unit tests for the pure functions. goal-completion-contract-test.rb also gained a test asserting the fail-closed error message when the sibling companion file is missing.

Performance

  • No concerns — all operations are on a single receipt/comment, bounded targets manifest, and a couple of sequential gh api calls with a configurable timeout.

Overall this looks like a solid, well-tested change that matches its stated goal (durable, verifiable audit receipts instead of raw marker text in chat). Only minor/non-blocking suggestions left inline.

@justin808
justin808 merged commit 49fa8e0 into main Jul 19, 2026
27 checks passed
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.

Improve completed-batch audit receipt formatting in final summaries

1 participant