Skip to content

fix: address the 2026-10-09 adversarial review - #18

Open
Co-Messi wants to merge 32 commits into
mainfrom
fix/roast-20261009
Open

Co-Messi wants to merge 32 commits into
mainfrom
fix/roast-20261009

Conversation

@Co-Messi

@Co-Messi Co-Messi commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Fixes every finding of the 2026-10-09 adversarial review (.roast/REPORT-20261009T133210Z.md), one commit per finding, tests first. Do not merge without review.

ID Finding Fix Test Commit
H1 Destructive filter missed money and irreversible verbs; skill and generate disagreed One policy in src/security/destructive.ts (Buy, Sell, Subscribe, Upgrade, Pay/Payment/Payout, Transfer, Send money, Order now, Checkout, Refund, Charge, Merge, Deploy, Publish, Approve, Restart, Discard, Clear all, Empty trash, Purge, Drop, Log out, Sign out, Leave team, Ban, Uninstall, Disconnect, Cancel order and more); view-only phrases set aside; fields whose form submits through a destructive control can be typed but never submitted (hard script gate); SKILL.md lists the same terms between markers destructive-policy.test.ts (two sided table, SKILL vs code both directions), director.test.ts (submit gate), generate.e2e forms fixture 01936ea
H2 Clicks pressed a stale point with no hit-test Hit-test the press point (open shadow roots, iframe target) right before pressing; re-aim once; fail the scene if still covered; log the box and point actually pressed capture.e2e "aiming": moved target (decoy never pressed), covered target (scene fails, no click logged) 070b780
H3 type select-all plus Backspace wiped editors and unfocused fields Keys only when the focused text-entry element is the target, inside it, or its contentEditable host; clear only the target's own input/textarea; append elsewhere; preview says "(replaces existing text)" / "(appends to existing text)" capture.e2e editor fixture (contentEditable survives, prefilled textarea replaced alone, unfocusable target gets no keys), preview unit test a6b9929
H4 QC re-takes repeated side effects up to 4 times under one consent Only cuts re-record, at most once (MAX_RETAKES = 1); hold and zoom patches applied to the take at render time (src/render/adjust.ts); changed only when a value differs; consent states the bound retake-policy.test.ts (loop bounded with injected record/QC, no-op patch films once, render-time freeze), generate.e2e hold-only verdict films once, cut re-films once cba1d0d
H5 No signed-in sessions; IdP login pages crawled --storage-state on generate and record (crawl and capture contexts, path only); off-site start page refused (http to https and apex/www allowed); login-only or empty crawl fails before any LLM call; record fails an off-site entry session.test.ts, storage-state.e2e.test.ts (refusal, signed-in run, cookie absent from prompts, logs and every output byte, dry run too, login-only before LLM) baa1a77
M1 Text-derived selectors leaked emails/tokens Elements whose selector redaction would change (also with CSS escapes removed) are left out, at crawl and at prompt egress prompt-egress.test.ts c4b41f8
M2 A QC failure after capture killed the run visionVerdictsOrNone: any failure logs and renders the recorded take vision-qc.test.ts, generate.e2e (QC call throws, video still renders) f07298f
M3 QC prompt lacked untrusted markers UNTRUSTED_RULES in the QC system prompt; scene names and retry feedback wrapped; frame-reader seam so the unit test reaches the model vision-qc.test.ts f07298f
M4 Any target could load private subresources by default Default decided from the target: private target keeps private requests, public target gets the guard; --block-private-network strict; --allow-private-network is the explicit opt in network-policy.test.ts, request-gate.e2e default posture both ways 8eaf11f
M5 Control characters could rewrite the action preview src/security/terminal.ts; preview fields quoted and escaped; every generate log line, record summary and CLI error escaped terminal-safety.test.ts 9e10bd7
M6 "Hard" ceiling not hard Worst case charged for resets and 5xx; CJK estimate one token per char; per-image cost learned from reported usage; docs say exactly what is enforced director-llm-budget.test.ts d2be430
M7 --bg typo failed after all spend assertBackground at preflight background.test.ts 59c5ff5
M8 Bogus route seeds for non-Next frameworks starved the crawl Routes read only under a Next.js root; @slot, (.), _private handled; seeds and links interleaved source-routes.test.ts (Remix, Angular, Vite yield none; Next segments; crawl order) 4cb6285
M9 Rows with a nested Delete were excluded Containers judged by their own label; a container whose centre is a destructive control stays out, found by geometry so a card below the fold is covered too generate.e2e CRUD fixture (including a Delete card under a 2000px spacer) 5b09101, 08e787b
M10 Unpinned Playwright playwright pinned to 1.60.0 package-shape.test.ts (exact pin equals lockfile) 5c838fc
M11 Release used a long-lived token, no CI gate, false provenance claim Trusted publishing (OIDC, Node 24, npm 11.6.2 for publish), ci.yml reusable and required on the tagged commit, npm-publish environment, CHANGELOG fixed release-workflow.test.ts, actionlint d5c4108
L1 Patched recipes skipped the 60s check Every patched recipe passes parseRecipe; holds that would break the cap are dropped retake-policy.test.ts cba1d0d
L2 repoNotes followed symlinks; aws_secret_access_key not redacted Regular files only; prefixed secret keys redacted when the value looks like a secret (8+ characters), so ?password_reset=1 stays a filmable URL repo-notes.test.ts d3c28a2, 3245273
L3 Crawl and capture viewports differed Both use CAPTURE_VIEWPORT (1920x1080) generate.e2e responsive fixture 1aeed7d
L4 :nth-match probing unbounded Index found by identity in one call (2,000 rows: 40.7s to 2.5s) generate.e2e 2,000 vs 20 rows 3ce8642, ee3eebb
L5 Library defaults disagreed generate(), record(), crawlApp() share the M4 default record.e2e, request-gate.e2e 8eaf11f
L6 --dry-run overpromised Destructive link paths never crawled; help and SECURITY.md say what a dry run does generate.e2e nav fixture (no GET to /logout) 00036bd
L7 Custom provider compatibility Retry with max_completion_tokens / without response_format; provider message shown (redacted); vision opt-in for custom provider-compat.test.ts 89a6c81
L8 CWD .env could redirect egress Warning naming file, variable and host for an unnamed .env dotenv-warnings.test.ts 5f4ac89
L9 Artifacts never cleaned; out/ collides with Next export Superseded takes deleted after render; temp stream removed on exit/SIGINT/SIGTERM; default supercut-out/ artifacts.test.ts, generate.e2e re-take removes take-0 ed3cd1a
L10 Typed text length unbounded Estimate counts typing time (820ms + 60ms per grapheme + 350ms Enter); text capped at 500 recipe-estimate.test.ts, package-docs.test.ts 59e5445
L11 Node 24 missing from CI Matrix 20, 22, 24; engines stays >=20 release-workflow.test.ts 2260eb0
L12 Stale docs SKILL uses the published package; spike results marked historical; model defaults verified to exist (not changed) package-docs.test.ts d8b9a2f
L13 check-pack was a denylist Allowlist plus " 2" duplicate rule check-pack.test.ts 77b9ee0
Style Dashes, review tags, history comments Mechanical sweep, no behaviour change full suites 0920914

