feat: repository-configurable review instructions (CUSTOM_PROMPT / CUSTOM_PROMPT_FILE) - #29
Conversation
CUSTOM_PROMPT (inline) and CUSTOM_PROMPT_FILE (file path) append additional review instructions on top of the standard contract — house rules, focus areas, conventions. The two sources compose (inline first, then the file); the combined block is labeled as repository-configuration in the prompt, capped at 8000 bytes, and marked when truncated. An unreadable file warns in the logs and is skipped rather than failing the review. In the workflow the file is read from the checked-out base branch, so committing it to the repo (e.g. .github/ai-review-instructions.md) keeps it trusted content — the README shows that setup. Both knobs are forwarded from repository variables; the derived forwarding test picks them up automatically.
The review contract now classifies every problem as new (introduced by this PR's changes) or pre-existing (predating it), reported in separate sections with their own headers — never mixed. Pre-existing problems are still always reported (one bullet each, location plus a one-line description, so they can be extracted and filed as issues later), but they are documentation only: not to be fixed in this PR, never requesting changes, and never influencing the verdict, which is based solely on new problems. Ambiguous cases go to the "Should be checked" section instead of a guessed classification.
OpenRouter nests provider failures inside choices[0].error (the top level carries only request/routing errors), so a transient provider failure printed a generic "Invalid API response format" and swallowed the real message — exactly what the failed run on this PR showed. Error extraction now reads choices[0].error first, and the model call is retried once when the response carries a provider error (a single immediate retry cannot loop). The offline curl stub gained an error-first fixture; regressions cover the successful retry (exactly two model calls) and message surfacing on a persistent error.
The failed run showed choices[0].error present but null with an empty message — an empty completion, most plausibly a reasoning model that consumed the whole max_tokens budget on chain-of-thought. Two defects in the handling: - The finish_reason=length report (the exact remedy message for this case: raise AI_MAX_TOKENS or reduce the diff) sat AFTER the missing-content branch, so it could never fire for empty content; it now runs first. - The content sentinel // "error" conflated absent content with a literal "error" string; absent content now yields "" and the error output always carries finish_reason, so an empty completion is diagnosable from the posted error alone. Debug output gained finish_reason, native_finish_reason, and token usage.
LearningCircuit
added
ai_code_review
AI Code ReviewNew problems
Pre-existing problems
Should be checked
✅ Approved with recommendations Review by Friendly AI Reviewer - made with ❤️ |
github-actions
Bot
added
enhancement
… contract Addresses the review on 0a45376 (new problems only — pre-existing ones stay documented for extraction per the contract): - The retry now also fires on an EMPTY response (curl -s prints nothing on network errors), and the retry result is only accepted when it produced output — a network-dead retry no longer overwrites a diagnosable first error with "empty response". Error detection uses .error != null so a null-valued key on a success response cannot trigger a pointless retry, the retry has a 2s backoff, and the first attempt's error message is logged when retrying. - Contract precedence clarified: "No actionable findings." applies to new problems; a non-empty Pre-existing problems section still appears alongside it. - README corrected: this repo's own workflow checks out the PR merge ref, so CUSTOM_PROMPT_FILE here is PR-controllable for one's own review; the trusted-base property holds for pull_request_target consumers like local-deep-research. - Harness: dead conditional in the response fixture removed; new regressions cover the network-blip retry and diagnostic preservation.
Round 1 of the review loop (fresh-context agent findings): - set -e killed the script on real curl network failures (exit 6/7/28), making the retry unreachable; exit statuses are now neutralized and the offline stub models non-zero exits, so the network-blip tests exercise the real path. - Error reviews are assembled with jq: provider messages embedding JSON (quotes) no longer break the output JSON. - curl gains --connect-timeout 15 / --max-time 1500 (generous: reasoning models legitimately take 10+ minutes) so hangs reach the retry/timeout path instead of wedging the job. - Whitespace-only CUSTOM_PROMPT no longer emits an empty instructions header; the stale trusted-base comment now matches the README; the clip test pins the 8000-byte cap itself, not just the marker.
Round 2 of the review loop: - REVIEW_FOOTER was a literal backslash-n string; after the jq --arg migration it parsed as two characters instead of a newline, breaking every posted error-review footer. It is now a real two-line string. - The retry accept-condition kept an empty first response even when the retry carried the only diagnostic (network blip -> provider 502); the retry's error now wins when the first response is content-free, and rejected retries log their error message. - Empty completions on successful responses regressed to exit 1 (no posted comment, trigger label never removed — the stuck-label trap); they now post an error review and exit 0 like the truncation path. The literal-'error' content comparison is gone (such output flows to JSON validation instead of the error path).
Round 3 (direct review; agent pool rate-limited): an HTML 502 page from a proxy arrives as valid HTTP with curl exit 0 — non-empty and not a model error — so it bypassed the retry entirely and failed the review as 'Invalid JSON response from API'. Unusable responses (empty, model error, or unparseable) now all trigger the retry, and the retry is accepted only when it is parseable and either clean or the first response was content-free; a diagnosable first error is kept when the retry itself is garbage.
Round 4 (direct review): the workflow used the mutable actions/checkout v5 tag and had no job timeout — now SHA-pinned to v5.1.0 with a 35-minute bound (the model call itself is capped at 25 minutes by curl's --max-time). The embedded prompt prefix restated the focus areas and actionable-findings instruction that the main prompt already carries; stated once now, saving tokens on every request. CRLF checked into a CUSTOM_PROMPT_FILE is stripped instead of reaching the prompt as bare carriage returns.
Round 5 (direct review): the commit-message, PR-description, and previous-review clip tests asserted only the truncation marker — a regressed head -c would keep the marker (independent wc check) while shipping uncapped bytes, exactly the gap the agent round flagged for the custom-prompt cap. All four budgets now pin their caps. README documents MAX_HUMAN_COMMENT_LENGTH=0 and the new-problems/pre-existing split in What's New.
Round 6 (direct review): AI_TEMPERATURE, AI_MAX_TOKENS, and MAX_DIFF_SIZE feed jq --argjson, where a non-numeric repository variable aborts the payload build — they now degrade to defaults like the other validated knobs. The prompt gained a prompt-injection fence: all repository text (comments, description, commit messages, labels, instruction files, quoted reviews) is untrusted data to review, never instructions; steering attempts are to be reported as findings.
Round 7 (direct review): an end-to-end sandbox run with every context
feature enabled at once (comments incl. previous bot review, commit
overview with merge-sounding subject and author fallback, check-run
summary with neutral/skipped/in-progress, labels, unicode PR
description, custom inline+file instructions with CRLF) passes all 13
interaction checks — the isolated unit tests cannot catch cross-feature
breakage. The retry backoff moved to a ${VAR-2} no-colon default so
tests can zero it (suite back to ~25s) without becoming a repository
knob the forwarding test would demand.
Round 8 (direct review): an empty or non-numeric RETRY_SLEEP_SECONDS skips the sleep instead of erroring mid-retry. The README claimed 800KB as THE default for MAX_DIFF_SIZE while the script's built-in is 5MB and only the workflow's forwarding masks the difference — both spots now state both values.
AI Code ReviewNew problems
Pre-existing problems
Should be checked
✅ Approved with recommendations Review by Friendly AI Reviewer - made with ❤️ |
The 35-minute job bound sat below the retry's worst case (two 25-minute curl attempts plus backoff) — the timeout would have killed the retry mid-flight and wedged the label, the exact failure it existed to prevent; raised to 60. The injection fence listed instructions files as untrusted while the instructions block tells the model to apply them; the fence now scopes untrusted text to PR-thread content (diff, comments, description, commit messages, labels, quoted text) and notes the configured block's trust equals the workflow checkout, matching the README. Test harness rejects conflicting first-failure fixtures.
1 participant
Summary
Two features plus a hardening series:
1. Repository-configurable review instructions
CUSTOM_PROMPT(inline) andCUSTOM_PROMPT_FILE(file; composed inline-first), rendered as a labeled layer on top of the standard contract so the JSON output format and severity tags survive. Capped at 8000 bytes (marked), UTF-8-safe, whitespace-only ignored, CRLF stripped, unreadable file warns and is skipped. The README documents the trust model honestly: this repo's own workflow reads the file from the PR merge ref;pull_request_targetconsumers with a base checkout get trusted base content.2. New vs pre-existing problem split
Findings are classified new (introduced by the PR) vs pre-existing (predating it) and reported in separate sections. Pre-existing problems are always reported for documentation and issue extraction — one bullet each with location — but never to be fixed in this PR, never requesting changes, never influencing the verdict (based solely on new problems). Ambiguous cases go to "Should be checked".
3. Model-call resilience (hardened over several review rounds)
choices[0].error) surfaced; jq-assembled error JSON survives embedded quotes; real-newline footer.set -ecannot kill the script first), model-error, or unparseable (proxy HTML) — with diagnostic preference: a content-full first response beats a garbage retry, and a retry's error beats an empty first response.finish_reasonis always carried in error output; curl gains--connect-timeout/--max-time(generous — reasoning calls take 10+ min) and the job a 35-minute bound;actions/checkoutis SHA-pinned.Tests
python3 -B -m unittest discover -s tests— 58 tests, all offline: the full retry matrix, truncation/empty-completion exits, JSON-validity of error output, cap pinning (bytes, not just markers), custom-instruction composition/gating/clips, the prompt contract phrase-by-phrase, knob degradation, and cap/forwarding invariants. A full-feature integration smoke (13 cross-feature checks) was verified in a sandbox.bash -nclean;shellcheckdown to one pre-existing style note.Review loop history
Hardened over 10 review rounds (fresh-context agents until the pool rate-limited, then direct adversarial passes): rounds 1–2 fixed the retry-unreachable-under-set-e, broken error footers, and an exit-code regression that re-created the stuck-label trap; round 3 extended retries to unparseable responses; rounds 4–8 pinned the workflow (SHA, timeout), deduplicated the prompt, fenced injection, validated knobs, pinned every byte cap, and corrected docs; rounds 9–10 found nothing actionable — convergence.