Repository navigation
Analyze retrieved video evidence while saving it - #217
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Verdict: approve 7ecc923. No blocking regression found in the reviewed changes. Checked cache snapshot immutability/LRU limits, per-store and per-run isolation, deletion-generation invalidation, late work after cancellation, current asset metadata/duration limits, explicit refresh, persistence-before-seeding and the unchanged billing path. Independent verification at this head:
The cache is correctly an optimization over existing verified storage reads, not a source of ownership or billing authority. Hits still check current session metadata; a generation change invalidates retained entries. The wrapper closes the cache on completion/failure/cancellation, and late writes cannot reopen it. Verification limits: no production latency or heap measurements, no live provider calls. The 16 MiB limit bounds retained serialized snapshots rather than total Worker memory or transient JSON/model allocations. Cache hits intentionally trust earlier same-run shared-source verification. The preexisting post-analysis packet-hash delete/restore race documented on PR216 remains outside these changes. Local auth-integration execution did not produce a completed result; the corresponding GitHub integration/auth checks passed. No implementation changes, pushes, merge or deployment performed. |
|
Verdict: changes needed for 392a2fe. This supersedes my earlier approval. Additional review feedback exposed cases the green suites missed. Merge blockers:
Additional confirmed regression:
Other feedback, qualified:
Kiro independently reached changes-needed for this exact head and agrees on the retry and scheduling blockers plus warning loss. Its repository suite was blocked by package-registry policy; it used code review and focused limiter/helper probes, not a full pass. The full suites below were run locally in this review. Verification: 2,021 unit tests passed / 59 skipped (three opt-in Fireworks tests not run); 367 session/runtime Workers integration tests, 11 catalog tests, 170 auth integration tests, 49 tests in each container suite passed. TypeScript/docs/whitespace checks and GitHub Verify 39386775043/Vercel passed. Six original safety probes passed, followed by four targeted characterization probes confirming the retry, warning, scheduler and empty-frame findings/qualifications above. No production changes, push, merge or deployment. Production latency/memory benefits remain unmeasured. |
|
Verdict: approve 2fac745. The three previously confirmed regressions are resolved; this supersedes the changes-needed verdict for 392a2fe, not its historical findings. Rechecked fixes:
Independent verification at this head:
No new blocking correctness or security finding. Non-blocking performance work remains: previews still start after inference, image identity checks serialize/hash twice, and some redundant validation/deadline work remains. These are not measured production latency regressions. A saved-analysis retry intentionally has no new preview payload; the original retrieval retains its previews. Verification limits: no live inference rerun or production latency/heap measurement in this review, and no end-to-end production validation of early callbacks across the real deployed Durable Object boundary. The preexisting post-analysis packet-hash delete/restore race noted in the earlier cache review remains outside this change. Only reviewer test probes were added locally; no production edits, pushes, merge or deployment. |
|
Verdict: approve a8d6c89. Reviewed the new credit-admission and preview-overlap commit against approved 2fac745; no new blocking correctness, billing or security finding. Verified behavior:
Independent checks at this exact head:
The prior retry, scheduler and storyboard-warning fixes remain covered and passing. No failed completed check. Limits: provider work was mocked in adversarial tests; no live inference rerun or deployed Durable Object end-to-end test. Production wait time, latency and heap benefits remain unmeasured. Full-image hashing and preexisting orphan-preview cleanup/race limitations are not solved by this commit. No production edits, pushes, merge or deployment performed. |
|
Verdict: approve 3b40ce0, with the non-blocking cleanup limitation below explicitly retained. Reviewed this delta against approved a8d6c89; no new merge-blocking regression found. This is not verification of an unconditional durable-cleanup guarantee. What is verified:
P3 / non-blocking existing cleanup gap: runtime/preview-lifecycle.ts:17 records descriptors only when the saver fulfills. In frame-previews.ts:80-82, if cancellation or a partial PUT failure triggers rollback and that rollback DELETE also fails, the saver rejects without returning its attempted keys. The wrapper therefore sees no created previews and never reaches discardPreviews at :35-36. Two failure-injection tests using the real saver reproduced one orphaned private object and zero queued cleanup keys: (1) cancel after successful PUT, then fail DELETE; (2) first PUT succeeds, second PUT fails, then fail DELETE. Restart cleanup cannot retry keys that were never queued. This is an incomplete hardening of the preexisting orphan-preview limitation, not a new leak introduced by this commit. The reproduced impact is unreferenced private storage, not demonstrated public exposure or loss of published previews. Recommend narrowing the PR/reference documentation's blanket failed-deletion claim, or following up with durable tracking of attempted keys before the saver can lose ownership. A process restart before finally runs remains another unverified interruption boundary. Do not describe all orphan paths as fixed. Verification at this head:
Limits: no live inference rerun, deployed end-to-end test, production heap/latency measurement or crash-injection test. No production edits, pushes, merge or deployment performed. |
|
Verdict: approve 31e989f. Reviewed the new progress/activity/outcome and dashboard changes against approved 3b40ce0. No blocking regression found. Verified:
Non-blocking display qualifications: a recovered analysis is a new successful analysis row; the original failed attempt remains accurately marked failed. Analysis-budget rejection can still show the generic Loading Failed label. A resumed unfinished call can retain its last recorded phase until new activity is emitted. These do not affect evidence or billing. Independent local checks passed: 2,055 platform unit tests (59 skipped), 381 Workers session/runtime/billing tests, 11 catalog, 170 auth integration, 49 processor and 49 frame container tests, and all 115 web tests. Platform TypeScript, generated documentation freshness and whitespace checks passed. Local dashboard build/browser verification was blocked by the execution environment: Next.js networkInterfaces failed with uv_interface_addresses; the allowed outside-sandbox retry failed before launch. No local visual/Playwright pass is claimed. Exact-commit GitHub Verify 39401265933 completed successfully, including Web build, both Dashboard E2E groups and Auth E2E. Thus applicable CI checks are green despite that local limitation. The previously documented orphan-preview rollback limitation is untouched, not fixed by this UI commit. I did not independently repeat the author's production smoke tests, run live inference, or measure production performance. Live phase labels require both backend and dashboard deployment. No production edits, pushes, merge or deployment performed. |
Problem / Motivation
Answering questions about newly extracted video content waits for saving, another model decision, and a storage download before analysis begins. The dashboard also calls combined retrieval and analysis “Loading Transcript” throughout.
What changed
Transcript, frame and storyboard tools accept an optional
analysisquestion. They analyze available bytes while saving continues, then return findings after both succeed. Transcript research uses this path for missing inputs. Retrieval-only calls remain available.%%{init: {"themeVariables": {"actorBkg": "#e2e8f0", "actorTextColor": "#0f172a", "signalColor": "#64748b", "signalTextColor": "#64748b", "sequenceNumberColor": "#ffffff"}}}%% sequenceDiagram autonumber participant App participant Storage as D1 and R2 participant Analyst Note over App: Extraction has returned the requested bytes alt Before App->>Storage: Save inputs Storage-->>App: Saved Note over App: Research model chooses analysis (removed) App->>Storage: Download inputs again (removed) Storage-->>App: Input bytes App->>Analyst: Analyze else After par Persistence retained App->>Storage: Save inputs Storage-->>App: Saved references and Analysis starts immediately App->>Analyst: Analyze retained bytes Analyst-->>App: Findings end Note over App: Return findings only after saving succeeds endAnalysis overlaps saving without another model decision or download.
Dashboard labels now follow actual activity: loading, analyzing, finishing, then analyzed. Analysis failures show a warning instead of a green check. Saved results also receive accurate completion labels.
Preview saving overlaps unfinished analysis. Rejected calls remove unpublished previews, with failed deletions queued for retry. Failed analysis preserves saved inputs for retries without another extraction or input charge. Saved-analysis warnings stay within the 50-warning limit.
Retrieval and inference have separate concurrency limits. Required tools wait for temporary credit holds and recheck quotas. Deletion, cancellation, failed saves and changed content prevent publication.
Progress responses gain optional activity and analysis outcome fields. No migration is required. The live labels require both backend and dashboard deployment.
Tests
3b40ce0passed for all three content types and confirmed overlap. The new UI changes are not deployed. Overall speedup remains unmeasured.