Repository navigation
fix: recover original saved captions and comments - #227
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Review: changes neededReviewed exact head The bounded, session-owned original-reader approach addresses the right problem. Ownership/deletion fences, immutable source identity, admission/billing and saved-only provider restrictions have meaningful coverage. However, the recovery path does not yet preserve the new work. 1. P2 — Restore delivered original pages when finalization resumesThe saved reader persists its page to Independent reproduction: a real Worker/DO integration test executes Impact: an interruption after successfully reading an original can leave resumed finalization without the quote/comment already read and charged for. Restore the admitted current-run original packets with their immutable references and existing billing marks; add a finalization-recovery regression. Do not fetch fresh data or charge again merely to recover them. 2. P2 — Persist the structured verification handoff
Independent two-path reproduction: identical current user text requests “Juniper,” exactly three numbered points, no extra prose, and fenced code. A mock finalizer returns one unnumbered “Cedar” block without fences. Normal execution records requirements, tries twice and rejects without persistence. Execution with Persist/recover the handoff before entering finalization, or reconstruct equivalent validated constraints on resume. Test resumed finalization, not only a timeout within the same invocation. 3. P2 — Regenerate the OpenAPI artifact and restore green checkscontracts.ts:27–28 and analogous route variants add Independently reproduced with Nonblocking P3 — Citation insertion can violate “only numbered points”Format validation precedes final citation rendering. A numbered block ending in a fenced code block with non-inline evidence gets a leading Verification
Verdict for this head: CHANGES NEEDED. Resolve recovery persistence and the failing checks, then independently re-review the updated head. No merge/deployment approval. |
Re-review correction: CHANGES NEEDED2026-10-27 15:13 UTC update for unchanged head P2 — Empty formatting constraints disable an otherwise available partial-answer fallback
Confirmed validator edge requiring repairverify-saved-evidence.ts:113–120 A valid three-block answer with a three-space-indented numbered sublist inside point 2 is rejected because nested markers are counted as additional top-level points. Independent unit reproduction passes, alongside controls for an ordinary three-point answer and Markdown Current verdict: CHANGES NEEDED. Do not rely on the earlier ready-to-merge guidance. This updates the existing review rather than creating a duplicate comment for the same head. Earlier 14:15 UTC review, retained for history; verdict superseded aboveRe-review: APPROVEReviewed exact head No material unresolved finding identified for this head. The three prior P2 findings are addressed:
The prior nonblocking P3 numbered-code citation case is also addressed: structured-answer.ts:274–285 keeps remaining citations in the numbered point, and the finalizer validates rendered blocks. Verification on this head
Limits: deterministic model fixtures and persisted-store reconstruction are not an actual process-kill test, production browser reload test or fresh external-inference replay. Author-reported earlier model replays are not my live verification. Live dashboard QA remains paused; this approves the code change, not broad live resolution of issue #1 or unrelated citation semantics/Memory correction issues. No product credits used by my local checks; ledger amounts are fixture data. Verdict for |
devhims
left a comment
devhims
left a comment
There was a problem hiding this comment.
Review notes on the saved-evidence verification path. The first three can make saved-verification runs fail or stall; the rest are smaller correctness and cleanup items.
| const failure = finalizationFailure(finalizationError, finalizationFailures); | ||
| // An unstructured evidence summary cannot satisfy a validated answer format. | ||
| // Keep the saved evidence reusable and fail clearly if the single repair failed. | ||
| if (verificationHandoff?.answerFormat) throw failure; |
There was a problem hiding this comment.
Partial-answer fallback never runs. answerFormat is required by savedVerificationHandoffSchema, so verificationHandoff?.answerFormat is truthy for every run that recorded requirements. Any saved-verification run whose finalizer fails twice now throws, even when the user set no name, count or code requirement and the admitted originals could have produced a partial answer.
Suggest gating on an actual constraint, e.g. answerFormat.name || answerFormat.numberedPoints !== null || answerFormat.fencedCode.
| ...(storyboardRetrievalBudget(options.researchDeadlineAt) < STORYBOARD_RETRIEVAL_MIN_MS ? ['get_video_storyboard'] : []), | ||
| ...(!canAnalyzeStoryboard(options.researchDeadlineAt) ? ['analyze_video_storyboard'] : []), | ||
| ...(needsVisualWork() ? [completionTool] : []), | ||
| ...(unreadOriginals().length ? [completionTool] : []), |
There was a problem hiding this comment.
Completion can stay locked for the whole run. unreadOriginals() only counts a source as read once an admitted packet with excerpts lands in evidence. If deliverEvidence withholds the packet (credit reserve exhausted), or every saved asset for the subject is empty (e.g. a comments page with zero comments), complete_research never becomes available. The model then burns reads until the step, tool-call or 120s limit forces finalization.
Consider treating a withheld or empty read of the asset as satisfying the requirement and passing it as a handoff gap instead.
| }); | ||
| if (format.name && !prose.join('\n').includes(format.name)) fail(`Include the current name ${JSON.stringify(format.name)} in the answer prose, inside a numbered point when only points are requested.`); | ||
| if (format.numberedPoints !== null) { | ||
| const numbers = [...prose.join('\n').matchAll(/^\s{0,3}(\d+)[.)]\s/gm)].map(match => Number(match[1])); |
There was a problem hiding this comment.
Nested numbered lines count as extra points. This regex matches every prose line starting with N. or N) across all blocks, so a short sub-list or a quoted line starting with a number inside a valid point fails validation (e.g. 3 points where point 2 contains 1. preheat / 2. mix gives [1,1,2,2,3]). Together with the rethrow at research-agent.ts:879, the run fails instead of returning a well-structured answer.
When onlyNumberedPoints is set, checking only the first line of each block (already done just below) seems sufficient; otherwise only count list markers at the start of each block.
| } | ||
| return chunks; | ||
| }); | ||
| const tokens = request.query?.toLowerCase().match(/[\p{L}\p{N}_]+/gu); |
There was a problem hiding this comment.
Tokenized query, substring match. Punctuation is stripped when splitting the query, but each token is then matched with includes against raw text. A search for don't becomes ["don", "t"]: t matches nearly every caption window and don matches London/done. That wastes the 16-read budget on huge result sets. Matching on word boundaries (tokenize the haystack the same way), or keeping intra-word apostrophes in tokens, would fix it.
| : comments.flatMap(({ comment, position }) => { | ||
| // Split long comments without discarding their tail or changing their words. | ||
| const text = comment.text; | ||
| const suffix = `\nAuthor: ${comment.author.name.slice(0, 200)}\nComment ID: ${comment.id.slice(0, 200)}\nSaved position: ${position.map(index => index + 1).join('.')}`; |
There was a problem hiding this comment.
Metadata labels are searchable. The search text includes Author:, Comment ID: and Saved position:, so query words like author, comment or position match every comment. Consider searching text plus the author name only, without the label words.
| characters += size; | ||
| } | ||
| const nextOffset = request.offset + page.length < matching.length ? request.offset + page.length : undefined; | ||
| const sourceMeta = (value as Transcript | CommentsPage).meta; |
There was a problem hiding this comment.
Assumes newer blob shapes. This assumes every saved blob has meta.warnings, and flattenComments assumes every comment has a replies array. The existing savedComments helper in session-evidence.ts is more tolerant. An older saved page missing either field throws a TypeError, and the model may then report the saved comments as unavailable, which is the failure this PR is fixing. Suggest meta?.warnings ?? [] and comment.replies ?? [].
| }; | ||
| const applyConstraints = (decision: Record<string, unknown>) => { | ||
| const fill = fillInto(decision); | ||
| if (constraints.savedOnly) { |
There was a problem hiding this comment.
savedOnly silently overrides a refresh request. In the last-resort merge, savedOnly from any candidate wins and deletes refreshEvidence/refreshDynamicData set by another candidate. A request like "get fresh comments and quote the saved transcript" can end up answered from stale comments with no note that the refresh was dropped. Conflicting candidates might be better resolved as a clarification, or at least surfaced as a warning.
| inputSchema: readSavedEvidenceInputSchema, | ||
| execute: async input => { | ||
| signal.throwIfAborted(); | ||
| if (++calls > SAVED_EVIDENCE_READ_LIMIT) throw new Error('Saved-evidence read limit reached. Answer from the passages already read and state remaining gaps.'); |
There was a problem hiding this comment.
Read cap is per tool instance. calls lives in the closure, so the 16-read limit resets whenever searchTools is rebuilt (recovery, a new phase), and the bound isn't enforced per run. It also counts calls that fail validation or hit a deleted asset. Consider persisting the count per run (or deriving it from delivered read_saved_evidence operations), and incrementing only after a successful read.
| { message: 'endMs must be at least startMs.', path: ['endMs'] }); | ||
|
|
||
| /** Reads only session-owned originals. There is deliberately no provider dependency. */ | ||
| export async function readSavedEvidence(store: SessionEvidenceStore, input: z.input<typeof readSavedEvidenceInputSchema>, signal?: AbortSignal) { |
There was a problem hiding this comment.
Overlaps the existing reader. This mostly repeats SessionEvidenceStore.readEvidence / readCommentEvidence (paging, query filtering, packet building), with different query rules (all-words vs phrase), different excerpt IDs (saved: vs evidence:) and different reply handling. Fixes to one path won't reach the other, and the same passage gets two citation IDs. Worth sharing a single excerpt builder for both.
| signal?.throwIfAborted(); | ||
| const request = readSavedEvidenceInputSchema.parse(input); | ||
| const generation = store.generation(); | ||
| const saved = await store.readAsset(request.assetVersion); |
There was a problem hiding this comment.
Repeated work per read. Each call does readAsset without a RunAssetCache, rebuilds the full excerpt array, and joins and lowercases a 3-segment window for every excerpt, up to 16 times per run. Passing the run's asset cache and precomputing lowercase segment text once per asset version would avoid it.
devhims
changed the title
Re-review: CHANGES NEEDEDReviewed exact head P2 — Gap-only answers fail the real persistence route contractresearch-agent.ts:940–945 switches a saved-original verification with no admitted excerpt IDs and a Independent reproduction: two real Worker/DO-boundary regressions persist an Impact: in this narrow no-admitted-evidence case, a user gets a failed answer instead of the honest empty/access-gap explanation the new branch intends. The earlier gate is unlocked, but completion still fails at persistence. Cases retaining admitted evidence are different and passed our tests. This is an incompletely repaired edge path, not evidence that every saved-source request fails or that production users encountered it. Make the gap-only output and persisted-route contract consistent without fabricating citations or silently enabling fresh retrieval. Add a regression through the real finalization boundary, not only Scoped improvements verified
Verification and limits
Verdict: CHANGES NEEDED for |
| add(packet); | ||
| continue; | ||
| } | ||
| if (packet.savedEvidence) { |
There was a problem hiding this comment.
Later original reads can be dropped whole. Originals that don't fit the finalizer budget are omitted in read order, and overlapping reads repeat the same captions. A topic_research saved-verification run without comparisonVideoIds gets the 40,000-character budget (research-agent.ts:937), while 16 reads can return up to ~192,000 characters. The first broad search can fill the budget, so the narrowed read that actually holds the quote is the one cut, and the answer may still say the quote is unavailable.
Suggest deduplicating excerpts by ID across saved-evidence packets before packing, and giving saved-verification runs the inspection budget.
| const result = await readSavedEvidence(store, input, signal); | ||
| signal.throwIfAborted(); | ||
| const packets = admit(result.packets, 'read_saved_evidence') ?? result.packets; | ||
| const outcome = result.coverage.totalExcerpts === 0 ? 'empty' : result.coverage.returnedExcerpts > 0 && packets.length < result.packets.length ? 'withheld' : undefined; |
There was a problem hiding this comment.
Repeating read errors never become terminal. Only empty and withheld are recorded. A read that throws every time (e.g. a saved transcript over the duration limit raises VideoTooLongError from readAsset, or the blob is unreadable) records nothing, so pendingOriginals keeps complete_research locked until all 16 reserved reads are used.
Suggest recording an unavailable outcome when readSavedEvidence throws for a known asset version.
| sql.exec(`CREATE TABLE IF NOT EXISTS session_memory_writes (id TEXT PRIMARY KEY, source_turn INTEGER NOT NULL)`); | ||
| sql.exec(`CREATE TABLE IF NOT EXISTS session_memory_state (id INTEGER PRIMARY KEY, version INTEGER NOT NULL)`); | ||
| sql.exec(`INSERT OR IGNORE INTO session_memory_state VALUES (1, 0)`); | ||
| sql.exec('CREATE TABLE IF NOT EXISTS session_original_reads (run_id TEXT PRIMARY KEY, attempts INTEGER NOT NULL)'); |
There was a problem hiding this comment.
session_original_reads is never cleared. Asset deletion removes outcomes, but nothing deletes these rows, and account deletion (agent-runtime-do.ts:1420) wipes session_run_generations and other run-keyed tables but not this one. Run IDs survive account deletion, and the table grows by one row per run.
Suggest adding it to the account-deletion table list and to delete() when called without a version.
| // Captions can split a phrase across consecutive segments. Comments never mix authors. | ||
| const text = asset.kind === 'comments' ? commentSearchTexts[index]! | ||
| : excerpts.slice(index, index + 3).map(item => item.text).join(' ').toLowerCase(); | ||
| const words = tokens ? new Set(searchWords(text)) : undefined; |
There was a problem hiding this comment.
Whole-word matching misses inflections. This fixes the apostrophe problem, but mix no longer matches mixing and lube no longer matches lubed. The skill tells the model to search one distinctive word from the request, so a request about "mixing greens" searched as mix returns nothing and costs extra reads.
Prefix matching for tokens of 3+ characters would keep the precision fix and recover common inflections.
| 'Prefer current assets over superseded versions unless the user asks for a historical comparison. A failed refresh does not make an old snapshot fresh; retain its collection time and explain the failure.', | ||
| 'Packets with savedEvidence contain selected original captions or comments, not analyst paraphrases. Use their exact text for requested quotations. Put supporting references only in each block\'s evidenceIds array; the application renders precise passage links from segment startMs. Do not put asset IDs, excerpt IDs, startMs/endMs fields, millisecond values or hand-written timestamp URLs into the prose. Preserve source identity. Do not claim timestamps or captions are absent when these packets supply them. A quote spanning several segments must cite those segments; do not guess corrected brand spellings from noisy captions. Coverage describes a selected page, not a complete transcript review.', | ||
| 'The current user message can correct earlier memory. Prefer explicit current corrections over old context.', | ||
| 'When continuing a named plan, use its latest user-specified name within the requested answer format. Preserve the latest formatting constraints from conversation history, including numbered-point counts and requests for no introduction or closing section.', |
There was a problem hiding this comment.
Named-plan guidance applies to every finalization. This line isn't gated, so every answer on every route gets instructions about plan names and point counts. SAVED_EVIDENCE_ANSWER_SKILL already covers this for saved-verification runs. Suggest gating it on usesSavedEvidenceSkill or moving it to #228 with the rest of the formatting work.
Changes needed —
|
Changes needed / not ready —
|
Move the opt-in live Fireworks replays out of this PR, since they also score naming and formatting (draft #228). Fold the deterministic Bicycle and JavaScript replays into session-evidence.integration.test.ts, merge the saved-evidence unit tests, and combine fixtures into two files. The vitest configs return to main.
Independent code review: approve / good to merge —
|
Problem / Motivation
The agent sometimes claimed retained quotations or comments were unavailable because it answered from shortened evidence. This affected the Watercolor, Bicycle, JavaScript and Pothos sessions.
What changed
Add
read_saved_evidenceand a focused verification skill for bounded reads of session-owned originals. Results preserve words, authors, timestamps and immutable citations. Saved-only requests cannot fetch fresh data.Research reads requested originals first. Overlapping reads share identical excerpts, preserving later quotations and source warnings. Saved verification receives a 250,000-character evidence budget. Admission and billing remain in place; recovery restores admitted passages without rereading or recharging.
Durable read limits survive retries and asset deletion. Finished-run bookkeeping is pruned, and account deletion clears it. Permanent read failures become explicit gaps; invalid requests and transient failures remain retryable. The production finalizer accepts an uncited gap response only when the same run's persisted state establishes terminal read outcomes or an exhausted read budget, with no admitted content. Other route substitutions remain rejected.
%%{init: {"themeVariables": {"sequenceNumberColor": "#ffffff", "actorBkg": "#eef2ff", "actorTextColor": "#111827", "signalColor": "#334155", "signalTextColor": "#334155"}}}%% sequenceDiagram autonumber participant Agent participant Store as Saved sources participant Answer alt Before Store-->>Agent: Shortened evidence Agent-->>Answer: May report originals unavailable else After Agent->>Store: Reserve bounded original read Store-->>Agent: Exact passages or explicit gap Agent->>Answer: Cited answer or verified access limitation endOriginal reads supplement existing source storage.
Formatting enforcement and numbered-point citation placement remain in draft #228. Prefix matching and reader consolidation remain deferred. Full replay fixtures preserve the original failure context.
Tests
Regression details and limitations. Presentation checks are diagnostic. Browser reload and Memory correction persistence remain unverified.