-
Notifications
You must be signed in to change notification settings - Fork 2
Address adversarial review findings: capture health, network gating, credentials, CI, packaging #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Co-Messi
wants to merge
33
commits into
main
Choose a base branch
from
fix-audit-findings
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
aa4add0
test: scope vitest collection to this checkout
Co-Messi b617865
feat(schema): declare the unified clock in the event log
Co-Messi 3a247b5
fix(render): hard capture-health gate; legacy skew comes from the sch…
Co-Messi 7e67c9b
fix(director): never send a provider-scoped key to a custom endpoint
Co-Messi 8ca3f02
fix(security): --block-private-network now gates every request type
Co-Messi 77c18c5
fix(security): gate record-stage requests; align allowPrivateNetwork …
Co-Messi 5e4b101
fix(director): treat page-derived text as data in prompts; add --dry-…
Co-Messi 6a10223
fix(director): preserve the take when QC cuts everything; report befo…
Co-Messi 1458cd3
fix(render): bound the ffmpeg mux with a timeout and explicit maxBuffer
Co-Messi a79f6f6
fix(render): catch hard tab deaths; size the render timeout from the …
Co-Messi be98d46
feat(director): meter usage-less providers; refuse oversized calls pr…
Co-Messi d9d8041
fix(director): bound the --repo walk with a file-count budget
Co-Messi 4e30b12
fix: low-severity audit findings across CLI, director, and render
Co-Messi 57ab54c
chore(ci): green audit via lockfile fix; verify build and package shape
Co-Messi 460d632
chore(npm): ship only render assets; add publish metadata; v0.1.0
Co-Messi 8f038cb
docs: lead with the AI director; npm-safe images; npx-first quick start
Co-Messi 7930e89
test: malformed take directories and the ffmpeg mux failure path
Co-Messi ae87272
fix(director): per-run nonce in untrusted markers; scrub to a fixpoint
Co-Messi 5114985
fix(capture): restore record()'s fail-closed allowPrivateNetwork default
Co-Messi f548478
feat(security): gate WebSocket connections when the private-network g…
Co-Messi c0461d4
fix(director): applyVerdicts throws a typed AllScenesCutError instead…
Co-Messi 1a10492
fix(director): preflight tolerates 401/403 roots; add --skip-preflight
Co-Messi 0f2cd10
fix(director): warn when SUPERCUT_ALLOW_SPARSE bypasses the generate …
Co-Messi 55e8fb7
fix(director): size the per-image token estimate as a cross-provider …
Co-Messi 19145c5
test: wiring coverage for the record request gate and the generate he…
Co-Messi ddec22e
chore(ci): run e2e by naming convention so new e2e files can't be ski…
Co-Messi 29b41ce
fix(render): stop the watchdog timer and fatal-poll loop on every exi…
Co-Messi 1f26566
fix(render): capture health counts cursor_path point timestamps as ta…
Co-Messi 785c6cd
fix(director): apply --app scoping inside the repo walk, before the f…
Co-Messi b4ac900
fix(security): request gate fails closed on DNS failure and never cac…
Co-Messi 057888c
fix(cli): dry-run's suggested record command keeps --block-private-ne…
Co-Messi bafe27b
fix(director): --dry-run no longer requires ffmpeg
Co-Messi b1f39e9
fix(director): wrap the full page-derived analysis in the untrusted m…
Co-Messi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gate has zero coverage of its wiring, and the e2e test that claims to cover H5 does not reach it.
test/record.e2e.test.ts:203— "refuses a private-network recipe when the guard is engaged (H5)" — callsrecord()with a127.0.0.1recipe andallowPrivateNetwork: false, and asserts it throws/private network/i. That throw comes fromassertRecipeNavigationPolicyat line 227, which ran onmaintoo. The test would pass unchanged if you deleted the entirecreateRequestGateblock and the--host-resolver-rulespinning above it.Worse, the assert makes the gate unreachable from that direction: with the guard on, every recipe URL must be public, so no localhost recipe can ever get far enough to install the route handler. The gate's only real coverage is
test/url-policy.test.ts, which testscreateRequestGateas a pure function with an injectedisPrivateHost— good tests, but they say nothing about whether it is wired to the right object, whetherroute.abort()is reached, or whether the pinning loop above produces a valid--host-resolver-rulesstring.Same shape on the generate side:
assessCaptureHealthis well covered as a unit and throughrenderTake, but the new call site ingenerate.ts:346has no test. A stubbed sparseframes-index.jsonafterrecordwould pin it.Concretely, what would close it: a fixture served on a public-looking hostname pinned to loopback via the existing resolve-and-pin path, or a test that constructs a
Pagewith the route handler installed and asserts a subresource to a private host aborts. Either one is more work than a unit test, which is exactly why the wiring is where the bug will be.Unrelated but adjacent: the pinning loop calls
resolveAndPinHostfor every recipe host afterassertRecipeNavigationPolicyalready resolved them — a second DNS lookup and a small TOCTOU window between the two. Harmless in practice, worth a comment noting it is deliberate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 19145c5. test/request-gate.e2e.test.ts reaches
createRequestGatethroughrecord(), and I mutation-checked it the way you checked the old one: with the gate block disabled, the guard-on test fails, so it cannot re-prove the assert the way the H5 e2e did. Shape: a probe page fires afetch()and a WebSocket at a private "internal" server that counts everything that actually leaves the browser. Guard on: zero requests and zero upgrade attempts while the vetted entry loads and the scene completes. Guard off: both probes reach the server (so the zeros measured a gate, not a broken page), and neithercreateRequestGatenorresolveAndPinHostis even called, pinning the no-interception-tax claim.One honest caveat and one finding from trying your suggested route. Only the gate's DNS classifier is injected; the fixture-on-a-pinned-public-hostname version turns out not to be portable, because on a machine whose resolver hijacks unknown names to fake IPs (Clash-style TUN, which this dev machine runs) even
MAP example.com 127.0.0.1never produces a TCP connection to loopback. That experiment surfaced something worth knowing beyond the tests: on such machines the--host-resolver-rulespin is bypassed in production too, since name resolution happens in the proxy/tunnel, which leaves the request gate as the load-bearing defense there.The generate-side call site is covered in test/health-gate.e2e.test.ts: a stubbed sparse frames-index trips the gate right after record, before any QC spend (llm.calls stays at 2), and the bypass path is covered in the same file. The double-resolve between the assert and the pin loop now carries the deliberate-TOCTOU comment you asked for. And ddec22e makes both CI and
test:e2eselect files by the*.e2e.test.tsconvention, because the old explicit list would have silently skipped these two new suites.