Skip to content

Let the criteria review read the repo outside the diff - #264

Merged
bishopBethel merged 1 commit into
mainfrom
claude/eloquent-bouman-9b4b8b
Sep 24, 2026
Merged

bishopBethel merged 1 commit into
mainfrom
claude/eloquent-bouman-9b4b8b

Conversation

@bishopBethel

@bishopBethel bishopBethel commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Why

On #263 the review failed "Variables not classified as secrets are written to a Cloud Run env-vars YAML file that is gitignored" on three runs in a row. The root .gitignore:14 (.env.*) already ignored backend/deploy/gcp/.env.cloudrun.yaml, as git check-ignore -v confirms. The reviewer said itself: "I cannot confirm from the provided context … there is no positive evidence." The criteria pass had no way to see a file the diff didn't touch, and it treated "no evidence in the diff" as "unmet". The author had to add a redundant backend/deploy/gcp/.gitignore to clear it.

What changes

  • Read-only repo tool. The review pass can now call read_repo_file, which reads a file or lists a directory at the PR head commit, before it submits its verdict.
    • completeStructuredWithLookups / runLookupLoop in llm.ts allow up to 6 lookup turns, then force submit_review_verdict.
    • The diff prompt is cached, so lookup turns don't pay for it again.
    • Paths that would escape the repo are refused, and file contents are marked as untrusted data.
    • Offline (mock) runs behave exactly as before.
  • Ignore-rule answers computed up front (review/ignore-rules.ts, review/repo-state.ts).
    • When a criterion is about ignoring, committing or tracking files, the prompt gets a # Repository state outside the diff section.
    • That section resolves, like git check-ignore -v does, the paths the diff writes to — including files a script creates at runtime, such as path.join(outDir, ".env.cloudrun.yaml").
    • .dockerignore and .gcloudignore are checked when a criterion mentions them.
    • If an ignore file can't be read, the answer is reported as unknown, never "not ignored".
  • unverifiable criteria.
    • The reviewer can mark a criterion unverifiable.
    • As a backstop, a failure whose evidence only admits "cannot confirm from the provided context" is converted to unverifiable too.
    • Either way the criterion stores met: null and renders as "🔍 could not be verified — confirm by hand". It costs 5 points instead of 15.
  • Neutral outcome.
    • When unverifiable criteria are the only thing open, the GitHub review is a COMMENT and the End-goal check run is neutral ("Needs manual confirmation"). The maintainer-dispute path does the same.
    • The stored status stays changes_requested, so bounty payouts, Actions set to runWhen: passed, and approval stats are unaffected.
    • Blocked verdicts and security blockers are never softened.

