mirror of
https://github.com/THU-MAIC/OpenMAIC.git
synced 2026-10-17 05:29:46 +08:00
wyuc
1a151c4a49
* refactor(media): share one stored-bytes resolver across export paths The classroom ZIP, PPTX, and video exports each re-implemented the same pool-first byte resolution with a Dexie compatibility-row fallback and a task-URL fallback. resolveStoredBytes in lib/media/resolve-stored-bytes.ts now owns the chain, with the per-path differences (which levels run, fetch validation strictness, state-machine gating) as explicit options. The record-id to ref mapping moves alongside it as mediaRefFromRecordId. Intended as a behavior-preserving extraction: same bytes, same fallback order, same error handling. Cross-review found two places where this commit did not achieve that, both repaired later on this branch -- it moved a supplied row's `error` verdict past the pool await (restored by the snapshot fix) and dereferenced `record.blob` unguarded (guarded by the option-surface fix). See the PR description for the resulting divergence from main, which is stated rather than claimed away. Refs #1007 * fix(media): make the resolver's option surface mean what it documents Cross-review round 1 surfaced the same class of defect from both reviewers: options and docstrings that promise more than the code does. - `taskUrlFallback` was inert without `resolutionGating`. The task lookup was derived from the gating flag alone, so the final level resolved with no task and always returned null. The lookup now runs whenever either concern needs it, while the gated levels still read it only when gating is on -- so no current caller's behavior changes, and the pre-refactor callers' own unconditional lookup is restored. - `compatRowBytes` dereferenced `record.blob.size` unguarded, which broke the module's "never throws" contract and lost the guard the pre-refactor video path had (`blob && blob.size > 0`). A row that lost its blob now falls through to the next byte source. - `loadCompatRow` (requires `stageId`) and `compatRowCdnFallback` (needs a row) now state their dependencies instead of silently doing nothing. Six tests pin the behavior; each fix was mutation-checked (reverting the task decoupling reds one, reverting the blob guard reds two). * test(media): pin the resolver behavior the round-2 review found unpinned Round 2 produced no behavior findings from either reviewer; every finding was a test that would still pass with the behavior broken. Six cases close them. Mutation-verified -- reverting the code each covers reddens it: - Gating suppresses stale POOL and row bytes, not only the final level (removing either gate reds it). - With gating off the compatibility row is still accepted while a task is generating. This is the discriminator for keeping `gate` separate from `task`: leaking the task into the gated levels reds it. - A zero-length row blob falls through to the CDN source when enabled, and never reaches it when not (breaking `compatRowCdnFallback` in either direction reds one of the two). - The effective ref comes from a supplied row's compound id, not the ref the caller passed (dropping the derivation reds it). - The classroom ZIP still ships a non-OK pool response body -- the rebase conflict resolution that was correct but untested (tightening `requireOk` reds it). Not mutation-verified, and marked as such in the file: "accepts pool bytes with gating off" documents the pool level's unconditional branch but survives every single-edit revert of the task/gate separation, because that level branches on the option rather than on the gate. It is kept as documentation, not as a sentinel. * fix(media): snapshot a supplied row's error verdict before the pool await Round 4 of cross-review found a divergence three rounds of level-by-level comparison had missed. The pre-refactor video path computed `usableRecord = record && !record.error` BEFORE awaiting the pool; the extraction moved that test after the await. The supplied row is a live object, so an `error` set (or cleared) while the pool lookup was pending changed which bytes the export shipped. The verdict is now taken where the caller took it, and -- the part the first attempt at this fix got wrong -- it is not re-derived afterwards: snapshotting only the reference still re-read `error` off the live object at the later check. A row this function loads itself is still judged when it is read, after the pool miss, because that read did not exist any earlier. This was the last value read at a different point in the SEQUENCE than the code it replaced. What remains is scheduling, not sequence: the row's `blob` and `ossKey` are still read after the pool as they were on main, only one microtask later. The PR description states that, and no writer in the repo mutates a row in place across that window. Two tests pin both directions (an error landing mid-lookup, and one clearing); judging the supplied row after the await reds both. * test(media): commit the frozen-base differential harness (F-1116-001) The PR description claimed a differential comparison against the verbatim pre-refactor implementations, but that comparison was run out of tree and never committed, so no reviewer could re-run it. A claim that cannot be reproduced is not evidence. The three implementations are now reproduced verbatim as they stand at the merge base -- still identical to main, since nothing between the two touched them -- and driven side by side with the current helper under each call site's real option set, over a declared matrix of ref shapes, task states, pool outcomes, fetch outcomes, record shapes, and stage scoping. Every combination compares the outcome (bytes, null, or the thrown type), blob identity by reference, and the ordered lease / Dexie / fetch sequence including arguments. Absolute microtask offsets are not compared, for the reason the description gives: the extraction moves later operations one or two ticks while leaving the sequence, the arguments, and the results alone. The blob-less PPTX row is the only permitted divergence, and the harness pins where it may occur rather than merely tolerating it: every divergent combination must be that row with a stage, and the count must equal the combinations that actually reach the row -- all of them except the ones where the frozen path returns pooled bytes first. The harness bites: dropping the zero-byte rejection reds ZIP and video, dropping the response.ok check reds PPTX and video, and dropping the compound-id ref derivation reds video. Its scope is documented in the file -- option combinations no call site uses, and state that mutates mid-await, are covered by the targeted tests instead. * test(media): extend equivalence harness for colon refs and PPTX divergence (F-1116-001)