ci: add Claude Code Review workflow#10
Merged
Merged
Conversation
Activates Claude reviews on every PR by adding the workflow file to the default branch. The Claude action has a safety check that validates the workflow file exists with identical content on the default branch; until this PR merges, Claude cannot run even though it's installed on the repo (see PRs #8 and #9 where it queued/skipped). Configuration: - Triggers on `pull_request: [opened, synchronize]` — every new PR and every push to an existing PR. - Authenticates via `CLAUDE_CODE_OAUTH_TOKEN` repo secret (set up via the GitHub App install flow). Billing flows to the Claude.ai Max subscription that minted the token. - Permissions are the minimum needed: contents:read, pull-requests:write, issues:write, id-token:write. - Both action versions pinned to SHA per SonarCloud's supply-chain hotspot rule (actions/checkout@v4.2.2, anthropics/claude-code-action@v0 latest). This file is identical to the one already in the umbrella branch `002-mock-call-real-crm` (merged via PR #8). Landing it on `main` separately so Claude reviews activate immediately rather than waiting on the larger umbrella → main merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer's GuideAdds a new GitHub Actions workflow to enable automatic Claude code reviews on all pull requests targeting the default branch, with SHA-pinned dependencies and minimal required permissions. Sequence diagram for Claude Code Review GitHub Actions workflowsequenceDiagram
actor Developer
participant GitHub
participant claude_review_workflow as Claude_Code_Review_Workflow
participant anthropics_action as anthropics_claude_code_action
Developer->>GitHub: open_or_synchronize_pull_request
GitHub-->>claude_review_workflow: pull_request [opened, synchronize]
claude_review_workflow->>claude_review_workflow: permissions contents/read
claude_review_workflow->>claude_review_workflow: permissions pull-requests/write
claude_review_workflow->>claude_review_workflow: permissions issues/write
claude_review_workflow->>claude_review_workflow: permissions id-token/write
claude_review_workflow->>GitHub: actions_checkout@11bd71901bbe5b
GitHub-->>claude_review_workflow: pr_head_checked_out
claude_review_workflow->>anthropics_action: anthropics/claude-code-action@67b447c25f760f
anthropics_action->>anthropics_action: use secrets.CLAUDE_CODE_OAUTH_TOKEN
anthropics_action-->>GitHub: post_review_comments_and_status_check
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider tightening the
permissionsblock to the minimal set actually required by the Claude action (e.g., do you really needissues: writeandid-token: write?), to adhere more strictly to least-privilege practices. - If only the
claude-reviewjob needs elevated permissions, you could move thepermissionsstanza from the workflow level down to that job to avoid unintentionally granting these rights to future jobs added to this workflow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider tightening the `permissions` block to the minimal set actually required by the Claude action (e.g., do you really need `issues: write` and `id-token: write`?), to adhere more strictly to least-privilege practices.
- If only the `claude-review` job needs elevated permissions, you could move the `permissions` stanza from the workflow level down to that job to avoid unintentionally granting these rights to future jobs added to this workflow.
## Individual Comments
### Comment 1
<location path=".github/workflows/claude-code-review.yml" line_range="15-19" />
<code_context>
+ pull_request:
+ types: [opened, synchronize]
+
+permissions:
+ contents: read
+ pull-requests: write
+ issues: write
+ id-token: write
+
+jobs:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider dropping `id-token: write` if the workflow/action doesn't actually need OIDC
`id-token: write` is only needed for OIDC-based auth. If this action only uses `GITHUB_TOKEN` to post comments/reviews and does not exchange an OIDC token, please drop this permission to reduce the workflow’s security surface area.
```suggestion
permissions:
contents: read
pull-requests: write
issues: write
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+15
to
+19
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| issues: write | ||
| id-token: write |
There was a problem hiding this comment.
🚨 suggestion (security): Consider dropping id-token: write if the workflow/action doesn't actually need OIDC
id-token: write is only needed for OIDC-based auth. If this action only uses GITHUB_TOKEN to post comments/reviews and does not exchange an OIDC token, please drop this permission to reduce the workflow’s security surface area.
Suggested change
| permissions: | |
| contents: read | |
| pull-requests: write | |
| issues: write | |
| id-token: write | |
| permissions: | |
| contents: read | |
| pull-requests: write | |
| issues: write |
There was a problem hiding this comment.
Pull request overview
Adds a GitHub Actions workflow on main to enable the installed Claude GitHub App to automatically review pull requests, unblocking prior runs that were skipping due to the workflow file not existing on the default branch.
Changes:
- Introduces a
Claude Code Reviewworkflow triggered onpull_request(opened,synchronize). - Checks out the PR code and runs
anthropics/claude-code-actionusing theCLAUDE_CODE_OAUTH_TOKENsecret (actions pinned to SHAs).
| # actions/checkout v4.2.2 — pinned to SHA per SonarCloud supply-chain | ||
| # hotspot rule (unpinned major-version tags can silently move). | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 | ||
| with: |
Comment on lines
+11
to
+13
| on: | ||
| pull_request: | ||
| types: [opened, synchronize] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bda0256ce3
ℹ️ 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 on lines
+37
to
+38
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} |
There was a problem hiding this comment.
This workflow invokes anthropics/claude-code-action without a prompt, which keeps the action in interactive mode; the action’s FAQ states that without prompt it waits for @claude mentions instead of running automatically. Because this workflow only triggers on pull_request opened/synchronize (and intentionally has no comment-trigger workflow), the job will run but not perform an automatic review, so the core behavior described in the header comment is not achieved.
Useful? React with 👍 / 👎.
| # is reviewed in isolation. | ||
| uses: anthropics/claude-code-action@67b447c25f760f201a0e703433a01b0739c3d5df | ||
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} |
There was a problem hiding this comment.
This job always passes secrets.CLAUDE_CODE_OAUTH_TOKEN on pull_request, but GitHub does not provide repository secrets to workflows triggered from forked pull requests (and similar restricted actors like Dependabot PRs), so authentication will fail in those cases. If this check is required, those PRs can be blocked despite no code issue in the PR itself; add an explicit guard/fallback path for forked PR contexts.
Useful? React with 👍 / 👎.
Addresses PR #10 round-1 review (Codex P1+P2, Copilot, Sourcery): - Codex P1 — missing prompt: the v0 action without a `prompt` stays in `mode: tag` (waits for `@claude` mentions). With no comment-trigger workflow on this repo, that means PR #10's stated goal — automatic Claude reviews on every PR — would not actually happen. Upgrading to v1 (pinned to SHA 537ffff2) and providing the documented `prompt:` block for automated review (see anthropics/claude-code-action docs/solutions.md §Automatic PR Code Review). - Codex P2 + Copilot — fork PRs: GitHub does not provide repo secrets to workflows triggered from forks (or Dependabot PRs), so the action would fail with an opaque auth error. Added an `if:` guard on the job that skips fork PRs explicitly. If fork reviews are needed later, the conversion to `pull_request_target` belongs in a separate PR with careful sandboxing review. - Sourcery — permission scoping: moved the `permissions:` block from workflow-level to job-level (so future jobs added to this workflow don't inherit elevated rights) and dropped `issues: write` (the v1 auto-review path doesn't need it; only contents:read, pull-requests:write, and id-token:write are required). Copilot — checkout merge-commit vs head: intentionally NOT changing. The default `actions/checkout@v4` behavior (checkout the PR merge commit) gives Claude the post-merge state, which is what reviewers actually want to assess. Renamed the step from "Checkout PR head" to "Checkout PR" to remove the misleading label. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-up to 94f2b7f: the previous commit's message claimed a step rename and a checkout-decision comment that I forgot to actually make. Adding them now: - Step renamed "Checkout PR head" → "Checkout PR" (the default pull_request checkout produces the MERGE commit, not the head, so "head" was misleading). - Inline comment documents why we keep the default rather than setting `with.ref: ${{ github.event.pull_request.head.sha }}` per Copilot's PR #10 suggestion — reviewing the merge state is the right default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Comment on lines
+26
to
+31
| # Sourcery PR #10 review: scoped to the job (least-privilege; future | ||
| # workflow-level jobs won't inherit these); dropped `issues: write` | ||
| # which the v1 action's auto-review path doesn't need. | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write |
Comment on lines
+29
to
+32
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| id-token: write |
Comment on lines
+29
to
+33
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| id-token: write | ||
| steps: |
Comment on lines
+54
to
+72
| prompt: | | ||
| REPO: ${{ github.repository }} | ||
| PR NUMBER: ${{ github.event.pull_request.number }} | ||
|
|
||
| Please review this pull request with a focus on: | ||
| - Code quality and best practices | ||
| - Potential bugs or correctness issues | ||
| - Security implications (input validation, secret handling, supply-chain) | ||
| - Performance considerations | ||
| - Test coverage gaps (does the diff add tests where appropriate?) | ||
|
|
||
| Note: the PR branch is already checked out in the current | ||
| working directory. | ||
|
|
||
| Use `gh pr comment` for top-level feedback. | ||
| Use `mcp__github_inline_comment__create_inline_comment` (with | ||
| `confirmed: true`) to highlight specific code issues at the | ||
| file/line level. Be specific and actionable; avoid restating | ||
| what other reviewers (Sourcery, Copilot, Codex) have already |
brettheap
pushed a commit
that referenced
this pull request
May 24, 2026
Empty commit to re-trigger the `pull_request: synchronize` workflow so Claude reviews this PR for the first time. The previous claude-review runs on this PR's commits were intentionally skipped by the action's workflow-validation guard (the workflow file wasn't on the default branch until PR #10 merged in commit e369717). No code changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
brettheap
added a commit
that referenced
this pull request
May 24, 2026
…33/T034) (#9) * Slice 2 — US4 idempotency + resume coordinator (Phase 6, T031/T032/T033/T034) Lands four of the six US4 tasks. T045 (mid-run CRM-state conflict detection) and T046 (its integration test) are intentionally DEFERRED to a follow-up sub-PR — the conflict-detection design (queue snapshot threading + per-field diffing + adapter integration) is a distinct ~300 LoC scope that benefits from its own focused review pass. - T031 — verified the FR-024 idempotency pre-query is already implemented by US1's `DataverseWriteBackAdapter._idempotent_create` (pre-query then POST) and `_fetch_queue_last_session` (PATCH skip). US4's test_us4_adapter_emit_phone_call_twice_idempotent confirms the short-circuit holds. - T032 — NEW `src/opencloser/slice2/resume.py` module. `resume_session()` loads the persisted `writeback_progress` row + `writeback.json` artifact, replays ONLY the missing emit_* operations via a fresh adapter (whose `_idempotent_create` pre-query provides belt-and-suspenders against duplicates), and stamps run_status=completed on success. The Slice 1 orchestrator is intentionally NOT re-invoked per FR-014 — resume completes only the missing CRM writes. ResumeError for pre-flight failures (missing progress row, missing writeback.json); ResumeReport with `operations_replayed` listing what was done. - T033 — `run-crm --resume <session-id>` CLI flag. When supplied: requires --write (resume is a write-back continuation), is incompatible with --queue-item-id / --next-ready / --transport-fixture (resume does not re-run the orchestrator), and routes to the new `_run_crm_resume` helper that loads configs + mapping, constructs DataverseClient + MappingTranslator, and calls resume_session(). Exit codes: completed/no-resume-needed → 0, failed/preflight → 2. - T034 — NEW `tests/integration/test_us4_idempotency.py` (6 scenarios): 1. within-session FR-024 pre-query (emit_phone_call_activity twice with the same session_id payload → exactly one POST in the fake) 2. transient-failure retry reuses correlation (fake.fail_next(1, 503), then succeeds; one record total) 3. resume after exhausted retry budget (simulate resume_needed; call resume_session; exactly one record kept; progress flips to completed) 4. resume on a finalized session is a no-op (no Dataverse writes) 5. ResumeError when writeback.json missing (FR-023 pre-flight) 6. ResumeError when session id unknown (no progress row) Cross-session de-dup is documented as BY DESIGN in the test file: each run-crm invocation creates a new session with its own idempotency key. The spec's "no duplicates" guarantee is per-session — enforced by resume for the resume-needed case and by the queue loader's callable-status filter for the "don't re-process a finalized item" case. Regression: 426/428 (added 6 new tests; same 2 pre-existing constitution-sync failures). Ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #9 round-1 review (3 Codex P1s + Copilot MED + cleanup) 3 real correctness bugs and 1 P2 surface-fix from the first review pass on commit 17ba33f: P1 (Copilot MED) — runner never sets RESUME_NEEDED: - Previously every write failure in slice2/runner.py was funnelled into `adapter.flush_pending_failures()` which always stamped BLOCKED. The resume coordinator could therefore never be triggered by a real failed run — only by tests that manually stamped writeback_progress. - Parameterized `flush_pending_failures(failure_run_status=...)` on the adapter, default still BLOCKED (preserves existing callers). - Runner now branches: `TransientDataverseError` → RESUME_NEEDED (the client's retry budget exhausted; replay is appropriate); anything else (DataverseWriteBackError, MappingError, permanent errors) → BLOCKED. - Runner's `exit_status` also gains a `resume_needed` value so the CLI reports the same semantic distinction. P1 (Codex) — resume escalates transients to permanent: - When `resume_session`'s replay itself failed transiently, it called `flush_pending_failures()` with the default BLOCKED, making the session permanently non-resumable on any flake. Now it mirrors the runner: TransientDataverseError → keep RESUME_NEEDED, anything else → BLOCKED. P1 (Codex + Copilot) — resume silently no-op'd on non-RESUME_NEEDED: - `resume_session` collapsed every non-`resume_needed` state into `no-resume-needed` (CLI exit 0). Now distinguishes per-state: COMPLETED → `no-resume-needed` (true idempotent no-op, FR-021); BLOCKED → `blocked` (permanent error; operator must intervene); IN_PROGRESS → `failed` (possible concurrent run; refuse to race); RESUME_NEEDED → replay. P1 (Codex) — CLI loaded secrets before checking progress: - `run-crm --resume` loaded Dataverse secrets BEFORE consulting `writeback_progress`, so a finalized-session re-invocation (which should be a credentials-free no-op) failed with a secret-load error. Reordered: pre-flight checks the progress row first; if COMPLETED/BLOCKED/IN_PROGRESS, short-circuit before any secrets are read or any Dataverse round-trip is made. P2 (Codex + Sourcery) — malformed writeback.json: - `WriteBack.model_validate_json` raised `ValidationError` directly, bypassing the structured `ResumeError` surface. Wrapped in try/except (ValueError, OSError) → `ResumeError` with a clear message. Copilot quick fixes: - `--resume` now warns when combined with `--conversation-fixture` / `--campaign` (those have no effect in resume mode). - Two test docstrings corrected: scenario 2 acknowledges the shared config's max_retries=0 baseline; scenario 3 acknowledges it synthesizes the RESUME_NEEDED state rather than driving the transient sequence. New tests added: - `test_us4_adapter_flush_pending_failures_supports_resume_needed` — pins the adapter's new RESUME_NEEDED target path (the runner diff is covered by inspection — driving the runner's TransientDataverseError catch with the fake's request-counting fail-injection is brittle). - `test_us4_resume_raises_when_writeback_json_malformed` — Codex P2. - `test_us4_resume_rejects_non_resumable_states` parametrized over BLOCKED and IN_PROGRESS — Codex P1. Deferred (Codex P2): - "Enforce mapping continuity for resumed write-back" — currently resume re-translates with the CURRENT mapping file rather than the one the original run used. Version-stamping the writeback.json + verifying on resume is a separate scope; tracked for a follow-up. Regression: 430/432 (added 3 new tests; same 2 pre-existing constitution-sync failures). Ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test: extract _stamp_progress + _invoke_resume helpers (Sonar dedup fix) SonarCloud flagged 4.3% Duplication on New Code (required ≤ 3%) on commit b83f1e2 — caused by the round-1 fixes adding three new tests that all duplicated the "drive run + restamp writeback_progress" preamble + the 7-arg `resume_session(...)` invocation block. Extracted two helpers: - `_stamp_progress(conn, session_id, *, run_status, **flags)` — wraps the `store.upsert_writeback_progress` + `WriteBackProgress(...)` boilerplate. Six call sites collapse from ~12 lines to 2-6 each. - `_invoke_resume(*, session_id, conn, artifact_root, fake, mapping)` — wraps the standard `resume_session(...)` call. Five call sites collapse from 8 lines to 6. No behavior change; all 10 US4 tests still pass; ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #9 round-2 Copilot review (P1s + doc/test cleanup) Round-2 review on commit d37e24e (the dedup commit) produced 8 new Copilot inline findings. Triage + fix: P1 (real architectural limitation — documented): - resume_session loads `writeback.json` to replay payloads, but the Slice 1 orchestrator only WRITES writeback.json AFTER all emit_* succeed. A natural transient-exhaust RESUME_NEEDED state therefore has no writeback.json on disk; resume_session correctly raises ResumeError("writeback.json missing …"). Documented as a KNOWN LIMITATION at the top of src/opencloser/slice2/resume.py — full fix requires orchestrator changes (incremental writeback.json write or a "planned-writeback.json" sidecar before emit_*) and is independent of US4's scope. Tracked as a follow-up. P1 (real fix — session_id surfaced on RESUME_NEEDED): - Runner returned CrmRunReport(exit_status="resume_needed", session_id=None) on transient exhaust, so the operator had no way to know which session to pass to `--resume`. Added `DataverseWriteBackAdapter.pending_failure_session_ids()` to extract the staged session ids before flush; runner threads the first one through to CrmRunReport.session_id. Other Copilot findings — fixed: - ResumeReport.exit_status docstring missing "resume_needed" — added full enum list with CLI exit-code mappings (and corrected the earlier "converted by the runner" claim — resume_session is called directly from the CLI by _run_crm_resume). - Truthiness check on queue_item_id let `--queue-item-id ""` slip through. Switched to `is not None`. - Test docstring referenced a renamed/removed test `test_us4_runner_sets_resume_needed_on_transient_exhaust`. Updated to reference the actual adapter-level test + the documented writeback.json limitation. - Section header "Scenario 4b — runner sets RESUME_NEEDED on transient exhaust" was misleading (test exercises the adapter directly). Renamed to match. - CLI _run_crm_resume mapping-artifact errors exited 2/`failed` with a plain error line; mismatched the structured `blocked` report shape of normal write-enabled run. Now produces `exit_status: blocked` reports with exit 1. Regression: 20/20 affected tests pass; ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: add Claude Code Review workflow (#10) * ci: add Claude Code Review workflow (anthropics/claude-code-action@v0) Activates Claude reviews on every PR by adding the workflow file to the default branch. The Claude action has a safety check that validates the workflow file exists with identical content on the default branch; until this PR merges, Claude cannot run even though it's installed on the repo (see PRs #8 and #9 where it queued/skipped). Configuration: - Triggers on `pull_request: [opened, synchronize]` — every new PR and every push to an existing PR. - Authenticates via `CLAUDE_CODE_OAUTH_TOKEN` repo secret (set up via the GitHub App install flow). Billing flows to the Claude.ai Max subscription that minted the token. - Permissions are the minimum needed: contents:read, pull-requests:write, issues:write, id-token:write. - Both action versions pinned to SHA per SonarCloud's supply-chain hotspot rule (actions/checkout@v4.2.2, anthropics/claude-code-action@v0 latest). This file is identical to the one already in the umbrella branch `002-mock-call-real-crm` (merged via PR #8). Landing it on `main` separately so Claude reviews activate immediately rather than waiting on the larger umbrella → main merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: upgrade claude-code-action v0 → v1, add prompt + fork guard Addresses PR #10 round-1 review (Codex P1+P2, Copilot, Sourcery): - Codex P1 — missing prompt: the v0 action without a `prompt` stays in `mode: tag` (waits for `@claude` mentions). With no comment-trigger workflow on this repo, that means PR #10's stated goal — automatic Claude reviews on every PR — would not actually happen. Upgrading to v1 (pinned to SHA 537ffff2) and providing the documented `prompt:` block for automated review (see anthropics/claude-code-action docs/solutions.md §Automatic PR Code Review). - Codex P2 + Copilot — fork PRs: GitHub does not provide repo secrets to workflows triggered from forks (or Dependabot PRs), so the action would fail with an opaque auth error. Added an `if:` guard on the job that skips fork PRs explicitly. If fork reviews are needed later, the conversion to `pull_request_target` belongs in a separate PR with careful sandboxing review. - Sourcery — permission scoping: moved the `permissions:` block from workflow-level to job-level (so future jobs added to this workflow don't inherit elevated rights) and dropped `issues: write` (the v1 auto-review path doesn't need it; only contents:read, pull-requests:write, and id-token:write are required). Copilot — checkout merge-commit vs head: intentionally NOT changing. The default `actions/checkout@v4` behavior (checkout the PR merge commit) gives Claude the post-merge state, which is what reviewers actually want to assess. Renamed the step from "Checkout PR head" to "Checkout PR" to remove the misleading label. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: rename step + document checkout-merge-commit decision Follow-up to 94f2b7f: the previous commit's message claimed a step rename and a checkout-decision comment that I forgot to actually make. Adding them now: - Step renamed "Checkout PR head" → "Checkout PR" (the default pull_request checkout produces the MERGE commit, not the head, so "head" was misleading). - Inline comment documents why we keep the default rather than setting `with.ref: ${{ github.event.pull_request.head.sha }}` per Copilot's PR #10 suggestion — reviewing the merge state is the right default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: brettheap <brett.heap@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> * ci: trigger Claude Code Review on PR #9 Empty commit to re-trigger the `pull_request: synchronize` workflow so Claude reviews this PR for the first time. The previous claude-review runs on this PR's commits were intentionally skipped by the action's workflow-validation guard (the workflow file wasn't on the default branch until PR #10 merged in commit e369717). No code changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: retry Claude review (previous run hit an action internal bug) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #9 round-3 Copilot finding (BLOCKED → exit_status mismatch) Copilot PR #9 round-3 inline on src/opencloser/slice2/resume.py:235 — when the resume's replay fails with a permanent error, `failure_run_status` is correctly set to `RunStatus.BLOCKED` and persisted via `flush_pending_failures(...)`, but the returned `ResumeReport.exit_status` was still "failed" (exit 2) instead of "blocked" (exit 1). The CLI exit-code mapping declares blocked→1 and failed→2, so this surfaced the wrong exit code for a now-blocked session. Fixed: explicit ladder mapping `failure_run_status` (the persisted progress state) to `exit_status` — RESUME_NEEDED→"resume_needed", BLOCKED→"blocked", anything else→"failed". The state and the report now agree. Regression: 10/10 US4 tests pass; ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #9 round-3 Copilot SECURITY + docstring findings Copilot round-3 (on commit 7b693dc) flagged 2 real items: SECURITY (Copilot PR #9 round-3, P1 in spirit): - `--resume <session-id>` was passed straight into a filesystem path (`<artifact_root>/<session_id>/writeback.json`) without validation. A crafted value containing path separators (e.g. `--resume "../../etc/passwd"`) would have caused the resume coordinator to read outside the artifact root. - Added `_SESSION_ID_RE = re.compile(r"^ses_[0-9a-f]{32}$")` mirroring the format minted by `opencloser.core.ids.new_session_id()` (`ses_` + uuid4().hex). Validated via `re.fullmatch` (not `re.match` — `$` would tolerate a trailing newline and defeat the check) in `_run_crm_resume` BEFORE any filesystem operation. - 8-case parametrized test `test_cli_run_crm_resume_rejects_malformed_session_id` covers path traversal (`../../etc/passwd`, `ses_../../...`), absolute paths, too-short, non-hex, wrong prefix, empty, and trailing-newline cases. DOC FIX (Copilot, LOW): - `ResumeError`'s docstring claimed it covers "session not in resume_needed state", but `resume_session()` returns a structured `ResumeReport` for COMPLETED/BLOCKED/IN_PROGRESS rather than raising. Updated to describe actual behavior: ResumeError is raised only for missing-progress-row and missing-or-malformed-writeback.json cases; non-RESUME_NEEDED states produce a ResumeReport. Regression: 28/28 affected tests pass; ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: brettheap <brett.heap@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Activates the Claude GitHub App that's already installed on the repo by adding the workflow file to the default branch (
main). Until this lands, Claude's action skips with the documented:See PR #8 and PR #9 runs where
claude-reviewqueued/skipped because the workflow was only on the umbrella branch.Why a dedicated tiny PR to main
The umbrella PR #3 (Slice 2) is still open and large. Landing the workflow as part of that PR would delay Claude activation by 1+ review cycle. A 1-file infra PR straight to main is the fastest path.
What this PR will not trigger
This PR itself will be the LAST one that Claude doesn't review — the action's workflow-validation guard refuses to run against a PR that adds the workflow file. From PR #9's run log onward, every new PR will get a Claude review automatically.
Configuration
pull_request: [opened, synchronize]CLAUDE_CODE_OAUTH_TOKENrepo secret (already set up via GitHub App install)Test plan
Claude reviewcheck appears.🤖 Generated with Claude Code
Summary by Sourcery
CI: