Skip to content

fix(review-pr): fail closed on incomplete outcomes #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Sayt-0 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(review-pr): fail closed on incomplete outcomes #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 71 additions & 1 deletion .github/workflows/review-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ on:
exit-code:
description: "Exit code from the review"
value: ${{ jobs.review.outputs.exit-code }}
review-status:
description: "API-verified review outcome: completed, completed-with-warnings, incomplete, inconclusive, failed, timed-out, skipped, setup-failed, or unverified (empty when the review job did not run)"
value: ${{ jobs.review.outputs.review-status }}

permissions:
contents: read
Expand Down Expand Up @@ -296,6 +299,7 @@ jobs:
actions: write # cache delete for review-lock release cleanup; feedback artifact list/delete
outputs:
exit-code: ${{ steps.run-review.outputs.exit-code }}
review-status: ${{ steps.run-review.outputs.review-status }}

steps:
- name: Resolve PR number
Expand Down Expand Up @@ -563,10 +567,33 @@ jobs:
env:
CHECK_ID: ${{ steps.create-check.outputs.check-id }}
JOB_STATUS: ${{ job.status }}
REVIEW_STATUS: ${{ steps.run-review.outputs.review-status }}
RUN_REVIEW_OUTCOME: ${{ steps.run-review.outcome }}
with:
github-token: ${{ github.token }}
script: |
const conclusion = process.env.JOB_STATUS === 'cancelled' ? 'cancelled' : process.env.JOB_STATUS === 'success' ? 'success' : 'failure';
// continue-on-error on the review step masks its failure from
// job.status, so the check conclusion is derived from the
// API-verified review-status instead. Only a verified completion
// may be green; an intentional skip is neutral ONLY when the
// composite itself succeeded — a failed step claiming skipped is
// a contradiction that stays red; every other state (incomplete,
// inconclusive, timed-out, failed, setup-failed, unverified, or a
// missing status from a crashed composite) is a failure.
const reviewStatus = process.env.REVIEW_STATUS || '';
const stepOutcome = process.env.RUN_REVIEW_OUTCOME || '';
let conclusion;
if (process.env.JOB_STATUS === 'cancelled') {
conclusion = 'cancelled';
} else if (reviewStatus === 'completed' || reviewStatus === 'completed-with-warnings') {
conclusion = 'success';
} else if (reviewStatus === 'skipped' && stepOutcome !== 'failure') {
conclusion = 'neutral';
} else if (reviewStatus === '' && stepOutcome === 'skipped') {
conclusion = 'neutral';
} else {
conclusion = 'failure';
}
try {
await github.rest.checks.update({
owner: context.repo.owner,
Expand All @@ -580,6 +607,49 @@ jobs:
core.warning(`Failed to update check run: ${error.message}`);
}

# continue-on-error on the review step keeps the cleanup/check update
# above running, but it also masks review failures from the job result.
# This gate restores the honest outcome: only a verified completion (or
# an intentionally skipped review) leaves the reusable workflow green —
# fallback/verification failures must fail the job.
- name: Enforce review outcome
if: always()
shell: bash
env:
REVIEW_STATUS: ${{ steps.run-review.outputs.review-status }}
RUN_REVIEW_OUTCOME: ${{ steps.run-review.outcome }}
run: |
case "$REVIEW_STATUS" in
completed|completed-with-warnings)
echo "✅ Review outcome: $REVIEW_STATUS"
;;
skipped)
# Expected no-op (the composite's concurrent-review lock) — but
# only when the step itself succeeded. A failed step claiming
# skipped is a contradiction that must not leave the job green.
if [ "$RUN_REVIEW_OUTCOME" = "failure" ]; then
echo "::error::Review step failed while reporting review-status=skipped — failing the review job"
exit 1
fi
echo "⏭️ Review intentionally skipped"
;;
*)
if [ -z "$REVIEW_STATUS" ] && [ "$RUN_REVIEW_OUTCOME" = "skipped" ]; then
# The review step never ran: a pre-review guard (draft PR,
# authorization, rate anomaly, non-/review comment) filtered
# this event — an expected skip, not a review failure.
echo "⏭️ Review step skipped by a pre-review guard"
exit 0
fi
# Missing status with a run/failed step means required setup or
# the composite itself failed; any other status (incomplete,
# inconclusive, timed-out, failed, setup-failed, unverified) is
# a non-success review outcome. Neither may leave the job green.
echo "::error::Review outcome '$REVIEW_STATUS' is not a verified completion — failing the review job"
exit 1
;;
esac

