Skip to content

fix(skills): reject occlusion timeout before verdict - #5491

Merged
jrusso1020 merged 1 commit into
heygen-com:mainfrom
xiajiahang:fix/occlusion-timeout-before-verdict
Oct 12, 2026
Merged

jrusso1020 merged 1 commit into
heygen-com:mainfrom
xiajiahang:fix/occlusion-timeout-before-verdict

Conversation

@xiajiahang

Copy link
Copy Markdown
Contributor

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 PASS or allows rendering to continue without an explicit override.

Why

A hanging analyzer produces no cap(s) FAIL line. 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_at marker to distinguish an analysis timeout from a post-verdict teardown hang. A timeout without a verdict sets OCC_RC=3, emits an analysis-incomplete diagnostic, and enters the existing abort/OCCLUSION_SKIP=1 handling. 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

  • Unit tests added/updated: eight cases cover silent/partial pre-verdict timeouts, explicit override, passing/failing teardown hangs, normal passing/failing exits, and analyzer errors. The three timeout/override regressions fail on unchanged main. The revised suite passed 30 consecutive local runs and 20 independent reviewer runs on macOS Bash 3.2.
  • Manual testing performed: 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 and git diff --cached --check. Normal contributor pre-commit and commit-message hooks passed, and the commit is SSH-signed.
  • Documentation updated (not applicable; no command or option changes).
  • Comments follow CONTRIBUTING.md "Comments": the watchdog comment states that incomplete analysis cannot pass; the test harness explains the clock substitution and invariant.

Validation limits: no full caption-video render or Linux execution was performed. A broader bun run test run 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.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_RC is already set, so the wait line 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 writes occlusion+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 fake check-occlusion.cjs lives.
  • The sleep function overrides the external sleep command. 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 to SECONDS resets 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 spawnSync has 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

@jrusso1020
jrusso1020 enabled auto-merge October 12, 2026 20:48
@jrusso1020
jrusso1020 added this pull request to the merge queue Oct 12, 2026
Merged via the queue into heygen-com:main with commit 4120042 Oct 12, 2026
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

embedded-captions: occlusion gate records PASS when check-occlusion times out before its verdict

2 participants