Repository navigation
fix: preserve cited evidence lineage and cite exact captions, frames and comments - #211
Conversation
… delivered context
… pages and record delivered operations
… context, remap shared visual references
…ges in model projection
|
@kiro-agent is attempting to deploy a commit to the devhims' projects Team on Vercel. A member of the Team first needs to authorize it. |
Independent review —
|
Revised independent review — e1139b1Verdict: CHANGES NEEDED. My earlier source-code approval for this head is withdrawn. Follow-up independent probes of the maintainer's feedback reproduce three material comment-search issues that the earlier suite did not catch. The Vercel authorization blocker also remains separate. P2 — Duplicate comment representations consume the search limitPacket indexing remains alongside whole-page indexing. Search limits rows before row-ID deduplication, but a packet excerpt and its saved-page equivalent have different IDs. Correction — Legacy packet truncation is not a new eight-entry regressionKiro identified a baseline-probe dependency error: the extracted base projection helper imported the head's collection-kind helper. On the real base, youtube_comments is excluded from collection kinds and already uses the generic eight-excerpt cap. The initial twelve-to-eight regression claim is withdrawn. Legacy packets can still lack a convenient continuation in model context, but raw entries remain saved and search/version-pinned reads can recover them without mandatory upstream refetch. Treat any improvement here as a P3 compatibility/discoverability follow-up, not newly introduced data loss. P2 — Matching passage can omit the actual search matchcommentPassage uses lowercase substring matching while SQLite FTS uses whole tokens with accent folding. Real SQLite FTS5 fixtures reproduced:
P2 — Per-hit serial full-page hydrationSearch loop awaits readSavedComment per hit. Each call rebuilds the asset inventory and reads/parses the full page; the pending-read map does not cache completed sequential reads. P3 — Filtered saved-read continuation describes the wrong offset domainFiltered read paginates matches, while coverage text describes provider-page positions and same-continuation paging. With 24 entries matching on even indices, the first eight matches return nextOffset=8. Following that through get_video_comments without pageVersion rejects; with the saved version it returns raw indices 8–15 rather than remaining matches 16,18,20,22. Calling the same saved-read method with the same query works. Nonblocking cleanup
New verification and limitsIndependent temporary probes ran against production-source methods/helpers; search probes used real SQLite FTS5 with mocked SDK/schema/storage transport, and paging probes used runtime Cloudflare shims with stubbed persistence/search lookup. All reproduction assertions passed. The initial legacy baseline comparison mixed dependency versions and was invalid; source inspection of the base helper AND its collection-kind dependency corrects that claim above. No new full-suite run, production Durable Object run, live model/provider test, implementation, push, merge or deploy. Working tree stayed clean. Earlier passing suite counts below remain valid historical results, but do not establish absence of these defects. Kiro agrees on the three P2 search findings and the latent P3 filtered-read mismatch, and correctly challenged the legacy regression claim. Overall verdict remains changes needed. Kiro's source assessment is separate from our executed probes. Earlier validation record (retained for provenance; revised verdict above supersedes its conclusions)Independent combined-head review — e1139b1Verdict: CHANGES NEEDED for merge readiness: resolve the outstanding Vercel authorization check, or have the maintainer explicitly confirm that this preview check is not required. No source-code changes are requested by this review. Independent code review and Kiro's source review both find the combined changes sound, but I am withholding final merge approval while the check remains unresolved. Keep the PR draft for now. Reviewed against merged PR212 base cade5df, including the previous 559e5dc changes, full current diff, surrounding code, prior review and all four conflict resolutions. Combined behavior verified
Fresh independent checks in my own terminal
Outstanding external checkVercel reports failure because this commit needs author deployment authorization. This is not a reproduced build/code defect. Please resolve it through the PR's Vercel check, or explicitly decide that preview deployment is not required. No authorization, merge or deployment was performed by this review. Verification limitsNo fresh live provider/model/dashboard test or production deployment verification. Kiro's review was source-only; executed counts above come from independent local checks. Full root production build, local browser E2E and Docker image build were not independently run; GitHub CI is separately reported. Skipped tests remain unverified. Structural provenance checks do not establish semantic truth of every generated claim. Previously noted nonblocking metadata warning/non-transcript-reservation and unexecuted storyboard-cancellation caveats remain; automatic visual reanalysis/dependent-memory correction remain outside scope. |
…e saved pages per search
Revised review — 86d6748 (October 19, 2026)Verdict: CHANGES NEEDED. Earlier source-code approval for this head is withdrawn. Three follow-up findings are independently reproduced. Historical passing tests below remain valid, but did not cover these cases. P2 — Duplicate suppression performs a correlated full packet scan
P2 — Unrelated deletions discard evidence-search results
Nuance: the lower-level readStored generation fence predates the cache, and deletion during awaited hydration also failed on e1139b1. The additional stale-cache failure after a successful fresh reload is new. Refresh/invalidate the per-search cache and safely retry unchanged versions or skip deleted ones; preserve deletion, final availability and resurrection fences. Removing one generation comparison alone does not fix every race. Controls verify deleted pages/full clears remain withheld. Existing tests cover deletion after a completed search, not these interleavings. P2 — Redundant persisted comment-search snapshots amplify storage and scans
These rows amplify the new correlated scan above. Separately, PR213's current inspectionRecords path calls evidence() before filtering visual packets, loading/parsing the whole packet table; that downstream interaction is being reviewed separately. Verification and limitsTwo local workerd production-method tests passed for SQL rowsRead/query plans and packet growth. Deletion probes used production store/search methods, real SQLite FTS, production schemas and the pinned SDK wrapper with controlled R2-adapter interleaving; same-page deletion and clear controls passed. A separate deletion workerd harness could not start due to Miniflare configuration incompatibility; no live DO scheduling, production bill or live Agent failure was measured. No implementation, push, merge, deployment or full-suite rerun. Feedback relayed to Kiro for source/design reconciliation. Historical validation record (the revised verdict above supersedes approval/readiness conclusions)Independent re-review — 86d6748Verdict: CHANGES NEEDED for merge readiness only: Vercel requires author deployment authorization. Verify CI has passed. The three reproduced P2 code findings are fixed and independently retested. Kiro and reviewer agree on the final code; no further code changes are requested by this review. Keep draft until outstanding checks are resolved or the maintainer explicitly determines the Vercel preview is not required. Verified the pushed SHA by PR API and Git fetch. Its tree Findings retested
Final independent validation
Outstanding checks and limitsVerify CI 39134675231 succeeded: documentation, library/skill bundle, web, both dashboard E2E groups, platform checks/integration and auth E2E all passed. Vercel reports author deployment authorization failure, not a reproduced code/build defect. No settings or credentials changed, no merge or deploy. No fresh live provider/model/dashboard or production performance test. The measured storage-call reduction is a local instrumented result, not a production latency guarantee. Full root web build/browser E2E/Docker build were not independently run. The pre-existing saved-read provider-continuation disclosure caveat is outside these changes. PR213 is stacked on the earlier PR211 base and will need its actual resulting base/head checked when retargeted. |
… reuse saved search rows The duplicate filter compared an expression over packet_id, which forced a full scan of session_packets for every match. It now compares the bare key. A comment search no longer fails when a different asset or memory is deleted mid-search; only a deleted page's own hits are skipped. A search hit's saved packet is named after its passage, so repeated searches replace one row instead of adding another.
|
Verdict: CHANGES NEEDED for 9ba6ba8. P2 — An unrelated deletion during comment-page hydration still silently drops the current search hit. Independent tests using actual local workerd, SQLite, R2 and the production SessionEvidenceStore reproduced:
The new repository test deletes before hydration starts; it does not cover this awaited-read interleaving. Retry the affected hit when the same immutable version remains valid, or explicitly surface incomplete results, preserving same-page deletion/full-clear/resurrection fences. Do not simply remove the lower-level generation check. Same-page deletion and full-clear controls pass; cached-page reuse after unrelated deletion also passes. This is an incomplete fix of the earlier invalidation finding, now exposed as silent omission rather than an error for the whole search. No live user answer was induced. Prior findings resolved in this head:
Separate merge-readiness blockers:
Verification: 2,021 platform unit tests passed / 55 skipped; 69 evidence Workers integration tests passed; 59 focused fidelity/lineage/comment tests passed (overlapping rerun, not additional unique total). Independent deletion probes: 2 all-hits assertions failed, 3 safety/reuse controls passed. Two additional passage/legacy-identity probes passed. Final TypeScript and full-PR whitespace checks passed. A temporary probe-only type error was corrected before the final passing typecheck; production files were not edited. Limits: no fresh live provider/model/dashboard tests, no full root/auth rerun, no integrated-main test. Kiro consultation for this head is currently blocked on sign-in; no new writer/reviewer agreement is claimed. No implementation, push, merge or deployment performed. |
A deletion elsewhere in the session fences an in-flight read, which returns nothing even though the page still exists. That hit is now read again, up to three times, instead of being dropped from the results. If the session keeps changing, the search fails with a retry message rather than returning an empty result. Deleting the page itself still returns nothing for it.
Conflicts: main moved frame and storyboard packet construction into shared analysis helpers that retrieval-time analysis also uses. The single-frame provenance from this branch now lives in those helpers. Both sides' new tests are kept; one test follows main's delete() return value.
Since retrieval tools can analyze content while saving it, a get_video_frames, get_video_storyboard or get_video_transcript packet can hold analyst findings. Its delivery label now follows the analysis artifact it carries, so the finalizer no longer calls those observations plain retrieval data.
Independent re-review — 48c27b8Verdict: CHANGES NEEDED for merge readiness only. No material source-code findings remain from this review. Resolve the Vercel author-authorization check, or have the maintainer explicitly determine it is not required. Keep draft pending writer/reviewer alignment; current Kiro consultation is blocked on sign-in. Previous P2 fixed
Integration and intended behavior
Independent checks
Remaining limitsAt 12:25 UTC on October 20, Vercel still reports |
3 participants
Problem / Motivation
Follow-up answers can lose cited evidence after a reuse-only turn. Comments can disappear between retrieval and model context. Captions and visual observations can cite a broader window or the wrong frame, and answers can overstate what they inspected.
What changed
Each turn preserves the evidence it received and cited, with exact matching and collision rejection. Received video metadata carries forward too.
Comment reads return eight entries at a time, pinned to a saved page version. Search covers complete comment bodies and authors. Shortened passages retain attribution and disclose omitted content. Search excludes duplicate packet representations before the 20-result limit, uses SQLite FTS match spans for snippets, and reuses each parsed comment page within a search while checking deletion/version validity. Filtered saved reads distinguish match offsets from provider-page offsets.
Transcript findings use exact caption citations when their explicit support is unique, with window fallback for ambiguous passages. Visual findings belong to one frame; shared observations remain separate. Older ambiguous visual evidence receives a warning.
The finalizer receives durable records of this turn's retrieval, analysis, saved reads and inherited evidence. Publisher context and metadata budget protections remain intact alongside these citation rules.
%%{init: {"themeVariables": {"sequenceNumberColor": "#ffffff", "signalColor": "#475569", "signalTextColor": "#0f172a", "actorBkg": "#f1f5f9", "actorTextColor": "#0f172a", "noteBkgColor": "#f1f5f9", "noteTextColor": "#0f172a"}}}%% sequenceDiagram autonumber participant Turn participant Store as Evidence store participant History rect rgb(254,226,226) Note over Turn,History: Before Turn->>History: Retain only newly created evidence end rect rgb(220,252,231) Note over Turn,History: After Turn->>Store: Record created and delivered evidence Store->>History: Retain exact cited evidence and received metadata endThe same storage now retains cited evidence across reuse-only turns.
Tests
Independent final-tree validation: library build, platform TypeScript and whitespace checks passed. Repository tests: 2,620 passed, 58 skipped. Separate auth/payment integration: 170 passed. Total: 2,790 passed, 58 skipped; suites ran sequentially. Additional independent probes covered deduplication, real SQLite token/accent matching, Unicode/control-character decoys, excerpt cutoffs, page caching, deletion/version safety, filtered paging and visual-provenance equivalence.
Regression coverage includes evidence lineage, comment paging, caption support, frame ownership, publisher context, metadata admission and answer-context budgets. Live provider/model and dashboard behavior have not been retested.
Compatibility
No additional model calls or budget increases. Per-run operation records initialize automatically in Durable Object storage. No D1 migration is required.