fix(1.AF): engine media plumbing — multi-agent review follow-ups (H1/H2/GC/egress-test/doc-drift)#36
Merged
Merged
Conversation
…gress-test/doc-drift)
13-dimension, double-verified end-to-end review of 1.AF (engine media plumbing).
This lands the two confirmed HIGH items, the behavioral mediums, and the doc-drift
cluster; the 1.AH-latent host-wiring items stay recorded in deferred-tasks.md.
H1 (I3): redact inline media from agent:tool_call.toolInput before it rides the
event/IPC/log stream. A model can emit a base64 data: URI or a {kind:'base64',data}
object as a tool argument; the result side (outputSummary) was redacted this
workstream but the symmetric input side was not. Reuse the same redactInlineMedia
walk at sanitizeInput — display-only (the dispatch already ran on the real args).
H2: unify the load-time output_modalities check (validate-catalog) and the runtime
FallbackChain pre-skip (capabilities) on ONE exact-membership predicate
(isOutputCombinationSupported, @relavium/llm). The runtime did text-strip + subset,
which wrongly ADMITTED wire-invalid combinations (the closed set exists to reject)
and diverged from the load-check + ADR-0031 decision #3. Both now share the
predicate; text-only stays always-emittable (no regression for []-combo models).
GC grace (db): bump media_objects.last_referenced_at on addReference AND the
terminal sweep (removeRunReferences), so the grace window measures from
drop-to-zero, not production time (ADR-0042 section 4). A long-lived handle losing
its last reference no longer gets zero effective grace.
Egress test (db): mock node:https to exercise the concrete nodeMediaEgressDeps
(the un-injectable pin/SNI/port/family wiring) + the default-deps range-block — the
one part of the SSRF-critical path with no executable proof (E43-7).
Docs: sse-event-schema (cost:updated.mediaUnits deferred — the layering forbids
shared importing the llm seam type), tool-registry (thirteen built-ins; add
media_scope_denied to the deny union), built-in-tools (read_media row); record the
keychain no-raw-key IPC test + the mediaUnits-surface deferrals in deferred-tasks.md.
Validation: lint/typecheck/test/build 16/16 green, prettier clean, Leakwatch 0.
Refs: ADR-0042, ADR-0043, ADR-0044
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, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR introduces an exact-match output-modality predicate ( ChangesMedia pipeline: exact-match modality gating, inline media redaction, GC cursor refresh, nodeMediaEgressDeps tests, docs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 introduces the read_media built-in tool, implements inline media redaction for tool input observability events, and unifies the load-time and runtime output modality combination checks using a shared predicate. It also improves the media reference store's garbage collection cursor logic by updating the last referenced timestamp during reference mutations, and adds comprehensive tests for the Node media egress mechanism. Feedback on the changes highlights a bug in the unified isOutputCombinationSupported helper where duplicate requested modalities of matching length could incorrectly pass validation, and provides a robust fix to ensure exact multiset equality.
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
+116
to
+125
| export function isOutputCombinationSupported( | ||
| outputCombinations: readonly (readonly string[])[], | ||
| requested: readonly string[], | ||
| ): boolean { | ||
| if (!requested.some((modality) => modality !== 'text')) return true; // text-only is always emittable | ||
| return outputCombinations.some( | ||
| (combo) => | ||
| combo.length === requested.length && requested.every((modality) => combo.includes(modality)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The current implementation of isOutputCombinationSupported checks if requested.every((modality) => combo.includes(modality)) when the lengths are equal. However, if requested contains duplicate elements (e.g., ['image', 'image']) and combo contains unique elements of the same length (e.g., ['text', 'image']), this check will incorrectly return true because both 'image' elements are present in combo. To prevent this and ensure exact multiset equality, we should verify that the elements match in both directions.
Suggested change
| export function isOutputCombinationSupported( | |
| outputCombinations: readonly (readonly string[])[], | |
| requested: readonly string[], | |
| ): boolean { | |
| if (!requested.some((modality) => modality !== 'text')) return true; // text-only is always emittable | |
| return outputCombinations.some( | |
| (combo) => | |
| combo.length === requested.length && requested.every((modality) => combo.includes(modality)), | |
| ); | |
| } | |
| export function isOutputCombinationSupported( | |
| outputCombinations: readonly (readonly string[])[], | |
| requested: readonly string[], | |
| ): boolean { | |
| if (!requested.some((modality) => modality !== 'text')) return true; // text-only is always emittable | |
| return outputCombinations.some( | |
| (combo) => | |
| combo.length === requested.length && | |
| combo.every((modality) => requested.includes(modality)) && | |
| requested.every((modality) => combo.includes(modality)), | |
| ); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/llm/src/capabilities.test.ts (1)
98-114: ⚡ Quick winAdd a duplicate-modality regression case to lock exact-set semantics.
Current assertions cover subset and invalid combo cases, but not duplicated entries. Add a case like
['text','image','image']against[['text','image','audio']]and assertfalseto prevent recurrence.Suggested test addition
it('isOutputCombinationSupported is the ONE exact-membership predicate shared by both gates (1.AF H2)', () => { const gemini = [['text'], ['text', 'image'], ['text', 'audio']]; // image+audio never together @@ expect(isOutputCombinationSupported([], ['text'])).toBe(true); expect(isOutputCombinationSupported(gemini, ['text'])).toBe(true); + expect( + isOutputCombinationSupported([['text', 'image', 'audio']], ['text', 'image', 'image']), + ).toBe(false); });🤖 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.test.ts` around lines 98 - 114, The test case for isOutputCombinationSupported is missing a regression case for duplicate modalities. Add an expect statement that calls isOutputCombinationSupported with duplicate modalities in the combination (for example, ['text','image','image']) and assert the result is false. This should be added after the existing test cases in the same test block to ensure exact-set semantics are enforced and prevent duplicate modalities from being incorrectly accepted.
🤖 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/db/src/media-egress.test.ts`:
- Around line 344-356: The mock setup for httpsRequest and the variable
declarations for options and onResponse use unsafe double-casts with as unknown
as which violates strict TypeScript requirements. Replace these unsafe casts
throughout the test by creating typed helper functions or type guards that
safely validate and cast the mock return values and callback arguments. For the
httpsRequest mock, CapturedHttpsOptions extraction, and FakeIncoming callback
parameter, either implement type predicates that verify the shape of the objects
before casting to their final types, or create strongly-typed mock helper
functions that return properly typed objects without intermediate unknown
assertions.
In `@packages/llm/src/capabilities.ts`:
- Around line 121-124: The current check in the anonymous function passed to
outputCombinations.some() only verifies one-way inclusion (that all requested
modalities exist in combo), but doesn't verify that combo doesn't contain
modalities not in requested. This allows false positives when requested has
duplicate modalities or different sets of equal length. Fix this by adding a
bidirectional check: after verifying that requested.every((modality) =>
combo.includes(modality)), also verify that combo.every((modality) =>
requested.includes(modality)) to ensure both arrays contain exactly the same
modalities without duplicates or extras.
---
Nitpick comments:
In `@packages/llm/src/capabilities.test.ts`:
- Around line 98-114: The test case for isOutputCombinationSupported is missing
a regression case for duplicate modalities. Add an expect statement that calls
isOutputCombinationSupported with duplicate modalities in the combination (for
example, ['text','image','image']) and assert the result is false. This should
be added after the existing test cases in the same test block to ensure
exact-set semantics are enforced and prevent duplicate modalities from being
incorrectly accepted.
🪄 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: a05403f8-b4bd-4b2f-a5a0-be11795f2077
📒 Files selected for processing (15)
docs/reference/contracts/sse-event-schema.mddocs/reference/shared-core/built-in-tools.mddocs/reference/shared-core/tool-registry.mddocs/roadmap/deferred-tasks.mdpackages/core/src/tools/bounding.tspackages/core/src/tools/registry.test.tspackages/core/src/tools/registry.tspackages/core/src/validate-catalog.test.tspackages/core/src/validate-catalog.tspackages/db/src/media-egress.test.tspackages/db/src/media-reference-store.test.tspackages/db/src/media-reference-store.tspackages/llm/src/capabilities.test.tspackages/llm/src/capabilities.tspackages/llm/src/index.ts
Comment on lines
+344
to
+356
| vi.mocked(httpsRequest).mockReturnValue({ | ||
| on: vi.fn(), | ||
| end: vi.fn(), | ||
| } as unknown as ReturnType<typeof httpsRequest>); | ||
| const request: HopRequest = { | ||
| url: 'https://media.example.com:8443/a/b?c=d', | ||
| hostname: 'media.example.com', | ||
| pinnedIp: '203.0.113.10', | ||
| }; | ||
| const pending = nodeMediaEgressDeps.openConnection(request, new AbortController().signal); | ||
| const callArgs = vi.mocked(httpsRequest).mock.calls.at(-1); | ||
| const options = callArgs?.[0] as unknown as CapturedHttpsOptions; | ||
| const onResponse = callArgs?.[1] as unknown as (incoming: FakeIncoming) => void; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining unsafe assertion patterns in TS/TSX tests/source.
rg -n --type=ts --type=tsx '\bas\s+unknown\s+as\b|\bas\s+any\b'Repository: HodeTech/Relavium
Length of output: 89
🏁 Script executed:
# Read the specific lines in media-egress.test.ts to confirm the unsafe casts
cat -n packages/db/src/media-egress.test.ts | sed -n '340,390p'Repository: HodeTech/Relavium
Length of output: 2907
🏁 Script executed:
# Search for unsafe casts with correct ripgrep syntax (ts only, no tsx)
rg -n --type=ts '\bas\s+unknown\s+as\b|\bas\s+any\b' packages/dbRepository: HodeTech/Relavium
Length of output: 725
🏁 Script executed:
# Check tsconfig or package.json to verify strict TypeScript mode
find . -name 'tsconfig*.json' -o -name 'package.json' | head -5 | xargs grep -l '"strict"' 2>/dev/null || echo "Checking tsconfig files..."
cat tsconfig.json 2>/dev/null || cat packages/db/tsconfig.json 2>/dev/null | grep -A 5 '"strict"' || echo "tsconfig found but no strict setting visible"Repository: HodeTech/Relavium
Length of output: 133
🏁 Script executed:
# Get the full tsconfig.json content to verify strict mode
cat tsconfig.json | grep -A 20 '"compilerOptions"' | head -30Repository: HodeTech/Relavium
Length of output: 107
🏁 Script executed:
# Also check packages/db/tsconfig.json if it exists and overrides
test -f packages/db/tsconfig.json && echo "=== packages/db/tsconfig.json ===" && cat packages/db/tsconfig.json | grep -A 20 '"compilerOptions"' || echo "No packages/db/tsconfig.json found"Repository: HodeTech/Relavium
Length of output: 611
🏁 Script executed:
# Find all tsconfig files and check for strict mode settings
find . -name 'tsconfig*.json' -type f | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && cat {} | grep -i strict'Repository: HodeTech/Relavium
Length of output: 476
🏁 Script executed:
# Also check for biome.json or eslint configs that might enforce strict typing
ls -la | grep -E 'biome|eslint|turbo' && (test -f biome.json && echo "=== biome.json ===" && cat biome.json || true)Repository: HodeTech/Relavium
Length of output: 1599
🏁 Script executed:
# Check the build tsconfigs which typically have the compiler options
cat packages/db/tsconfig.build.json | head -40Repository: HodeTech/Relavium
Length of output: 321
🏁 Script executed:
# Check if there's a root-level tsconfig that other configs extend
find . -maxdepth 1 -name 'tsconfig*.json' -type f -exec cat {} \;Repository: HodeTech/Relavium
Length of output: 2046
Replace unsafe double-casts with typed mock helpers or guards.
Lines 347, 355–356, 376, 382–383 use as unknown as ..., which bypasses strict typing and can hide invalid mock/request callback shapes.
The root tsconfig enforces "strict": true with additional strictness flags. Per coding guidelines, "All source code must be written in TypeScript with strict mode enabled; no any types or unsafe as casts" and "Use type guards instead of unsafe type assertions."
Suggested direction
-vi.mocked(httpsRequest).mockReturnValue({
- on: vi.fn(),
- end: vi.fn(),
-} as unknown as ReturnType<typeof httpsRequest>);
+const makeClientRequestMock = (): Pick<ReturnType<typeof httpsRequest>, 'on' | 'end' | 'destroy'> => ({
+ on: vi.fn(),
+ end: vi.fn(),
+ destroy: vi.fn(),
+});
+vi.mocked(httpsRequest).mockReturnValue(
+ makeClientRequestMock() as ReturnType<typeof httpsRequest>,
+);
-const options = callArgs?.[0] as unknown as CapturedHttpsOptions;
-const onResponse = callArgs?.[1] as unknown as (incoming: FakeIncoming) => void;
+if (!callArgs) throw new Error('https.request was not called');
+const [rawOptions, rawOnResponse] = callArgs;
+if (typeof rawOnResponse !== 'function') throw new Error('missing response callback');
+const options: CapturedHttpsOptions = rawOptions as CapturedHttpsOptions; // can be removed entirely by typing mock implementation signature
+const onResponse = rawOnResponse as (incoming: FakeIncoming) => void;Also applies to: 373–383
🤖 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-egress.test.ts` around lines 344 - 356, The mock setup
for httpsRequest and the variable declarations for options and onResponse use
unsafe double-casts with as unknown as which violates strict TypeScript
requirements. Replace these unsafe casts throughout the test by creating typed
helper functions or type guards that safely validate and cast the mock return
values and callback arguments. For the httpsRequest mock, CapturedHttpsOptions
extraction, and FakeIncoming callback parameter, either implement type
predicates that verify the shape of the objects before casting to their final
types, or create strongly-typed mock helper functions that return properly typed
objects without intermediate unknown assertions.
Source: Coding guidelines
…ntained egress-test casts Three external (CodeRabbit) findings on the 1.AF review-follow-up PR #36, each verified against current code: - isOutputCombinationSupported did one-way inclusion: a DUPLICATE requested modality (e.g. ['image','image']) matched a clean equal-length combo (['text','image']) because forward inclusion + equal length passes. Add the reverse-inclusion check so equal length + both directions means exact set equality. No normal-case change (unique sets were already exact). +regression test. - media-egress.test: contain the node:https mock casts in two named helpers (stubClientRequest + lastHttpsCall, the latter runtime-guarding the (object, function) shape and excluding the string|URL overload members), removing the six scattered double-casts from the test bodies — down to one contained single cast (the mock callback's unknowable parameter type). Validation: lint/typecheck/test/build 16/16, prettier clean, Leakwatch 0. Refs: ADR-0031, ADR-0044 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
Follow-up fixes from a 13-dimension, double-verified end-to-end review of 1.AF (engine media plumbing), run after PR #35 merged. The review confirmed the merged 1.AF is sound (no exploitable SSRF bypass, the I3 handle-only invariant holds end-to-end, toolchain green) and surfaced 30 confirmed findings. This PR lands the two HIGH items, the behavioral mediums, and the doc-drift cluster; the 1.AH-latent host-wiring items stay recorded in
deferred-tasks.md.Changes
H1 (I3 leak) —
agent:tool_call.toolInputis redacted before it rides the event/IPC/log stream. A model can emit a base64data:URI or a{kind:'base64',data}object as a tool argument; the result side (outputSummary) was redacted this workstream but the symmetric input side was not. Reuses the sameredactInlineMediawalk atsanitizeInput— display-only (the dispatch already ran on the real args). Regression test added.H2 (load-check ↔ runtime divergence) — the load-time
output_modalitiescheck and the runtimeFallbackChainpre-skip now share one exact-membership predicate (isOutputCombinationSupported,@relavium/llm). The runtime did text-strip + subset, which wrongly admitted wire-invalid combinations (the closed set exists to reject) and diverged from the load-check + ADR-0031 decision #3. Text-only stays always-emittable (no regression for[]-combo models). Tests on both sides.GC grace (ADR-0042 §4) —
media_objects.last_referenced_atis now bumped onaddReferenceand the terminal sweep (removeRunReferences), so the grace window measures from drop-to-zero, not production time. A long-lived handle losing its last reference no longer gets zero effective grace. Two tests added.Egress mechanism test (E43-7) —
node:httpsis mocked to exercise the concretenodeMediaEgressDeps(the un-injectable IP-pin / SNI / port / family wiring) plus the default-deps range-block — the one part of the SSRF-critical path with no executable proof. Four tests added.Docs —
sse-event-schema(thecost:updated.mediaUnitsfield is deferred — the layering forbids@relavium/sharedimporting the@relavium/llmseam type),tool-registry(thirteen built-ins;media_scope_deniedadded to the deny union),built-in-tools(aread_mediarow); the keychain no-raw-key IPC test and themediaUnits-surface deferrals are recorded indeferred-tasks.md.Validation
pnpm turbo run lint typecheck test build— 16/16 greenpnpm run format:check— cleanleakwatch scan fs packages— 0 findingsDeferred to 1.AH (recorded, not lost)
read_mediahostMediaReadAccess+ scope population (inert today), thevalidateWorkflowWithCatalogloader wiring,media_cost_estimatehost threading,resolveForEgressfailover wiring, the keychain no-raw-key IPC test (no desktop IPC layer yet), and surfacingUsage.mediaUnitsoncost:updated(needsMediaUnitsEntryrelocated to@relavium/shared).Refs: ADR-0042, ADR-0043, ADR-0044
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
read_mediabuilt-in tool for reading gated media via durable handles with scope-based authorization.Documentation
cost:updatedcontract to fold realized media spend into cumulative reporting and clarified deferred media unit breakdown.Bug Fixes
Refactor
Tests