Repository navigation
Preserve integration concerns and diagnostics for large reviews - #425
Conversation
|
Advisory cross-file review against Lean Pool's supervisory rubric. This is not by itself a significance, source-verification, or merge verdict.  The PR is not yet safe to merge because model-directed source retrieval can perform unbounded full-diff work before enforcing the input budget. Findings
Summary
Reviews (3) · Last reviewed commit: "Fit serialized follow-up evidence and av..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return replace( | ||
| final, portions=len(results), coverage=manifest, source_reviews=tuple(results) |
There was a problem hiding this comment.
source_reviews retains the same source-portion results that _ReviewSession.accounted already stores in requests. Since _write_review_evidence recursively serializes both fields, large portioned reviews keep and write every source-review payload twice, unnecessarily increasing memory use and evidence-artifact size.
Vilin97
changed the title
| if: always() && steps.ci.outputs.conclusion == 'success' | ||
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 | ||
| with: | ||
| name: review-evidence | ||
| path: ${{ runner.temp }}/review-evidence.json | ||
| path: ${{ runner.temp }}/review-evidence* |
There was a problem hiding this comment.
.github/workflows/ unless they were explicitly requested, so this requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| queries = final.payload.get("source_requests", []) | ||
| if ( | ||
| not isinstance(queries, list) | ||
| or len(queries) > 20 | ||
| or any(not isinstance(query, str) or not query.strip() for query in queries) | ||
| if not isinstance(queries, list) or any( | ||
| not isinstance(query, str) or not query.strip() for query in queries | ||
| ): | ||
| raise ValueError( | ||
| "Integration source requests must be at most 20 nonempty strings" | ||
| f"Integration source requests must be nonempty strings: {queries!r}" | ||
| ) |
There was a problem hiding this comment.
A model response with many source requests now passes validation. _fit_source_followups then scans the complete diff once for every request and allocates all excerpts before checking the candidate message against the input budget. A large request list can therefore exhaust review-worker CPU or memory and prevent the review from producing a verdict. Bound the retrieval work with the available budget before calling source_excerpts.
Knowledge Base Used: Fit oversized pull-request diffs to the review budget
| if destination := os.environ.get("REVIEW_EVIDENCE_PATH"): | ||
| with Path(destination).with_suffix(".calls.jsonl").open("a") as stream: | ||
| stream.write(json.dumps(asdict(result), default=vars) + "\n") |
There was a problem hiding this comment.
If the artifact filesystem is full, unavailable, or misconfigured, this unguarded checkpoint write raises after the model call has succeeded. The filesystem error then aborts the entire review, so a best-effort diagnostic can discard a successful result and waste the completed model call. Handle checkpoint persistence failures separately from the review result.
Knowledge Base Used: Continuous integration and repository automation
1 participant
Integration could report pass while retaining its own unresolved findings or questions. Treat those final concerns as obligations that prevent approval. Convert remaining input tokens to characters and validate the fully serialized follow-up input, including escaping and per-query metadata, when retrieving source, and let the input budget bound source-request lists instead of rejecting more than 20 requests.
Keep source reports in portion order in the completed evidence artifact. Checkpoint every completed model call and upload evidence even on failure, so malformed integration output does not lose the preceding source work.
Validation: all 374 Python tests pass; Ruff check/format and diff checks pass. Regressions cover final concerns, excerpt budget units, ordered source reports, 25-source-request retrieval, and incremental evidence persistence. This follows up on #424 and a source-request validation failure in the first #393 re-review.