Tier A hardening: close five silent-failure paths in demo rendering - #11
Merged
Merged
Conversation
Each item fixes a failure this pipeline has already produced in a real render, sourced from the 2026-07-25 benchmark against OpenMontage and HyperFrames. Concepts only from the AGPL side; every mechanism here is ours. S1 fail-closed narration. resolveTtsMode() replaces an implicit disjunct that inferred silent mode from an ABSENT api key, so a forgotten `doppler run` rendered a complete, correctly-timed, captioned video with no voice at all, and parity verification could not see it because the estimated duration IS the clock. Fake mode must now be requested explicitly. S2 readiness settle gate. waitForReady() runs after every goto and before recording starts: fonts ready plus in-viewport images decoded, bounded by capture.settleMs (default 500). Fails OPEN, warning and recording anyway, since aborting would discard an already-paid render. Makes the logged all-white frozen-segment failure structural instead of relying on authors remembering to hand-author `wait ms=`. S3 declarable duration cap. maxDurationSec replaces the hardcoded 300 literal and is carried across the render manifest so a remote render enforces the same limit as a local one; pre-cap manifests default to the 300 they were rendered under. Events ship against 2:00 and 3:00 caps, and a keyless rehearsal now reaches the check with measured capture wall-clock. S4 render provenance. render-report.json records the resolved voice, toolchain versions, config and script digests, the per-shot timeline, and the parity result. Written after a successful render and never gating. This is the missing answer to a judged 2:57 cut re-rendering at 3:25: an unpinned voice model default, not vendor nondeterminism. S5 strict config schemas. Every schema object rejects unrecognised keys, so a typo in the remedy for a known defect fails loudly instead of silently applying the default it was written to override. All four shipped configs validate clean. Verified: 161 tests green, typecheck clean, and each gate observed firing in a real keyless render rather than only in unit tests.
…bound tests Three reviewers (security, correctness, adversarial) ran against the diff. Two BLOCKING findings were real defects, not test gaps. The settle gate did nothing. Recording starts at screencast.start() and only then were the actions run, so waitForReady waited ON CAMERA and dwell() absorbed the time from the trailing pad: identical content, identical length. An adversarial run sampling frame colour confirmed the hero image was absent for the first 1.4s of the shipped segment. Fixed by hoisting the shot's first goto and its settle ahead of the recorder (openShotPage), so frame one is painted. The first goto is found anywhere in the sequence, not only at index 0, so a chapter-first cold open is covered too. The legacy recordvideo engine binds capture at context creation and cannot be fixed this way; that is documented in the spec rather than hidden. A stale dist-remote bundle silently ignored the declared cap. existsSync was the only guard and `pnpm demo` never rebuilds, so an old renderer enforced the previous hardcoded 300 while the report recorded the declared limit beside a parity pass that never checked it. Offloading now refuses a bundle older than the sources. Three changes had no test binding them: reverting the cap to a literal 300, deleting the report write, and gutting the readiness probe each left the whole suite green. Added tests that kill all three mutants, verified by re-running each mutation. Also: default maxSec at the enforcement point so an absent value cannot mean uncapped; consume the probe result instead of treating "evaluate resolved" as "settled"; make a deliberate settleMs 0 silent; strip control characters from page-controlled error text before it reaches the operator's log; validate maxDurationSec at the manifest trust boundary so a non-numeric value cannot make the comparison NaN; digest a config projection that excludes machine-resolved paths; record the MEASURED timeline rather than the narration estimate; capture the narration mode once before spend rather than re-deriving it after; delete a stale receipt before rendering; drop internal Doppler project names from a public package's error text. Corrected an overclaim rather than keeping it: the keyless rehearsal measures the word-count estimate, not real narration, so it surfaces an overage but does not guarantee one is caught pre-spend. Spec and code comment both say so now. README documents both new knobs, CHANGELOG records the breaking narration change, version 0.3.0. Verified: 170 tests, typecheck, build all green; S1 and S3 observed firing in real renders; report timeline matches the rendered runtime exactly.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @OrionArchitekton, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughThis release adds strict configuration validation, configurable duration and readiness limits, explicit narration-mode resolution, readiness-gated capture, remote bundle freshness checks, and ChangesRender hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant TTS
participant Renderer
participant Report
Pipeline->>TTS: Resolve narration mode
Pipeline->>Renderer: Render with maxDurationSec
Renderer-->>Pipeline: Return timeline and parity
Pipeline->>Report: Build and write render report
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d048f61924
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 230: Update the capture.settleMs documentation in README.md at lines
230-230 to qualify the pre-recording readiness guarantee as applying only to the
default screencast engine and note that recordvideo starts at context creation,
shifting pre-settle frames later. Add the same qualification to the
corresponding release note in CHANGELOG.md at lines 18-20.
In `@specs/tier-a-hardening-spec.md`:
- Around line 84-92: Update the S4 specification to document render-report
generation as best-effort, matching the failure handling in the pipeline that
logs report construction, tool-version lookup, or write failures while returning
the rendered result. Adjust the unconditional acceptance language so a completed
render may lack render-report.json when report generation fails.
- Around line 66-82: Rewrite the S3 specification and its related acceptance
language to remove any promise of authoritative pre-spend duration enforcement.
Describe keyless or FAKE_TTS rehearsal as a no-spend estimate/preflight, and
state that the cap check becomes authoritative only after rendering, when
narration may already have been purchased; retain the configurable cap and
overage reporting requirements.
In `@src/capture.ts`:
- Around line 511-520: Update the recording failure handling around openShotPage
and the guard so recordvideo artifacts are deleted when opening navigation fails
before runShot starts. Ensure the catch handler closes/finalizes the context,
removes the produced video, and preserves the existing behavior for other
capture engines; also revise the nearby comment so it does not claim recordvideo
fails closed without recording.
In `@src/pipeline.ts`:
- Around line 187-198: Update the renderRemote/RenderResult flow so remote tool
versions are returned with the render result and used when constructing the
report in buildRenderReport. Replace the local toolVersions() probe for remote
renders, while preserving local tool capture for local rendering and clearly
recording the applicable toolchain in render-report.json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f12267d4-d325-493f-8e8d-3136e5f8e131
📒 Files selected for processing (20)
CHANGELOG.mdREADME.mdpackage.jsonspecs/tier-a-hardening-spec.mdsrc/capture.tssrc/config.test.tssrc/manifest.tssrc/pipeline.tssrc/provenance.test.tssrc/provenance.tssrc/ready.test.tssrc/ready.tssrc/render.tssrc/tts.test.tssrc/tts.tssrc/types.test.tssrc/types.tstests/manifest.test.tstests/pipeline.smoke.test.tstests/remote-render.parity.test.ts
A relative dashboardBaseUrl ("./") is rewritten by loadConfig into a file:// URL
of the current checkout, so hashing it verbatim re-introduced exactly the
machine dependence stableConfigJson exists to remove. The same committed config
produced different configHash values per checkout, defeating the cross-operator
comparison the report is for. Verified before fixing: demos/smoke hashed with
the absolute worktree path embedded.
A local fixture base is not part of a config's identity, so it collapses to a
sentinel; an http(s) base IS part of the identity and is already
machine-independent, so it is kept verbatim and two different ports still
digest differently. Three tests cover both directions plus the auth profileDir
case.
Found by the post-push review pipeline (P2 bot thread on PR #11). It is the
residual half of a finding the pre-PR adversarial pass raised: that fix covered
capture.auth.profileDir and out, and missed this one.
Verified: 173 tests, typecheck, build all green.
…stop overclaiming in docs The security finding was real and is the substantive one. For target: live under the legacy recordvideo engine, Playwright binds recording at context creation, so by the time the auth guard trips during the opening navigation the WebM already exists and context.close() FINALISES it. The code claimed "an expired session fails closed with nothing recorded" while leaving a recording of a logged-out page on disk. The catch now reads the video path before closing and deletes it: fails closed has to mean no artifact, not merely no return value. A test asserts no .webm survives, and it fails when the deletion is removed. Docs and spec corrections, all cases of promising more than the code delivers: - README and CHANGELOG now qualify the readiness guarantee as screencast-only and name the recordvideo limitation, instead of implying both engines exclude pre-settle frames. - S3 renamed from "checked before spending" to "declarable and enforced". Enforcement is post-render: a real run synthesises all narration before the render stage, so an over-cap real run has already spent. The keyless rehearsal is the no-spend path and now says exactly that. - S4 states the report is best-effort. A provenance failure warns and leaves the render standing rather than discarding an artifact already paid for, so the report is evidence when present, never a guarantee of presence. Also records that a parity failure produces no report at all. Reversed an earlier decline rather than defending it: the remote-toolchain gap was raised by the pre-PR adversarial pass, declined as a follow-up, then re-raised independently. The spec says "the versions of the tools that rendered it", so the spec wins. The report now carries renderedOn: local | remote, so locally-probed versions can never be silently read as the remote renderer's. Probing the remote toolchain remains a follow-up, now stated in the spec. Verified: 175 tests, typecheck, build green; a real render carries renderedOn: local.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/provenance.ts (1)
98-110: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize local authentication URLs before hashing.
stableConfigJson()hashesdashboardBaseUrlandprofileDirafter stripping checkout-specificfile://values, butcapture.auth.loginUrlis left untouched. Sincetests/live-capture.smoke.test.tsresolvesloginUrlfromresolve("tests/fixtures/saas-app.html"), identical live configs in different checkouts can get differentconfigHashvalues. Apply the same local-file normalization or preserve the intended stable fixture path while still keeping remote login URLs intact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provenance.ts` around lines 98 - 110, Update stableConfigJson() to normalize local file-based capture.auth.loginUrl values before hashing, using the same checkout-independent treatment as dashboardBaseUrl and auth.profileDir. Preserve remote HTTP(S) login URLs unchanged and retain the existing auth/profileDir sanitization.
🧹 Nitpick comments (1)
src/provenance.test.ts (1)
50-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a schema-valid typed fixture instead of
as never.These tests bypass
DemoConfig, so required-field or schema changes can leave them compiling while they exercise malformed objects. Build the fixture withDemoConfigSchema.parse(...)or type it asDemoConfig, then vary only the fields under test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provenance.test.ts` around lines 50 - 51, Replace the `as never` casts in the `base` and `mk` fixtures with a schema-valid `DemoConfig` fixture, using `DemoConfigSchema.parse(...)` or explicit `DemoConfig` typing. Keep the shared valid fields intact and vary only the properties under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/provenance.ts`:
- Around line 15-20: Update the provenance model around renderedOn and tools to
explicitly identify that tools was probed locally, adding a serialized toolHost
or equivalent source field. Populate it consistently in the pipeline and ensure
remote reports distinguish the local tool snapshot from the remote renderer’s
toolchain.
---
Outside diff comments:
In `@src/provenance.ts`:
- Around line 98-110: Update stableConfigJson() to normalize local file-based
capture.auth.loginUrl values before hashing, using the same checkout-independent
treatment as dashboardBaseUrl and auth.profileDir. Preserve remote HTTP(S) login
URLs unchanged and retain the existing auth/profileDir sanitization.
---
Nitpick comments:
In `@src/provenance.test.ts`:
- Around line 50-51: Replace the `as never` casts in the `base` and `mk`
fixtures with a schema-valid `DemoConfig` fixture, using
`DemoConfigSchema.parse(...)` or explicit `DemoConfig` typing. Keep the shared
valid fields intact and vary only the properties under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dd77ae8-e524-4a49-b41c-88b8f93e700c
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdspecs/tier-a-hardening-spec.mdsrc/capture.tssrc/pipeline.tssrc/provenance.test.tssrc/provenance.tstests/live-capture.smoke.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGELOG.md
- README.md
- specs/tier-a-hardening-spec.md
- src/pipeline.ts
- src/capture.ts
Comment on lines
+15
to
+20
| /** Where the ffmpeg work actually happened. `tools` below is probed LOCALLY, so | ||
| * on "remote" it describes the machine that captured and synthesised, NOT the | ||
| * one that rendered. Recorded explicitly so a reader is never misled into | ||
| * attributing a runtime change to a toolchain that did not produce it. */ | ||
| renderedOn: "local" | "remote"; | ||
| tools: ToolVersions; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files of interest:\n'
git ls-files | rg '(^|/)provenance\.ts$|(^|/)pipeline\.ts$|package\.json$|schema|report' | sed -n '1,120p'
printf '\n--- provenance outline ---\n'
ast-grep outline src/provenance.ts --view expanded || true
printf '\n--- provenance relevant lines ---\n'
sed -n '1,140p' src/provenance.ts
printf '\n--- pipeline relevant lines ---\n'
sed -n '160,220p' src/pipeline.ts
printf '\n--- toolVersions usages ---\n'
rg -n "toolVersions|tools:|renderedOn|RenderReport|provenance" src -SRepository: OrionArchitekton/agent-demo-video
Length of output: 8919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- package scripts/deps ---\n'
cat package.json
printf '\n--- provenance tests ---\n'
sed -n '1,140p' src/provenance.test.tsRepository: OrionArchitekton/agent-demo-video
Length of output: 262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package scripts/deps ---'
cat package.json
printf '%s\n' ''
printf '%s\n' '--- provenance tests ---'
sed -n '1,180p' src/provenance.test.tsRepository: OrionArchitekton/agent-demo-video
Length of output: 5164
Make the provenance source of tools explicit.
renderedOn can be "remote" while tools is always await toolVersions() locally from the pipeline. The serialized report only contains the local version snapshot, so consumers can mistake the host’s toolchain for the rendered VM’s. Add a serialized field such as toolHost/local-only tools, or separate probe/provided tool versions, and cover the remote report shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/provenance.ts` around lines 15 - 20, Update the provenance model around
renderedOn and tools to explicitly identify that tools was probed locally,
adding a serialized toolHost or equivalent source field. Populate it
consistently in the pipeline and ensure remote reports distinguish the local
tool snapshot from the remote renderer’s toolchain.
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five changes that close paths where the pipeline completes successfully and still ships a defective video. Each fixes a failure this pipeline has already produced in a real render. Sourced from a benchmark against two external video-production systems (
knowledge-vault/research/2026-07-25-demo-video-production-pipeline-benchmark.md, PR #94 there); ideas only from the AGPL-licensed side, every mechanism here is ours.Spec:
specs/tier-a-hardening-spec.md.What changes
ELEVENLABS_API_KEYis an error, not an implicit silent modegoto-only shot could end capture before first paint, shipping a blank segment, in a real render even when the keyless smoke was cleanmaxDurationSecreplaces a hardcoded 300, carried across the render manifestrender-report.jsonrecords voice, toolchain, digests, measured timelineReview
Three reviewers ran against the diff. Two BLOCKING findings were real defects, not test gaps, and both are fixed:
dwell()absorbed the time from the trailing pad: identical content, identical length. An adversarial run sampling frame colour confirmed the hero image absent for the first 1.4s of the shipped segment. Fixed by hoisting the opening navigation ahead of the recorder.dist-remotebundle silently ignored the declared cap.existsSyncwas the only guard andpnpm demonever rebuilds, so an old renderer enforced 300 while the report recorded the declared limit next to a parity pass that never checked it. Offloading now refuses a bundle older than its sources.Three changes had no test binding them. Reverting the cap to a literal
300, deleting the report write, and gutting the readiness probe each left the entire suite green. Tests now kill all three mutants, verified by re-running each mutation.Also fixed from review: default
maxSecat the enforcement point so an absent value cannot mean uncapped; consume the probe result rather than treating "evaluate resolved" as "settled"; make a deliberatesettleMs: 0silent; strip control characters from page-controlled error text; validatemaxDurationSecat the manifest trust boundary so a non-numeric value cannot make the comparisonNaN; digest a config projection excluding machine-resolved paths, which previously gave the same committed config different hashes per operator; record the measured timeline rather than the narration estimate; capture the narration mode once before spend; delete a stale receipt before rendering; drop internal Doppler project names from a public package's error text.One overclaim was corrected rather than kept: the keyless rehearsal measures a word-count estimate, not real narration, so it surfaces an overage but does not guarantee catching one pre-spend. The spec and the code comment both say so now.
Verification
pnpm typecheck,pnpm buildall greenrender-report.jsonfrom a real render carries a measured timeline matching the rendered runtime exactlyBreaking
A missing API key now exits non-zero instead of producing a silent video. Every existing test already requested fake mode explicitly, so this is internally non-breaking; it is breaking for npm consumers, hence 0.3.0 and a CHANGELOG.
Known limitations, stated not hidden
The legacy
recordvideoengine binds capture at context creation, so the settle shifts rather than removes unsettled frames there. The default screencast engine does not have this limitation. Reviewers also flagged that the DEMO_SCRIPT parser still swallows typos silently (S5 covers the JSON config only) and that a remote render records the local toolchain versions; both are logged as follow-ups rather than scope creep here.Summary by CodeRabbit
maxDurationSecrender length cap (default 300) andcapture.settleMsreadiness wait budget.render-report.jsonprovenance with voice/TTS selection, tool versions, per-shot timeline, and parity results.FAKE_TTS=1for deliberate silent placeholder audio.FAKE_TTS=1.