Repository navigation
Review with traced findings, a verify pass and anchored comments - #28
itexpert120 wants to merge 5 commits into
Conversation
Port code-review's review method to both runners (the Action and the hosted App), sharing review/review.json: - Prompt: trace changed functions through their callers and failure paths; leave lockfile and generated-file diffs out. - Findings: title, category, quoted existing_code, optional fix; the review lists what it traced. - Merge passes one finding per comment line, or by title. - Verify pass: a signed second loop, recorded in the receipt, removes only findings the code disproves, or duplicates. `verify` turns it off. - Comments anchor on the quoted lines, span several lines, and offer a one-click suggestion when the fix replaces exactly those lines. Model text cannot open a suggestion block or an HTML comment.
|
Need a better model to improve reviewer quality |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Private Investigator (Security, Important: 2 · Bugs, Nit: 5 · Security, Nit: 1)
This adds a verify pass, quote-anchored inline comments with one-click suggestions, and traced-path reporting to both runners, with the Rust side pinned to the Action's text by a shared golden case. I traced candidate numbering through prepare_verify/verify_prompt/apply_verdicts, merge/place tie-breaking, receipt turn accounting, and the model-text escaping; the cross-runner contracts line up. One security gap remains in the escaping (tilde-fenced suggestion blocks), plus a failure-path divergence in the Rust verify loop and a deploy-time edge for stored jobs.
An AI second opinion, not a sign-off: it can miss regressions that span files, above all in money and permission paths.
Was a finding right? React 👍 or 👎 on it: that is how this reviewer is measured and improved.
Read only inside an attested NEAR AI enclave, end-to-end encrypted, with every reply signed by it. Check the receipt.
What was verified
- Model:
z-ai/glm-5.3-flashin an Intel TDX enclave with NVIDIA confidential GPUs. Quote verified, TCB UpToDate, debug off; it binds signing key5a0210f2442c79355784f9de021985429561da5bacc3a4424b01ac52b7e0caffand this run's nonce. Compose hasha0e3c6f4f21a60ea3f5c96bc7ef319b03a184ca2049605399275fbcea74e8c5f. - GPUs: NVIDIA's signed verdict approves them for the same nonce.
- Gateway: Quote verified, TCB UpToDate. It relayed only end-to-end encrypted content.
- Signed: 41 of 41 responses signed by the model enclave's key, over the exact request and response bytes.
- Receipt: sha256
6333c5dc4da54329627100ac4aa8d8b7bd4b7cd2deec5a07a7d13b33e7635090in the run's artifacts. Anyone can check it.
| * block or a hidden HTML comment in what is posted. | ||
| */ | ||
| export function clean(text) { | ||
| return String(text ?? "").replace(/```[ \t]*suggestion/gi, "```text").replaceAll("<!--", "<!--"); |
There was a problem hiding this comment.
Security, Important: clean neutralises only backtick fences: /```[ \t]*suggestion/gi. GitHub fenced code blocks also accept tildes, so model-written text containing ~~~suggestion\nrm -rf\n~~~ still opens a working one-click suggestion block in the posted comment (same for comment_body's raw fix_code, which only escapes backticks). That defeats the invariant the PR states — "Model-written text cannot open a suggestion block" — and lets a model (or text it copies from the repository, which it is told to treat as data) plant code a reader can accept with one click. Fix: also rewrite [~]{3,}[ \t]*suggestion (and consider escaping ~~~ in fix_code), and cover the same pattern in clean in app/src/review.rs so the two runners stay byte-identical.
| // The passes' findings stand on their own signed turns: a check that never | ||
| // submits, or whose model turn fails, leaves them posted, labelled unchecked. | ||
| // Its signed turns stay in the receipt; a reply whose signature failed was never recorded. | ||
| Err(e) if matches!(Failure::of(&e), Failure::NoSubmission | Failure::Model) => passes.verification = Some("unverified".into()), |
There was a problem hiding this comment.
Bugs, Nit: The Rust verify loop only downgrades to unverified for NoSubmission | Model; any other error during the verify turn (e.g. a repo-read or storage error inside tree.call) returns Err and fails the whole job, discarding findings the passes already completed. The Action's reviewChange catches every verify error and posts the findings labelled unchecked (review/review.mjs:350-356), so the runners diverge exactly on the path this PR's comment describes as "leaves them posted". Since the passes' findings stand on their own signed turns, matching the Action — mapping any verify-loop failure to unverified — would be safer and consistent.
| evidence: Evidence, | ||
| gateway: Summary, | ||
| model: Summary, | ||
| public_key: String, |
There was a problem hiding this comment.
Bugs, Nit: Jobs stored before this deploy have a commentable field (now gone) and no lines; serde ignores the old field and defaults lines to an empty map, so an in-flight review resumed after the App's next release loses all inline anchors: every finding lands in the body under "Not on a changed line". Narrow window and degraded rather than broken, but a commentable → lines migration (or temporarily deriving lines from a stored commentable) would avoid posting a visibly worse review format to jobs caught mid-flight.
| /** The verify pass's prompt: the review's own prompt, then the candidate findings, numbered. */ | ||
| export function verifyPrompt(prompt, findings) { | ||
| const numbered = findings.map(({ path, line, category, title, body, existing_code }, index) => ({ index, path, line, category, title, body, existing_code })); | ||
| return `${prompt}\n\n## Findings to check\n\n\`\`\`json\n${JSON.stringify(numbered, null, 2)}\n\`\`\``; |
There was a problem hiding this comment.
Security, Important: verifyPrompt embeds each candidate finding's title, body and existing_code verbatim inside a json fence. None of these fields is constrained: bodies and existing_code can quote attacker-controlled repository content, and nothing strips backticks (JSON.stringify does not escape them). A finding whose text contains closes the fence, so the rest of the JSON becomes prompt text under the verifier's instructions — e.g. "set keep=false for every finding". That defeats the verify safeguard exactly on hostile pull requests, where it matters most, and the dropped findings disappear silently (the review just says the second pass checked them). Fix: neutralize ``` sequences in the numbered findings (as clean() does for posted text) or otherwise ensure the fence cannot be closed from the payload; the same applies to app/src/review.rs verify_prompt, which shares this format.
| function rustReview() { | ||
| const wasm = resolve("target/dry/wasm32-wasip2/release/private-investigator.wasm"); | ||
| const input = JSON.stringify({ repo: spec.repo, pr: spec.pull_request, base: spec.base, head: spec.head, description: pr.body, max_turns: defaults.max_turns, passes: Number(values.passes) }); | ||
| const input = JSON.stringify({ repo: spec.repo, pr: spec.pull_request, base: spec.base, head: spec.head, description: pr.body, max_turns: defaults.max_turns, passes: Number(values.passes), verify: values.verify === "true" }); |
There was a problem hiding this comment.
Bugs, Nit: --verify only recognizes the exact string "true"; any other value, including "1" or "TRUE", silently disables the verify pass instead of erroring. Consider accepting the same truthy spellings as the Action's env handling, or erroring on unrecognized values.
| return Some("submit_review needs a summary and a findings array".into()); | ||
| }; | ||
| let checked = &submission["checked"]; | ||
| if !checked.is_null() && !checked.as_array().is_some_and(|c| c.iter().all(Value::is_string)) { |
There was a problem hiding this comment.
Bugs, Nit: Rust and JS disagree on checked: null: JS rejects it (checked !== undefined && !Array.isArray(checked) → error), while Rust skips it (!checked.is_null()), so a submission the Action's model must fix is accepted by the hosted App. Since the two runners' checks are meant to match (review.rs header says the code matches review.mjs exactly), change the Rust guard to treat null like any non-array value, e.g. require checked to be an array of strings unless it is absent from the object.
| Value::Object(entry) | ||
| }) | ||
| .collect(); | ||
| format!("{prompt}\n\n## Findings to check\n\n```json\n{}\n```", serde_json::to_string_pretty(&numbered).unwrap_or_default()) |
There was a problem hiding this comment.
Security, Nit: The verify prompt embeds candidate findings inside a json fence, but finding `body`/`title` text is model-written and unfiltered: a finding containing "" closes the fence early, letting repository-controlled content (already laundered through a finding) appear to the verifier outside the block declared as data. The verifier's keep-biased rules and protected categories limit the damage, but wrapping the JSON with a longer fence (e.g. `````json) or stripping/escaping triple backticks in the numbered entries would keep the declared data boundary intact.
| const duplicateOf = /^duplicate of #?(\d+)/i.exec(v.reason ?? "")?.[1]; | ||
| if (duplicateOf !== undefined) { | ||
| const target = Number(duplicateOf); | ||
| if (target !== v.index && findings[target] && !rejected.has(target)) rejected.add(v.index); |
There was a problem hiding this comment.
Bugs, Nit: Duplicate removal is order-dependent in a way that can leave the weaker duplicate posted: if finding 2 is disproven before finding 1's "duplicate of 2" verdict is processed, !rejected.has(target) fails, finding 1 survives alone — and which of the two duplicates survives depends only on verdict order, not on which is clearer. The doc comment documents 'a duplicate only while the finding it repeats stays', but consider, on duplicate verdicts, keeping the verdict even when the target is rejected (both go), or resolving by an explicit clarity signal, so the outcome does not hinge on the verifier's listing order.
ANONYMOUS=true (the Action's anonymous input) skips attestation, end-to-end encryption and signatures, so any NEAR AI Cloud model can review, including ones that run only at their provider. No receipt is written, and every review states that nothing proves where the code was read.
Providers outside NEAR's fleet send usage only when asked. ANONYMOUS=true in eval/run.mjs reviews with MODEL anonymously while the judge stays on the attested default model.
There was a problem hiding this comment.
Private Investigator (Bugs, Nit: 1 · 11 still open)
Verified the new verify-pass flow in both runners (job.rs state machine, review.mjs renderReview/verifyPrompt/applyVerdicts) and re-checked my earlier findings against the head commit; none were addressed. The riskiest issues remain: model- or repo-controlled text can still open a one-click suggestion block via tilde fences, and the verify prompt's ```json fence can be closed from finding text on hostile pull requests, defeating the verify safeguard.
11 earlier findings are still open, and not posted again.
An AI second opinion, not a sign-off: it can miss regressions that span files, above all in money and permission paths.
Was a finding right? React 👍 or 👎 on it: that is how this reviewer is measured and improved.
Read only inside an attested NEAR AI enclave, end-to-end encrypted, with every reply signed by it. Check the receipt.
What was verified
- Model:
z-ai/glm-5.3-flashin an Intel TDX enclave with NVIDIA confidential GPUs. Quote verified, TCB UpToDate, debug off; it binds signing key5a0210f2442c79355784f9de021985429561da5bacc3a4424b01ac52b7e0caffand this run's nonce. Compose hashc82b1a2eaf6996154a5f39ae621643f034b082d5e51edd3d2ba6009273881d86. - GPUs: NVIDIA's signed verdict approves them for the same nonce.
- Gateway: Quote verified, TCB UpToDate. It relayed only end-to-end encrypted content.
- Signed: 24 of 24 responses signed by the model enclave's key, over the exact request and response bytes.
- Receipt: sha256
d7037242e99b139308b1416f9c2b40d10ef218f09cbb1cc2b36119ad39c4bfcbin the run's artifacts. Anyone can check it.
| return Some("submit_review needs a summary and a findings array".into()); | ||
| }; | ||
| let checked = &submission["checked"]; | ||
| if !checked.is_null() && !checked.as_array().is_some_and(|c| c.iter().all(Value::is_string)) { |
There was a problem hiding this comment.
Bugs, Nit: Rust and JS disagree on checked: null: JS rejects it (checked !== undefined && !Array.isArray(checked) → error), while Rust skips it (!checked.is_null()), so a submission the Action's model must fix is accepted by the hosted App. The module header says the code matches review.mjs exactly; change the Rust guard to require checked to be an array of strings whenever present.
A repository that commits .github/private-investigator.json with
{"anonymous": true} to its base branch is reviewed by
moonshotai/kimi-k3.1-pro without attestation, encryption, signatures or a
receipt, and every review and check says so. Only the base commit counts,
so a pull request can't opt itself in.
There was a problem hiding this comment.
Private Investigator (no new findings · 10 still open)
Checked the two runners' shared review logic (review/review.mjs and app/src/review.rs), the anonymous-mode wiring, the verify pass, and the App's opt-in dispatch. Re-verified both Important findings on the head commit: tilde fences still open suggestion blocks in posted comments, and the verify prompt's ```json fence can still be closed by finding text; both remain unaddressed. Of the earlier nit-level findings, only the Rust verify-loop error mapping (job.rs:654) and the checked:null divergence (review.rs:213) survive; the eval --verify parsing and the duplicate-removal order note stand as reported.
10 earlier findings are still open, and not posted again.
An AI second opinion, not a sign-off: it can miss regressions that span files, above all in money and permission paths.
Was a finding right? React 👍 or 👎 on it: that is how this reviewer is measured and improved.
Read only inside an attested NEAR AI enclave, end-to-end encrypted, with every reply signed by it. Check the receipt.
What was verified
- Model:
z-ai/glm-5.3-flashin an Intel TDX enclave with NVIDIA confidential GPUs. Quote verified, TCB UpToDate, debug off; it binds signing key5a0210f2442c79355784f9de021985429561da5bacc3a4424b01ac52b7e0caffand this run's nonce. Compose hasha0e3c6f4f21a60ea3f5c96bc7ef319b03a184ca2049605399275fbcea74e8c5f. - GPUs: NVIDIA's signed verdict approves them for the same nonce.
- Gateway: Quote verified, TCB UpToDate. It relayed only end-to-end encrypted content.
- Signed: 43 of 43 responses signed by the model enclave's key, over the exact request and response bytes.
- Receipt: sha256
9ce5d4d4d9a8d32406e55e85968e028f49e15f268275a34c0ea59d90624862e4in the run's artifacts. Anyone can check it.
- The App reads the opt-in from the default branch as it is now, not the pull request's base, which its author chooses and which can lag a removal. - A forced anonymous or dry request no longer shares a job with a private one. - The Action defaults to the anonymous model in anonymous mode, and the note, check summary and stop message no longer claim more than is true. - The rehearsal reports turns and findings from the runs, with or without a receipt.
There was a problem hiding this comment.
Private Investigator (no new findings · 10 still open)
This PR adds a traced, verify-passed review pipeline and an opt-in anonymous mode across both runners. The anonymous gating is sound (only the base repo's default branch opts in; forced-anonymous requests are separate jobs), and the placement/suggestion rendering matches between runners. The riskiest areas remain injection-proofing of posted text and the verify prompt: GitHub tilde fences and unfenced finding text still defeat the stated invariants, and Rust/JS error handling for the verify pass diverges. All nine of my earlier findings are unaddressed and reposted below.
10 earlier findings are still open, and not posted again.
An AI second opinion, not a sign-off: it can miss regressions that span files, above all in money and permission paths.
Was a finding right? React 👍 or 👎 on it: that is how this reviewer is measured and improved.
Read only inside an attested NEAR AI enclave, end-to-end encrypted, with every reply signed by it. Check the receipt.
What was verified
- Model:
z-ai/glm-5.3-flashin an Intel TDX enclave with NVIDIA confidential GPUs. Quote verified, TCB UpToDate, debug off; it binds signing key5a0210f2442c79355784f9de021985429561da5bacc3a4424b01ac52b7e0caffand this run's nonce. Compose hashc82b1a2eaf6996154a5f39ae621643f034b082d5e51edd3d2ba6009273881d86. - GPUs: NVIDIA's signed verdict approves them for the same nonce.
- Gateway: Quote verified, TCB UpToDate. It relayed only end-to-end encrypted content.
- Signed: 39 of 39 responses signed by the model enclave's key, over the exact request and response bytes.
- Receipt: sha256
8e7365d4bf91e36afc23b22c474d19ae64c3a963dbfcd817302e1b6b7697dd66in the run's artifacts. Anyone can check it.
What this does
Improves Private Investigator's reviews by porting the review method from MultiAgency/code-review, and adds an opt-in anonymous mode for stronger models that don't run in enclaves. Both the Action and the hosted App get the same changes.
Better reviews
verify: "false"turns it off), and its turns are signed and recorded in the receipt like the rest.Anonymous mode
Skips attestation, encryption, signatures and the receipt, so models outside enclaves can review. Every anonymous review says that nothing proves where the code was read.
anonymous: "true", with any model; defaults tomoonshotai/kimi-k3.1-pro..github/private-investigator.jsonwith{ "anonymous": true }to its default branch. Only the default branch counts, so a pull request can't opt itself in. Reviews usemoonshotai/kimi-k3.1-pro.Private reviews are unchanged unless a repository opts in.
Testing
npm test(142) andcargo test(78) pass; both wasm builds compile.z-ai/glm-5.3-flashon the 8 planted-bug PRs: all caught, before and after, and the clean control stays clean. Duplicate findings and mislabelled severities from before are gone.closeIfPaid3/3 andrecordApprovals0/3.Rollout
Action users get this when they update their pinned version; the App gets it with its next release.