Skip to content

ci: the enrollment review resumes the last review's conversation on each push - #47

Merged
sjawhar-agent[bot] merged 7 commits into
mainfrom
ci/review-resume
Oct 26, 2026
Merged

sjawhar-agent[bot] merged 7 commits into
mainfrom
ci/review-resume

Conversation

@sjawhar-agent

@sjawhar-agent sjawhar-agent Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Why

The maintainers asked that the CI reviewer be "resuming the same conversation on each push and not running fresh every time." Until now each push started a new enrollment review: review-run called claude -p "$prompt" with no session, so a maintainer who fixed four requests got a review that found everything again from the start.

What changes

The review of each push resumes the conversation of the last review of the same pull request and project. The conversation goes from one push to the next sealed, as an artifact of the review's run, not in the Actions cache (why: the next section).

  • review-session (new). seal encrypts review-run's conversation directory (session and <id>.jsonl) with AES-256-GCM under a key derived (HKDF-SHA256) from REVIEW_CONVERSATION_KEY, a secret of the environment enroll-review. The authenticated data names the repository, the pull request and the project, so a conversation opens only for the review of the same pull request and project. restore lists the artifacts named enroll-review-<pull request>-<project> (paginated), newest first, and reads only those uploaded by build.yaml as main has it: pull_request_target runs, or workflow_dispatch runs started on main. A pull request's own run can upload an artifact of any name, so the others are skipped. They do not count toward the 10 artifacts tried, and run lookups are cached and capped. It writes the first artifact that opens with the key and whose review is the one the pull request's review comment shows (the commit in the comment's metadata line). So a review whose conversation was not kept, or whose comment was not posted, never makes the next push resume an older review and call its comment the posted one. Otherwise, and on any error, it writes nothing, the review starts fresh, and it exits 0 and says which.
  • build.yaml, review job. The job uses the environment enroll-review with deployment: false (enroll-review-test, which has no key, on a ci-test/ branch). "Restore the last review's conversation" runs review-session restore (actions: read, which the job already has). After a review that wrote a comment, "Seal this review's conversation…" and "Keep the sealed conversation…" (actions/upload-artifact v7.0.2, pinned by SHA, overwrite: true for a re-run) keep it. The key is in the environment of those two steps only, not of the Review step, and gh does not inherit it.
  • review-run. It takes REVIEW_SESSION and HEAD_SHA. When the directory holds a complete conversation, it puts it back in ~/.claude/projects/<folder>/. Of the two tries that Review: keep the identity token usable for a long review, try once more, and say why in the summary #52 gave the review, the first runs claude -p "<later-push prompt>" --resume <last id> --fork-session --session-id <new id>, and the second, if the first ends without a review for any reason, is a fresh review with its own id. The fork keeps the last conversation whole if a try fails. After a review that wrote a comment it keeps the new conversation, unless it holds what looks like a credential (Review: keep the identity token usable for a long review, try once more, and say why in the summary #52's scan), the collector's token, or the identity token in the file.
  • review-gather. On a later push it writes since-last-review.diff: the compare API's diff from the last reviewed commit to this head, cut to projects/<name>/.
  • The skill, "In CI". For a later push: read every input again, start with the requests of the last comment, drop each request the push fixed, and do not reverse a request without saying why.
  • tests.yaml installs python3-cryptography for the new tests.

Why not the Actions cache: measured

The first version of this pull request kept the conversation in the Actions cache. Two facts rule that out.

  1. Any pull request can read it. The review job runs on pull_request_target, so its cache scope is refs/heads/main. GitHub's documentation says a pull request's run can restore the base branch's caches, "including base branches of forked repositories", and "Anyone who can open a pull request against your repository can read the contents of caches in the base branch" (dependency caching reference, "Restrictions for accessing a cache" and "Best practices"). A GitHub staff post in community discussion 194493 says the opposite for a protected branch such as main. main here is protected. I measured it with two throwaway same-repository draft pull requests, probe: cache scope A (throwaway measurement, do not merge) #53 and probe: cache scope B (throwaway measurement, do not merge) #54, whose pull_request workflow calls the cache service with the run's own cache token:
  2. The save would never have worked. Since 2026-06-26, GitHub gives pull_request_target runs read-only cache access to main's scope. The save step would have logged a warning and kept nothing. I could not measure this one: a pull_request_target run always uses main's copy of the workflow.