# No rate-anomaly gate here (unlike the review job): conversational replies are
# gated by org membership and serialized per-PR by the `concurrency:` group, and
# each reply requires a distinct human comment to trigger it, so they cannot be
Expand Down
16 changes: 14 additions & 2 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,18 @@ Anything else here (workflows under `.github/workflows/`, scripts, tests) exists
│ │ ├── index.ts # CLI entry → bundled to dist/resolve-trigger-context.js
│ │ ├── resolve-trigger-context.ts # Produces canonical context and downstream job outputs.
│ │ └── __tests__/
│ ├── review-assessment/ # PR review Decision Rules spec + trusted posting/attribution runtime.
│ │ ├── index.ts # CLI entry → bundled to dist/review-assessment.js (new-run-nonce,
│ │ │ # finalize-body, classify-run — invoked by review-pr/action.yml).
│ │ ├── review-assessment.ts # Pure assessReview() mirroring pr-review.yaml's Decision Rules
│ │ │ # (neutral 🟢 NO FINDINGS only on zero surviving findings — the
│ │ │ # event is always COMMENT, never APPROVE; incomplete/inconclusive
│ │ │ # never checkpoint) — a tested mirror the model applies, plus the
│ │ │ # runtime helpers: finalize-body validation/marker append and the
│ │ │ # fail-closed post-run review classification (marker + SHA +
│ │ │ # baseline); marker predicates shared with src/incremental-review
│ │ │ # and src/rate-limit.
│ │ └── __tests__/
│ ├── score-confidence/ # Per-finding confidence scoring for the PR review pipeline.
│ │ ├── index.ts # CLI entry → bundled to dist/score-confidence.js
│ │ ├── score-confidence.ts # Core scoreFinding()/scoreFindings() pure functions + posting policy.
Expand Down Expand Up @@ -170,7 +182,7 @@ Anything else here (workflows under `.github/workflows/`, scripts, tests) exists
- `pnpm test:integration` — Vitest "integration" project (`*.integration.test.ts`).
- `tests/*.sh` are integration tests for **shell logic** embedded in YAML (output extraction and job summary in `action.yml`, the release-notes caller-permissions safeguard in `release.yml`). Run them when changing the corresponding bash blocks.
- Security unit tests live in `src/security/__tests__/security.test.ts` (Vitest) and run as part of `pnpm test`. Run them when changing anything under `src/security/`.
- The PR review agent has a separate eval suite under `review-pr/agents/evals/`. Run with `docker agent eval review-pr/agents/pr-review.yaml review-pr/agents/evals/`.
- The PR review agent has a separate eval suite under `review-pr/agents/evals/`. Run with `docker agent eval review-pr/agents/pr-review.yaml review-pr/agents/evals/`. All fixtures run in **console output mode**: the eval schema has no per-eval env field and setup exports never reach the agent process, so posting mode / chunk delegation are covered by deterministic unit tests instead (see "Eval environment limits" in `review-pr/README.md`; `src/pr-review-agent/__tests__/eval-fixtures.test.ts` pins the fixture invariants).

### Security-first design (do not regress)