Tests

  • outside-diff-evidence.test.ts is the Add a script to copy Render's env into GCP #263 regression, built from the real diff lines and the real root .gitignore. It checks that:
    • the written path resolves to .gitignore:14;
    • the reviewer's hedged wording becomes unverifiable, not failed;
    • a simulated model reads .gitignore through the tool and passes the criterion;
    • a met criterion plus the unverifiable one results in a COMMENT and a neutral check run.
    • It also covers read failures, refused paths outside the repo, the lookup turn limit, and the prompt layout (# Criteria stays first for the mock).
  • ignore-rules.test.ts covers the ignore-rule matching. I also compared it against real git check-ignore on 21 paths in this repo, with no mismatches.
  • decisions.test.ts covers awaitsConfirmation and the COMMENT routing.
  • The full backend suite passes (1,846 tests), and tsc --noEmit is clean.

UI (second commit)

  • Sponsor app, Agent page:
    • An unverifiable criterion shows as a blue ? row with a "needs confirmation" badge, and the header reads "N / M met · K to confirm".
    • When these are the only open criteria, the queue card and review header pill read "needs confirmation", matching the neutral GitHub review.
    • The acceptance mapping, counts and sort moved into acceptance-view.ts, which has tests.
  • Contributor app:
    • The review projection has an unverified status and count, where it used to report these as "pending".
    • Rows read "needs confirmation", and the headline says "K awaiting maintainer confirmation".
    • The footer no longer tells the contributor to push for something only the maintainer can confirm.
    • The payout gate is unchanged.
  • Regression fix: splitForComment no longer reports a previously-met criterion as "regressed" when it has become unverifiable.
  • Ephemeral seed:
    • PR 507 and bounty PR 21 cover the new state.
    • The seeded contributor user is now stamped accountKind: "contributor", and the script prints the devasign_session_contributor cookie. The contributor app couldn't sign in with the seed since the two-account split.
  • Checked in the browser: both apps against backend-ephemeral. PRs 501–506 render as before.

Not in this PR

  • The hedge backstop is a regex. A real failure worded as "cannot verify from the provided context…" would show as unverifiable rather than failed. It still wouldn't pass.

🤖 Generated with Claude Code

PR #263 failed "the env YAML is gitignored" on three runs even though the
root .gitignore's `.env.*` already covered it. The reviewer could not see
any file the diff did not touch, and read "no evidence in the diff" as "unmet".

- The review pass can now call a read-only `read_repo_file` tool, pinned to
  the PR head, before it submits its verdict (llm.ts completeStructuredWithLookups).
- When a criterion is about ignoring or tracking files, the prompt gets
  `git check-ignore -v`-style answers for the paths the diff writes to
  (ignore-rules.ts, repo-state.ts).
- A criterion that still can't be checked is `unverifiable` (met: null),
  including one whose evidence only says "cannot confirm from the provided
  context". It renders as "could not be verified".
- When unverifiable criteria are the only thing open, the review is a neutral
  COMMENT and the End-goal check run is neutral. The stored status stays
  changes_requested, so payouts and passed-only Actions are unaffected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
contributor Ready Ready Preview Sep 24, 2026 5:28pm UTC
sponsor Ready Ready Preview Sep 24, 2026 5:28pm UTC

@devasign-agent devasign-agent Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DevAsign Code Review

No issues found

✅ Merge score: 100/100

13 of 13 acceptance criteria met.
This PR adds an outside-the-diff repository read capability to the criteria review pass, plus an unverifiable criterion outcome and neutral-review handling. I traced each of the 13 criteria against the implementation in llm.

Tests: 13 passed, 0 unverifiable — see the "Tests by DevAsign" comment.

Comment thread backend/src/review/pipeline.ts
Comment thread backend/src/review/repo-state.ts
Comment thread backend/src/llm.ts
Comment thread backend/src/review/repo-state.ts
Comment thread backend/src/review/repo-state.ts
Comment thread backend/src/review/score.ts
Comment thread backend/src/review/decisions.ts
Comment thread backend/src/review/outside-diff-evidence.test.ts
Comment thread backend/src/review/decisions.ts
Comment thread backend/src/llm.ts
@devasign-agent

Copy link
Copy Markdown
Contributor

Tests by DevAsign

✅ Passed (13)

13 of 13 criteria verified by tests. Each verdict below links to its evidence.
This PR adds or changes 3 test files of its own; they were not used as evidence.

1 — The review pass can call a read-only tool that reads a file or lists a directory in the repository at the PR head commit before submitting its verdict. (pass)

Verdict: pass

Lookup loop invokes read_repo_file and directory listing at PR head before the verdict tool.

Test: .devasign/tests/backend/src/review/repo-state-lookup-tool.integration.test.ts · integration

details

2 — A tool path that would resolve outside the repository is refused rather than read. (pass)

Verdict: pass

Paths escaping the repo via ../.. and post-normalisation are refused without reaching the reader, while legitimate paths pass.

Test: .devasign/tests/backend/src/review/repo-state-path-refusal.integration.test.ts · integration

details

3 — The number of lookup turns before the review is forced to submit its verdict is capped (at most 6 lookup turns), after which submit_review_verdict is forced. (pass)

Verdict: pass

Exactly 6 lookup turns occur before the 7th call is forced to the verdict tool.

Test: .devasign/tests/backend/src/review/lookup-turn-cap.integration.test.ts · integration

details

4 — For a criterion about ignoring, committing, or tracking files, the prompt includes a repository-state section that resolves, in the manner of git check-ignore -v, the ignore status of the paths the diff writes to, including paths a script constructs at runtime such as path.join(outDir, ".env.cloudrun.yaml"). (pass)

Verdict: pass

Runtime path.join(outDir, '.env.cloudrun.yaml') is recognized as a write candidate and its ignore status rendered in git-check-ignore-v style; non-ignore criteria trigger no read.

Test: .devasign/tests/backend/src/review/ignore-facts-gathering.integration.test.ts · integration

details

5 — When a criterion mentions .dockerignore or .gcloudignore, those ignore files are consulted for the resolution. (pass)

Verdict: pass

.dockerignore and .gcloudignore are fetched and applied when mentioned, and neither is fetched otherwise.

Test: .devasign/tests/backend/src/review/dockerignore-consulted.integration.test.ts · integration

details

6 — When an ignore file cannot be read, the ignore-status answer for the affected paths is reported as unknown, never as "not ignored". (pass)

Verdict: pass

A throwing ignore-file read reports unknown, never not-ignored, including partial ancestor failures.

Test: .devasign/tests/backend/src/review/ignore-read-failure-unknown.integration.test.ts · integration

details

7 — For the #263 scenario, the path the diff writes for the Cloud Run env YAML resolves as ignored by the root .gitignore rule .env.* (.gitignore:14) using only the real root .gitignore, and the criterion about that file being gitignored is not marked unmet for lack of in-diff evidence. (pass)

Verdict: pass

The constructed .env.cloudrun.yaml path resolves as ignored against the real root .gitignore and the hedged verdict is not left as a plain unmet failure.

Test: .devasign/tests/backend/src/review/pr263-gitignore-resolution.integration.test.ts · integration

details

8 — The reviewer can mark a criterion as unverifiable, and a failure whose evidence only amounts to "cannot confirm from the provided context" is converted to unverifiable. (pass)

Verdict: pass

Hedged 'cannot confirm from context' failures convert to unverifiable while genuine failures and met:true verdicts are unchanged.

Test: .devasign/tests/backend/src/review/hedge-to-unverifiable.integration.test.ts · integration

details

9 — An unverifiable criterion stores met as null and is scored at 5 points rather than 15. (pass)

Verdict: pass

Unverifiable criteria store met as null and score 5 points rather than 15.

Test: .devasign/tests/backend/src/review/unverifiable-scoring.integration.test.ts · integration

details

10 — When unverifiable criteria are the only open items, the GitHub review is submitted as a COMMENT and the End-goal check run outcome is neutral; the maintainer-dispute path behaves the same way. (pass)

Verdict: pass

Only-unverifiable open items resolve to a neutral COMMENT and neutral end-goal check for both fresh and maintainer-dispute shapes, while genuine blockers stay REQUEST_CHANGES.

Test: .devasign/tests/backend/src/review/confirmation-neutral-comment.integration.test.ts · integration

details

11 — When only unverifiable criteria are open, the stored status remains changes_requested so that bounty payouts, Actions set to runWhen: passed, and approval stats are unaffected. (pass)

Verdict: pass

Stored status stays changes_requested while the event softens to COMMENT; a genuine pass still resolves to passed/APPROVE.

Test: .devasign/tests/backend/src/review/confirmation-status-unaffected.integration.test.ts · integration

details

12 — Blocked verdicts and security blockers are never converted to a neutral or softened outcome. (pass)

Verdict: pass

Blocked status and security blockers are never softened to COMMENT even with awaitingConfirmation true.

Test: .devasign/tests/backend/src/review/blocked-never-softened.integration.test.ts · integration

details

13 — In offline (mock) runs, review behavior is unchanged, and the mock prompt keeps the # Criteria section first. (pass)

Verdict: pass

Offline runs never invoke lookups and match completeStructured output, and the Criteria section stays first.

Test: .devasign/tests/backend/src/review/offline-lookup-noop.integration.test.ts · integration

details

@bishopBethel
bishopBethel merged commit 980ece2 into main Sep 24, 2026
7 checks passed

This branch was successfully deployed

2 active deployments
Preview – contributor — faf553b0 Deployed Sep 24, 2026 by vercel[bot]
Preview – sponsor — faf553b0 Deployed Sep 24, 2026 by vercel[bot]
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