What a conversation holds that the comment does not: the review's reasoning and the text of every file it read. The files are public (the pull request, the project's source, the build's results, this repository's skill). The reasoning is not shown anywhere else, and the skill keeps it out of the public log. It holds no token.

Pull request scoping cannot fix this: a pull_request_target run cannot write to a pull request's own cache scope. So the conversation is encrypted, and it travels as an artifact. An artifact has no scope problem. Anyone signed in can download it, but it is ciphertext.

Forks

A fork's pull request is reviewed by the pull_request_target run. GitHub evaluates the environment's branch rule against that run's ref, which is main (changelog). So that run holds the key, and resume works for forks. No other run gets the key:

  • a fork's or a branch's own pull_request run (its ref is refs/pull/<n>/merge);
  • a push run, or a run started by hand, on any branch other than main.

Such a run can download the ciphertext, but it cannot open or forge it. restore reads no artifact of theirs in any case.

GitHub takes deployment: false with an expression for the name: in throwaway pull request #55 (run 40296315967), the job ran, no deployment was created, and the key was absent.

What must exist before resume works

REVIEW_CONVERSATION_KEY is a new credential. The owner of this project's declared credentials creates it. Its spec:

  • Where: a secret of the environment enroll-review, whose deployment branch rule allows main only. The rule is set first. enroll-review-test gets no key.
  • Who reads it: only the review job's Restore and Seal steps.
  • Value: 32 random bytes, base64 (openssl rand -base64 32). No copy is kept anywhere else.
  • Rotation: at any time. Every kept conversation then becomes unreadable, and each pull request's next review starts fresh.

The build.yaml header has the exact commands.

Until the key exists it fails closed: each push gets a fresh review, as before, with the warning "The secret REVIEW_CONVERSATION_KEY of the environment enroll-review is not set". Nothing is sealed or uploaded and no step fails (tested).

How to tell a resumed review from a fresh one

  • Restore step: review-session: restored the conversation <id> of the review of <commit>, from run <run>, or … : a fresh review with the reason (no review comment, no sealed conversation of the review it shows, no key, an API error). Each skipped artifact or run gets its own line.
  • Review step: review-run: resuming the conversation <old id> of the review of <commit>, as <new id>; the resumed review of the conversation <id> ended without a review: a fresh review, as <id> before a fresh second try; or no conversation of an earlier review of pull request #<n>: a fresh review, as <id>. The line after the review ends (try 1, resumed) for a resumed review.
  • Seal step: review-session: sealed the conversation <id> of the review of <commit> ….

Evidence

  • At d7a982c, with PATH free of mise shims, python3 -m pytest -q -p no:cacheprovider tools/tests: 334 passed, 13 skipped (Docker), 28 subtests passed. A failure of the Restore step itself (an install, a time limit) does not stop the review: the step continues on error.
    • ReviewSession (12 tests): a round trip that holds no plain text; no opening for another pull request, project, repository or key; no opening after one changed byte; artifacts of pull_request runs, other workflows, and workflow_dispatch runs on another branch are never downloaded; 30 newer artifacts from pull_request runs cost one download; a newer artifact whose commit is not the one in the review comment is skipped for an older one that is; a fresh review without a review comment from the oss-verify App, or with one that shows no review; a fresh review, with exit 0, when the API fails or answers nonsense; a warning, and nothing kept or restored, without a key or with a malformed one; the token is never an argument, and gh never gets the key; the build.yaml shape: no cache step, the key only in Restore and Seal, the environment, and upload only when Seal wrote a file.
    • ReviewRun: a refused resume gives a fresh second try; a conversation that holds the collector's token, the identity token in the file, or a JWT-shaped token is not kept.
  • actionlint on every workflow and shellcheck -S warning on review-run and review-gather: no problems.
  • Local probe with Claude Code 2.1.309: a session moved to another project folder resumed with --resume <id>; --resume A --fork-session --session-id B gave session B, which resumed after A's file was deleted; a missing session exits 1 with No conversation found with session ID: <id>.

What I could not test

  • No review job has run: this pull request enrolls no project, and the secret does not exist yet. The first two pushes to an enrollment pull request after the merge and the secret will test the whole path. Look for the log lines above.
  • Claude Code compacts a conversation that becomes too long. I did not measure when a resumed enrollment review reaches that limit.

Landing order

