Phase 1 Wave 1 — LLMProvider seam types (1.A) + ToolNormalizer (1.E) + CostTracker (1.B)#7
Merged
Conversation
Phase 1 Wave 0 — the @relavium/shared reconciliation (1.L.0) — is merged. Flip the phase status to In progress, mark the 1.L.0 workstream ✅ Done, and rewrite current.md's immediate next steps to the two Wave-1 lanes per the sequencing plan: 1.A (freeze the LLMProvider seam types, scaffold packages/llm) and 1.L (WorkflowYAMLParser, scaffold packages/core). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase-1 Wave-1 workstream 1.A — scaffold @relavium/llm and freeze the immovable LLMProvider seam in src/types.ts, expressed only in Relavium/Zod types (ADR-0011, llm-provider-seam.md). No provider SDK yet — adapters land in 1.C/1.G/1.H. @relavium/llm (new package, mirrors @relavium/shared: tsc build, catalog deps, types:[] platform-free): - Zod schemas (+ inferred types): LlmRequest, LlmRole/LlmMessage, ToolDef, ToolChoice, Usage, CapabilityFlags, LlmErrorKind/LlmError, LlmResult, StreamChunk (the 6-variant discriminated union). LlmProvider is a TS interface (it carries methods: id + generate(req,key) + stream(req,key) + supports). - ToolDef.parameters is typed JSONSchema7 (z.custom; @types/json-schema is a dev/type-only catalog dep — erased at build, no ADR) — the deep subset reshape is the ToolNormalizer's job (1.E). - Curated index.ts (not export *) re-exporting the seam + the shared substrate. - 19 tests: per-schema accept/reject + type-level pins proving the public types are pure Relavium types (ProviderId set, StopReason set, a stub LlmProvider compiles) — a leaked vendor type would fail them, and the import-zone fence forbids the import outright. @relavium/shared — the seam substrate that the dependency direction (shared → llm) requires to live in shared so the future session schemas can reuse it without a circular import: - content.ts: ContentPartSchema (text | tool_call | tool_result) and AbortSignalLike (the minimal structural cancellation handle — lib:["ES2023"] has no AbortSignal, and a real one satisfies it structurally, so the platform-free packages need no DOM lib / @types/node). - +6 tests (173 total). The seam re-exports ContentPart/StopReason from here. Full gate green (12/12 turbo); seam fence airtight (packages/llm/src/types.ts is fenced — only src/adapters/* may import a vendor SDK); db drift gate unchanged. Refs: ADR-0011 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two pure-TS Wave-1 workstreams on the Relavium side of the seam (no provider SDK; the fence
forbids vendor imports outside src/adapters/*). Both build on the frozen 1.A seam types.
1.B — CostTracker + pricing:
- pricing.ts: the canonical MODEL_PRICING table keyed on canonical model id — the in-code source
of truth the adapters and the tracker share, seeded later into the (empty) model_catalog.
Prices are integer micro-cents per million tokens (USD x 1e8) — the DB unit, no float. Anthropic
rows confirmed via claude-api (Opus 4.8 $5/$25, Sonnet 4.6 $3/$15, Haiku 4.5 $1/$5, + cache
read/write); OpenAI/Gemini/DeepSeek are best-known placeholders (VERIFY at 1.G/1.H, tracked).
- cost-tracker.ts: priceModel (unknown id -> typed UnknownModelError, never a silent zero),
cost(modelId, usage) = round(tokens x ratePerMtok / 1e6) per token class billed once
(inputTokens net of cache reads), and a CostTracker accumulator producing the cost:updated
figures across per-attempt usage.
- errors.ts: typed UnknownModelError + ToolSchemaError (discriminated `code`).
1.E — ToolNormalizer:
- toWire(toolDef, providerId): one canonical ToolDef -> the three native wire shapes (OpenAI/
DeepSeek function, Anthropic input_schema, Gemini functionDeclarations) as Relavium-defined
plain objects — no vendor type.
- reshapeForGemini: the OpenAPI-subset reshape — allow-lists the keywords Gemini honors (incl.
anyOf and the numeric/string bounds), strips the rest, drops unsupported `format` values, and
THROWS a typed ToolSchemaError on a `$ref` it cannot express.
- GeminiToolCallIds + normalizeToolCall: synthesize stable tool-call ids (Gemini has none) and
match functionResponses FIFO per name; fold an extracted {id,name,args} into a canonical
tool_call ContentPart, rejecting an empty id/name from a misbehaving provider.
Verified by a 3-skeptic adversarial workflow (cost arithmetic, ToolNormalizer, seam/fence/spec).
Cost dimension clean; fixed the findings it surfaced: the Gemini reshape was over-stripping
`anyOf` (destroying unions) and the numeric/string bound keywords — now kept; normalizeToolCall
now rejects empty ids; and llm-provider-seam.md §6 is clarified to state `inputTokens` is net of
cache reads (the OpenAI/DeepSeek adapter subtracts the cached subset), matching the CostTracker.
+20 llm tests (39 total). Full gate green; seam fence airtight; db drift gate unchanged.
Refs: ADR-0011
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideFreezes the provider-agnostic LLMProvider seam in @relavium/llm using Zod schemas and a TS interface, adds a ToolNormalizer that maps canonical ToolDef into provider-specific wire formats (including a Gemini OpenAPI-subset reshaper and synthesized tool-call IDs), introduces a CostTracker built on a canonical micro-cent pricing table, and wires these into the package’s public API and docs while updating roadmap status and shared content types. Sequence diagram for ToolNormalizer and Gemini tool-call IDssequenceDiagram
participant Caller
participant ToolNormalizer as ToolNormalizer_toWire
participant GeminiIds as GeminiToolCallIds
participant Adapter
participant Provider as Gemini
Caller->>ToolNormalizer: toWire(ToolDef, providerId)
alt providerId == gemini
ToolNormalizer->>ToolNormalizer: reshapeForGemini(parameters, name)
ToolNormalizer-->>Caller: GeminiToolWire
else other providers
ToolNormalizer-->>Caller: OpenAiToolWire / AnthropicToolWire
end
Caller->>GeminiIds: synthesize(name)
GeminiIds-->>Caller: id
Caller->>Adapter: send functionCall(id, name, args)
Adapter->>Provider: functionCall
Provider-->>Adapter: functionResponse(name, result)
Adapter->>GeminiIds: resolveResponse(name)
GeminiIds-->>Adapter: id
Adapter->>Caller: normalizeToolCall(provider, { id, name, args })
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Linter diff in the way? Review this PR in Change Stack to focus on meaningful changes and expand context only when needed. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughScaffolds ChangesLLM Provider Seam Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant LlmProvider
participant Adapter
participant ToolNormalizer
participant CostTracker
Client->>LlmProvider: generate(LlmRequest, apiKey)
LlmProvider->>Adapter: send vendor request (ToolWire)
Adapter->>ToolNormalizer: provider response (tool payload)
ToolNormalizer->>ToolNormalizer: normalizeToolCall() / reshapeForGemini
ToolNormalizer-->>LlmProvider: ContentPart[] (canonical tool_call/tool_result)
LlmProvider->>CostTracker: record(modelId, Usage)
CostTracker-->>LlmProvider: CostUpdate
LlmProvider-->>Client: LlmResult (content, stopReason, usage)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 scaffolds the @relavium/llm package, establishing the provider-agnostic LLMProvider seam types, a CostTracker with a canonical model-pricing table, and a ToolNormalizer to reshape tool definitions for various providers. The review feedback highlights several robustness improvements: adding defensive type checks on untrusted provider tool-call IDs and names to prevent runtime crashes, using hasOwnProperty for model pricing lookups to avoid prototype property collisions, and refining the custom Zod validator for tool parameters to explicitly reject arrays.
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.
Comment on lines
+202
to
+208
| if (extracted.id.length === 0 || extracted.name.length === 0) { | ||
| throw new ToolSchemaError( | ||
| provider, | ||
| extracted.name.length === 0 ? '(unnamed)' : extracted.name, | ||
| 'a tool call must carry a non-empty id and name', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Directly accessing .length on extracted.id and extracted.name assumes they are always defined strings. However, since these values originate from untrusted provider SDK responses, they could be undefined, null, or of an unexpected type at runtime, leading to a TypeError crash. Adding defensive type and presence checks prevents runtime exceptions.
if (
typeof extracted.id !== 'string' ||
extracted.id.length === 0 ||
typeof extracted.name !== 'string' ||
extracted.name.length === 0
) {
throw new ToolSchemaError(
provider,
typeof extracted.name === 'string' && extracted.name.length > 0 ? extracted.name : '(unnamed)',
'a tool call must carry a non-empty id and name',
);
}
Comment on lines
+12
to
+20
| export function priceModel(modelId: string): ModelPricing { | ||
| const pricing: ModelPricing | undefined = (MODEL_PRICING as Record<string, ModelPricing>)[ | ||
| modelId | ||
| ]; | ||
| if (pricing === undefined) { | ||
| throw new UnknownModelError(modelId, KNOWN_MODEL_IDS); | ||
| } | ||
| return pricing; | ||
| } |
There was a problem hiding this comment.
Using direct property lookup MODEL_PRICING[modelId] can lead to unexpected behavior or bypasses if modelId matches built-in Object.prototype properties (such as 'toString', 'valueOf', or 'constructor'). Since modelId is a dynamic string (potentially coming from user-authored YAML or external inputs), it is safer to verify ownership using Object.prototype.hasOwnProperty.call before accessing the pricing record.
export function priceModel(modelId: string): ModelPricing {
if (!Object.prototype.hasOwnProperty.call(MODEL_PRICING, modelId)) {
throw new UnknownModelError(modelId, KNOWN_MODEL_IDS);
}
return (MODEL_PRICING as Record<string, ModelPricing>)[modelId]!;
}
Comment on lines
+37
to
+39
| parameters: z.custom<JSONSchema7>((value) => typeof value === 'object' && value !== null, { | ||
| message: 'parameters must be a JSON-Schema object', | ||
| }), |
There was a problem hiding this comment.
The current custom validator for parameters only checks typeof value === 'object' && value !== null, which incorrectly accepts arrays (since typeof [] === 'object'). Since tool parameters must be a JSON-Schema object, arrays should be explicitly rejected by adding an !Array.isArray(value) check.
Suggested change
| parameters: z.custom<JSONSchema7>((value) => typeof value === 'object' && value !== null, { | |
| message: 'parameters must be a JSON-Schema object', | |
| }), | |
| parameters: z.custom<JSONSchema7>((value) => typeof value === 'object' && value !== null && !Array.isArray(value), { | |
| message: 'parameters must be a JSON-Schema object', | |
| }), |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ToolDefSchema, theparametersvalidator only checks fortypeof value === 'object' && value !== null, which will allow arrays despite the comment and JSON Schema expectations; consider tightening this (e.g., rejecting arrays explicitly) so only object schemas are accepted at the seam. - In the Gemini reshape logic,
GEMINI_SAFE_FORMATSincludes'enum'as aformatvalue, which doesn’t correspond to a standard JSON Schema/Gemini format; it might be worth double‑checking this list against Gemini’s documented formats to avoid silently passing through unsupported values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ToolDefSchema`, the `parameters` validator only checks for `typeof value === 'object' && value !== null`, which will allow arrays despite the comment and JSON Schema expectations; consider tightening this (e.g., rejecting arrays explicitly) so only object schemas are accepted at the seam.
- In the Gemini reshape logic, `GEMINI_SAFE_FORMATS` includes `'enum'` as a `format` value, which doesn’t correspond to a standard JSON Schema/Gemini format; it might be worth double‑checking this list against Gemini’s documented formats to avoid silently passing through unsupported values.
## Individual Comments
### Comment 1
<location path="packages/llm/src/types.ts" line_range="34-40" />
<code_context>
+export const ToolDefSchema = z.object({
+ name: nonEmptyString,
+ description: z.string().optional(),
+ parameters: z.custom<JSONSchema7>((value) => typeof value === 'object' && value !== null, {
+ message: 'parameters must be a JSON-Schema object',
+ }),
+});
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Tighten the ToolDef.parameters guard to reject arrays and booleans explicitly.
This predicate accepts any non-null object, including arrays, but later logic assumes a record-like JSON Schema root. Consider tightening it to something like `isRecord(value)` (`typeof value === 'object' && value !== null && !Array.isArray(value)`) or, if needed, explicitly allow only object/boolean roots. This avoids accidentally accepting array schemas and keeps `parameters` aligned with the expected shape.
```suggestion
export const ToolDefSchema = z.object({
name: nonEmptyString,
description: z.string().optional(),
parameters: z.custom<JSONSchema7>(
(value): value is JSONSchema7 =>
typeof value === 'object' && value !== null && !Array.isArray(value),
{
message: 'parameters must be a JSON-Schema object with an object root',
},
),
});
```
</issue_to_address>
### Comment 2
<location path="packages/llm/src/tool-normalizer.ts" line_range="94-95" />
<code_context>
+]);
+
+/** `format` values Gemini accepts; an unsupported format is dropped, not sent. */
+const GEMINI_SAFE_FORMATS = new Set(['enum', 'date-time', 'int32', 'int64', 'float', 'double']);
+
+function reshapeNode(node: unknown, toolName: string): unknown {
</code_context>
<issue_to_address>
**suggestion:** Revisit `'enum'` in GEMINI_SAFE_FORMATS; it’s not a `format` keyword and may never appear here.
Since `GEMINI_ALLOWED_KEYWORDS` already covers `'enum'`, it will be preserved independently of `format`. Including `'enum'` in `GEMINI_SAFE_FORMATS` implies `format` might be set to `'enum'`, which isn’t a standard `format` and could mislead callers. I suggest removing `'enum'` from this set, or adding a brief comment if there’s a deliberate reason to allow a non-standard `format` value here.
```suggestion
/** `format` values Gemini accepts; an unsupported format is dropped, not sent. */
const GEMINI_SAFE_FORMATS = new Set(['date-time', 'int32', 'int64', 'float', 'double']);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/llm/src/tool-normalizer.test.ts (1)
28-28: ⚡ Quick winReplace unsafe
ascasts in tests with narrowing helpers.These assertions bypass the strict typing guarantees and violate the TS guideline. Prefer shape checks (
in) + local assertion helpers so the tests stay type-safe without forced casts.Example pattern
-const wire = toWire(toolDef, 'anthropic') as AnthropicToolWire; +const wire = toWire(toolDef, 'anthropic'); +expect('input_schema' in wire).toBe(true); +if (!('input_schema' in wire)) throw new Error('expected AnthropicToolWire');As per coding guidelines: “Use TypeScript with strict mode enabled; no
anytypes and no unsafe type assertions withas”.Also applies to: 37-37, 46-46, 76-76, 96-96, 117-117, 131-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/llm/src/tool-normalizer.test.ts` at line 28, The test uses unsafe "as" casts (e.g., const wire = toWire(toolDef, 'anthropic') as AnthropicToolWire) which bypass TypeScript's strict checks; replace these with runtime narrowing helpers and shape checks: add type-guard functions (e.g., isAnthropicToolWire(obj): obj is AnthropicToolWire) that use "in" or property checks, call the guard right after calling toWire and assert its truth (or throw) before treating the value as AnthropicToolWire, and apply the same pattern for the other casts referenced (lines for other tool wires); this preserves type-safety in tool-normalizer.test.ts while avoiding unsafe "as" assertions.packages/llm/src/tool-normalizer.ts (1)
97-140: ⚡ Quick winReduce
reshapeNodecomplexity to satisfy the quality gate.Line 97 is over the configured cognitive complexity limit. Splitting format/properties/array-key handling into helpers should clear the gate and make future keyword changes safer.
Refactor direction
-function reshapeNode(node: unknown, toolName: string): unknown { +function reshapeNode(node: unknown, toolName: string): unknown { if (Array.isArray(node)) { return node.map((entry) => reshapeNode(entry, toolName)); } if (!isRecord(node)) return node; if ('$ref' in node) { throw new ToolSchemaError('gemini', toolName, '`$ref` is not expressible in the Gemini schema subset'); } - const out: Record<string, unknown> = {}; - for (const [key, value] of Object.entries(node)) { - ... - } - return out; + return reshapeObjectNode(node, toolName); }(Extract
reshapeObjectNode,reshapeFormatKeyword,reshapePropertiesKeyword, andreshapeNestedSchemaKeyword.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/llm/src/tool-normalizer.ts` around lines 97 - 140, reshapeNode is exceeding cognitive complexity; extract keyword-specific logic into small helpers to simplify it. Create reshapeFormatKeyword(nodeValue, toolName) to handle 'format' filtering (using GEMINI_SAFE_FORMATS), reshapePropertiesKeyword(propertiesValue, toolName) to iterate and call reshapeNode for each property, and reshapeNestedSchemaKeyword(value, toolName) to handle 'items' and 'anyOf' by delegating to reshapeNode; then refactor reshapeNode to call these helpers for keys 'format', 'properties', 'items' and 'anyOf', keep the '$ref' check and the final passthrough assignment, and ensure helper names are used (reshapeObjectNode optional) so the main reshapeNode becomes a simple dispatcher and passes the quality gate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/roadmap/phases/phase-1-engine-and-llm.md`:
- Around line 3-5: Update the roadmap wording that currently states Wave 1 is
“next” to reflect that Wave‑1 workstreams are underway; replace the phrase
describing Wave 1 as “next” with “in progress” (or equivalent) and note that PR
`#7` (2026-06-05) landed Wave‑1 workstreams 1.A/1.B/1.E so the line referencing
Wave 1, 1.A, and 1.L is accurate and up-to-date.
In `@packages/llm/src/cost-tracker.ts`:
- Around line 13-15: Replace the unsafe cast when indexing MODEL_PRICING:
instead of casting MODEL_PRICING as Record<string, ModelPricing>, first narrow
modelId with a type guard like `if (modelId in MODEL_PRICING)` (or a custom
`modelId is CanonicalModelId` check) and then assign `pricing =
MODEL_PRICING[modelId]` without `as`; this preserves strict typing for
ModelPricing and avoids weakening MODEL_PRICING’s type.
In `@packages/llm/src/pricing.ts`:
- Line 146: Create and export a type-guard function like
isCanonicalModelId(value: string): value is CanonicalModelId that checks
membership against Object.keys(MODEL_PRICING) (e.g., build a Set of keys and
test value). Replace KNOWN_MODEL_IDS = Object.keys(MODEL_PRICING) as readonly
CanonicalModelId[] with KNOWN_MODEL_IDS =
Object.keys(MODEL_PRICING).filter(isCanonicalModelId) so the filter uses the
predicate and yields CanonicalModelId[] without an unsafe cast. Then import and
use isCanonicalModelId in packages/llm/src/cost-tracker.ts to narrow modelId
before indexing MODEL_PRICING[modelId] (remove the (MODEL_PRICING as
Record<string, ModelPricing>)[modelId] cast).
In `@packages/llm/src/tool-normalizer.ts`:
- Around line 146-150: After calling reshapeNode in reshapeForGemini, also
validate that the reshaped schema is an object-root (not just a record shape);
update the check to throw ToolSchemaError('gemini', toolName, ...) when either
!isRecord(reshaped) or reshaped.type !== 'object' so schemas like { type:
'string' } are rejected; locate reshapeForGemini and adjust the post-reshape
validation (currently using isRecord) to include the type === 'object' guard
referencing reshapeNode, isRecord, and ToolSchemaError.
In `@packages/llm/src/types.ts`:
- Around line 37-39: The predicate for ToolDef.parameters currently accepts any
non-null object (including arrays); update the z.custom<JSONSchema7> predicate
in packages/llm/src/types.ts (the parameters field) to explicitly reject arrays
by adding a check like !Array.isArray(value) so only plain objects pass, and
keep or adjust the error message 'parameters must be a JSON-Schema object'
accordingly.
- Around line 122-123: The `signal` schema uses z.custom without a predicate so
it accepts anything; update the `signal` entry in packages/llm/src/types.ts to
use a type-guard predicate that validates the shape of AbortSignalLike from
`@relavium/shared` (check that value is an object, has a boolean `aborted`
property and `addEventListener`/`removeEventListener` methods) by replacing
`z.custom<AbortSignalLike>().optional()` with `z.custom<AbortSignalLike>((v): v
is AbortSignalLike => typeof v === 'object' && v !== null && typeof (v as
any).aborted === 'boolean' && typeof (v as any).addEventListener === 'function'
&& typeof (v as any).removeEventListener === 'function').optional()`, keeping
the `signal` name and optionality intact.
---
Nitpick comments:
In `@packages/llm/src/tool-normalizer.test.ts`:
- Line 28: The test uses unsafe "as" casts (e.g., const wire = toWire(toolDef,
'anthropic') as AnthropicToolWire) which bypass TypeScript's strict checks;
replace these with runtime narrowing helpers and shape checks: add type-guard
functions (e.g., isAnthropicToolWire(obj): obj is AnthropicToolWire) that use
"in" or property checks, call the guard right after calling toWire and assert
its truth (or throw) before treating the value as AnthropicToolWire, and apply
the same pattern for the other casts referenced (lines for other tool wires);
this preserves type-safety in tool-normalizer.test.ts while avoiding unsafe "as"
assertions.
In `@packages/llm/src/tool-normalizer.ts`:
- Around line 97-140: reshapeNode is exceeding cognitive complexity; extract
keyword-specific logic into small helpers to simplify it. Create
reshapeFormatKeyword(nodeValue, toolName) to handle 'format' filtering (using
GEMINI_SAFE_FORMATS), reshapePropertiesKeyword(propertiesValue, toolName) to
iterate and call reshapeNode for each property, and
reshapeNestedSchemaKeyword(value, toolName) to handle 'items' and 'anyOf' by
delegating to reshapeNode; then refactor reshapeNode to call these helpers for
keys 'format', 'properties', 'items' and 'anyOf', keep the '$ref' check and the
final passthrough assignment, and ensure helper names are used
(reshapeObjectNode optional) so the main reshapeNode becomes a simple dispatcher
and passes the quality gate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9402e818-7816-4f60-81b9-229baa13cbdb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
docs/reference/shared-core/llm-provider-seam.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-1-engine-and-llm.mdpackages/llm/README.mdpackages/llm/package.jsonpackages/llm/src/cost-tracker.test.tspackages/llm/src/cost-tracker.tspackages/llm/src/errors.tspackages/llm/src/index.tspackages/llm/src/pricing.tspackages/llm/src/tool-normalizer.test.tspackages/llm/src/tool-normalizer.tspackages/llm/src/types.test.tspackages/llm/src/types.tspackages/llm/tsconfig.build.jsonpackages/llm/tsconfig.jsonpackages/shared/src/content.test.tspackages/shared/src/content.tspackages/shared/src/index.tspnpm-workspace.yaml
Address PR #7 review on the @relavium/llm seam (1.A/1.B/1.E): - types.ts: ToolDef.parameters now rejects arrays/non-objects and the request `signal` is validated structurally against AbortSignalLike, so malformed input is caught at the seam instead of failing later at the provider call or when the signal is observed. - tool-normalizer.ts: reshapeForGemini requires an object root (a primitive-root schema now throws, matching its docstring); a nullable `type: [x, "null"]` union collapses to a scalar type + `nullable: true`, and an inexpressible non-null union throws ToolSchemaError, instead of shipping an invalid Gemini payload. Dropped the bogus `enum` entry from GEMINI_SAFE_FORMATS. - pricing.ts / cost-tracker.ts: replace two unsafe `as` casts with an isCanonicalModelId type guard (Set-of-own-keys, so it is also immune to prototype-chain keys); clarify that cacheWrite is in-code-only with no model_catalog column. - tests: drop unsafe `as ...Wire` casts; add reject/accept cases for array params, signal, primitive-root, and nullable/inexpressible type unions. No vendor SDK type crosses the seam. Refs: ADR-0011 Phase 1 · 1.A/1.B/1.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update the status blurbs so they reflect reality: Wave 1 is in progress rather than "next", with the seam workstreams 1.A/1.B/1.E in review under the open PR #7 (2026-06-05). Deliberately "in review", not "landed" — PR #7 is not yet merged to main, and a workstream is only Done once its PR merges. Phase 1 · 1.A/1.B/1.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/llm/src/tool-normalizer.test.ts (1)
159-159: ⚡ Quick winPrefer a type guard over the type assertion.
The
as Record<string, Record<string, unknown>>assertion narrowsunknownwithout runtime validation. As per coding guidelines, prefer type guards over unsafeascasts. The source code already uses anisRecordhelper for this pattern.♻️ Proposed refactor using a type guard
- const props = reshaped['properties'] as Record<string, Record<string, unknown>>; + const props = reshaped['properties']; + if (typeof props !== 'object' || props === null || Array.isArray(props)) { + throw new Error('expected properties to be a record'); + } + const typedProps = props as Record<string, Record<string, unknown>>; - expect(props['opt']).toEqual({ type: 'string', nullable: true }); + expect(typedProps['opt']).toEqual({ type: 'string', nullable: true });Alternatively, import and use the
isRecordhelper from the source code for cleaner guard logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/llm/src/tool-normalizer.test.ts` at line 159, Replace the unsafe assertion "const props = reshaped['properties'] as Record<string, Record<string, unknown>>" with a runtime type guard using the existing isRecord helper: check that reshaped['properties'] satisfies isRecord and that each value is a record (or use nested isRecord checks) before assigning to props; import isRecord if not already imported and handle the failure branch (e.g., throw or fail the test) so props is properly narrowed without using "as".Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/llm/src/tool-normalizer.test.ts`:
- Line 159: Replace the unsafe assertion "const props = reshaped['properties']
as Record<string, Record<string, unknown>>" with a runtime type guard using the
existing isRecord helper: check that reshaped['properties'] satisfies isRecord
and that each value is a record (or use nested isRecord checks) before assigning
to props; import isRecord if not already imported and handle the failure branch
(e.g., throw or fail the test) so props is properly narrowed without using "as".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2e2dfb7-1622-48ef-b5fe-e92096a32dc6
📒 Files selected for processing (8)
README.mddocs/roadmap/phases/phase-1-engine-and-llm.mdpackages/llm/src/cost-tracker.tspackages/llm/src/pricing.tspackages/llm/src/tool-normalizer.test.tspackages/llm/src/tool-normalizer.tspackages/llm/src/types.test.tspackages/llm/src/types.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/roadmap/phases/phase-1-engine-and-llm.md
- packages/llm/src/cost-tracker.ts
- packages/llm/src/pricing.ts
- packages/llm/src/tool-normalizer.ts
- packages/llm/src/types.test.ts
- packages/llm/src/types.ts
Address the SonarQube findings on the seam: - tool-normalizer.ts: split reshapeNode (cognitive complexity 29 → well under the 15 limit) into small per-keyword helpers (reshapeFormat / reshapeTypeArray / reshapeProperties / reshapeKeyword) behind a thin dispatch loop. Behaviour is unchanged — all 42 tests stay green. Also invert the toWire `desc` ternary to drop the negated-condition readability smell. - pricing.ts: drop the redundant `.0` zero-fractions in the usd(...) literals (5.0 → 5, 25.0 → 25, …); the priced values are identical. Refs: ADR-0011 Phase 1 · 1.B/1.E Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A 7-dimension adversarial review of PR #7 surfaced 8 confirmed findings (1 medium, 5 low, 2 info; 3 false-positives filtered). No blockers — all hardening/polish on the 1.E ToolNormalizer plus test coverage. Fixed: - [MED security] reshapeForGemini walked an untrusted tool-parameters JSON-Schema with UNBOUNDED recursion — a deeply-nested schema overflowed the stack with a raw RangeError. Added a MAX_SCHEMA_DEPTH (100) cap that raises a typed ToolSchemaError instead. - [LOW security] a property literally named `__proto__` under `properties` was assigned via bracket syntax, invoking the prototype setter (silently dropping the param + mutating the emitted node's prototype — not global pollution). reshapeProperties now writes members with Object.defineProperty, so `__proto__` survives as an own key. - [LOW correctness] the db9206c tightening over-rejected: a no-argument tool's `parameters: {}` (a typeless object root, valid for Anthropic/OpenAI/DeepSeek) threw for Gemini. reshapeForGemini now defaults a typeless root to `type: 'object'`, throwing only on a non-object root. - [LOW maintainability] a `type: ['string','null']`-derived `nullable: true` could be clobbered by a contradictory verbatim sibling `nullable`; the type-union null is now authoritative. Tests (+ coverage): cases for the depth cap, the `__proto__` own-key, the `{}` default, the nullable precedence, and the primitive-node passthrough branch (`items: true`); deepened the AbortSignalLike validation test (partial-object + wrong-typed `aborted` rejection); added a MODEL_PRICING table-invariant test (keys = KNOWN_MODEL_IDS, every catalog-projection field complete + integer). Suite: 48 green. Review nitpick: replaced the unsafe `reshaped['properties'] as Record<...>` test assertions with a runtime `recordAt` guard (isRecord-based), removing the unsafe `as`. Full gate green; seam fence airtight; no DB drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/llm/src/cost-tracker.test.ts`:
- Line 84: The test's key-order assertion uses Array.sort() without a comparator
which can be unstable; update the assertion to sort both sides with an explicit
locale-aware comparator using String.prototype.localeCompare so both
Object.keys(MODEL_PRICING) and [...KNOWN_MODEL_IDS] are sorted via (a, b) =>
a.localeCompare(b) before comparison; locate the assertion in
packages/llm/src/cost-tracker.test.ts that references MODEL_PRICING and
KNOWN_MODEL_IDS and change the .sort() calls accordingly.
In `@packages/llm/src/tool-normalizer.test.ts`:
- Around line 37-43: The test currently uses unsafe casts to JSONSchema7; update
the parseSchema implementation and the other literal cast so we validate at
runtime instead of asserting types. For parseSchema, change the boundary cast
JSON.parse(json) as JSONSchema7 to parse into unknown, perform a runtime check
that the result is an object (and optionally has expected root keys like "type"
or "$schema") and only then return it typed as JSONSchema7; for the remaining
object literal cast (the `} as JSONSchema7` usage) remove the `as` cast and
either use TypeScript's `satisfies JSONSchema7` for a static literal or add a
small runtime guard when the value is derived, mirroring the check used by
parseSchema. Ensure you update references to parseSchema and the specific object
literal to use these runtime guards or `satisfies` so no direct `as JSONSchema7`
assertions remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8eafbe87-56d3-4931-825d-1bb0af28858a
📒 Files selected for processing (4)
packages/llm/src/cost-tracker.test.tspackages/llm/src/tool-normalizer.test.tspackages/llm/src/tool-normalizer.tspackages/llm/src/types.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/llm/src/types.test.ts
- packages/llm/src/tool-normalizer.ts
- cost-tracker.test.ts: give the MODEL_PRICING key-set `.sort()` calls an explicit `localeCompare` comparator (Sonar flags a comparator-less sort as type-dependent). - tool-normalizer.test.ts: use ES2022 `Object.hasOwn(props, '__proto__')` instead of `Object.prototype.hasOwnProperty.call(...)`. Test-only; behavior unchanged. Suite 48 green; gate + fence clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the PR #7 inline nitpick — no direct `as JSONSchema7` assertion remains in the suite: - parseSchema now parses to `unknown` and narrows with an `isObjectSchema` runtime guard (throws on a non-object), instead of `JSON.parse(json) as JSONSchema7`. - the `$ref` test's literal uses `satisfies JSONSchema7` instead of `as JSONSchema7`. The L84 sort nitpick was already resolved in 8473263 (localeCompare comparator) — no change. Test-only; suite 48 green; gate + fence clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 6, 2026
The Wave-1 seam trio — 1.A (LLMProvider seam types), 1.B (CostTracker + pricing), 1.E (ToolNormalizer) — merged in PR #7. Flip their headings to ✅ Done, update the phase Status line, and point current.md's next-steps at the adapter lane (1.C → {1.D ‖ 1.F}, with 1.I) running parallel to the 1.L parser. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 tasks
cemililik
added a commit
that referenced
this pull request
Jun 11, 2026
… merged) 1.K landed in PR #13 (2026-06-11). Reflect it everywhere status lives: - phase-1: §1.K header ✅ Done; 1.m2 milestone complete (1.B PR #7, 1.K PR #13); the dependency-matrix 1.K row gets its Done note; the top status blockquote now points past 1.K (next is 1.L, which scaffolds packages/core). - current.md: the seam policy lane is complete (1.K merged); the engine lane (1.L) is the active next step; the multimodal + PR #12 notes no longer call 1.K "next". - llm-provider-seam.md: the ADR-0030 strip-on-failover is now enforced by 1.K, not "not yet exercised — no consumer exists". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 19, 2026
Merged
cemililik
added a commit
that referenced
this pull request
Jun 24, 2026
…xt pickup 2.S 2.L (CLI packaging, distribution & install verification) merged via PR #49, closing the last Phase-3 go/no-go exit criterion (#7) — so the Phase-2 spine is complete and all seven criteria now hold. Move 2.L from "next pickup" into the Landed/Done list and re-point the next pickup to 2.S (media host-wiring, the first additive lane — off the M3 critical path and the go/no-go). Audited every status surface; updated only the live trackers and left the static plan views (Mermaid DAGs, wave/dependency tables, exit-criteria definitions) unchanged per their own "the plan, not a live tracker" framing. - current.md: 2.L Done entry + next pickup 2.S; re-tense the NPM_TOKEN maintainer obligation ("once 2.L lands" → "now that 2.L has landed (PR #49)"). - phase-2-cli.md: §2.L heading ✅ Done badge; header status line; Remaining-build-order banner; drop the 2.L row from the pickup queue and renumber 2.S/2.R/chat/2.J to 1–4; re-tense the gate-closing-backbone, 2.K-closed, and 2.S-timing bullets. - CLAUDE.md, README.md: append the 2.L packaging deliverable to the landed Phase-2 list; next pickup 2.S; all seven Phase-3 exit criteria now hold. - runbooks/release-a-surface.md: Status "draft" → "partial — CLI (npm) flow complete (2.L)". ADR-0051's "(today a stub)" aside about the runbook is left unedited: ADRs are append-only (CLAUDE.md rule 9) and it is a harmless point-in-time authoring note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Jun 25, 2026
… status, defer log, concurrency) Five review findings on the CLI media surface (all low/nit): - #12 (nit, 3-lens): wireSaveToPort called mkdirSync on every write (blocking sync I/O in an async port) while the JSDoc implied once-only. Switch to await mkdir (node:fs/promises) — matches the FilesystemMediaStore.put pattern, keeps the port non-blocking — and reword the JSDoc to "every write". - #13 (nit): TERMINAL_RUN_STATUSES typed Set<RunStatus> (was Set<string>), so a misspelled status is a compile error and .has() narrows the closed union. - #7 (low): createWorkflowModelCatalog deferred silently on a CapabilityFlagsSchema.safeParse failure — indistinguishable from "model absent". Add an optional, per-model-deduped, secret-free warn sink (model id + Zod issue messages) threaded from run/gate via io.writeErr, so a future schema evolution that invalidates previously-valid rows is observable. Still fail-open (FallbackChain backstop). - #6 (low): document the grace-window soft-delete→unlink resurrection gap (within ADR-0042 §3 best-effort) so a future graceMs shortening triggers a re-verify-before-delete. - #5 companion (low, PERF-CONCURRENCY-2): the grace-reclaim and orphan-sweep CAS deletes ran one await at a time on the exit path — fan them out with Promise.all (independent unlinks). Refs: ADR-0042 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
First Wave-1 batch of Phase 1: the pure-TS seam trio in
@relavium/llm, all built on thefrozen seam — no provider SDK (those land with the adapters in 1.C/1.G/1.H). Plus the roadmap
marker flipping 1.L.0 to ✅ done.
Commits
2024549docs(roadmap): mark 1.L.0 done (PR Phase 1 — 1.L.0: reconcile @relavium/shared to the 2026-06-05 contracts (+ sequencing plan) #6 merged); open Wave 1.a5aa707feat(llm): 1.A — freeze theLLMProviderseam types.7ca81b3feat(llm): 1.B CostTracker + pricing and 1.E ToolNormalizer.What's here
1.A — the immovable seam (
packages/llm/src/types.ts):LlmRequest/LlmMessage/ToolDef/
Usage/CapabilityFlags/LlmError/LlmResult/StreamChunkas Zod schemas,LlmProvideras a TS interface.
ContentPart+AbortSignalLikelive in@relavium/shared(the dependencydirection requires it) and are re-exported by the seam.
@types/json-schemais a dev/type-only dep.1.E — ToolNormalizer: one canonical
ToolDef→ the three native wire shapes (no vendor type);the Gemini OpenAPI-subset reshape (keeps
anyOf+ numeric/string bounds, strips the rest, throwsa typed
ToolSchemaErroron a$ref); synthesized + FIFO-matched Gemini tool-call ids.1.B — CostTracker + pricing:
pricing.ts(the canonical table, integer micro-cents/MTok — theDB unit; Anthropic confirmed via claude-api, others VERIFY-marked);
cost()(each token class billedonce,
inputTokensnet of cache),CostTracker(per-attempt accumulator →cost:updatedfigures);typed
UnknownModelError(never a silent zero).Conformance
any/ unsafeas@types/json-schemais dev/type-only)inputTokens-net clarified)pnpm turbo run lint typecheck test buildgreen · +59 tests in @relavium/llm (39) + @relavium/shared content (+6) · format clean · seam fence airtight · no DB driftVerification
Each workstream passed an independent adversarial multi-agent review. The cost dimension was clean;
the review caught and we fixed: the Gemini reshape over-stripping
anyOf/bounds, an empty-id gap innormalizeToolCall, and a seam-doc↔codeinputTokensconvention divergence (§6 now clarified).Tracked follow-ups (deferred-tasks.md)
cache_writecolumn tomodel_catalog(or knowingly drop it) when the catalog seeder lands.Refs: ADR-0011
🤖 Generated with Claude Code
Summary by Sourcery
Introduce the frozen provider-agnostic LLMProvider seam in @relavium/llm, including normalized messaging and streaming types, tool normalization, and cost tracking with canonical model pricing, while updating roadmap docs to mark Phase 1 as in progress.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Documentation
Tests