Conversation
ReviewReviewed the middleware, event/storage path, tests, and cross-checked assumptions against the pinned Minor, non-blocking notes:
Test coverage is good — the adapter-level tests exercise real Google/Vertex code paths (not mocks), and the headless e2e asserts both propagation and non-leakage of fixture secrets. Docs in EVENTS.md match the implemented behavior. Reviewed SHA: 206c0e4 |
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| const result = await doStream() | ||
| let rawReason = field(undefined, () => false) | ||
| let diagnostic = field(undefined, () => false) | ||
| const headers = result.response?.headers |
There was a problem hiding this comment.
🔴 result.response?.headers never exists in v2; requestID dead.
--- a/packages/cli/src/provider/termination.ts
+++ b/packages/cli/src/provider/termination.ts
@@ -61,7 +61,7 @@
const result = await doStream()
let rawReason = field(undefined, () => false)
let diagnostic = field(undefined, () => false)
- const headers = result.response?.headers
+ const headers = result.responseHeaders
const requestID = field(
headers?.["x-request-id"] ?? headers?.["x-goog-request-id"],
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #114, packages/cli/src/provider/termination.ts:64-68):
Problem: result.response?.headers never exists in v2; requestID dead
Detail: The middleware declares `LanguageModelV2Middleware` from `@ai-sdk/provider` (v2 / AI SDK 5), where `doStream()` resolves `LanguageModelV2StreamResult = { stream, request, responseHeaders? }` — there is no `response` wrapper property. So `result.response?.headers` is always `undefined`, `requestID` is permanently `{status:"unavailable"}`, and the feature's request-ID capture never works. The PR's own tests assert `requestID: {status:"available", value:"req_fixture"|"req_123"}` (test/cli/run-termination.test.ts, test/provider/termination.test.ts), which fail; it is also a TypeScript error (`Property 'response' does not exist`), so typecheck should flag it.
Suggested fix: Replace `const headers = result.response?.headers` with `const headers = result.responseHeaders` (the v2 `LanguageModelV2StreamResult` exposes response headers directly as `responseHeaders`).
Suggested patch:
--- a/packages/cli/src/provider/termination.ts
+++ b/packages/cli/src/provider/termination.ts
@@ -61,7 +61,7 @@
const result = await doStream()
let rawReason = field(undefined, () => false)
let diagnostic = field(undefined, () => false)
- const headers = result.response?.headers
+ const headers = result.responseHeaders
const requestID = field(
headers?.["x-request-id"] ?? headers?.["x-goog-request-id"],
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The middleware declares LanguageModelV2Middleware from @ai-sdk/provider (v2 / AI SDK 5), where doStream() resolves LanguageModelV2StreamResult = { stream, request, responseHeaders? } — there is no response wrapper property. So result.response?.headers is always undefined, requestID is permanently {status:"unavailable"}, and the feature's request-ID capture never works. The PR's own tests assert requestID: {status:"available", value:"req_fixture"|"req_123"} (test/cli/run-termination.test.ts, test/provider/termination.test.ts), which fail; it is also a TypeScript error (Property 'response' does not exist), so typecheck should flag it.
async wrapStream({ doStream }) {
const result = await doStream()
let rawReason = field(undefined, () => false)
let diagnostic = field(undefined, () => false)
const headers = result.response?.headers
const requestID = field(
headers?.["x-request-id"] ?? headers?.["x-goog-request-id"],
(value) => /^[a-zA-Z0-9_-]+$/.test(value) && !/^(sk-|gh[pousr]_|github_pat_|AIza|AKIA|ASIA)/.test(value),
)
return {| messageID: string | ||
| type: "step-finish" | ||
| reason: string | ||
| termination?: { |
There was a problem hiding this comment.
🟠 Hand-edited generated SDK types; OpenAPI mirror not updated.
| termination?: { | |
| Source the field from the zod schema (already added in message-v2.ts with `.meta({ ref: "ProviderTermination" })`) and run the OpenAPI + SDK codegen steps so docs/architecture/openapi.yaml and packages/sdk/src/gen/types.gen.ts are regenerated together instead of hand-editing generated output. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #114, packages/sdk/src/gen/types.gen.ts:321-328):
Problem: Hand-edited generated SDK types; OpenAPI mirror not updated
Detail: The repo generates its OpenAPI contract (scripts/generate-openapi.ts → docs/architecture/openapi.yaml) and packages/sdk/src/gen/types.gen.ts is generated output of that contract. This PR hand-edits types.gen.ts to add `StepFinishPart.termination` but touches neither openapi.yaml nor the generator inputs, so the next regeneration clobbers the hand edit and the published OpenAPI contract never declares the new field — SDK types and API schema drift apart.
Suggested fix: Source the field from the zod schema (already added in message-v2.ts with `.meta({ ref: "ProviderTermination" })`) and run the OpenAPI + SDK codegen steps so docs/architecture/openapi.yaml and packages/sdk/src/gen/types.gen.ts are regenerated together instead of hand-editing generated output.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The repo generates its OpenAPI contract (scripts/generate-openapi.ts → docs/architecture/openapi.yaml) and packages/sdk/src/gen/types.gen.ts is generated output of that contract. This PR hand-edits types.gen.ts to add StepFinishPart.termination but touches neither openapi.yaml nor the generator inputs, so the next regeneration clobbers the hand edit and the published OpenAPI contract never declares the new field — SDK types and API schema drift apart.
|
|
||
| ### `step_start` / `step_finish` | ||
|
|
||
| `step_finish.part.termination` is an optional additive diagnostic object. Its |
There was a problem hiding this comment.
🟡 Termination block splits heading from section intro sentence.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #114, EVENTS.md:298-354):
Problem: Termination block splits heading from section intro sentence
Detail: The 57-line termination subsection is inserted directly under `### step_start / step_finish`, pushing the section's own one-line description "Emitted at step boundaries during multi-step tool use." to after the JSON example and policy paragraphs. Readers now hit termination-specific detail before being told what the events are, inverting the intro-then-detail structure used by surrounding sections.
Suggested fix: Move the insertion so the intro sentence stays immediately after the heading — either place the termination content after "Emitted at step boundaries during multi-step tool use." or under its own `#### step_finish.part.termination` subheading.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The 57-line termination subsection is inserted directly under ### step_start / step_finish, pushing the section's own one-line description "Emitted at step boundaries during multi-step tool use." to after the JSON example and policy paragraphs. Readers now hit termination-specific detail before being told what the events are, inverting the intro-then-detail structure used by surrounding sections.
| const headers = result.response?.headers | ||
| const requestID = field( | ||
| headers?.["x-request-id"] ?? headers?.["x-goog-request-id"], | ||
| (value) => /^[a-zA-Z0-9_-]+$/.test(value) && !/^(sk-|gh[pousr]_|github_pat_|AIza|AKIA|ASIA)/.test(value), |
There was a problem hiding this comment.
🟡 requestID guard admits xoxb-/glpat-/npm_ tokens.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #114, packages/cli/src/provider/termination.ts:67):
Problem: requestID guard admits xoxb-/glpat-/npm_ tokens
Detail: The only secret guard on persisted request IDs is the charset `^[a-zA-Z0-9_-]+$` plus a prefix denylist (sk-, gh[pousr]_, github_pat_, AIza, AKIA, ASIA). Several real credential formats satisfy both and stay under the 128-char cap — e.g. Slack `xoxb-…`/`xoxp-…`, `glpat-…` GitLab tokens, `npm_…` registry tokens, and prefix-less hex/base64url keys — so a provider or intermediary echoing a secret into `x-request-id`/`x-goog-request-id` would have it persisted verbatim into session storage and headless NDJSON output. Defense-in-depth (requires a misbehaving provider, which this PR explicitly treats as untrusted), but EVENTS.md presents the denylist as the secret guard.
Suggested fix: Tighten toward an allowlist of provider-expected shapes instead of enumerating secret prefixes: e.g. require length ≤ 64 and `^[a-zA-Z0-9]{8,64}$` (Google-style IDs use alphanumerics only), or persist a non-reversible truncated form (first 8 chars + length) when the header does not match the known provider shape.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The only secret guard on persisted request IDs is the charset ^[a-zA-Z0-9_-]+$ plus a prefix denylist (sk-, gh[pousr], github_pat, AIza, AKIA, ASIA). Several real credential formats satisfy both and stay under the 128-char cap — e.g. Slack xoxb-…/xoxp-…, glpat-… GitLab tokens, npm_… registry tokens, and prefix-less hex/base64url keys — so a provider or intermediary echoing a secret into x-request-id/x-goog-request-id would have it persisted verbatim into session storage and headless NDJSON output. Defense-in-depth (requires a misbehaving provider, which this PR explicitly treats as untrusted), but EVENTS.md presents the denylist as the secret guard.
| } | ||
| } | ||
| } | ||
| import { ProviderTermination } from "@/provider/termination" |
There was a problem hiding this comment.
🟡 Import appended at EOF instead of top import block.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #114, packages/cli/src/session/message-v2.ts:902):
Problem: Import appended at EOF instead of top import block
Detail: The new `import { ProviderTermination } from "@/provider/termination"` is appended at line 902, after the closing brace of the `MessageV2` namespace, while every other import in this file (and the sibling changes in llm.ts/processor.ts in this same PR) sits in the top import block. ES-module hoisting makes it work, but it breaks the file's import convention, trips import-ordering lint (import/first), and risks a duplicate import when someone later adds it at the top.
Suggested fix: Move the import into the top import block, alphabetically between the existing `@/provider/error` and `@/provider/transform` imports, and delete the trailing line 902.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The new import { ProviderTermination } from "@/provider/termination" is appended at line 902, after the closing brace of the MessageV2 namespace, while every other import in this file (and the sibling changes in llm.ts/processor.ts in this same PR) sits in the top import block. ES-module hoisting makes it work, but it breaks the file's import convention, trips import-ordering lint (import/first), and risks a duplicate import when someone later adds it at the top.
}
}
}
}
import { ProviderTermination } from "@/provider/termination"
Code reviewVerdict: Do not merge — 1 blocker(s) must be fixed. · 🔴 1 · 🟠 1 · 🟡 3 · ⚪ 0 · 0/5 resolved
🤖 Fix all 5 open findings with your agent📋 Out-of-diff findings (5)
Reviewed 8 files · 0 inline · view all 5 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Review response — PR #114Verified all five findings against the pinned dependencies and actual repository codegen workflow; fixed four and rejected one false API-shape claim. Issues addressed (pushed to this PR)
Validation: 48 focused tests passed. CLI and repository-wide typechecks passed (6 tasks). The actual SDK build ran successfully, and repeated codegen left both canonical and legacy type outputs byte-identical. Prettier and diff checks passed. Pushed normally without verification bypass flags. Review claims verified false (no change needed)
Not addressed hereNone of the verified code findings are deferred. Raw request IDs and diagnostic free text are intentionally unavailable as values under the privacy policy; availability flags remain. Verdict data-layer persistence is unavailable in this session: matched 0, written 0, verified 0, failed 0, unrecorded 5. |
| paths: {}, | ||
| components: { | ||
| schemas: { | ||
| StepFinishPart: z.toJSONSchema(MessageV2.StepFinishPart, { target: "openapi-3.0" }), |
There was a problem hiding this comment.
🟡 Zod meta `ref` leaks non-standard keyword into OpenAPI.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #114, packages/cli/src/cli/cmd/generate.ts:18):
Problem: Zod meta `ref` leaks non-standard keyword into OpenAPI
Detail: ProviderTermination.Info carries `.meta({ ref: "ProviderTermination" })` (termination.ts:20), but zod 4.1.8 (pinned in bun.lock) only honors the `id` meta key for $defs/$ref extraction; every other meta key is Object.assign'ed verbatim onto the emitted schema (to-json-schema.ts:637-638, 764-769). So the published OpenAPI components gain a non-standard `ref` keyword and StepFinishPart gets the termination schema fully inlined rather than referencing the named component — the apparent componentization intent never materializes. Secondarily, the document declares `openapi: "3.1.1"` while both schemas are generated with `target: "openapi-3.0"`, which would emit 3.0-dialect constructs (e.g. `nullable`) invalid in a 3.1.1 document if a nullable field is ever added. Harmless to today's codegen (the generated v2 SDK types are correct), but it pollutes the published contract that SDK codegen consumes.
Suggested fix: Register the schema for componentization with `.meta({ id: "ProviderTermination" })` (zod's id meta drives $defs/$ref extraction — the generate test's `toEqual` between the inlined property and the component would then need adjusting to a $ref expectation), or drop the meta key entirely to stop leaking `ref` into the spec. Also align the toJSONSchema target with the declared 3.1.1 document (default draft-2020-12) or declare the spec 3.0.3.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
ProviderTermination.Info carries .meta({ ref: "ProviderTermination" }) (termination.ts:20), but zod 4.1.8 (pinned in bun.lock) only honors the id meta key for $defs/$ref extraction; every other meta key is Object.assign'ed verbatim onto the emitted schema (to-json-schema.ts:637-638, 764-769). So the published OpenAPI components gain a non-standard ref keyword and StepFinishPart gets the termination schema fully inlined rather than referencing the named component — the apparent componentization intent never materializes. Secondarily, the document declares openapi: "3.1.1" while both schemas are generated with target: "openapi-3.0", which would emit 3.0-dialect constructs (e.g. nullable) invalid in a 3.1.1 document if a nullable field is ever added. Harmless to today's codegen (the generated v2 SDK types are correct), but it pollutes the published contract that SDK codegen consumes.
version: "1.0.0",
},
paths: {},
components: {
schemas: {
StepFinishPart: z.toJSONSchema(MessageV2.StepFinishPart, { target: "openapi-3.0" }),
ProviderTermination: z.toJSONSchema(ProviderTermination.Info, { target: "openapi-3.0" }),
},
},| const headers = result.response?.headers | ||
| // A provider/proxy can echo arbitrary credentials into an ID header. | ||
| // Format allowlists cannot distinguish an opaque ID from an opaque key. | ||
| const requestID = field(headers?.["x-request-id"] ?? headers?.["x-goog-request-id"], () => false) |
There was a problem hiding this comment.
⚪ Empty x-request-id masks nonempty x-goog-request-id.
| const requestID = field(headers?.["x-request-id"] ?? headers?.["x-goog-request-id"], () => false) | |
| Select the first nonempty value instead of the first non-nullish one: `const id = [headers?.["x-request-id"], headers?.["x-goog-request-id"]].find((v) => typeof v === "string" && v.length > 0); const requestID = field(id, () => false)` |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #114, packages/cli/src/provider/termination.ts:67):
Problem: Empty x-request-id masks nonempty x-goog-request-id
Detail: `headers?.["x-request-id"] ?? headers?.["x-goog-request-id"]` falls through only on null/undefined, so an empty-string `x-request-id` (which field() maps to unavailable) masks a populated `x-goog-request-id`. The stored termination then reports requestID `unavailable` even though a nonempty ID header exists, contradicting EVENTS.md's contract that all nonempty values from either header are reported (as redacted). Edge case — requires a proxy sending an empty x-request-id alongside a nonempty x-goog-request-id.
Suggested fix: Select the first nonempty value instead of the first non-nullish one: `const id = [headers?.["x-request-id"], headers?.["x-goog-request-id"]].find((v) => typeof v === "string" && v.length > 0); const requestID = field(id, () => false)`
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
headers?.["x-request-id"] ?? headers?.["x-goog-request-id"] falls through only on null/undefined, so an empty-string x-request-id (which field() maps to unavailable) masks a populated x-goog-request-id. The stored termination then reports requestID unavailable even though a nonempty ID header exists, contradicting EVENTS.md's contract that all nonempty values from either header are reported (as redacted). Edge case — requires a proxy sending an empty x-request-id alongside a nonempty x-goog-request-id.
const result = await doStream()
let rawReason = field(undefined, () => false)
let diagnostic = field(undefined, () => false)
const headers = result.response?.headers
// A provider/proxy can echo arbitrary credentials into an ID header.
// Format allowlists cannot distinguish an opaque ID from an opaque key.
const requestID = field(headers?.["x-request-id"] ?? headers?.["x-goog-request-id"], () => false)| export namespace ProviderTermination { | ||
| const Field = z.object({ | ||
| status: z.enum(["available", "unavailable", "redacted"]), | ||
| value: z.string().max(128).optional(), |
There was a problem hiding this comment.
⚪ Field value cap 128 diverges from diagnostic 2048 limit.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #114, packages/cli/src/provider/termination.ts:7):
Problem: Field value cap 128 diverges from diagnostic 2048 limit
Detail: The Field schema hardcodes `value: z.string().max(128)` while `field()` applies a per-call limit (2048 for finishMessage diagnostics, documented in EVENTS.md). The limit is encoded twice with divergent values: today the diagnostic allowlist is `() => false` so no value is ever emitted and nothing breaks, but if a diagnostic value is ever marked available in the 129–2048 range, `field()` would emit it while `Info.safeParse` in `from()` rejects the whole termination object (silently dropping it). Share named constants and keep the schema cap in sync with the runtime limit.
Suggested fix: Define `const VALUE_LIMIT = 128` and `const DIAGNOSTIC_LIMIT = 2048`, parameterize the Field schema (e.g. a factory `Field(limit)`) or widen the stored cap to the largest limit used, so `from()`'s safeParse can never reject a value `field()` emitted.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The Field schema hardcodes value: z.string().max(128) while field() applies a per-call limit (2048 for finishMessage diagnostics, documented in EVENTS.md). The limit is encoded twice with divergent values: today the diagnostic allowlist is () => false so no value is ever emitted and nothing breaks, but if a diagnostic value is ever marked available in the 129–2048 range, field() would emit it while Info.safeParse in from() rejects the whole termination object (silently dropping it). Share named constants and keep the schema cap in sync with the runtime limit.
export namespace ProviderTermination {
const Field = z.object({
status: z.enum(["available", "unavailable", "redacted"]),
value: z.string().max(128).optional(),
truncated: z.boolean(),
})
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 1 · ⚪ 2 · 0/3 resolved
🤖 Fix all 3 open findings with your agent📋 Out-of-diff findings (3)
Reviewed 14 files · 0 inline · view all 3 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Review response — PR #114Verified and fixed the three new findings from the review of Issues addressed (pushed to this PR)
Validation: 51 focused tests passed. CLI, SDK, and repository-wide typechecks passed (6 tasks). The actual SDK codegen succeeded; a second generation produced byte-identical canonical and legacy type outputs. Prettier and diff checks passed. Pushed normally without verification bypass flags. Review claims verified false (no change needed)None in this review round. Not addressed hereNone in this review round. Data-layer verdict persistence is unavailable: matched 0, written 0, verified 0, failed 0, unrecorded 3. |
1 participant
Relates to #109
Intent
Normalized provider finish reasons currently lose the safe context needed to distinguish an observed raw reason from an unavailable diagnostic. This change carries bounded termination metadata through the CLI event path without collecting prompt, tool, or full response content.
Expected Impact on Users
Operators and downstream consumers can correlate normalized and available raw termination reasons with provider/model/request identity in
step_finishNDJSON events. Free-form provider diagnostic text remains suppressed by default.Expected Outcomes
Implementation
step_finish.terminationstorage, event serialization, SDK types, and availability/redaction documentation.Scope Caveat
This is the safe first slice of #109. Free-form finish-message text and broader provider coverage remain intentionally out of scope until their privacy and availability policy is approved.
Test Plan
Verification
Risks and Rollout
The new field is optional and additive. Raw chunks remain internal to middleware; no database migration or opt-in is required.