Unnumbered suggestions done (e8964b5): SIIT ::ffff:0:0/96 and local-use NAT64 64:ff9b:1::/48 in the private-network policy, Retry-After honoured (capped at 60s), run id plus per-stage timings in director-report.json (run id also in render-report.json), Dependabot for npm and GitHub Actions. CHANGELOG ## Unreleased: 944668d.

Decisions

  • H1: bare Publish, Deploy, Merge, Approve, Restart and every Transfer are destructive (the old tests that pinned "Publish" and "Transfer ticket" as filmable moved sides). The pay stem covers Pay, Paying, Payment, Payout but not the plural nouns "Payments" or the slug payments-worker, so dashboards keep their rows. "Reset view", "Reset zoom", "Reset filters", "Clear filters", "Clear all filters", "Drag and drop" are view-only and stay filmable. "Send" and "Send feedback" stay filmable; "Send money" and "Send $50" do not. No new flag for generate --yes; the residual risk is documented.
  • H2: a press re-aims once (300ms corrective move); the 70ms press hold remains a narrow window (a click then fires on the common ancestor), documented in the code.
  • H3: the focused element must be a text-entry element; a contentEditable child of the focused host counts as focused. Select elements and button-type inputs get no keys.
  • H4: re-takes that must re-record are stated in the confirmation question ("each action can run up to 2 times"); MAX_RETAKES is 1 because a re-take now only drops scenes. A shorter hold suggested by QC is noted and not applied. Holds are inserted as a frozen stretch at frame cadence, so the health and skew gates stay quiet. L1 is in the same commit.
  • H5: refuse only off-site start pages; later off-site pages are skipped. Login-only detection: every crawled page has a password field and nothing but fields and sign-in links.
  • M4/L5: "the existing explicit opt in" is --allow-private-network, previously a deprecated no-op, now meaningful. A public target gets the existing full guard rather than a lighter one, so the guard is never weakened; --block-private-network additionally refuses a private target.
  • M6: free failures are only those known to precede sending (DNS, refused, unreachable, TLS) plus 429 and other 4xx; resets, unknown network errors and 5xx are charged the worst case.
  • M9: a container whose centre is a nested destructive control is excluded, because the recorder presses the centre. The centre control is found by geometry, not hit-testing, since the crawl does not scroll and a card below the fold must be judged too.
  • L2: a bare key name (password, token, api_key) keeps the old rule (any value redacted); a prefixed name (aws_secret_access_key, access_token) is redacted only when its value has 8 or more characters, because the same rule decides whether a URL is a credential.
  • L7: vision default off for custom providers. L9: default output supercut-out/. L13: allowlist plus duplicate rule.
  • M2 and M3 share one commit (same function); L4 has a follow-up commit that replaced a wall-clock bound flaky on CI with a relative one.

