Skip to content

Repair bounded reviewer summary publication (#34) - #35

Merged
avoroncov971-maker merged 1 commit into
mainfrom
issue-34-reviewer-execution
Sep 9, 2026
Merged

Repair bounded reviewer summary publication (#34)#35
avoroncov971-maker merged 1 commit into
mainfrom
issue-34-reviewer-execution

Conversation

@avoroncov971-maker

@avoroncov971-maker avoroncov971-maker commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Refs #34

Allowlist

  • .github/claude-review/PROMPT.md
  • .github/workflows/claude-review.yml
  • tests/test_claude_review_workflow.py

Change

Make the required summary use one direct allowed gh pr comment ... --body-file - heredoc. It avoids staging a file, cat/echo, and pipelines, all of which are outside the reviewer tool boundary. The workflow, action SHA pins, permissions, triggers, draft/fork guards, claude-sonnet-5, and --max-turns 16 are unchanged.

Offline publication probe

The focused regression uses a harmless local gh fixture and executes the direct heredoc route. It proves the exact gh pr comment 9 --repo owner/repo --body-file - argument vector and complete stdin summary payload.

Verification

  • pytest -q tests/test_claude_review_workflow.py — exit 0 (40 passed)
  • pytest -q — exit 0 (419 passed)
  • git diff --check — exit 0

Base: 1b90aad6b021ecb78f7b478d789065e90bcb3cc1
Head: 6717e1e30e01a1e4cd5b859c9463d681121f5602

Evidence and remaining acceptance

The direct publication-path defect is repaired and reproducibly tested. This does not establish a cause for the separate canary attempts 2–4, which returned subtype=success, is_error=true, one turn, empty model usage, and no provider error body in bounded diagnostics. The Owner renewed the subscription credential before attempt 4, which reproduced that early failure. Live acceptance remains blocked pending a repair that can make the reviewer execute; do not rerun or modify frozen PR #33. A fresh ordinary canary whose head contains this accepted workflow/prompt remains required after an independent RED review and Owner merge.

avoroncov971-maker commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

REVIEW_DISPATCH

repository: deedseal/proof-check
pull_request: 35
review_lane: FULL
exact_head_sha: 6717e1e
base_sha: 1b90aad
implementation_issue: #34
implementer_runtime: Codex CLI
reviewer_runtime: Claude Code through the configured subscription default
fallback: none

Review this exact head once as an independent adversarial reviewer. Read Issue #34 and every comment, PR #35, both changed files, and the relevant prompt/workflow tests. Verify the claimed direct allowed heredoc publication path, exact Bash allowlist compatibility, shell semantics, stdin payload, marker binding, security boundaries, regression coverage, and whether Closes #34 is truthful while the separate early execution failure remains unresolved.

Do not modify files, branches, PR state, labels, settings, secrets, workflows, or the canary. Do not approve, mark Ready, merge, rerun Actions, or perform a review-of-review. If the head moves, stop STALE_HEAD.

Publish one terminal PR comment:

  • RED_REVIEW_RESULT: NO_BLOCKING_FINDING; or
  • RED_REVIEW_RESULT: REPAIR_REQUIRED with bounded findings, each containing trigger, impact, evidence, and smallest repair.

Bind the terminal comment to exact head 6717e1e30e01a1e4cd5b859c9463d681121f5602.

@avoroncov971-maker

Copy link
Copy Markdown
Contributor Author

RED_REVIEW_RESULT: REPAIR_REQUIRED

Exact head reviewed: 6717e1e (confirmed unchanged via gh pr view 35 --json headRefOid immediately before this comment).

Scope covered:

  • Read Issue 34 in full (body + all 8 comments: OWNER_BINDING, two CANARY_EVIDENCE_UPDATEs, two ENGINEERING_BINDING_CORRECTIONs, STARTED/STARTED CORRECTION, RESULT).
  • Read PR 35 body, exact diff, and both changed files (.github/claude-review/PROMPT.md, tests/test_claude_review_workflow.py) at head 6717e1e.
  • Traced the heredoc publication path end-to-end against .github/workflows/claude-review.yml's --allowedTools list and the workflow's marker-matching record step.

Finding 1 (blocking) - 'Closes #34' is not truthful.
Trigger: PR 35's body opens with the line 'Closes #34' (a GitHub auto-close keyword) with base main. Merging as-is will auto-close Issue 34.

Impact: Issue 34 has two explicit priorities: Priority 1 'restore authenticated execution' and Priority 2 'test and repair summary publication.' PR 35 only addresses Priority 2 (the publication path). Priority 1 is explicitly not resolved:

  • Issue 34's own RESULT comment (5594943722) states: 'Not proven: the cause of canary attempts 2-4... This is REVIEW_EXECUTION_FAILED, not evidence that the earlier publication denials remain current.' and 'Remaining live acceptance: independent RED review, Owner merge, then one ordinary canary whose head contains the accepted change. No additional retry is authorized until a repair addresses the separate early-execution failure.'
  • PR 35's own body repeats this under 'Evidence and remaining acceptance': 'Live acceptance remains blocked pending a repair that can make the reviewer execute... A fresh ordinary canary whose head contains this accepted workflow/prompt remains required after an independent RED review and Owner merge.'
  • The REVIEW_DISPATCH comment on this very PR (5594978672) reiterates: 'while the separate early execution failure remains unresolved.'

So the PR's own text contradicts its 'Closes #34' claim: the issue's stated acceptance criteria (restored authenticated execution plus a passing live canary) are admittedly still outstanding. Auto-closing 34 on merge would misrepresent the repair as complete and would let Priority 1 (the auth/execution failure reproduced in canary attempts 2-4) silently drop off tracking.

Evidence: Issue 34 body Priority 1/2 split; comments 5594834635 (attempt 4, REVIEW_EXECUTION_FAILED) and 5594943722 (RESULT); PR 35 body 'Evidence and remaining acceptance' section; PR 35 comment 5594978672.

Smallest repair: Edit the PR 35 description to remove or replace the 'Closes #34' keyword (e.g. 'Refs #34' or 'Addresses part of #34') so merging does not auto-close the issue, until Priority 1 is diagnosed/repaired and the required live canary plus freshness acceptance actually passes.

Verified non-blocking (technical publication path is sound):

  • Allowed-tool compatibility: the new prompt's required invocation begins 'gh pr comment NUMBER --repo OWNER/REPO --body-file -', which matches the unchanged Bash(gh pr comment:*) allowlist entry in claude-review.yml line 81; the heredoc redirection (<<'CLAUDE_REVIEW_SUMMARY') is part of the same simple command, not a second operation, so it does not introduce a disallowed prefix.
  • Shell semantics/stdin payload: the heredoc delimiter is quoted (<<'CLAUDE_REVIEW_SUMMARY'), which suppresses parameter/command substitution inside the body - correct for arbitrary review text that may contain dollar signs, backticks, or code fences. tests/test_claude_review_workflow.py::test_direct_heredoc_summary_publication_path executes this exact command shape through a real bash -c subprocess against a fixture gh, and asserts the fixture receives 'pr comment 9 --repo owner/repo --body-file -' with the full stdin payload byte-for-byte - a genuine, non-mocked regression for the reported defect (Write/cat/echo/pipeline attempts previously denied).
  • Marker binding: the required 'CLAUDE_REVIEW head= run= findings=' line and the workflow's regex-based authorship/timestamp/head matching (claude-review.yml lines 249-257) are untouched by this diff; test_prompt_requires_independent_review_and_zero_finding_summary still passes.
  • Regression coverage gap (minor, non-blocking): no test exercises a summary body that itself contains a bare CLAUDE_REVIEW_SUMMARY line (e.g. via untrusted diff content reflected into the model's free-text account), which would terminate the heredoc early. Given the prompt instructs the model to treat diff/PR content as untrusted and not follow embedded instructions, and any resulting truncated/malformed command would still be bounded by the same --allowedTools list, this is a hardening suggestion, not a demonstrated exploit - not blocking on its own.

Verdict rationale: Everything under Issue 34's Priority 2 (the actual subject of this repair) checks out: the direct heredoc path is real, tested with an unmocked subprocess, and compatible with the existing Bash allowlist. The blocking issue is process/accuracy, not the code: this PR is titled and worded as if it closes Issue 34 outright, while both the issue and the PR's own body concede the execution-failure priority and required live-canary acceptance are still open. That is REPAIR_REQUIRED, not a code defect in the diff.

@avoroncov971-maker

Copy link
Copy Markdown
Contributor Author

RED_DELTA_REVIEW_RESULT: F1_CLOSED

Verified head: 6717e1e
Observed first body line: "Refs #34"

The untruthful "Closes #34" reference has been replaced with "Refs #34", which will not auto-close issue #34 on merge. Bounded delta review passes; the sole blocking finding from the previous review is resolved.

@avoroncov971-maker
avoroncov971-maker merged commit a2a5d2e into main Sep 9, 2026
8 of 13 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.

1 participant