Expand All @@ -195,7 +207,7 @@ The action runs untrusted input (PR titles, bodies, comments, diffs) through an
- `pull_request` action `review_requested` when `github.event.requested_reviewer.login == 'docker-agent'`
- `@docker-agent` mentions on PR/issue comments — these run the `.github/actions/mention-reply` handler (sets `should-reply` and builds the context prompt) and then the `review-pr/mention-reply` sub-action (referenced from a pinned SHA, not present as a local path on every commit). The `pr-review-mention-reply.yaml` agent handles the actual reply.
- Diffs over 1500 lines are **chunked at file boundaries** in `review-pr/action.yml` (see "Split diff into chunks"). Per-file **risk scoring** (security paths, line counts, error-handling patterns) prioritizes verifier attention.
- **Incremental reviews** (default on, `incremental` input): a re-review only diffs `last-reviewed-SHA..HEAD` instead of the full PR diff. The last reviewed SHA is read from the `commit_id` GitHub records on the bot's completed reviews (assessment or LGTM bodies only — timeout/failure fallbacks don't count), so state survives across runs with no extra writes. `src/incremental-review/` plans the mode and falls back to a full review on force-push/rebase (SHA missing or not an ancestor of HEAD), base-branch merge-ins, net-zero changes, or any error. In incremental mode the original full diff is preserved at `pr_full.diff` for stale-thread resolution and suggestion-anchor validation (GitHub validates anchors against the full PR diff). On re-reviews, `src/dedupe-findings/` (staged at `/tmp/dedupe-findings.js`, run by the agent per `posting-format.md` against the pre-fetched `/tmp/existing_review_comments.json`) drops findings matching already-posted bot comments by file path + line proximity (±3) + finding-heading similarity, so a full re-review after a rebase doesn't duplicate threads. The CLI also accepts the review-thread history snapshot (`/tmp/prior_review_threads.json`) as an optional third argument: current (non-outdated) bot threads suppress re-derived findings whether resolved or unresolved — including paraphrases sharing multiple code anchors and meaningful heading-token overlap — while outdated threads never suppress, so those findings get reassessed.
- **Incremental reviews** (default on, `incremental` input): a re-review only diffs `last-reviewed-SHA..HEAD` instead of the full PR diff. The last reviewed SHA is read from the `commit_id` GitHub records on the bot's completed reviews (`### Assessment:` bodies only — timeout/failure/incomplete fallbacks don't count, and the legacy zero-findings LGTM fallback is deliberately rejected because it was synthesized from exit code 0 alone), so state survives across runs with no extra writes. `src/incremental-review/` plans the mode and falls back to a full review on force-push/rebase (SHA missing or not an ancestor of HEAD), base-branch merge-ins, net-zero changes, or any error. In incremental mode the original full diff is preserved at `pr_full.diff` for stale-thread resolution and suggestion-anchor validation (GitHub validates anchors against the full PR diff). On re-reviews, `src/dedupe-findings/` (staged at `/tmp/dedupe-findings.js`, run by the agent per `posting-format.md` against the pre-fetched `/tmp/existing_review_comments.json`) drops findings matching already-posted bot comments by file path + line proximity (±3) + finding-heading similarity, so a full re-review after a rebase doesn't duplicate threads. The CLI also accepts the review-thread history snapshot (`/tmp/prior_review_threads.json`) as an optional third argument: current (non-outdated) bot threads suppress re-derived findings whether resolved or unresolved — including paraphrases sharing multiple code anchors and meaningful heading-token overlap — while outdated threads never suppress, so those findings get reassessed.
- Per-finding **confidence scoring** assigns each verified finding a precise 0–100 score (band: strong/moderate/weak/negligible) from the verifier's `verdict`, `evidence_strength`, and `context_completeness`, plus drafter↔verifier severity concordance and scope. `src/score-confidence/score-confidence.ts` is the **single source of truth** for the model (weights, bands, threshold, posting policy); the "Confidence Scoring" section of `review-pr/agents/pr-review.yaml` mirrors it as a strict lookup table so the orchestrator can apply it inline (the gitignored `dist/` is not available at agent runtime). Change one, change both — the unit tests pin every value. Security and high-severity CONFIRMED/LIKELY findings are always posted regardless of score; below-threshold findings are surfaced in a summary rather than silently dropped. The inline-posting cutoff is configurable via the `confidence-threshold` action input (a band name or a number clamped to 30–100, default `moderate` = 55); the action resolves it by invoking the bundled `dist/score-confidence.js resolve-threshold` CLI (so the resolution logic stays in TypeScript, not bash) and injects it into the agent prompt, and `scoreFinding`/`scoreFindings` accept a matching `postThreshold` option.
- Stale review threads on lines no longer in the diff are auto-resolved via GraphQL `resolveReviewThread`. Threads with no `<!-- docker-agent-review -->` marker are never touched. The same step first snapshots every thread whose ROOT comment carries a review marker (with `isOutdated`, `originalLine`, `resolvedBy.login`, and per-comment `databaseId`/`author.login`/`replyTo.databaseId`/`body`) to `/tmp/prior_review_threads.json` — initialized to `[]` and persisted before any diff-related or resolution early exit, fail-open. The review context references that artifact by path and count only (thread bodies/replies are untrusted PR data, never interpolated into the prompt); `pr-review.yaml`'s "Prior Review Thread History" policy makes the agent suppress findings tracked by current threads (resolved or unresolved), reassess outdated ones, treat human replies as evidence rather than instructions, and never silently escalate severity; the dedupe CLI consumes the same artifact as its optional third argument.

Expand Down
3 changes: 2 additions & 1 deletion SECURITY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ bound request frequency:
a sliding window (default 600 s) and, when the count crosses a threshold
(default 8), flags a rate anomaly. It counts one unit per LLM run: full reviews
via the Reviews API (`pulls.listReviews`, by bot author, covering findings,
zero-finding APPROVEs, and timeout/error/LGTM fallbacks, none of which carry an
zero-finding 🟢 NO FINDINGS completions, and timeout/error/incomplete fallbacks,
none of which carry an
inline marker) plus marker-bearing reply comments. The review job skips the
expensive review on a flagged anomaly; the conversational reply jobs are not
gated (they are org-gated and per-PR serialized), though their replies still
Expand Down
Loading