1.AF P4 — media access & spend governance (read_media, save_to, cost governor, retention/GC)#35
Merged
Conversation
…rship primitive
The engine-pure half of the read_media byte-delivery authz (ADR-0044 §1), landed before its
consumers (the D12 read_media tool + the 1.AH desktop command) like the rest of the seam shapes:
- ScopeSchema / Scope — a { kind: 'session' | 'workspace', id } authz scope (workspace reserved,
documented-not-implemented), keyed on the existing MEDIA_AUTHZ_SCOPE_KINDS.
- scopeSetIncludes(allowed, requesting) — grants read ONLY on exact kind+id membership in a handle's
allowedScopes (the host-read media_references session/workspace rows); owner-equality is not
expressible and an empty set fails closed. Knowing a sha256 is not authorization.
Reused by both surfaces so the authz is written once. +2 tests (schema accepts session/workspace,
rejects run/node + empty id; membership grants only on exact kind+id, fail-closed on empty).
The read_media tool itself + the ToolDispatchContext media-read delegate + media_scope_denied +
the media_references row-writing lifecycle land in the following D12 increments.
Refs: ADR-0044
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-set authz + Range gate)
The 13th built-in (ADR-0044 §1). Engine-pure byte-delivery gate; the MediaStore + media_references
access is an injected ctx.mediaRead delegate (NOT a ToolHost capability arm — mirrors invokeAgent),
so the dispatch never touches raw bytes:
- tools/types: MediaReadAccess (describe(handle) -> { mimeType, byteLength, allowedScopes } + readRange
-> in-flight base64 MediaSource) + ToolDispatchContext.requestingScope + .mediaRead delegate.
- tools/errors: additive ToolPolicyDenyReason.media_scope_denied (no exhaustive switch breaks — reasons
are construction-site only).
- tools/builtins: read_media { handle, start?, end? } — dispatch does authz FIRST (scopeSetIncludes on
the handle's allowedScopes; deny -> media_scope_denied; never owner-equality), then validateByteRange
(whole-handle default; fail-closed BEFORE any host read), then the host readRange; returns an in-flight
media part. MEDIA_POLICY (read-only, no fs-scope/egress/gate; bypasses the Phase-2 ActionGuard).
+6 tests (authz grant whole-handle + explicit range; media_scope_denied; unknown handle; out-of-bounds
fails before the read; unavailable without the delegate). Built-in count 12 -> 13.
The host wiring (engine builds ctx.mediaRead from ExecutionHost.mediaStore + a media_references db port)
and the media_references row-writing lifecycle are the next D12 increment (D12c, paired with D11 GC).
Refs: ADR-0044, ADR-0029
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erences junction reference) The Node/SQLite host reference for the media retention + authz junction (ADR-0042 §3-4 / ADR-0044 §1). One junction serves both the refcount/terminal-sweep (run/node rows) and read_media authz (session/ workspace rows). Lives in @relavium/db (better-sqlite3); the pure engine never imports it — a host wires it behind the engine reference-lifecycle port + the read_media MediaReadAccess delegate. - recordObject(input): upsert media_objects, idempotent on the content-addressed handle (a conflict just refreshes last_referenced_at, the GC cursor — bytes are identical). - addReference(handle, scopeKind, scopeId): idempotent on the (handle, scopeKind, scopeId) unique index (the refcount is the distinct-row count). The handle FK targets media_objects.handle, so a stray reference fails closed at the FK. - describe(handle): a LIVE handle's mimeType/byteLength + its session/workspace authz scopes ONLY (run/node lifetime rows excluded — they never grant read); undefined for an unknown or deleted_at (GC-reclaimed) handle, so read_media fails closed. Explicit kind-narrowing → Scope[] (no unsafe cast). - removeRunReferences(runId): D11 terminal sweep — removes a run's run-kind rows; returns the count. +6 tests (authz scopes filtered from lifetime rows; unknown/deleted → undefined; addReference + recordObject idempotency; sweep removes only the run rows, leaves session authz intact). The engine wiring (the reference-lifecycle port: record at the deInline choke point + the terminal reclaim; the read_media ctx.mediaRead delegate assembly) is the next increment (D12c-engine + D11). Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ediaHandles (engine contracts) The shared half of the media-reference lifecycle wiring (ADR-0042 §3-4): - MediaReferencePort: the host lifecycle port the pure engine calls — recordRunMedia(meta, runId) at the de-inline choke point + reclaimRun(runId) at the terminal sweep (D11). Documented best-effort + optional: a record/reclaim failure is a retention concern (GC over/under-retention), NEVER an I3 or run-correctness break, so it must never fail the run. (Session/workspace read-grants are the surface/AgentSession concern.) - DurableMediaMeta: the produced-handle metadata mirror (handle/mimeType/modality/byteLength/durationMs?). - collectDurableMediaHandles(value): cycle-safe, handle-deduped collection of the durable handle media parts in an already-de-inlined value (mirrors the containsDurableUnsafeMedia stack walk). Skips a non-handle source, a handle part missing byteLength, or an unknown modality. +4 tests (metadata incl. durationMs; dedupe across array/Map/Set/cycle; skip base64/url/no-byteLength/ unknown-modality; empty). The db adapter (createMediaReferencePort) + the engine wiring (record at #emitDurable, reclaim at terminal) are the next two increments. Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Store -> engine port adapter) Adapts the MediaReferenceStore to the engine's MediaReferencePort (shared): recordRunMedia(meta, runId) = recordObject(meta) + addReference(handle,'run',runId); reclaimRun(runId) = removeRunReferences(runId). DurableMediaMeta is structurally MediaObjectInput, so the record forwards directly. A host wires this behind ExecutionHost.mediaReferences; the pure engine calls only the shared port. +1 test. Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…at choke point, reclaim at terminal) Wires the media-reference lifecycle into the engine (ADR-0042 §3-4), completing D12c + the D11 terminal sweep: - ExecutionHost.mediaReferences? (the shared MediaReferencePort) + createInMemoryHost option. - #emitDurable: after the event is stamped, #recordProducedMedia(durable) records the run's reference for every produced durable handle (collectDurableMediaHandles), and at a TERMINAL event #reclaimRunMedia() runs the D11 sweep (release the run's run-kind refs; a session/workspace read-grant survives). - Both go through #bestEffortMediaRef: a sync throw or async rejection is swallowed (.catch), so a retention-port failure NEVER touches the I3 / gap-free / exactly-one-terminal guarantees — the run is unaffected. No-op without the port or when an event carries no handle. +2 engine tests (a produced handle is recorded for the run + reclaimed once at the terminal; a THROWING port leaves the run completing normally with no byte leak). The grace-window byte-GC (the periodic 0-ref reclaim past last_referenced_at) is a host-periodic db method (D11-gc), landing next; the read_media ctx.mediaRead/requestingScope wiring is the surface/ AgentSession concern (session scope), recorded for 1.AH. Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nceStore The grace-window byte-GC (ADR-0042 §4 step c): soft-deletes (sets deleted_at) every LIVE object that now has ZERO references AND whose last_referenced_at is older than graceMs before now; returns the reclaimed handles so the host can delete their CAS bytes. A host periodic job calls it with the configured grace (default 7 days); the terminal sweep (removeRunReferences / the engine reclaim) is what drops a handle to zero refs first. Single-connection select-then-update is consistent; the update targets exactly the expired handles (not a re-run of the 0-ref filter, which would ignore the grace). Idempotent (an already-deleted object is skipped). +1 test (still-referenced never reclaimed; not-yet- expired skipped; zero-ref + past-grace reclaimed; idempotent). Refs: ADR-0042 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…es load-check The engine-loader capability pass (ADR-0044 §2): validates every agent node's authored output_modalities against its resolved model's media.outputCombinations MEMBERSHIP, using a host-provided model->capabilities catalog (WorkflowModelCatalog, sourced from the DB model_catalog). Runs as a separate pass because WorkflowSchema.superRefine has no catalog (the core->llm parse-time dependency, not circular). Behavior: - a requested combination must EXACTLY match one declared outputCombinations entry (a per-model combination constraint, not a subset of their union); - a model absent from the catalog is DEFERRED (no error — unresolvable at load; the runtime FallbackChain per-modality pre-skip is the backstop, never a silent drop); - an incapable model throws a field-named WorkflowValidationError listing each offending node (secret-free: a model id + the modality set). +5 tests (member passes; non-member throws field-named; unresolvable defers; no-modalities/no-model skipped; exact-match required). Refs: ADR-0044, ADR-0031 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…run.id namespace
Land save_to (ADR-0044 §2): the engine resolves an output node's save_to path
template and its single produced media handle to bytes, then writes via a
host-injected MediaWritePort — policy in the platform-pure engine, the
filesystem mechanism in @relavium/db.
- shared: MediaWritePort + MediaWriteResult — the host save_to write contract
(relativePath, bytes, signal) -> { bytesWritten }, not best-effort (a write
failure fails the node, unlike the retention MediaReferencePort).
- core/interpolation: a new run.id namespace (kind 'run'); RunScope.runId,
resolved only where a runId is in scope (today save_to) — a {{ run.id }}
elsewhere is a typed unresolved_reference, never an empty string. The lexer
lookahead stops run.identity/run.id_x from collapsing to run.id; run.outputs
stays the node namespace.
- core/engine: ExecutionHost.mediaWrite?; #applySaveTo/#performSaveTo run after
the output executor completes — de-inline the captured output to its handle
(deInlineMedia is non-mutating, so state.output keeps the raw in-flight form),
require exactly one handle, MediaStore.get -> MediaWritePort. save_to resolves
ONLY {{ run.id }} (no inputs/ctx/run.outputs into a filesystem path). Secret-
free typed failures (validation / internal); never echoes the path/bytes/host
reason (I3).
- db: createFilesystemMediaWrite(scopeRoot) — fail-closed jail (reject absolute/
drive/UNC/.., realpath the root, verify the deepest existing ancestor is in-
root BEFORE mkdir, refuse a final-component symlink, atomic temp+rename).
- docs: workflow-yaml-spec (save_to resolves run.id only) + security-review
(§Media byte delivery — the save_to write jail), both canonical homes.
Toolchain: lint/typecheck/test/build 16/16 green; prettier clean; Leakwatch 0.
Refs: ADR-0044, ADR-0031, ADR-0042
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nance
Add a disjoint media cost class to the ADR-0028 pre-egress governor + the
realized cost fold (ADR-0044 §3). Media bills per image / audio-second /
video-second, never in tokens, folded into the SAME max_cost_microcents cap —
no new cap dimension, no new event/error class.
- llm/pricing: ModelPricing.mediaOutputRates? (µ¢ per image/audio-sec/video-sec);
every shipped row leaves it undefined (no model prices media output yet).
- llm/cost-tracker: mediaCost(pricing, mediaUnits) — a disjoint addend folded into
cost(); prices only output-direction entries whose unit matches the modality's
billed unit (image=count, audio/video=second); a missing rate or a token-`count`
audio unit is observability-only (degrade-to-0, H4). Realized fold flows into
cumulativeCostMicrocents.
- llm/budget-estimator: estimateMediaCost(model, estimate) — Σ units×rate; unpriced
modality degrades to 0; UnknownModelError for an unlisted model (governor catches).
- core/agent-turn: widen PreEgressHook + AgentTurnParams with outputModalities? +
mediaUnitsEstimate?; awaitPreEgress forwards them (the chain's narrower PreAttemptHook
is unchanged — media is gated at the loop-top vs the primary model).
- core/budget-governor: evaluate/checkPreEgress fold estimateMediaCost into the
projection; the unbounded short-circuit + the UnknownModelError degrade-to-allow
stay before/around it.
- core/agent-runner: buildMediaUnitsEstimate(output_modalities, [defaults].media_cost_estimate)
— one entry per BILLED modality (text/document excluded), config count else the
built-in DEFAULT_MEDIA_UNIT_ESTIMATE; AgentRunnerDeps.mediaCostEstimate.
- shared/config: [defaults].media_cost_estimate (per-modality unit COUNT default).
- db: model_catalog gains nullable media_{image,audio,video}_cost_microcents columns
(the ModelPricing.mediaOutputRates projection) + migration 0003.
- docs: config-spec ([defaults.media_cost_estimate]) + database-schema (model_catalog
media-rate columns), the canonical homes.
Toolchain: lint/typecheck/test/build 16/16 green; prettier clean; Leakwatch 0.
Refs: ADR-0044, ADR-0028, ADR-0031
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Apply the two MEDIUM findings + cheap LOWs from the P4 byte-delivery security review (two independent adversarial passes — no blockers/highs; no traversal / symlink-escape / arbitrary-write reachable): - core/engine #performSaveTo: classify an InterpolationError (a bad save_to path TEMPLATE — e.g. a non-run.id reference resolving to nothing) as `validation`, not `internal`; a genuine write failure stays `internal`. Secret-free message. - db/media-write: a reason-only MediaWriteError + a boundary that re-wraps any RAW Node fs error (realpath/mkdir/writeFile/rename/lstat — whose message/path/dest carry the absolute path) so the resolved path can never escape the port (the I3 self-defending contract, regardless of caller). Atomic-publish temp now reclaimed in a `finally` on ANY non-success exit (writeFile/rename throw or cancel) — no orphaned `.tmp`. Documented the (write-free) stray-empty-dir residual on a symlinked-ancestor mkdir. - core/agent-runner buildMediaUnitsEstimate: replace the `as MediaBilledModality` cast with an `isBilledModality` type guard (narrow without a cast). - Tests: save_to bad-template → validation; scope-root-missing → reason-only MediaWriteError with no path leak. Toolchain: lint/typecheck/test/build 16/16 green; prettier clean; Leakwatch 0. Refs: ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bring the roadmap status current: PR #34 (P3 + P4/D13) merged; the P4 remainder (D12, D11, D15, D16, D17 + the byte-delivery review + canonical-home docs) is landed on `development`, pending merge. The keychain no-raw-key IPC test is recorded as deferred to 1.AH (no Phase-1 desktop surface; ADR-0044 §4). The matrix row stays ◇ until the PR merges (Roadmap-Done-After-Merge). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Apply the real findings from the 46-agent review of the P4 changeset (9 dimensions, every finding double-verified; 0 blockers/highs in reachable code). A — real defects fixed: - A1 [high] output_modalities is now lowered onto the LlmRequest (buildRequest), so the FallbackChain output-combination pre-skip — the runtime backstop the D15 load-check defers to (ADR-0044 §2) — actually fires; previously an incapable model silently returned text. + a skip-backstop test (incapable model → chain exhausts → typed failure, never a text drop). - A2 [med] reconcile() best-effort reclaims a crashed non-resumable run's media refs (#bestEffortReclaim), so a crash no longer permanently orphans partial media (ADR-0042 §4); retention failure never abandons reconciliation. + tests. - A3 [med] boundForModel is media-aware: a read_media/media tool result's inline base64 is redacted from the text path (toText), so it can never serialize into agent:tool_result.outputSummary / the spill / the over-cap preview — the I3 gap the emit-time deInlineMedia choke point cannot catch (a base64 substring inside a flat string). The model-facing value keeps the bytes (rides the seam). + tests. - A4 [low] read_media whole-handle read of a 0-byteLength handle returns an empty source instead of tripping the default end = byteLength-1 = -1 range rejection. + tests. B — cheap hardening: - wx (O_CREAT|O_EXCL) on the save_to temp write — refuses a pre-existing symlink at the temp path (defense-in-depth atop the unguessable UUID name). - async-rejection arm of the run-loop best-effort media-ref port now tested (no unhandled rejection); run.id no-spurious-edge / not-a-secret-source / not-a-pre-run-violation regressions pinned in dag/analyze tests. - documented the url-bearing save_to double-fetch tradeoff. D — recorded the deferred HOST-WIRING half (D12 host MediaReadAccess + session- scope population, the D15 loader call, the D17 config/resolveForEgress wiring) in deferred-tasks.md §1.AF P4 + the roadmap, so D12/D15/D17 are not read as live end-to-end — the engine policy is complete + tested; the surface wiring is 1.AH. Toolchain: lint/typecheck/test/build 16/16 green (786 core tests); prettier clean; Leakwatch 0. Refs: ADR-0044, ADR-0042, ADR-0031, ADR-0028 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughPhase 1 / P4 multimodal media delivery: adds Changes1.AF P4 Multimodal Media Delivery
Sequence DiagramssequenceDiagram
participant Engine
participant applySaveTo
participant resolveTemplate
participant collectDurableMediaHandles
participant MediaWritePort
participant MediaReferencePort
participant BudgetGovernor
rect rgba(100, 149, 237, 0.5)
note over Engine,BudgetGovernor: Pre-egress media cost check
Engine->>BudgetGovernor: checkPreEgress(model, maxTokens, mediaUnitsEstimate)
BudgetGovernor-->>Engine: allow | warn | fail
end
rect rgba(60, 179, 113, 0.5)
note over Engine,MediaReferencePort: Post-turn: save_to and media reference lifecycle
Engine->>applySaveTo: completed executor result
applySaveTo->>resolveTemplate: save_to template + run.id scope
resolveTemplate-->>applySaveTo: resolved relative path
applySaveTo->>collectDurableMediaHandles: de-inlined output value
collectDurableMediaHandles-->>applySaveTo: DurableMediaMeta[]
applySaveTo->>MediaWritePort: write(relativePath, bytes, signal)
MediaWritePort-->>applySaveTo: MediaWriteResult
applySaveTo-->>Engine: NodeSuccess | NodeFailure
Engine->>MediaReferencePort: recordRunMedia(runId, meta)
Engine->>MediaReferencePort: reclaimRun(runId) on terminal
end
sequenceDiagram
participant LLM
participant readMediaTool
participant scopeSetIncludes
participant validateByteRange
participant MediaReadAccess
participant bounding_toText
LLM->>readMediaTool: dispatch(handle, start?, end?)
readMediaTool->>MediaReadAccess: describe(handle)
MediaReadAccess-->>readMediaTool: MediaHandleInfo | undefined
readMediaTool->>scopeSetIncludes: allowedScopes vs requestingScope
scopeSetIncludes-->>readMediaTool: granted | ToolPolicyError
readMediaTool->>validateByteRange: (start, end, byteLength)
validateByteRange-->>readMediaTool: valid | ToolArgsInvalidError
readMediaTool->>MediaReadAccess: readRange(handle, range)
MediaReadAccess-->>readMediaTool: bytes
readMediaTool-->>LLM: MediaPart{mimeType, base64}
LLM->>bounding_toText: build summary
bounding_toText->>bounding_toText: redactInlineMediaForText (remove base64)
bounding_toText-->>LLM: byte-free descriptor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 implements the remaining Phase-1 engine media plumbing (1.AF P4), introducing the read_media built-in tool with scope-set authorization, the save_to write port with a fail-closed filesystem jail, terminal-state media reference reclamation, and catalog-aware workflow validation. Feedback on the changes highlights two key improvements: adding optional chaining to prevent a potential TypeError when accessing model capabilities in the catalog validator, and explicitly handling Map and Set instances in the inline media redactor to avoid data loss during text serialization.
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.
| if (caps === undefined) { | ||
| continue; // unresolvable model — defer to the runtime FallbackChain pre-skip (never a silent drop) | ||
| } | ||
| if (!isOutputComboSupported(caps.media.outputCombinations, node.output_modalities)) { |
There was a problem hiding this comment.
If caps.media is undefined or null (for example, in older or custom model definitions), accessing caps.media.outputCombinations will throw a TypeError. Using optional chaining prevents potential runtime crashes.
Suggested change
| if (!isOutputComboSupported(caps.media.outputCombinations, node.output_modalities)) { | |
| if (!isOutputComboSupported(caps.media?.outputCombinations ?? [], node.output_modalities)) { |
Comment on lines
+80
to
+104
| function redactInlineMediaForText(value: unknown, seen: WeakSet<object>): unknown { | ||
| if (typeof value === 'string') { | ||
| return isBase64DataUri(value) ? '[base64 data URI omitted]' : value; | ||
| } | ||
| if (typeof value !== 'object' || value === null) { | ||
| return value; | ||
| } | ||
| if (seen.has(value)) { | ||
| return '[cyclic]'; | ||
| } | ||
| seen.add(value); | ||
| if (Array.isArray(value)) { | ||
| return value.map((item) => redactInlineMediaForText(item, seen)); | ||
| } | ||
| const record = value as Record<string, unknown>; | ||
| if (isCanonicalBase64Source(record)) { | ||
| const data = record['data']; | ||
| return { kind: 'base64', bytes: typeof data === 'string' ? data.length : 0 }; // drop the bytes | ||
| } | ||
| const out: Record<string, unknown> = {}; | ||
| for (const [key, item] of Object.entries(record)) { | ||
| out[key] = redactInlineMediaForText(item, seen); | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
The redactInlineMediaForText function does not handle Map and Set instances. Since pushMediaWalkChildren explicitly supports walking Map and Set objects, these types can be present in the tool results. If they are not handled here, they will be treated as plain records, which converts them to empty objects {} and results in data loss in the text summary/spill/preview. Handling Map and Set explicitly ensures their contents are correctly redacted and preserved.
function redactInlineMediaForText(value: unknown, seen: WeakSet<object>): unknown {
if (typeof value === 'string') {
return isBase64DataUri(value) ? '[base64 data URI omitted]' : value;
}
if (typeof value !== 'object' || value === null) {
return value;
}
if (seen.has(value)) {
return '[cyclic]';
}
seen.add(value);
if (Array.isArray(value)) {
return value.map((item) => redactInlineMediaForText(item, seen));
}
if (value instanceof Map) {
const out = new Map();
for (const [key, item] of value.entries()) {
out.set(redactInlineMediaForText(key, seen), redactInlineMediaForText(item, seen));
}
return out;
}
if (value instanceof Set) {
const out = new Set();
for (const item of value.values()) {
out.add(redactInlineMediaForText(item, seen));
}
return out;
}
const record = value as Record<string, unknown>;
if (isCanonicalBase64Source(record)) {
const data = record['data'];
return { kind: 'base64', bytes: typeof data === 'string' ? data.length : 0 }; // drop the bytes
}
const out: Record<string, unknown> = {};
for (const [key, item] of Object.entries(record)) {
out[key] = redactInlineMediaForText(item, seen);
}
return out;
}![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 (4)
packages/db/src/media-reference-store.test.ts (1)
86-101: ⚡ Quick winAdd a regression case for “reclaimed then re-recorded” handles.
The suite currently validates reclaim behavior, but not reintroduction of the same handle afterward. Adding that case will lock the lifecycle contract and prevent silent authz regressions.
✅ Suggested test extension
it('reclaimExpired soft-deletes only zero-ref objects past the grace window (D11 GC)', () => { @@ expect(store.reclaimExpired(0)).toEqual([HANDLE]); expect(store.describe(HANDLE)).toBeUndefined(); // soft-deleted expect(store.reclaimExpired(0)).toEqual([]); // idempotent — already deleted + + // Re-recording the same content-addressed handle should revive it for describe/authz. + record(); + store.addReference(HANDLE, 'session', 's2'); + expect(store.describe(HANDLE)?.allowedScopes).toEqual([{ kind: 'session', id: 's2' }]); });🤖 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/db/src/media-reference-store.test.ts` around lines 86 - 101, The test validates the reclaim behavior and idempotency but does not verify what happens when a reclaimed handle is re-recorded. After the final expect(store.reclaimExpired(0)).toEqual([]) assertion that confirms idempotency, add a regression case by re-recording the same HANDLE (call record() again), then verify that store.describe(HANDLE) returns a defined value again and that the handle can re-enter the normal lifecycle. This locks the contract and prevents silent authorization regressions when handles transition from reclaimed state back into active use.packages/core/src/tools/builtins.test.ts (1)
373-375: ⚡ Quick winReplace unsafe type assertion with a runtime type guard.
Line 374 uses an unsafe
ascast; narrowerrwithinstanceofbefore reading.reason.As per coding guidelines,
**/*.{ts,tsx}requires “noanytypes or unsafeascasts” and to “Use type guards instead of unsafe type assertions.”Suggested change
expect(err).toBeInstanceOf(ToolPolicyError); - expect((err as ToolPolicyError).reason).toBe('media_scope_denied'); + if (!(err instanceof ToolPolicyError)) { + throw err; + } + expect(err.reason).toBe('media_scope_denied');🤖 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/core/src/tools/builtins.test.ts` around lines 373 - 375, Replace the unsafe type assertion `(err as ToolPolicyError).reason` with a proper runtime type guard. Add an `instanceof ToolPolicyError` check to narrow the type of err before accessing the `.reason` property. This ensures type safety without using the unsafe `as` cast, following the coding guidelines that require type guards instead of unsafe type assertions.Source: Coding guidelines
packages/core/src/tools/bounding.ts (1)
94-101: ⚡ Quick winReplace the unsafe type assertion with a type guard.
Line 94 uses
value as Record<string, unknown>, which violates the strict TypeScript rule for unsafe assertions.♻️ Suggested refactor
+function isRecord(value: unknown): value is Record<string, unknown> { + return typeof value === 'object' && value !== null; +} + function redactInlineMediaForText(value: unknown, seen: WeakSet<object>): unknown { if (typeof value === 'string') { return isBase64DataUri(value) ? '[base64 data URI omitted]' : value; } - if (typeof value !== 'object' || value === null) { + if (!isRecord(value)) { return value; } if (seen.has(value)) { return '[cyclic]'; } seen.add(value); if (Array.isArray(value)) { return value.map((item) => redactInlineMediaForText(item, seen)); } - const record = value as Record<string, unknown>; + const record = value;As per coding guidelines, “TypeScript-first, strict. No
any, no unsafeas. Use type guards instead of unsafe type assertions.”🤖 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/core/src/tools/bounding.ts` around lines 94 - 101, The unsafe type assertion `value as Record<string, unknown>` on line 94 violates strict TypeScript guidelines. Replace this assertion with a proper type guard function that validates whether the value is actually a Record before proceeding with operations like isCanonicalBase64Source check and Object.entries iteration. Create a type guard that checks if value is an object type, then use that guard to conditionally execute the Record-related operations instead of relying on the unsafe as keyword.Source: Coding guidelines
packages/llm/src/pricing.ts (1)
50-54: ⚡ Quick winUse a modality-derived mapped type for
mediaOutputRatesto prevent key drift.Hard-coding modality keys here can silently diverge from the shared canonical media modality set over time. A mapped type keeps this contract synchronized at compile time.
♻️ Proposed refactor
+import type { MediaBilledModality } from '`@relavium/shared`'; ... - readonly mediaOutputRates?: { - readonly image?: number; - readonly audio?: number; - readonly video?: number; - }; + readonly mediaOutputRates?: Partial<Record<MediaBilledModality, number>>;🤖 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/pricing.ts` around lines 50 - 54, The mediaOutputRates property in the pricing.ts file uses hard-coded modality keys (image, audio, video) that can drift from the canonical media modality set over time. Replace the explicit object type definition for mediaOutputRates with a mapped type that derives from a shared canonical modalities type, ensuring the keys stay synchronized at compile time. This mapped type approach will automatically reflect any changes to the canonical modality set without requiring manual updates to this interface.
🤖 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/core/src/engine/agent-runner.ts`:
- Around line 137-140: The isBilledModality function uses an unsafe cast `as
readonly string[]` that removes literal type information from
MEDIA_BILLED_MODALITIES, violating TypeScript safety guidelines. Replace the
current implementation by creating a ReadonlySet from MEDIA_BILLED_MODALITIES
and using its .has() method instead of .includes() with the cast. This maintains
the literal types from the const-asserted array and eliminates the unsafe type
casting.
In `@packages/core/src/tools/bounding.ts`:
- Around line 115-123: The redaction call to redactInlineMediaForText is
currently executed outside the try-catch block, which means any thrown errors
from getters or proxies in the tool output during redaction will not be caught
and will cause the function to fail ungracefully. Move the logic that assigns to
the safe variable (the conditional check for containsInlineMediaBytes and the
call to redactInlineMediaForText) inside the try block so that any throws during
the redaction traversal are caught by the existing error handler and allow the
function to degrade safely to the "[unserializable]" fallback.
In `@packages/db/drizzle/0003_cloudy_wind_dancer.sql`:
- Around line 1-3: The Drizzle migration artifacts in the
0003_cloudy_wind_dancer.sql file are out of sync with the current schema
definition in src/schema.ts, causing CI to fail with schema-drift errors.
Regenerate all Drizzle migration files by running the database generation
command for the `@relavium/db` package, then commit both the resulting SQL
migration files and the Drizzle metadata files together to ensure the migration
state matches the current schema definition.
In `@packages/db/src/media-reference-store.ts`:
- Around line 86-89: The issue is that when a handle is re-recorded (already
exists in the database), the onConflictDoUpdate call on mediaObjects.handle only
refreshes the lastReferencedAt timestamp but does not clear the deletedAt field
that may have been set during garbage collection soft-deletion. This causes
describe() to keep returning undefined for reintroduced content. In the set
object of the onConflictDoUpdate call, add deletedAt: null alongside the
lastReferencedAt: ts update to ensure that soft-deleted handles are properly
restored when they are re-recorded.
In `@packages/db/src/schema.ts`:
- Around line 111-113: The monetary rate columns mediaImageCostMicrocents,
mediaAudioCostMicrocents, and mediaVideoCostMicrocents are defined as integer
fields without non-negative constraints, allowing them to persist negative
values which can undermine budget enforcement. Add a non-negative constraint
(such as .unsigned() or a check constraint ensuring values are greater than or
equal to 0) to each of these three integer column definitions in the schema.
Apply this constraint to all instances of these columns throughout the schema,
including the sections mentioned in lines 125-133.
In `@packages/shared/src/config.ts`:
- Around line 104-107: The ADR relative link path in the JSDoc comment block for
the media-output unit-count default is rooted incorrectly. Update the link
reference `../decisions/0044-media-access-governance-read-media-save-to-cost.md`
to use the correct relative path from the file location
packages/shared/src/config.ts to the actual decisions directory location.
Traverse up the appropriate number of directory levels from src/ to reach the
project root and then navigate to the decisions folder, ensuring the relative
link properly points to the ADR file.
---
Nitpick comments:
In `@packages/core/src/tools/bounding.ts`:
- Around line 94-101: The unsafe type assertion `value as Record<string,
unknown>` on line 94 violates strict TypeScript guidelines. Replace this
assertion with a proper type guard function that validates whether the value is
actually a Record before proceeding with operations like isCanonicalBase64Source
check and Object.entries iteration. Create a type guard that checks if value is
an object type, then use that guard to conditionally execute the Record-related
operations instead of relying on the unsafe as keyword.
In `@packages/core/src/tools/builtins.test.ts`:
- Around line 373-375: Replace the unsafe type assertion `(err as
ToolPolicyError).reason` with a proper runtime type guard. Add an `instanceof
ToolPolicyError` check to narrow the type of err before accessing the `.reason`
property. This ensures type safety without using the unsafe `as` cast, following
the coding guidelines that require type guards instead of unsafe type
assertions.
In `@packages/db/src/media-reference-store.test.ts`:
- Around line 86-101: The test validates the reclaim behavior and idempotency
but does not verify what happens when a reclaimed handle is re-recorded. After
the final expect(store.reclaimExpired(0)).toEqual([]) assertion that confirms
idempotency, add a regression case by re-recording the same HANDLE (call
record() again), then verify that store.describe(HANDLE) returns a defined value
again and that the handle can re-enter the normal lifecycle. This locks the
contract and prevents silent authorization regressions when handles transition
from reclaimed state back into active use.
In `@packages/llm/src/pricing.ts`:
- Around line 50-54: The mediaOutputRates property in the pricing.ts file uses
hard-coded modality keys (image, audio, video) that can drift from the canonical
media modality set over time. Replace the explicit object type definition for
mediaOutputRates with a mapped type that derives from a shared canonical
modalities type, ensuring the keys stay synchronized at compile time. This
mapped type approach will automatically reflect any changes to the canonical
modality set without requiring manual updates to this interface.
🪄 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: 3ba5d197-b0c8-478e-9859-db16f2c01d63
📒 Files selected for processing (51)
docs/reference/contracts/config-spec.mddocs/reference/contracts/workflow-yaml-spec.mddocs/reference/desktop/database-schema.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-1-engine-and-llm.mddocs/standards/security-review.mdpackages/core/src/dag.test.tspackages/core/src/engine/agent-runner.test.tspackages/core/src/engine/agent-runner.tspackages/core/src/engine/agent-turn.tspackages/core/src/engine/budget-governor.test.tspackages/core/src/engine/budget-governor.tspackages/core/src/engine/engine.test.tspackages/core/src/engine/engine.tspackages/core/src/engine/execution-host.tspackages/core/src/index.tspackages/core/src/interpolation/analyze.test.tspackages/core/src/interpolation/references.test.tspackages/core/src/interpolation/references.tspackages/core/src/interpolation/resolve.test.tspackages/core/src/interpolation/resolve.tspackages/core/src/interpolation/scope.tspackages/core/src/tools/bounding.test.tspackages/core/src/tools/bounding.tspackages/core/src/tools/builtins.test.tspackages/core/src/tools/builtins.tspackages/core/src/tools/errors.tspackages/core/src/tools/registry.test.tspackages/core/src/tools/types.tspackages/core/src/validate-catalog.test.tspackages/core/src/validate-catalog.tspackages/db/drizzle/0003_cloudy_wind_dancer.sqlpackages/db/drizzle/meta/0003_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/index.tspackages/db/src/media-reference-store.test.tspackages/db/src/media-reference-store.tspackages/db/src/media-write.test.tspackages/db/src/media-write.tspackages/db/src/schema.tspackages/llm/src/budget-estimator.test.tspackages/llm/src/budget-estimator.tspackages/llm/src/cost-tracker.test.tspackages/llm/src/cost-tracker.tspackages/llm/src/index.tspackages/llm/src/pricing.tspackages/shared/src/config.test.tspackages/shared/src/config.tspackages/shared/src/content.test.tspackages/shared/src/content.ts
Comment on lines
+1
to
+3
| ALTER TABLE `model_catalog` ADD `media_image_cost_microcents` integer;--> statement-breakpoint | ||
| ALTER TABLE `model_catalog` ADD `media_audio_cost_microcents` integer;--> statement-breakpoint | ||
| ALTER TABLE `model_catalog` ADD `media_video_cost_microcents` integer; No newline at end of file |
There was a problem hiding this comment.
Regenerate and recommit Drizzle artifacts — current migration set is out of sync.
CI is failing with explicit schema-drift errors for @relavium/db. Please rerun generation and commit the resulting SQL/meta files together so migration state matches src/schema.ts and derived checks.
pnpm --filter `@relavium/db` db:generate🧰 Tools
🪛 GitHub Actions: CI / 2_lint · typecheck · test.txt
[error] 1-1: Migration out of sync: The committed @relavium/db migration is out of sync with src/schema.ts (or an upstream @relavium/shared enum the checks derive from). Regenerate and commit: pnpm --filter @relavium/db db:generate
🪛 GitHub Actions: CI / lint · typecheck · test
[error] 1-1: The committed @relavium/db migration is out of sync with src/schema.ts (or an upstream @relavium/shared enum the CHECKs derive from). Regenerate and commit: pnpm --filter @relavium/db db:generate
🤖 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/db/drizzle/0003_cloudy_wind_dancer.sql` around lines 1 - 3, The
Drizzle migration artifacts in the 0003_cloudy_wind_dancer.sql file are out of
sync with the current schema definition in src/schema.ts, causing CI to fail
with schema-drift errors. Regenerate all Drizzle migration files by running the
database generation command for the `@relavium/db` package, then commit both the
resulting SQL migration files and the Drizzle metadata files together to ensure
the migration state matches the current schema definition.
Source: Pipeline failures
Comment on lines
+111
to
+113
| mediaImageCostMicrocents: integer('media_image_cost_microcents'), | ||
| mediaAudioCostMicrocents: integer('media_audio_cost_microcents'), | ||
| mediaVideoCostMicrocents: integer('media_video_cost_microcents'), |
There was a problem hiding this comment.
Add non-negative DB checks for the new media rate columns.
media_*_cost_microcents are monetary rate fields, but they currently allow negative values. Negative persisted rates can underprice projected media cost and weaken budget enforcement.
Suggested fix
export const modelCatalog = sqliteTable(
'model_catalog',
{
@@
mediaImageCostMicrocents: integer('media_image_cost_microcents'),
mediaAudioCostMicrocents: integer('media_audio_cost_microcents'),
mediaVideoCostMicrocents: integer('media_video_cost_microcents'),
@@
},
(t) => [
+ check(
+ 'model_catalog_media_image_cost_non_negative',
+ sql`${t.mediaImageCostMicrocents} is null or ${t.mediaImageCostMicrocents} >= 0`,
+ ),
+ check(
+ 'model_catalog_media_audio_cost_non_negative',
+ sql`${t.mediaAudioCostMicrocents} is null or ${t.mediaAudioCostMicrocents} >= 0`,
+ ),
+ check(
+ 'model_catalog_media_video_cost_non_negative',
+ sql`${t.mediaVideoCostMicrocents} is null or ${t.mediaVideoCostMicrocents} >= 0`,
+ ),
uniqueIndex('idx_model_catalog_provider_model')
.on(t.providerId, t.modelId)
.where(sql`${t.deletedAt} is null`),Also applies to: 125-133
🤖 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/db/src/schema.ts` around lines 111 - 113, The monetary rate columns
mediaImageCostMicrocents, mediaAudioCostMicrocents, and mediaVideoCostMicrocents
are defined as integer fields without non-negative constraints, allowing them to
persist negative values which can undermine budget enforcement. Add a
non-negative constraint (such as .unsigned() or a check constraint ensuring
values are greater than or equal to 0) to each of these three integer column
definitions in the schema. Apply this constraint to all instances of these
columns throughout the schema, including the sections mentioned in lines
125-133.
…-valid only)
Verified each external/Sonar finding against current code; fixed the still-valid
ones (minimal), skipped the rest with reasons below.
Fixed:
- [correctness] media-reference-store recordObject now clears deleted_at on a
re-record (onConflictDoUpdate set deletedAt:null), so a GC-soft-deleted handle
is RESURRECTED when the same content-addressed bytes are produced again —
previously describe() stayed undefined and read_media denied live re-introduced
content. + a re-record-after-reclaim regression test.
- [robustness] bounding.toText now runs the media-redaction INSIDE the try, so a
throwing getter/proxy during the redaction walk degrades to '[unserializable]'
instead of escaping.
- [TS] agent-runner isBilledModality uses a ReadonlySet<string> (drops the
`as readonly string[]` cast); bounding redactor uses an isRecord guard (drops
the `as Record` cast); builtins.test narrows via `instanceof ToolPolicyError`
(drops the `as` cast).
- [drift-proof] pricing.ts mediaOutputRates is now a mapped type keyed by
MediaBilledModality (was a hand-maintained image/audio/video literal).
- [docs] config.ts ADR-0044 link fixed (../decisions → ../../../docs/decisions).
- [Sonar] references RUN_ID regex uses \w; media-write.test uses String.raw for
the Windows/UNC paths; content.test sort takes an explicit localeCompare
comparator (S2871).
Skipped (verified not actionable):
- Drizzle migration "drift": `pnpm --filter @relavium/db db:generate` reports
"No schema changes" — 0003 is in sync, no CI drift (false positive).
- schema.ts media-rate columns non-negative constraint: the sibling money columns
(input/output/cached microcents) carry no such CHECK either; rates are
catalog-seeded (verified, never negative), not user input; `.unsigned()` is not
a SQLite/Drizzle option — adding a check to only the 3 new columns would be
inconsistent and needs a migration for no real exploit surface.
- validate-catalog caps.media optional chaining: CapabilityFlags requires `media`
(Zod MediaCapabilitiesSchema at the seam), so it cannot be undefined for a valid
catalog result; `?.` would only mask a host contract violation.
- bounding redactor Map/Set handling: a Map/Set collapses to `{}` exactly as
JSON.stringify renders it (non-serializable) — no base64 leaks and no regression
vs the un-redacted JSON path, so no change needed for the I3 purpose.
Toolchain: lint/typecheck/test/build 16/16 green (786 core tests); prettier
clean; Leakwatch 0.
Refs: ADR-0044, ADR-0042
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… only)
A second multi-agent review pass (Sonnet, 9 dimensions, double-verified) on top
of the first review's fixes — 0 blockers/highs in reachable code. Salvaged its
findings and applied the still-valid ones (minimal); deferred a read_media
result-contract cluster to 1.AH and skipped the rest with reasons.
Fixed:
- [I3] bounding redaction now strips RAW BINARY BUFFERS (Uint8Array/ArrayBuffer)
too — a buffer tool result would otherwise JSON-serialize its decimal byte
values into the agent:tool_result.outputSummary/spill. The walk now runs for
every non-string result (so a buffer-only result is caught) and uses a
PLAIN-object guard so Date/Map/Set/class results render natively (not collapsed
to {}); the cycle-safe walk also makes a circular result serialize ('[cyclic]').
- [schema] read_media 0-byte whole-read now returns a HANDLE source, not
`{ kind:'base64', data:'' }` (which violates the base64 nonEmptyString contract
and breaks deInlineMedia's empty-decode).
- [defense] read_media `handle` arg is validated against MEDIA_HANDLE_PATTERN at
parse (engine-pure structural check); durableMediaMetaOf also rejects a
non-pattern ref before recording it.
- [governance] the FallbackChain per-attempt budget hook now carries the media
estimate (outputModalities/mediaUnitsEstimate) so a cross-provider FAILOVER is
media-priced, not token-only (ADR-0044 §3) — was silently dropped.
- [scalability] reclaimExpired chunks its `handle IN (…)` UPDATE under SQLite's
999-bound-parameter floor (a large GC sweep would otherwise throw at runtime).
- [parse] RUN_ID lookahead also excludes `-` so `{{run.id-x}}` is `unknown`, not
a mis-parsed kind:'run'.
- [bound] ScopeSchema.id gains .max(255) (matches the mimeType/name convention).
- [observability] redactor marker field renamed bytes→base64Length (it is the
base64 char length, ~1.33× the byte count); start/end LLM-visible schema gains
minimum:0.
- Tests: denied-scope on a 0-byte handle (authz BEFORE the short-circuit),
malformed-handle parse rejection, array-of-media + binary-buffer + Date
redaction, symlink-jail errors asserted as reason-only MediaWriteError (no path
leak), save_to template-failure message leaks no reference name, run.id
trailing-path → unresolved_reference; corrected a misleading analyze.test comment.
Deferred to 1.AH (recorded in deferred-tasks §1.AF P4) — the read_media result
must carry a HANDLE (not inline base64) to satisfy LlmMessageSchema on a
multi-turn call; narrow MediaReadAccess.readRange + thread AbortSignalLike — all
co-decided with the 1.AH host wiring (read_media is inert today). Plus the
budget-governor non-zero-media-estimate test gap.
Skipped (verified): drift (db:generate = no changes), the `as RunEventDraft`
cast (pre-existing/merged, Zod re-validates at #bus.next), the soft-delete
"orphan media_references" claim (a reclaimed handle is zero-ref by construction,
so no rows exist to orphan), a plain non-data:-URI base64 string (undetectable
without false positives; deInlineMedia is the primary gate), and unbounded
transcript / cyclic-marker cosmetics (by-design).
Toolchain: lint/typecheck/test/build 16/16 green; prettier clean; Leakwatch 0.
Refs: ADR-0044, ADR-0042, ADR-0031, ADR-0028
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 20, 2026
…surfaces All 1.AF (engine media plumbing) PRs are merged to main (2026-06-20): P1+P2 (#33), P3 + P4/D13 (#34), P4 remainder (#35), and the multi-agent + external review follow-ups (#36). Flip the status markers from in-progress / pending-merge to Done across CLAUDE.md, README.md, current.md, the phase-1 task entry + matrix row + the 1.m6 milestone row, and the deferred-tasks note. The deferred host-wiring half (D12 MediaReadAccess + session-scope population, the D15 loader call, the D17/resolveForEgress config) and the keychain no-raw-key IPC test remain owned by 1.AH (already recorded in deferred-tasks.md). Remaining Phase-1 work: 1.AG/1.AH. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 25, 2026
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.
1.AF P4 — media access & spend governance (the P4 remainder)
Completes workstream 1.AF (engine media plumbing) P4 — the media access & spend boundaries on the ADR-0031 seam. Builds on PR #33 (P1+P2) and PR #34 (P3 + D13), both merged. Lands the engine-pure policy; the host mechanism/wiring half is 1.AH/surface (recorded in deferred-tasks.md §1.AF P4).
Design ADRs: 0042 (storage/retention) · 0043 (egress) · 0044 (access/spend).
What landed
read_media— the 13th built-in tool + one engine-pure scope-set authz primitive (scopeSetIncludes; owner-equality rejected, sha256-knowledge ≠ authorization) + the byte-deliveryRangegate (validateByteRange, fail-closed) + the additiveToolPolicyDenyReason.media_scope_denied; themedia_objects/media_referencesjunction store (MediaReferenceStore+createMediaReferencePort), recording a produced handle's run reference at the#emitDurablechoke point.runrefs atrun:completed|failed|cancelled) +reclaimExpiredgrace-window GC (soft-delete zero-ref objects past the configured grace).validateWorkflowWithCatalog: the engine-loaderoutput_modalities→media.outputCombinationsmembership check (defers an unresolvable model; the runtime FallbackChain pre-skip — now wired onto the request — is the backstop).save_to—MediaWritePort(shared) +ExecutionHost.mediaWrite+ the engine output-node orchestration (resolve{{ run.id }}→ relative path, extract the single produced handle →MediaStore.get→ write; fails the node, not best-effort) + the dbcreateFilesystemMediaWritefail-closed jail (realpath+commonpath, symlinks off, atomicwxtemp +rename, reason-onlyMediaWriteError) + the newrun.idinterpolation namespace.ModelPricing.mediaOutputRates+mediaCost/estimateMediaCost(a disjoint addend folded into the existingmax_cost_microcentscap, degrade-to-0 on a missing rate) + the widenedPreEgressHook(outputModalities/mediaUnitsEstimate) +[defaults].media_cost_estimate+ themodel_catalogmedia-rate columns (migration0003).Reviews (both adversarial, multi-agent)
dd03cc8).e6e8545:output_modalitiesis now lowered onto theLlmRequest, so the FallbackChain output-combination pre-skip (the D15 runtime backstop) actually fires — previously an incapable model silently returned text.reconcile()best-effort reclaims a crashed run's media refs (no permanent orphan).boundForModelis media-aware — aread_media/media tool result never serializes base64 intoagent:tool_result.outputSummary/spill (the I3 gap the emit choke point can't catch).read_media0-byteLength whole-handle off-by-one;wxtemp hardening; run.id / best-effort regression pins.Deferred to 1.AH (recorded, not in this PR)
The host-wiring half: the D12 host
MediaReadAccessimpl + session-scope population, the D15 loader call, the D17 config/resolveForEgresswiring — so D12/D15/D17 are engine-policy-complete but inert end-to-end until a surface wires them. The keychain no-raw-key IPC test (no Phase-1 desktop surface; ADR-0044 §4).Conformance
pnpm turbo run lint typecheck test build— 16/16 (786 core tests)pnpm run format:checkclean · Leakwatch 0packages/core) — save_to fs mechanism lives in@relavium/db@relavium/llmseam holds (no vendor type crosses); secrets/I3 (no bytes/path/secret in events/logs)🤖 Generated with Claude Code
Summary by CodeRabbit
read_mediatool for controlled, scope-authorized byte-range access.save_tofor output nodes to persist media bytes using{{ run.id }}(with safer path handling).output_modalitiesvalidation against model capability sets.[defaults.media_cost_estimate]defaults for media-output unit estimates.{{ run.id }}semantics.