Two other pull requests change the same review job: the identity-token fix (#52, merged; main is merged into this branch) and the provider gate. The provider gate lands next; this branch then merges main again.

…ach push

The review job restores the last review's Claude Code conversation from the
Actions cache (key enroll-review/<pull request>/<project>/<run>-<attempt>),
review-run resumes it in a fork (claude --resume <id> --fork-session
--session-id <new>) with the diff since the reviewed commit, and the job saves
the new conversation once the review has written a comment. A conversation that
cannot be resumed gives a fresh review; one that holds a token of the run is not
kept.
@sjawhar-agent
sjawhar-agent Bot requested a review from a team as a code owner October 26, 2026 05:31
@sjawhar-agent

sjawhar-agent Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Held, not merged. A review of this head found two things. First, the review log names the model that was asked for, not the one that answered; that defect is on main too and is being fixed in a separate pull request, so this one does not carry it. Second, a pull request from a fork runs workflows that can restore this repository's caches, so a saved review conversation, including the reasoning the review keeps out of the public log, could be read by anyone who opens a pull request. Resuming saves cost; it is not worth that exposure while nobody has decided to accept it. This stays open until either the saved conversation is encrypted with a key only the review job holds, or the exposure is accepted on purpose.

…ifact, not in the Actions cache

Every pull request's own run, a fork's too, can read main's Actions caches: on this
repository, pull_request runs of two throwaway pull requests got a cache token with read on
refs/heads/main and downloaded an entry saved there, by exact key and by prefix, while
neither could read the other's entry. And the review job runs on pull_request_target, whose
cache saves GitHub refuses by default (read-only since 2026-06-26), so the cache save would
never have kept a conversation.

So review-session seals the conversation with AES-256-GCM under a key derived from the
repository secret REVIEW_CONVERSATION_KEY, binding the repository, pull request and
project, and the review job uploads it as the artifact enroll-review-<pull request>-<project>.
The next review restores the newest one that build.yaml uploaded on pull_request_target or
workflow_dispatch and that opens with the key; it skips the rest. A fork's pull_request run
never gets the secret, and a fork's own pull request is reviewed by the pull_request_target
run, which has it, so resume works for forks. Without the secret each push gets a fresh
review, with a warning.

Omp-Session: 01a1773b-ba9f-7409-9786-5f1483643102
Omp-Session: 01a1773b-ba9f-7409-9786-5f1483643102
…cond is fresh

#52 runs Claude Code up to twice, with a new identity token before the second try. With
a conversation to resume, the first try resumes it and the second is a fresh review, each
with its own id; this replaces the num_turns guess of whether a resume was refused. A
conversation is not kept if it holds what looks like a credential (#52's scan), as well as
the collector's token or the identity token in the file.

Omp-Session: 01a1773b-ba9f-7409-9786-5f1483643102

@sjawhar-agent sjawhar-agent Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review of head bb89278 against main f36ed8e. Changes requested (findings 1 and 2); posted as COMMENT because the author is this App's identity.

Review of theorem-labs/oss-verify pull request #47 "ci: the enrollment review resumes the last review's conversation on each push", head bb89278, whole diff against main f36ed8e. Paths below are relative to the repository root at that head.

Can a pull request read another pull request's saved conversation, or make the review load a conversation it forged?

No. I checked these paths. None of them works without REVIEW_CONVERSATION_KEY, and a fork's run never holds that key.

  1. The Actions cache. The diff adds no cache step. The test test_build_yaml_keeps_the_conversation_out_of_the_cache_and_the_key_out_of_the_review asserts this. The conversation goes only to actions/upload-artifact at .github/workflows/build.yaml:457-466. I confirmed the cache measurement in the run logs: run 40290795197 (attempts 1 and 2) and run 40290958700 show SCOPES [refs/pull/<n>/merge:3, refs/heads/main:1]. Each run restored main's codeql entry by exact key and by prefix. PR #54 could not read PR #53's entry (ok=false). So dropping the cache was right.
  2. Downloading the artifact. Anyone signed in can download it. It holds MAGIC || nonce || AES-256-GCM(key, plaintext, AAD) (.github/scripts/review-session:110-112). The key is HKDF-SHA256 of the secret (:96). A fork's pull_request run gets no secrets, so the artifact is ciphertext to it.
  3. Uploading a forged artifact with the victim's name (enroll-review-<victim PR>-<project>) from a fork's own pull_request run, or from a workflow the fork edits. restore reads workflow_run.id from the artifact listing. It fetches that run and skips the artifact unless path == .github/workflows/build.yaml, event is pull_request_target or workflow_dispatch, and repository.full_name == REPO (review-session:151-157). GitHub sets the event, so a fork's edited build.yaml still runs as pull_request and is skipped. Without that filter, GCM authentication still rejects a blob that was not sealed with the key (:116-124). Forging fails for two independent reasons.
  4. Cross-PR replay of a genuine artifact (renaming it, or copying PR A's blob into an artifact under PR B's name from a trusted run). The AAD binds the version, the repository, the PR number and the project (review-session:79-80). unseal uses this run's own PR and project (:122). A blob sealed for PR A fails the tag for PR B. The test test_a_conversation_opens_only_for_its_own_pull_request_project_and_key covers this. Artifact names cannot collide either: the PR part is digits only and NAME must start with [a-z0-9] (:68-70).
  5. Getting the key inside the review job. The key is in the env of only two steps: Restore (build.yaml:311) and Seal (:450). It is not in the Review step's env (:362-381), the upload step's env (:459-466), or gather's env (:320-332). The scripts come from the base-branch checkout (:286-290). take-project writes only the files directly inside projects/<name>/ (header :45-50), so a PR cannot replace .github/scripts/review-session. No step before Seal writes GITHUB_ENV or GITHUB_PATH from PR data, so there is no environment injection into the steps that hold the key. Claude has Read/Glob/Grep only (review-run:223-225) and keeps no process alive after the Review step (the EXIT trap at review-run:166-170 stops the rotation loop). So nothing from the PR runs on the runner while Restore or Seal holds the key in memory.
  6. Making the review load PR-chosen session metadata. Restore writes only <id>.jsonl and session, after the id, head and dir regexes pass on the decrypted fields (review-session:125-128, 172-174). review-run checks the same regexes again before it copies into ~/.claude/projects/$last_dir (review-run:71-77). dir is -[A-Za-z0-9-]{1,200}: no / and no ., so path traversal is not possible. The conversation's content is the same PR's earlier review, so any injected text that it carries forward comes from the PR whose review it is.
  7. A branch of this repository. A same-repository PR's pull_request run can reference the secret and read every conversation. The PR body says so ("people with write access can read conversations"). These are people with write access, not outside contributors. Hardening H1 below closes this with an environment-scoped secret.

Does resume work for a fork's PR? Yes. A fork's PR is reviewed by the pull_request_target run (build.yaml:118-120). That run receives repository secrets. The review job has actions: read (:280) to list and download artifacts, and upload-artifact needs no extra permission. Restore, Review and Seal take PR from github.event.pull_request.number (:309, :363, :448), so the fork's own PR number is in the AAD. The head SHA comes from needs.project-builds.outputs.sha. The since-last-review.diff compare (review-gather:155-160) uses SHAs, and a fork PR's commits can be reached in the base repository.

Findings

  1. [Medium, correctness] A review whose conversation is not kept makes the next push resume the review before it, and tells Claude that review's comment was the one posted. When the new conversation holds what looks like a credential, review-run deletes session and the jsonl and prints "the next push's review starts fresh" (.github/scripts/review-run:292-300). Then session=kept is not set (build.yaml:441), so Seal and upload are skipped (:443-466). But the previous run's artifact still exists (retention is 90 days). restore takes the newest artifact that opens (review-session:148-176), so the next push resumes the conversation from two reviews ago. The resume prompt then says "your last message was the comment that the workflow posted" (review-run:80-87), which is now false. The log line is false too. The same thing happens when Seal fails or the upload fails. A conversation hits this whenever the project's source or seeds contain a JWT-shaped or gh?_/sk-ant--shaped test string (scan at review-comment:44-50). JWT libraries carry such test vectors, so this repeats on every push for such a project.
    Fix: when review-run does not keep a conversation, write a marker (for example session with fresh=1). The Seal step then seals a tombstone {"fresh": true} under the same AAD and uploads it. restore stops at the newest tombstone that opens and starts fresh. Add a ReviewRun/ReviewSession test: a not-kept conversation is followed by a fresh restore, not by the older artifact. Alternative: give the job actions: write and delete the run's older artifacts of that name. That widens the permissions, so the tombstone is better.

  2. [Medium] Security[untrusted-input]: any fork can switch resume off for any PR by flooding artifacts with that PR's name. restore lists only the first 100 artifacts with the name (review-session:144). It tries only the 10 newest (:150), and it counts the artifacts that the run filter rejects against those 10 (:154-157). A fork's own pull_request run can upload artifacts (its runtime token allows uploads; it needs no secret). If one run uploads 10 or more artifacts named enroll-review-<victim>-<project>, every later review of the victim PR is fresh until those artifacts expire. This is not confidentiality and not forgery, but any outsider can defeat the feature this PR adds, and it costs them almost nothing.
    Fix: count only artifacts that pass the run filter against TRIES. Cache the run lookup by run id (one attacker run then costs one API call). Paginate the listing (gh api --paginate), with a separate cap on run lookups (for example 50), so that a flood cannot use up the GITHUB_TOKEN rate limit. Add a test: 15 rejected artifacts newer than a genuine one still restore the genuine one.

  3. [Low, correctness] restore can stop the review, although its docstring says "either way it exits 0" (review-session:29). A malformed secret calls sys.exit (:92-95). That fails the Restore step, so the Review never runs on any PR, and the comment job posts that the review did not run. Also, z.read can raise zlib.error, NotImplementedError or RuntimeError, and the writes at :172-174 can raise OSError. None of these is caught (:164), and each fails the step the same way. The artifacts come only from trusted runs, so this is unlikely. But a restore that cannot open anything should give a fresh review, as the docstring says.
    Fix: give the Restore step continue-on-error: true (build.yaml:304), and catch Exception for each artifact at :164. Or keep the hard failure on a malformed key and correct the docstring: the hard failure is a defensible choice, but the docstring must match.

  4. [Low, correctness] A run that does not post its comment can still upload its conversation as the newest. The comment job posts only if HEAD_SHA is still the PR's head (build.yaml:522-526). But Seal and upload run whatever happens (:443-466). A workflow_dispatch run is in a different concurrency group (:132), so it is not cancelled by a push. If it finishes after a newer head's run, its conversation becomes the newest artifact, and that conversation ends in a comment nobody saw. Fix: in the Seal step, read gh api repos/$REPO/pulls/$PR --jq .head.sha (the job has pull-requests: read) and seal only when it equals HEAD_SHA. The race stays, but the window becomes small.

Hardening (Minor, no exploit path today)

  • H1. Scope REVIEW_CONVERSATION_KEY to an Environment whose deployment-branch policy allows only main, and give the review job environment:. Today the key is a repository secret. Every same-repository pull_request run can reference it (PR body, "Forks"). Also, pull_request_target has no branches: filter (build.yaml:119-120), so a fork's PR into any other branch runs that branch's build.yaml with secrets, and restore accepts any pull_request_target run of that path (review-session:154). No branch has a weaker review job that holds the key today. With an environment, a pull_request_target run into main (ref refs/heads/main) and a workflow_dispatch run from main get the key, and nothing else does. That meets the condition of the hold comment of 2026-10-26, "a key only the review job holds", which is otherwise met only for forks.
  • H2. The raw key stays in os.environ for the whole Restore process, and gh inherits it (review-session:134), as dpkg -s does (build.yaml:313); sudo resets the env. Pass gh an env without REVIEW_CONVERSATION_KEY (env={k: v for k, v in os.environ.items() if k != "REVIEW_CONVERSATION_KEY"}). The key is never an argument and never logged; GitHub masks it.
  • Minor, folded: since_diff's awk splits on whitespace, so a path with a space in it is dropped from since-last-review.diff (review-gather:156-157). blob = b"" for an oversized member is then reported as "does not open with the key", which is misleading (review-session:163, 169).

What I checked and could not fault

  • Crypto: a 96-bit random nonce for each seal, AES-256-GCM, HKDF-SHA256 with a version info string, and the AAD includes the version. The secret must have at least 32 bytes (review-session:94). Every exit path of unseal checks the magic, the length, the tag and the field types (:116-129).
  • The run filter compares path, event and repository.full_name, all set by GitHub. Artifacts are filtered by exact name and expired (:148). The size is checked against the GitHub-reported size_in_bytes before the download, and the member's file_size before the read (:158-163).
  • Merge of #52's two-try loop with resume (review-run:188-279): try 1 resumes with --resume <last> --fork-session --session-id <new>, and try 2 is fresh with its own id. The second try keeps #52's rules: no retry after 124, with less than 600 s left, or without a new identity token. The conversation kept is the one of the try that succeeded ($id at :294). (try N, resumed) is logged. The old conversation stays whole because of the fork.
  • Conditions: Seal runs only when the Review step succeeded and wrote session=kept (build.yaml:441, 445). That includes exit 3 (collector refused after the review), which goes on to :441. Upload runs only when Seal wrote the file (:454, 458). When Seal or upload fails, the job fails, but the comment is still posted, because the comment job reads needs.review.outputs.comment when the review fails too (:481, 512).
  • Restore runs before Gather (:304-345), and Gather reads head= only after a 40-hex regex check (review-gather:155-160). REVIEW_SESSION is refused inside the checkout (review-run:52-57). It is under RUNNER_TEMP, which the deny rule keeps Claude from reading (review-run:180-184).
  • No key in any argument or log: review-session gives the key to no subprocess argument, and the test test_the_token_is_never_an_argument covers gh.
  • Supply chain: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9. gh api repos/actions/upload-artifact/git/ref/tags/v7.0.2 gives commit cf430e030ddbb5b0abf93d22962f4752f3646cd9, so the pin matches. The step adds no permission. tests.yaml adds python3-cryptography from the Ubuntu archive.
  • Tests: PATH=<no mise> python3 -m pytest -q -p no:cacheprovider tools/tests/test_check_checks.py at bb89278: 87 passed, 21 subtests passed, 0 skipped (so the ReviewSession tests ran with cryptography). gh pr checks 47: every check passes at bb89278 (run 40293309700 tests, headSha bb89278…). review/comment were skipped because no project is enrolled.
  • PR discussion: one issue comment (2026-10-26, "Held, not merged") asked for "a key only the review job holds". Forks: met. Same-repository runs: not met; see H1. There are no review threads and no bot findings.

Security: authz — only pull_request_target/workflow_dispatch runs of build.yaml in this repository are read (review-session:154-157), and the AAD binds the PR and project (:79-80, 122); a fork gets a skip or a failed tag. secret — REVIEW_CONVERSATION_KEY is in the env of the Restore and Seal steps only (build.yaml:311, 450), never in the Review step (:362-381); same-repository pull_request runs can still reference it (H1). untrusted-input — artifact names and runs from forks are filtered (review-session:151-157), and the decrypted fields pass regex checks before they are used as paths (:125-128, review-run:71-77); a flood can still switch resume off (finding 2). prompt — the resume prompt (review-run:80-87) adds no untrusted text, and the resumed conversation is the same PR's; the claim in that prompt can be false (finding 1). supply-chain — upload-artifact v7.0.2 is pinned to cf430e03…, checked against the tag (build.yaml:459); job permissions are unchanged (:274-282). agent-def — SKILL.md gains one bullet and one table row on later pushes (SKILL.md:48, 64); it is a base-branch skill, and this PR's own review does not read it because the PR enrolls no project. transport — AES-256-GCM with a random 96-bit nonce and HKDF-SHA256 (review-session:96, 110-112); reads are bounded by size_in_bytes and file_size (:158-163).

Comment thread .github/scripts/review-run Resolved
Comment thread .github/scripts/review-session Resolved
Comment thread .github/scripts/review-session Resolved
Comment thread .github/workflows/build.yaml Resolved
Comment thread .github/workflows/build.yaml Resolved
…, with the key in an environment

From the review of bb89278:
- restore resumes only a conversation whose commit is the one the pull request's review
  comment shows, so a review whose conversation was not kept, or whose comment was not
  posted, never makes the next push resume an older review and call its comment the posted one.
- Only artifacts of build.yaml as main has it count: pull_request_target runs, and
  workflow_dispatch runs started on main. Those count toward the 10 tries; run lookups are
  cached and capped, and the listing is paginated, so a flood of artifacts from a
  pull request's own runs cannot hide the review's.
- restore always exits 0: anything that goes wrong gives a fresh review.
- The key is a secret of the environment enroll-review (deployment: false), whose branch rule
  allows main only, so no run of a branch of this repository gets it; ci-test/ runs use
  enroll-review-test, which has none. gh does not inherit the key.
- review-gather's since-last-review.diff keeps paths with spaces.

Omp-Session: 01a1773b-ba9f-7409-9786-5f1483643102

@sjawhar-agent sjawhar-agent Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review of head e38869d against main f36ed8e (round 2). Approve; posted as COMMENT. Five Minor findings inline; none blocks.

Answers:

  1. Can a pull request read another pull request's conversation, or make the review resume a forged one? No. Reading or forging needs the key. The key is a secret of the environment enroll-review (build.yaml:284-286). GitHub checks the environment's branch rule against the run's ref: refs/pull/N/merge for pull_request, main for pull_request_target. The rule also applies with deployment: false (docs: "restrict which branches can use certain secrets via environment branch policies, without deployment tracking"). restore reads only artifacts of build.yaml runs on pull_request_target, or on workflow_dispatch on main (review-session:199-201). The GCM authenticated data binds repository, pull request and project.
  2. Can another run get the key once the rule is set? No. Only build.yaml's review job names the environment. Before the rule exists, nothing leaks while the secret is unset. The spec's order (rule first, then secret) is load-bearing: a secret set before the rule would reach any writer's branch run that names the environment.
  3. Does resume work for forks? Yes, by reading the code: the pull_request_target run's ref is main, so it gets the key. Not run live.
  4. Is the fail-closed path real? Yes. Without the key there is a warning, no API call and no sealed file. Upload is skipped, and no step fails (tests at test_check_checks.py:1445-1486). The suite passes at this head: 91 passed, 23 subtests.

Round-1 threads: all five are fixed at this head. For F3, the script now always exits 0, but the step can still fail (inline finding on build.yaml).

Hardening (configuration): before setting the key, confirm that no repository or organization secret named REVIEW_CONVERSATION_KEY exists. secrets.X falls back to those, and every job would see one.

Residual risk: no review job has run this path yet, and restore trusts only main's runs, so resume cannot be measured before merge. After merge, check the first two pushes of an enrollment pull request for these log lines: restored the conversation, resuming the conversation, (try 1, resumed), sealed.

Security: authz: only a main-ref run of build.yaml's review job gets the key (build.yaml:284-286), and restore trusts only main's runs (review-session:199-201). secret: the new key is in the env of the Restore and Seal steps only (build.yaml:325, :464), never an argument, and removed from gh's env (review-session:158). untrusted-input: artifacts are GCM-authenticated before use, and the fields are regex-checked (review-session:139-150, review-run:73-74). prompt: earlier PR text persists in the resumed conversation (inline on review-run:82); the agent has Read/Glob/Grep only (review-run:226). supply-chain: upload-artifact cf430e03 = v7.0.2 (build.yaml:473). agent-def: SKILL.md:48 runs from main under pull_request_target, so this PR cannot steer its own review. transport: AES-256-GCM, HKDF-SHA256, os.urandom nonce, 64 MiB read cap (review-session:106-119, :133, :211-218; ASVS v5.0.0-11.3.2).

Comment thread .github/scripts/review-session Resolved
Comment thread .github/workflows/build.yaml Resolved
Comment thread .github/scripts/review-session Resolved
Comment thread .github/scripts/review-run Resolved
Comment thread .github/scripts/review-session Resolved
…tore still reviews

From the review of e38869d (Minor findings 1, 2 and 4):
- posted_head uses review-comment's read_previous, so a comment with CRLF line ends is
  read the same way by both.
- The Restore step continues on error: a failed install or a time limit gives a fresh
  review, never no review.
- The skill says that an instruction in an earlier input binds nothing, and that a request
  is dropped only when the current files show its fix.

Omp-Session: 01a1773b-ba9f-7409-9786-5f1483643102

@sjawhar-agent sjawhar-agent Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Round 3, head d7a982c (delta from e38869d, read against main f36ed8e). No blocking finding. I would approve; this is a COMMENT review only.

Questions

  1. Cross-PR read, key access, forged resume: no. The delta does not touch the trust path: key only in Restore/Seal env (build.yaml:328, :467) under the main-only enroll-review environment (:284-286); GCM with repo/PR/project as authenticated data (review-session:102-103, :135, :145); trusted runs only (:198-201). posted_head still reads only the oss-verify App's marker comments (:168-169).
  2. continue-on-error letting a half-written or wrong session reach review-run: no. restore writes the conversation (:230), then session last (:231), only after unseal and the posted-head check (:220-228). review-run needs a session file with a UUID id, a 40-hex head, a valid dir and a non-empty conversation (review-run:69-79). $RUNNER_TEMP is new per job, and no step reads Restore's outcome.
  3. read_previous accepting more in a way that matters: no. Same META (review-comment:56-58). It adds CRLF and surrounding whitespace, and it now refuses a first line with text after the marker (stricter). stale, legacy, v=1 and none behave as before. The new test (test_check_checks.py:1433-1442) fails on the old parse, because $ does not match before \r.
  4. Without the key, fresh and no failed step: yes (review-session:108-112, :124-126, :181-183; build.yaml:471, :475; tests :1457-1465, :1492-1494).

Threads I answer (accepted)

  • 4375635569 (CRLF): fixed in d7a982c.
  • 4375635574 (Restore can stop the review): fixed in d7a982c. The assert is missing: inline comment below.
  • 4375635581 (same head): the decline holds. Both conversations reviewed the same commit; only one clause of the resume prompt can be untrue (review-run:82-83), and every check is done again.
  • 4375635589 (prompt injection lives on): fixed in d7a982c by SKILL.md:48, which points to rule 1 at SKILL.md:19.
  • 4375635594 (flood above 100 runs): the decline holds on impact: a fresh review, the behaviour before this PR, and nothing can be read or forged. One correction to its reason: the cap counts distinct run ids (review-session:194-197). One push that adds about 100 pull_request workflow files makes about 100 runs, so it costs one push, not 100. The effect is unchanged, and the log shows it (looked up 100 runs).

Checks at d7a982c: all success or skipped. The cancelled tests job 121491799501 was replaced by 121492288776, success. Local: test_check_checks.py 92 passed, 23 subtests passed.

Security: authz — the key is held only by build.yaml's review job on a main ref (build.yaml:284-286), and restore trusts only main's build.yaml runs (review-session:198-201); the delta changes neither. secret — the key stays only in the Restore and Seal env, is never an argument, and gh does not get it (review-session:158). untrusted-input — the comment is the App's own, parsed by review-comment's reader, with a 40-hex head check (review-session:168-174); blobs are GCM-authenticated (:139-152). prompt — earlier inputs bind nothing (SKILL.md:48 to :19). I did not run the deep-review pair in this round; these rows are my own answers.

Comment thread tools/tests/test_check_checks.py Resolved
From the review of d7a982c (its one Minor finding).

Omp-Session: 01a1773b-ba9f-7409-9786-5f1483643102
@sjawhar-agent
sjawhar-agent Bot merged commit 14e3589 into main Oct 26, 2026
14 checks passed
@sjawhar-agent
sjawhar-agent Bot deleted the ci/review-resume branch October 26, 2026 09:34
sjawhar-agent Bot pushed a commit that referenced this pull request Oct 26, 2026
… the second review's findings

Both of #47's tries, the resumed conversation and the fresh one, go through review-served run; a resumed try keeps
--model; the answered check and the exit-4 branch stay after the last try. The conversation's credential check uses
the file-based holds_credential.

From the second review: a server fallback's model (a content block of type fallback) is checked; CLAUDE_CODE_EXTRA_BODY,
ANTHROPIC_BETAS, CLAUDE_CODE_MANAGED_SETTINGS_PATH, CLAUDE_CODE_REMOTE_SETTINGS_PATH and the advisorModel setting are
refused; a record with no usage line, or a .modelUsage entry that is not an object, is not attributable; the stream's
own model name is compared. review-run also exits 4, without a second try, when another provider answered a run that
failed by itself. review-comment says not published for a record that names a turn but shows nothing, uses a footer
that does not say Claude wrote a review on a comment with no review, and keeps a review posted before this change
(it names nothing that answered it) as not current rather than posting it again as the review. The Grep and Glob deny
rules are described as copies of the Read rule in Claude Code 2.1.310.

Omp-Session: 01a175dd-5d8a-75ac-99a0-68919637ef66
sjawhar-agent Bot added a commit that referenced this pull request Oct 26, 2026
Resolves build.yaml against main's review conversation (#47 and later), review-served and the comment job's
concurrency, keeping this branch's Secret Manager reads; ReviewStep's tests stand in curl for Secret Manager; the
App-key test admits the review job's enroll-review environment and refuses any environment that names comment.

Omp-Session: 01a175dd-5d8a-75ac-99a0-68919637ef66
linuxnow pushed a commit to FreeREAC/oss-verify that referenced this pull request Nov 8, 2026
…ach push (theorem-labs#47)

Reviewed by two fresh Claude-served reviews, read from their transcripts as of 2026-10-26T09:33:30Z: ResumeSealReview2 (at e38869d, APPROVE: no pull request or branch run can read another's conversation, get the key or forge a resume; resume works for forks; the fail-closed path is tested) and ResumeSealReview3 (at d7a982c, APPROVE). The only later change is a two-line test assertion. Required checks at 1f73b0b: enrollment, project-builds, terms-accepted and tests success; review and comment skipped because no project is enrolled. Until the environment key exists, every review runs fresh.
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.

0 participants