#232: bring AGY review finalization onto the shared contract#252
Merged
Conversation
AGY's adapter fed the shared buildJobRecord helper the wrong inputs in
three ways, each destroying produced review output:
- P7: retain parsed.result on quality/failure paths instead of nulling it;
add stdout/stderr sidecars (mirrors gemini's writeExecutionSidecars).
- P4: drop the local hasSubstantiveReview literal-'Blocking findings' gate;
delegate substance to the shared review_quality.failed_review_slot.
- P5: compute redaction fields via redactionFieldsForSelected (returns {}
when no source bodies) instead of hardcoding sourceRedactionRequired,
which made empty-source completed reviews throw in assertRequiredSourceRedaction.
Leak-safe: buildJobRecord already redacts result for every record.
Git-policy-escape hardening (#218/#240/#241) preserved. New
tests/unit/agy-shared-finalization.test.mjs drives the real finalize path
(fail-on-revert); P4/P5/P7 AB probes updated to assert fixed behavior;
relay/relay-agy synced. Full suite 2845 tests / 2839 pass / 0 fail.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aligns the AGY companion's finalization and review-quality gating with the shared sibling-provider contract. Key changes include removing the redundant local hasSubstantiveReview check, dynamically computing redaction fields to support empty-source reviews, retaining produced result text on failure paths, and writing stdout/stderr sidecars. The feedback suggests adding defensive checks for potentially null or undefined selectedFiles and execution objects, as well as improving error-message extraction in the sidecar writing function.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Address gemini review on PR #252: redactionFieldsForSelected coalesces selectedFiles ?? [] before sourceFilesForRedaction; writeExecutionSidecars bails on a null execution and extracts the sidecar-failure message via e?.message ?? String(e). Both null states are unreachable on the live paths (execution is the freshly-produced run result, selectedFiles the parsed array) — applied to remove the finding class. Byte-synced to relay/relay-agy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR #252 review split 2:2 on whether the new raw stdout.log/stderr.log sidecars leak source. Adjudicated at source: they do not. Persistent raw diagnostics are the owned, owner-only (0o700) companion contract per docs/artifact-cleanup-inventory.md line 19 — parity with claude/gemini/kimi — and are never read back into any transmitted surface; the canonical JobRecord stays redacted (job-record.mjs:879). The 'does not persist stdout/stderr logs' clause governs the API-reviewer/Grok no-subprocess paths, not the companion path. The one actionable residual: no test pinned the boundary on a source-bearing run. This adds it — a >200-char source line quoted in the review collapses to [redacted_source_excerpt] in the canonical record while the raw stdout.log/ stderr.log sidecars keep it verbatim. Fail-on-revert proven: routing raw parsed.result into record.result fails this test. Test-only; no production change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
seungpyoson
added a commit
that referenced
this pull request
Jun 26, 2026
#234: replace the single-flight relay lock with a bounded per-provider counting semaphore so multiple source-bearing reviews run concurrently under a limit. - Counting-semaphore lease: slot files + inner mkdir count-then-claim gate - Composes #254's gate-race ENOENT fix (deadline → recreate → reclaim ordering) - Identity-free block messages (§8), exit-listener cleanup, 11-copy canonical byte-sync - Brought current to post-#252 main (1af8ef0) - Reclaim-test flake fixed test-only (256c5ea): GATE_TIMEOUT_MS "1" → single-source "250" Fix-delta re-review: unanimous SHIP (Kimi/Claude/GPT/GLM), CI 8/8 + SonarCloud green. Closes #234 Follow-ups tracked in #239
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Brings AGY's review finalization onto the shared
buildJobRecordcontract that gemini/kimi/claude already use. AGY's adapter fed the shared helper the wrong inputs in three ways, each destroying produced review output:parsed.resulton quality/failure paths instead of nulling it; add stdout/stderr sidecars (mirrors gemini'swriteExecutionSidecars).hasSubstantiveReviewliteral-'Blocking findings' gate; delegate substance to the sharedreview_quality.failed_review_slot.redactionFieldsForSelected(returns{}when no source bodies) instead of hardcodingsourceRedactionRequired, which made empty-source completed reviews throw inassertRequiredSourceRedaction.Why it's safe
buildJobRecordalready redactsresultfor every record → retaining a quality-failed body is leak-safe.Verification
tests/unit/agy-shared-finalization.test.mjsdrives the real finalize path (fail-on-revert proven by hand).relay/relay-agysynced byte-identical;npm run lintclean.Part of #232 (lossy review transforms). Down-payment on #251 (shared-runtime consolidation).
🤖 Generated with Claude Code