Skip to content

fix(sync): offload tool-result images at ingest - #1729

Open
rodboev wants to merge 14 commits into
kenn-io:mainfrom
rodboev:pr/1718-3-offload-on-ingest
Open

fix(sync): offload tool-result images at ingest#1729
rodboev wants to merge 14 commits into
kenn-io:mainfrom
rodboev:pr/1718-3-offload-on-ingest

Conversation

@rodboev

@rodboev rodboev commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

When you set tool_result_images = "offload", AgentsView moves supported tool-result images into its asset store during ingestion and keeps readable references in the archive. New sessions retain image access without storing image bytes inside tool-result text.

The setting applies to new and resynced sessions, incremental updates, and late tool results. AgentsView publishes each asset before saving its reference, and keeps the inline image if publication fails. The Settings control accepts offload, and existing inline images can be moved with agentsview db migrate --images.

Refs #1718

@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (678908f)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/db/artifact_import_session.go:75-78: ApplyArtifactImportedSession sanitizes and stores imported messages without calling the policy projection used by normal batch paths. With tool_result_images set to offload, inline input_image blocks from imported artifact segments remain in SQLite and no asset is created, making storage behavior depend on the ingestion route. Apply projectSessionBatchMessages or an equivalent policy projection before sessionAndMessagesForStorage, and add an artifact-import test asserting that offload produces an asset reference and asset file.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 8s | Total: 22m52s

@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (7f9dac8)

No issues found.


Reviewers: 2 done | Synthesis: codex | Total: 18m22s

@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (aa95b56)

Verdict: Changes require fixes for 2 findings.

Medium

  • internal/sync/engine.go:17016; internal/sync/parsediff.go:731: ParseDiff now invokes the offload projection, which writes asset files via assets.Put even though ParseDiff is report-only; re-parsing can mutate the asset store and leave unreferenced objects. Use a side-effect-free projection for ParseDiff or disable offload while comparing, and verify that the assets directory remains unchanged.

    Reported by: codex

  • internal/db/tool_result_images.go:393-406: Drop mode only strips input_image blocks. Existing agentsview_image placeholders created by offload retain their image_ref and linked markdown, so resync or imported sessions previously offloaded can continue rendering the asset after switching to drop. Extend drop projection to recognize offload placeholders, remove image_ref, and rewrite linked markdown to the normal non-asset placeholder.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 9s | Total: 33m48s

@roborev-ci

roborev-ci Bot commented Sep 11, 2026

Copy link
Copy Markdown

roborev: Combined Review (0bfbce0)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/db/tool_result_images.go:369; frontend/src/lib/utils/markdown.ts:815: The new asset:// placeholders resolve to relative /api/v1/assets/... URLs, but image requests cannot include the bearer Authorization header and the resolver ignores configured remote server URLs, causing 401 errors or requests to the wrong host. Load assets through an authenticated fetch/blob URL or provide a narrowly scoped authenticated asset URL mechanism, resolve asset URLs against the configured remote API base, and add an authenticated remote-rendering test.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 7s | Total: 13m42s

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (0cac7b7)

Verdict: Changes require fixes for 1 finding.

High

  • frontend/src/lib/utils/markdown.ts:843; internal/server/server.go:906; desktop/src-tauri/tauri.conf.json:25: loadAssetImages replaces fetched assets with blob: URLs, but the server and Tauri CSP img-src directives omit blob:, causing browsers to block the images. Add blob: to the relevant img-src directives in the server and desktop CSP configurations, and update CSP coverage accordingly.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 7s | Total: 8m46s

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (ad9695c)

Verdict: Changes require fixes for 1 finding.

Medium

  • internal/sync/engine.go:17029: Image projection writes asset files during session preparation before cwd filtering, preservation vetoes, and validation, so skipped or unstored sessions can leave unreferenced assets outside the configured archive scope. Keep preparation side-effect-free and publish assets only after all session acceptance gates pass, or use a no-write projection until the final database write boundary.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 9s | Total: 20m23s

@wesm

wesm commented Sep 12, 2026

Copy link
Copy Markdown
Member

Kind of makes me think that this should be the default for new archives

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (9e37ec2)

Verdict: Changes require fixes for 3 findings.

Medium

  • internal/db/artifact_import_session.go:76: Artifact imports preserve asset:// references but artifact transport does not include the corresponding asset bytes, so imported sessions on another archive render broken images unless the asset directory is copied separately. Include referenced asset objects in artifact transport, or downgrade unresolved references to readable placeholders during import and document the behavior.

    Reported by: codex

  • internal/sync/secret_scan.go:163: Signal and secret computation uses a no-write offload projection, while storage falls back to inline content when an asset write fails. Derived values and secret match offsets can therefore refer to different content than the stored transcript. Compute derived state from the actual storage projection, including its write-failure fallback, or defer computation until the committed content is known.

    Reported by: codex

  • internal/sync/codex_staging.go:553: Large Codex transcript staging writes image bytes to the persistent asset directory before CWD filtering, validation, and publication acceptance, leaving assets behind for sessions that are excluded, rejected, or fail later publication. Keep staged parsing side-effect free by retaining inline content in the private staging store, then call assets.Put only after acceptance. Alternatively, reliably remove every asset created by a staged parse that is not committed.

    Reported by: codex (security)


Reviewers: 2 done | Synthesis: codex, 10s | Total: 18m21s

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (76558a2)

Verdict: Changes require fixes for 1 finding.

High

  • frontend/src/lib/utils/markdown.ts:824-825: getAssetURL URL-encodes the entire asset:// reference, producing an invalid asset request path that the asset route cannot resolve. Strip the asset:// prefix before encoding and append only the filename.

    Reported by: codex


Reviewers: 2 done | Synthesis: codex, 6s | Total: 10m50s

@roborev-ci

roborev-ci Bot commented Sep 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (5e1f3b6)

Verdict: No findings at or above medium severity.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 15m40s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants