Use output validator to retry aggregator on parse failures and GitHub 422s#6
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CI Agent Review
The only finding is an architectural coupling issue: TryResult<T> is exported from configuration.mts solely for use by ai.mts, creating a spurious dependency from the AI analysis layer to the configuration parsing module. This generic utility type belongs in typescript-helpers.mts alongside other shared type helpers like Guard<T>.
| export type Configuration = CommentTriggerConfiguration | PullRequestConfiguration | LocalDiffConfiguration | ||
|
|
||
| type TryResult<T> = { ok: true; value: T } | { ok: false; reason: string } | ||
| export type TryResult<T> = { ok: true; value: T } | { ok: false; reason: string } |
There was a problem hiding this comment.
Exporting TryResult from configuration.mts creates inappropriate coupling — ai.mts now imports from configuration only to use this generic result wrapper, which has nothing to do with configuration. TryResult<T> is a generic discriminated-union building block that belongs in typescript-helpers.mts alongside other shared utility types like Guard<T>. If configuration.mts is ever refactored or split (e.g., separating GitHub-specific from local-diff configuration), ai.mts breaks despite having no real dependency on any configuration concern. As more modules need TryResult, they'll each accumulate this same spurious dependency on configuration.mts, entrenching the coupling.
| import type { ProviderProfile } from "./provider-profiles.mts" | ||
| import type { BaseCommitContext } from "./base-commit.mts" | ||
| import type { SpawnGit } from "./diff.mts" | ||
| import type { CompletionsMessage } from "./completions.mts" |
There was a problem hiding this comment.
This import of TryResult from configuration.mts represents a spurious dependency — the AI analysis layer does not depend on any configuration concern. The type should be imported from typescript-helpers.mts instead (see finding on configuration.mts:26).
|
/review |
There was a problem hiding this comment.
CI Agent Review
The changeset introduces a validate-and-retry mechanism for aggregator output with several issues: (1) a security concern where unsanitized GitHub API error response bodies are injected into the AI model's prompt as user messages, creating a prompt injection vector and removing a fail-safe defense against malicious reviews; (2) a bug where emptyTurnCount is not reset when the outputValidator feedback loop continues, breaking the consecutive-empty-turns invariant; (3) an architectural separation-of-concerns issue where the orchestrator layer authors LLM prompt text instead of passing structured data to the AI layer; (4) an inconsistent error-handling contract within github.mts where submitReview returns result objects while sibling functions still throw.
| export async function submitReview(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration, review: GitHubReviewPayload): Promise<void> { | ||
| export type SubmitReviewResult = { ok: true } | { ok: false, status: number, body: string } | ||
| export async function submitReview(dependencies: { githubFetch: GitHubFetch }, configuration: GitHubConfiguration, review: GitHubReviewPayload): Promise<SubmitReviewResult> { | ||
| const { apiUrl, token, owner, repositoryName, pullRequestNumber } = configuration |
There was a problem hiding this comment.
Architectural inconsistency: submitReview now returns a result object (SubmitReviewResult) instead of throwing, while fetchPullRequestDiff and fetchPullRequestBaseCommit in the same module still throw on non-OK HTTP responses. All three functions handle the same class of error (GitHub API returning unexpected status codes), yet they communicate failure through two fundamentally different mechanisms. This splits the module's contract — callers must remember which error-handling strategy each function uses. Consider either having submitReview throw and letting the orchestrator catch and interpret errors (preserving the original contract), or switching all three functions to result objects for consistency.
|
/review |
1 participant
No description provided.