Repository navigation
fix(skills): reject occlusion timeout before verdict - #5491
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 610c7dee. The change is correct and I found no blocker. I'm commenting instead of approving only because CI hasn't run at this head: every workflow is waiting on maintainer approval (action_required), so none of the required checks have results yet. I'll approve once they're green.
The fix. When the watchdog fires with no verdict line ([v2] … word-fail), it now sets OCC_RC=3 and breaks out. Before this, it grepped for cap(s) FAIL, didn't find one, and set OCC_RC=0, so a stuck analysis was recorded as occlusion+overflow PASS. The kill after a verdict is printed (the sharp teardown hang) is unchanged.
rc=3 goes through the existing abort.
OCC_RCis already set, so thewaitline is skipped and the killed child's exit code doesn't overwrite it.(( OCC_RC != 0 ))prints the ABORTED lines and exits 2.- With
OCCLUSION_SKIP=1, the run continues and writesocclusion+overflow OVERRIDDEN, never PASS.
The tests run in CI. Test: skills (ubuntu, gated on changes.outputs.skills) collects them with find skills … -name "*.test.mjs", so skills/embedded-captions/scripts/render-and-composite.test.mjs is picked up, and this PR touches skills/, so the job runs.
The harness is sound.
- It slices the real gate block from the script, so the tests can't drift from it.
$(dirname "$0")resolves to the temp root, where the fakecheck-occlusion.cjslives. - The
sleepfunction overrides the externalsleepcommand. For the hanging cases it waits for the fake analyser's ready file, which is written after its output. That ordering keeps the verdict grep from racing. - It then does
SECONDS=$((SECONDS + 10)). Assigning toSECONDSresets bash's counter, so the gate's clock moves 10 s per loop: the 150 s timeout fires on the 16th pass, and the 8 s after-verdict kill one pass after the verdict is seen. - The cases that exit normally use a real 10 ms sleep and leave the clock alone.
- The trap kills the fake analyser on exit, and
spawnSynchas a 10 s timeout as a backstop.
Locally: 8/8 pass in 0.36 s. Against main's script, the three timeout cases fail and the five verdict and exit cases pass. gen-skills-manifest.ts --check says the manifest is in sync: the embedded-captions hash changed and the file count went from 144 to 145.
Nit. On rc=3 the abort still says "fix plan.json layout to reduce subject occlusion / frame-edge overflow". That's misleading for a timeout, where the layout may be fine. The rc=3 line above it explains it, but a timeout-specific hint ("re-run; if it keeps timing out, check measure-layout/Chromium") would point the reader the right way.
#5496 makes the same 5-line shell change without the manifest regeneration or the tests, so this PR supersedes it.
— Rames
What
Abort the embedded-captions occlusion/overflow gate when its analysis watchdog expires before a verdict is printed. An incomplete analysis no longer records
occlusion+overflow PASSor allows rendering to continue without an explicit override.Why
A hanging analyzer produces no
cap(s) FAILline. The watchdog treated that absence as success even when the analyzer had never reached its verdict, silently bypassing the layout gate and misreporting the timeout as a sharp teardown hang.Related work
Fixes #5485.
No overlapping implementation PR was found in the open inventory or searches for #5485 and occlusion. #5482 and #5483 address separate embedded-captions CLI/audio concerns; they are not changed here.
How
Use the existing
verdict_atmarker to distinguish an analysis timeout from a post-verdict teardown hang. A timeout without a verdict setsOCC_RC=3, emits an analysis-incomplete diagnostic, and enters the existing abort/OCCLUSION_SKIP=1handling. The render script still exits 2 when the gate aborts. Watchdog budgets and post-verdict pass/fail interpretation are unchanged.The new Node tests execute the production shell gate against controlled analyzer processes. Only the test harness advances Bash's clock, in ten-second steps for hanging analyzers; normal-exit fixtures use short real polling. No production timeout configuration or new dependency is introduced. The required pre-commit hook also updates only the embedded-captions fingerprint and file count in skills-manifest.json.
Test plan
bash -n,bun run build,bun run lint,bun run test:skills(869 passed, 2 skipped), standalone skill import checks (2 passed), additional skills CI guards (17 passed), test reachability, formatting, comment checks andgit diff --cached --check. Normal contributor pre-commit and commit-message hooks passed, and the commit is SSH-signed.Validation limits: no full caption-video render or Linux execution was performed. A broader
bun run testrun on an unmodified archive of the same main SHA exited 1 with unrelated package failures (including unavailable LFS media, timing-sensitive checks, and local runtime/process cases); the core runner exited 129. This is not a claim that the full workspace suite passes. Complete baseline logs and named failures are available in the review materials.