Needs user action

  1. On npmjs.com, add a trusted publisher for @co-messi/supercut: GitHub Actions, repository Co-Messi/supercut, workflow file release.yml, environment npm-publish. Without it the next tag's publish fails.
  2. Optionally add required reviewers to the npm-publish environment (repository Settings, Environments), and a tag protection rule for v*.
  3. After the trusted publisher works, delete the NPM_TOKEN repository secret (no longer read).
  4. Deprecate the placeholder: npm deprecate @co-messi/supercut@0.0.0-stage "placeholder; install the latest version".
  5. Publish 0.1.1: bump version in package.json and the plugin manifests, merge, push v0.1.1. The release workflow runs CI on the tag and publishes with provenance.
  6. Read "Behaviour changes for existing users" in the CHANGELOG before releasing.

Known limits

  • The destructive filter is English and label based; icon-only buttons, other languages and side effects behind harmless labels pass. With --yes no human reviews the actions.
  • Closed shadow roots are not descended by the hit-test or focus checks; elements inside iframes cannot be targeted by recipe selectors.
  • Session support is Playwright storage state only (no --cookie); login-only detection is a heuristic; record's off-site check runs right after the entry load, so a delayed JavaScript redirect can be missed.
  • Public targets now go through the guard by default: responses are buffered (a response that never completes fails after 30s), service workers are blocked. --allow-private-network restores the old path.
  • The token ceiling limits tokens, not money; one call can exceed its estimate (the next is refused); charging 5xx at worst case can exhaust a budget sooner on a flaky provider.
  • A prefixed secret key with a value under 8 characters is not redacted.
  • Source routes are read for Next.js only. Provider compatibility retries key on the provider's error text. The .env warning warns, it does not refuse.
  • Model defaults (deepseek-v4-pro, anthropic/claude-sonnet-4.6) exist on the providers' published lists as of 2026-10-10, but a provider can change what a name points to.
  • DNS rebinding remains best effort (unchanged, documented). Teredo addresses are not decoded.
  • npm run check:pack correctly fails in a checkout that holds a README.md.bak leftover (npm always packs README*); it was validated on a clean export.

Verification

All on macOS arm64 at the PR head unless noted.

  • npx tsc --noEmit: clean. npm run test:fast: 45 files, 727 tests passed. npm run test:e2e: 12 files, 98 tests passed.

  • npm run check:pack on a clean export of the head (git archive): ok, 129 files, 18.2 MB packed, 19.1 MB unpacked.

  • actionlint on ci.yml and release.yml: clean.

  • Packed install: npm pack, installed into a fresh project: npx supercut --help prints the new help; npx supercut doctor: all checks passed.

  • Bundled demo (examples/demo.recipe.json, served on port 4391 because another session's server held 4319; the same port-substituted recipe for both runs), rendered with --music daybreak:

    baseline (a06aa22) this branch
    record 1028 frames, 95.6 fps source, 8 events 1027 frames, 95.6 fps source, 8 events
    render framed 4 of 5 beats (skipped: 1 target fills the viewport); 1 page change (1 scene-reload); 11.8s video; 711 frames framed 4 of 5 beats (skipped: 1 target fills the viewport); 1 page change (1 scene-reload); 11.8s video; 710 frames

    No new warnings. A contact sheet of the new video shows the CTA click, the typed email, the Join confirmation, the dashboard hover and the fade out, with no error or blank frames.

  • L4 measurement: crawling the 2,000-row fixture took 40.7s before and 2.5s after.

  • Real generate (DeepSeek deepseek-v4-pro, DOM-only, --env-file pointing at the local .env, --max-tokens 150000, --yes) against the demo app: exit 0, recipe valid after 1 attempt(s): 3 scenes, QC clean, framed 3 of 3 beats; 2 page changes (2 scene-reload); 14.3s video. Token usage line: ~58628 tokens metered against the budget (provider reported 31214; analyze 38632, script 19996): the gap is attempts that ended without a usage report and were charged their worst case, as M6 intends. Stage timings from the new report: crawl 1.0s, analyze 203.6s, script 260.9s, record 14.1s, render 4.7s; the director and render reports share run id d1fd0f7b-62c0-4784-a82f-a7f5b040415e. The key never appears in the log or the reports.

  • The full e2e run (98 tests) was on 777a1b5; the two follow-up fixes after it (08e787b, 3245273) were re-checked with the crawler e2e block (9 tests), the session e2e file (5 tests) and the unit suite.

  • CI: see the checks on head 0920914.

… the skill

The destructive lexicon missed most money and irreversible verbs (Buy,
Subscribe, Upgrade, Submit payment, Transfer, Merge, Deploy, Log out, Clear
all, Empty trash, Purge, Drop, Refund, Charge, Uninstall, Disconnect, Ban,
Leave team, Cancel order and more), and the agent skill listed a different
set than the generate filter used.

- src/security/destructive.ts is now the single policy: a term list plus
  the regex built from it. View-only phrases ("Clear all filters", "Reset
  view", "Drag and drop") are set aside before matching.
- "pay" covers Pay, Paying, Payment and Payout, but not the plural nouns
  "Payments" or the slug row "payments-worker", so the fleet dashboard keeps
  its six filmable rows.
- Policy change: Publish and every Transfer are now destructive. The old
  tests that pinned "Publish", "Transfer ticket" and friends as filmable
  move to the excluded side, as the review decision mandates.
- The crawl flags a text field whose form submits through a destructive
  control (its default submit button, an outside form= button, or its
  action URL). The script stage refuses `type` with `submit: true` on it, as
  a hard whitelist gate, and the inventory line warns the model.
- SKILL.md lists the exact terms between markers; a test compares the list
  with the code in both directions.
- README and SECURITY.md state the list and the residual risk of --yes runs.
  No new flag is required for `generate --yes`.
A click was a raw CDP press at the centre of a box read before up to 70% of
the slot of cursor travel. On a live page the target can move or be covered
in that time, and the press then landed on whatever took its place while the
take logged a clean click on the intended selector.

Immediately before the press, the recorder now hit-tests the press point:
the topmost element there (descending through open shadow roots) must be the
target or inside it in the composed tree; an iframe target passes when the
iframe is on top. On a miss it re-reads the target (scrolling it back into
view if needed), makes one short corrective cursor move, and checks again.
If the point is still wrong the scene fails with what is in the way, and
nothing is pressed. The click event is logged only after the check passes,
with the box and point that were actually pressed.

New e2e fixtures: a target pushed down mid travel with a decoy in its old
place (the decoy is never pressed), and an overlay that covers the target
(the scene fails and no click is logged). The capture, record, pixel and
health e2e suites pass unchanged.
… an editor

`type` cleared whatever had focus with select-all plus Backspace. In a
contentEditable editor that deletes the whole document, and when the click
did not focus the target the keys went to some other field.

Before any key is sent, the recorder now checks that the focused element
(through open shadow roots) is a text-entry element and is the target,
inside it, or the contentEditable host the editable target lives in.
Otherwise the scene fails and no keys are sent. Select-all plus Backspace
runs only when the focused element is the target itself and is an input or
textarea holding text; everywhere else the caret moves to the end and the
text is appended.

The action preview says "(replaces existing text)" for an input or textarea
target, "(appends to existing text)" otherwise, and names both cases when
the field's tag is unknown.

New e2e: a contentEditable editor keeps its content and gets the text
appended, a prefilled textarea is replaced while its neighbours are not
touched, and a target that swallows the focusing press gets no keys.
…s at render time

Each QC re-take performed every click, type and submit of the recipe
against the live app again, up to 4 times under one "y", and even a no-op
`patch: {}` or a hold-only verdict forced one. Patched recipes were never
re-validated, so holds could push past the 60s cap (L1).

- decideQc separates what a verdict changes. A cut needs a re-take. A hold
  or zoom patch becomes a render-time adjustment of the take as recorded:
  renderTake inserts a frozen stretch at frame cadence (frames and events
  shift together, so the health and skew gates stay quiet) and records the
  zoom as a QC focus on the right beat. The take on disk is not changed.
- `changed` is true only when a value differs: `patch: {}`, a hold equal to
  the recorded one, and a cut of a scene the recipe lacks change nothing.
  A shorter hold needs a re-record and is noted, not applied.
- Every patched recipe passes parseRecipe; holds that would break the 60s
  cap are dropped with a note (L1). The public applyVerdicts keeps that
  contract too.
- The loop lives in retakes.ts (filmWithRetakes) with injected record and
  QC, so the bound is unit tested without a browser: at most
  1 + MAX_RETAKES performances, whatever QC answers. MAX_RETAKES is now 1,
  since a re-take only drops scenes.
- The bound is stated where consent is given: confirmCapture receives
  maxPerformances and the TTY question says each action can run up to 2
  times. --yes runs log the same bound before capture.
- director-report.json records the take count and the render adjustments.
…site start pages

There was no session support, so an app behind a login filmed its sign-in
page, an off-origin redirect to an identity provider was crawled (its email
field became a type target), and analyze paid for up to 3 calls on a crawl
with nothing in it.

- `--storage-state <file>` on generate and record takes a Playwright
  storage state and applies it to the crawl context and every take's
  context. Only the path reaches the browser: the contents never reach a
  prompt, the take directory, director-report.json or a log line, and a bad
  file fails with an error that does not quote it. The dry-run and
  cancelled-capture follow-up commands carry the flag.
- The crawl refuses a start page that settles on another site, pointing at
  --storage-state; an http to https upgrade and an apex to www redirect of
  the same site are allowed (isSameSite, unit tested both ways). Later pages
  that settle off site are skipped, so the comment about off-origin
  redirects now matches the code.
- Before any LLM call, generate stops when the crawl found nothing to click
  or only a sign-in form (a password field and nothing but fields and
  sign-in links), naming --storage-state.
- record fails a scene whose entry page settles on another site: that is
  what an expired session looks like.
- README has a "Filming a signed-in app" section; SKILL.md tells the agent
  how to ask for a session and never to type real credentials.

The two preflight e2e tests served a page with no controls and expected the
run to die in analyze; that page now has one button so the run still gets
past the crawl and dies in analyze, which is what those tests measure.
…mpts

Element text was redacted before egress, but a selector built from that
text (`a:has-text("jane@customer.com")`) or an id holding a token travelled
raw. Selectors are validation keys the model must copy exactly, so they
cannot be redacted; an element or region whose selector redaction would
change is now left out of the inventory. The check also runs with CSS
escapes removed, since cssIdent writes an email id as `jane\@customer\.com`,
which a plain email pattern misses.

The crawl applies the filter, and the analyze and script prompts apply it
again at the egress point, so a hand-built digest is held to the same rule.
…k QC input as untrusted

A vision QC failure after capture (budget exhausted, 401, 5xx exhaustion, a
timeout) propagated and aborted the run after the whole crawl, LLM and
capture spend. visionVerdictsOrNone now catches any failure of the pass,
logs it, and the run renders the recorded take with the deterministic
verdicts only.

The QC prompt broke the untrusted-marker invariant: scene names are model
output derived from page text, and the retry feedback can echo them, yet
neither was wrapped and the system prompt did not declare the markers. The
system prompt now carries UNTRUSTED_RULES, every scene name and the retry
feedback travel between the per-run markers, and a verdict that echoes a
name with its markers still maps to its scene.

visionQc takes a frame-reader seam, so the unit test reaches the model
without ffmpeg (CI's unit job has none) and asserts what it was sent.
…oint one default

Any target, including an untrusted public one, could load private
subresources by default (the user's router admin, localhost services, cloud
metadata) into screenshots and the video (M4). The library defaults also
disagreed: generate() allowed private networks while record() and crawlApp()
refused them (L5).

The posture is now decided once per run from the target
(src/security/network-policy.ts), the same way in the CLI and in generate(),
record() and crawlApp() when the option is unset:

- a private or localhost target keeps its private requests, so a frontend
  on :3000 calling an API on :8000 still works;
- a target that resolves public, or cannot be resolved, gets the existing
  guard, so its pages cannot reach private addresses;
- --block-private-network stays the strict mode (refuses a private target);
- --allow-private-network, previously a deprecated no-op, is the explicit
  opt in that turns the guard off even for a public target. Passing both
  flags fails.

The "target origin only" rule from the review is not used: it breaks the
common local setup above. The printed record command carries an explicit
network flag. The record e2e that pinned record()'s old fail-closed library
default now pins the shared default (a local target films); the request-gate
e2e covers both sides of the default with the target classifier faked.
README, SECURITY.md and the AGENTS.md invariant describe the posture.
… model or page text

Typed text, scene names, selectors and excluded labels are model output
derived from page text, and JSON allows ESC, CR and LF in them. Printed raw,
a crafted value could erase or overwrite action preview lines, forge
harmless-looking ones, or emit OSC sequences: the preview is the human gate,
and it could be hidden from the human.

src/security/terminal.ts writes C0 and C1 controls, DEL, line and paragraph
separators and bidi marks as visible \uXXXX escapes. The preview quotes
typed text and scene names as escaped JSON literals and escapes selectors,
URLs and focus selectors; every generate log line passes through the same
filter; record's failure summary and the CLI's top-level error printer
escape too, keeping a message's own line breaks.
…at the ceiling enforces

The "hard" LLM ceiling had three gaps: any non-timeout fetch error was
assumed unbilled, the prompt estimate was chars/4 (a CJK page runs near one
token per character, so the pre-send refusal underestimated up to 4x), and
the 2000-token image constant is far below what some providers bill
(gpt-4o-mini reports about 36k prompt tokens for one frame).

- A connection failure is free only when it is known to precede sending
  (DNS, refused, unreachable, TLS); a reset or other dropped connection is
  charged the attempt's worst case. A 5xx is charged the worst case; a 429
  and other 4xx answers are not.
- estimateTokens counts ASCII at 4 characters per token and every other
  character as one token (two outside the BMP).
- BudgetedLlmClient learns the provider's real per-image cost from the
  reported prompt usage of a call with images and uses it for every later
  pre-send check, so the next vision call is refused when it no longer fits.
- README, SECURITY.md, AGENTS.md, the CLI help and the code comments now say
  what the code does: a ceiling checked before every attempt, metered at
  reported usage or worst case, where one call can exceed its estimate and
  the run then stops at the next call. It limits tokens, not money.
generate validated --music at preflight but not --bg, so a typo such as
`--bg cobaltt` burned the crawl, both LLM stages, the capture and QC, then
failed at render. assertBackground resolves the value the way the renderer
does (an image file, a bundled wallpaper, or a procedural palette) and
throws the renderer's own "unknown background palette" error up front.
…ave seeds with links

Route derivation treated any index.* under any folder named app, and any
.tsx under any pages folder, as a route: an Angular src/app/store/index.ts
became /store, a Remix app/routes/index.tsx became /routes, a Vite
src/pages/Dashboard.tsx became /Dashboard. Next.js @slot and (.) segments
were kept. The bogus seeds were queued before every link-discovered page,
so they could fill the whole crawl, and the prompt called them the real
pages of the product.

- Routes are read only under a Next.js app root (a next.config.* or a
  package.json depending on next, among the walked files or at the repo
  path and its nearest parents), and only from its app/, src/app/, pages/
  and src/pages/ router directories. Other frameworks yield no routes.
- App router: only page.* makes a route; a parallel @slot adds no segment;
  intercepting (.)x segments and _private folders yield none. Pages router:
  _app, _document, _error, 404, 500 and api/ are not pages.
- The crawl takes the start page, then seeds and link-discovered pages in
  turn (CrawlQueue), so neither can crowd the other out of the page cap.

The existing route fixtures model Next apps but had no Next marker; they now
carry a package.json that depends on next.
…able

tr[id], li[id] and [data-testid] candidates were judged by their full
innerText, which includes their nested action buttons: a row "Acme  Edit
Delete" was excluded, so an admin table with per-row Delete lost every row.

A candidate that contains interactive descendants is now judged by its own
text with those descendants left out; the nested buttons are candidates of
their own and are judged separately (the Delete buttons stay out, Edit
stays in). One case stays excluded: a container whose centre, where the
recorder presses, is a nested destructive control, since pressing the row
would press it. "Reset view" was a false positive too; the view-only
phrases of the shared policy keep it filmable.

New e2e fixture: a customers table with Edit and Delete per row, a "Reset
view" camera control, and a card that is all Delete button.
package.json allowed ^1.53.0 while the lockfile locked 1.60.0, so users
installing with npx got the newest 1.x, a version CI never ran. Capture
depends on CDP screencast timing, routeWebSocket and route.fetch, and each
Playwright version needs its own browser download. The dependency is now
the exact lockfile version, and a test keeps package.json and the lockfile
on the same exact pin. doctor's install hint runs the CLI of the Playwright
supercut resolves, so it installs the matching browser unchanged.
… tagged commit

The release workflow used a long-lived NPM_TOKEN although id-token: write
was already granted, ran no e2e tests, had no environment gate, and any v*
tag published, even on a commit that never passed CI. The CHANGELOG claimed
"a release workflow with npm provenance", but 0.1.0 was published by hand
and has no provenance.

- ci.yml is also a reusable workflow; release.yml calls it on the tagged
  commit (typecheck and unit tests on every Node in the matrix, the browser
  e2e suite, the packed-install smoke test), and publish needs it.
- The publish job uses npm trusted publishing: no token, id-token: write,
  Node 24, and npm 11.6.2 pinned for the publish (npm's documentation
  requires npm CLI 11.5.1+ and Node 22.14.0+). Provenance is attached
  automatically for a public repository and package. No dependency cache in
  the release build, as npm's docs advise.
- The publish job runs in an `npm-publish` environment, so required
  reviewers can be added in the repository settings.
- A version already on npm still skips CI and publish instead of failing.
- The CHANGELOG 0.1.0 entry no longer claims provenance; CONTRIBUTING
  describes the release path.

Nothing was published, deprecated, or changed on npm or GitHub. The trusted
publisher must be configured on npmjs.com before the next tag.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-10T02:19:52.190175Z d5c4108 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

…t prefixed secret keys

repoNotes read README.md and package.json with existsSync and readFileSync,
which follow symlinks, while the source walk skips symlinks precisely to keep
~/.ssh out of prompts. A hostile repo's README.md symlinked to
~/.aws/credentials shipped aws_secret_access_key to the provider, and the
redaction rule missed it because \b never fires after the underscore in
"aws_secret".

repoNotes now reads only a regular file (lstat), and SECRET_ASSIGNMENT
matches a key name with a prefix or suffix around api_key, access_key,
token, password, passwd, secret or bearer (aws_secret_access_key,
DB_PASSWORD, github_token).
The crawl ran at 1280x800 and capture at 1920x1080, so hidden flags,
region sizes (MIN_AREA assumed 1280x800) and responsive layouts were judged
on a layout that was never filmed: a control shown only on wide screens was
"hidden" to the director, and one shown only on narrow screens looked
clickable. Both now use CAPTURE_VIEWPORT (src/capture/viewport.ts), and the
region size floor derives from it.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d5c41085eb

ℹ️ 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".

Comment on lines +34 to +35
return priv
? { allowPrivateNetwork: true, reason: "private-target" }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep the guard enabled for mixed DNS answers

When a hostname resolves to both public and private addresses, urlResolvesPrivate() returns true because it uses some(...), and this branch consequently disables the request guard for the entire run. Such a target still resolves publicly, so an attacker-controlled mixed-answer hostname can cause its page to access localhost, router, or metadata endpoints without --allow-private-network; only targets whose complete address set is private should take this branch.

AGENTS.md reference: AGENTS.md:L58-L58

Useful? React with 👍 / 👎.

Comment on lines 62 to 65
- uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
with:
node-version: 22
cache: npm
node-version: 24
registry-url: https://registry.npmjs.org

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Remove setup-node's dummy token before OIDC publishing

In the tagged-release publish job, this registry-url input is handled by the pinned setup-node v4 action by writing _authToken=${NODE_AUTH_TOKEN} and exporting a dummy token when none exists, as shown in the pinned action source. Because this workflow deliberately removed NODE_AUTH_TOKEN, npm 11 treats the generated dummy credential as traditional authentication instead of initiating trusted publishing, a failure reproduced upstream as ENEEDAUTH/E404 in actions/setup-node#1551; tagged releases therefore cannot publish until the auth line is removed or setup-node is upgraded to an OIDC-compatible version.

Useful? React with 👍 / 👎.

…e lookup

Disambiguating a selector shared by many elements probed the bounding box
of every match, k = 1 up to the match count, with no cap: on a 2,000-row
table each inventoried row cost 2,000 CDP round trips (a crawl of the
fixture page took 40.7s). The element's index among the selector's matches
is now read by identity with one evaluateAll call, which is also exact where
the box-distance guess could pick a neighbour. The same page now crawls in
well under the 8s the new e2e test allows.
… --dry-run does

The help said --dry-run stops "before touching the app", but the crawl had
already GET-navigated same-origin links, and an app that mutates on GET
(/logout, /history/clear-all) was changed by it. Links whose label is
destructive were already left out of the inventory (and so never
followed); now a link whose path reads as destructive is never requested
either, whatever its label. The help text and SECURITY.md say what a dry
run does: it crawls with plain page loads, then stops before any click or
typing.
…sponse_format; vision opt-in for custom

max_tokens and response_format were always sent. OpenAI's reasoning models
reject max_tokens with a 400 that was treated as final, and its reason was
hidden unless SUPERCUT_VERBOSE was set. Vision also defaulted on for custom
providers, so a text-only local model failed mid-run.

- A 400 that refuses max_tokens switches the client to
  max_completion_tokens for the rest of the run and asks again; a 400 about
  response_format drops JSON mode (the prompts already ask for JSON only).
- A rejection prints the provider's own error message (the error.message
  field), shortened, redacted and terminal-escaped; the raw body is still
  shown only with SUPERCUT_VERBOSE.
- Vision defaults on only for OpenRouter; a custom endpoint opts in with
  SUPERCUT_VISION=true. README and .env.example say so.
…ifts the ceiling

`generate` reads .env from the current directory, usually the app being
filmed, and a cloned repo can ship one that sets SUPERCUT_PROVIDER=custom
and a base URL (sending crawled app content to another endpoint) or
SUPERCUT_MAX_TOKENS=off. Only the summary line's host hinted at it.

loadDotEnv now reports the variables a file supplied, and dotEnvWarnings
turns an implicit file's endpoint redirect or disabled ceiling into a
warning naming the file, the variable and the endpoint's host, never a
value that can be a secret. A file named with --env-file is the user's
choice and gets no warning. SECURITY.md describes it.
…output to supercut-out/

Artifacts were never cleaned: every take of a run stayed on disk (4K JPEG
frames each), an interrupted render left supercut-<token>.h264 in tmpdir,
and the default out/ is the directory a Next.js static export owns, so a run
inside a Next app could be wiped by the next build or shipped with it.

- After a successful render, generate removes the takes a re-take
  replaced; the rendered take, recipe and reports stay.
- removeOnExit (src/render/temp.ts) deletes the render's temporary stream
  on process.exit, SIGINT or SIGTERM, then lets the signal end the process.
- The CLI defaults are supercut-out/take, supercut-out/final.mp4 and
  supercut-out/generate. README, CONTRIBUTING, SKILL.md and SECURITY.md
  use supercut-out/ and say to add it to .gitignore.

New tests: an interrupted child process loses its temp file and still dies
by the signal, and a generate e2e that re-films once after a QC cut renders
take-1 and removes take-0.
…ond cap

The 60s estimate used duration_ms only, while the recorder types at 60 to
100ms per key and overruns a short slot (the schedule shifts), so long typed
text made videos longer than the estimate promised, and nothing bounded it.

A type action now counts as max(duration_ms, the least time its typing
takes): the focusing press and the beat before the first key (820ms), 60ms
per grapheme after the first (the recorder's fastest mean pace, pinned by a
test against cursor.ts), and 350ms before Enter with submit. Typed text is
capped at 500 characters. SKILL.md states the rule, and a docs test keeps
its numbers equal to the schema's.
Node 20 reached end of life on 2026-04-30 and the matrix [20, 22] skipped
the current LTS. The unit job now runs on 20, 22 and 24. engines stays
>=20: nothing in the code needs a newer Node, and dropping 20 would be a
breaking change for no gain.
…historical

SKILL.md still said @co-messi/supercut was "not published yet"; it is on
npm, so the skill uses the npx form and keeps the source checkout only as an
option. spikes/RESULTS.md recommends PNG capture and calls JPEG too slow,
the opposite of production (JPEG q92 at 2x DPR with the repaint beacon); it
now opens with a note that it is a historical record. Docs tests keep both
from regressing.

The model defaults (deepseek-v4-pro, anthropic/claude-sonnet-4.6) were
checked against the providers' published lists on 2026-10-10: both exist
(DeepSeek's pricing page, OpenRouter's /api/v1/models), so they are not
changed.
…nc copies

tools/check-pack.mjs was a denylist, so an iCloud conflict copy such as
dist/cli/index 2.js or a node 2/ folder would ship in a hand publish
without a word. The check is now an allowlist of what the package ships
(package.json, README.md, LICENSE, dist JS and declarations, the bundled
backgrounds and music, the demo app and its recipe), a path that looks like
a " <n>" sync duplicate fails with its own message, and the entry points
and size ceiling are enforced as before. The logic is an exported function
the new unit test drives; running the script still checks the real
npm pack list.
… wall time

The L4 e2e test added in this branch asserted an absolute 8s bound; the
shared CI runners took 8.6s to 10.9s for the same crawl that takes 2.5s
locally. It now crawls the same page with 20 rows and with 2,000 rows on
the same machine and requires the long one to stay under 4x the short one
plus 2s. The behaviour it guards against (one probe per row) cost over 15x.
… run id, Dependabot)

- The private-network policy judges SIIT IPv4-translated addresses
  (::ffff:0:0/96) by their IPv4, and treats the local-use NAT64 prefix
  64:ff9b:1::/48 as private. SECURITY.md lists both and says Teredo is not
  decoded.
- The LLM client honours a Retry-After header (seconds or an HTTP date,
  capped at 60s) on a retryable answer instead of the linear backoff.
- director-report.json carries a run id and the wall time of each stage
  (crawl, analyze, script, record, qc, render); render-report.json carries
  the same run id, so a report pair can be matched.
- Dependabot opens weekly update PRs for npm dependencies and the
  SHA-pinned GitHub Actions.
…w-the-fold cards stay excluded

The M9 container check found the control at a container's centre with
elementFromPoint, which sees nothing outside the viewport. The crawl does
not scroll, so a card below 1080px whose centre is its Delete button
("Danger zone: Delete workspace") passed the check, and the recorder,
which scrolls the card into view and presses its centre, would press that
button. The centre control is now found by geometry (every nested
interactive descendant whose box contains the centre), which works off
screen. The CRUD fixture gains such a card below a 2000px spacer.
The L2 rule that redacts prefixed key names (aws_secret_access_key) also
feeds pageUrlHasSecret and selectorLeaks, so harmless URLs such as
?password_reset=1, ?pageToken=abc and ?show_secrets=0 were refused as
credentials. A bare key name (password, token, api_key, secret, bearer)
keeps the old rule, any value redacted; a prefixed name needs a value of 8
or more characters, so flags and short cursors pass while
access_token=<long> and aws_secret_access_key=<key> are still redacted.
AGENTS.md asks for comments that state present-tense invariants (history
belongs in commit messages) and for prose without em or en dashes. This
commit changes no behaviour: punctuation in comments, log and error
messages and prompt text, plus comment and test-name wording.

- Every em and en dash in src, test, ci.yml, .env.example and
  spikes/RESULTS.md becomes a comma (a numeric range becomes "a to b"; a
  dash before closing punctuation is dropped). Two tests that assert "no
  dashes" now spell the characters as Unicode escapes (U+2013 and U+2014).
- Review tags (A3, A5, H1, H-new-1, M-new-2, B4 (review), trust-review,
  review PoC, the reviewer's repro) are gone from comments and test names.
- History comments are restated as invariants: the rebinding window in
  url-policy.ts, the navigation-only gate, the all-cut marker in qc.ts, the
  dead-air hold patch, the theme probe's tie-break, and the test comments
  that described what code used to do (cli-exit, config, director,
  generate, prompt-egress, source-routes).

The 30 local *.bak leftovers are untouched.
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.

1 participant