[kixelated]

Summary

The test-inventory gate now checks JS bench scripts. Any JS bench entry point that no workflow runs gets flagged, the same way an unreached test script is. This finishes quest/m2/js-bench-inventory.md, so the PR deletes that quest and its link in quest/m2/README.md.

• sh/gh/test-inventory.py:

  • New target kind. A JS bench entry point is a *.ts/*.mts/*.js/*.mjs file under a bench/ directory that no other non-test JS/TS source imports or names. "Imports" covers a relative import with or without its extension, including a dynamic import() or a re-export. "Names" covers a mention like a Bun.build entrypoint string. Block comments are stripped first, because ring-page.ts's doc comment names ring.ts. An entry point with no reachable caller is reported as script <path>.
  • The /bench/ exemption is gone. drives_browser no longer skips /bench/, and runs_tests counts a bench entry point the way it counts a test script. So a recipe that runs an unreached bench is flagged too, the same as a recipe that runs an unreached *.test.py.

• sh/gh/test-inventory.test.py gets a new test that copies Nightly's pattern: a for loop of literal names calls javascript "$benchmark" just js bench "$benchmark" through a helper that runs "$@", and the recipe uses {{ name }}. The test covers:

  • an unreached entry point, and the recipe that runs it;
  • modules imported with and without an extension;
  • a bundled page entry;
  • a doc comment naming the bench that loads the file;
  • a test that imports its subject;
  • removing a name from the loop, or removing the browser bench's call, orphans its script.

  The test fails against main's checker.

Public API: none. Wire: none.

Choices made without asking

• No Nightly change. The Plan asked for a bench list the checker can read: "a static list Nightly loops over, or a recipe per bench". sh/gh/nightly.sh already has one. The checker (as of moq-dev/moq#7316) expands the literal for benchmark in … loop and binds each name to js::bench's name parameter. It reads the just words at the call site, so the javascript helper's "$@" never needs resolving. On main, the walk already reaches every entry point. Nothing checked whether a bench script had a caller, and that was the gap.
• How an entry point is found. A bench file counts as an entry point if no other source imports or names it. This needs no list of entry points and no naming convention. It currently finds the 33 js/*/bench entry points plus bench/browser's build.ts, driver.ts, and micro.ts. It leaves out helpers (compression-fanout.ts, fixtures/*, ring-page.ts, serve-browser-entry.ts, qmux-{eager,lazy}.ts, bench/browser/page/*). A *.test.ts that imports its subject (serve-browser.test.ts) does not turn the subject into a module.
• Scope. The check covers every bench/ directory, not only js/*/bench. That brings in the root bench/browser harness, whose three entry points are already reached through bench/browser/run.sh.
• Comments are stripped before scanning. Block comments go first, then // line comments, so a doc comment naming the bench that loads a file, or a commented-out import, does not turn its target into a module.
• resume-history.ts stays a module of bench/browser/micro.ts, not an allowlist entry. moq-dev/moq#7514 merged js/net/bench/resume-history.ts. It runs only in Chromium, through bench/browser/run.sh's for benchmark in frames drift ledger resume-history loop into micro.ts. micro.ts built every entry except frames from a template path, so the checker could not see it and flagged resume-history.ts as having no CI caller. An allowlist entry would be false, since the allowlist is for targets with no CI caller, and the bench has one. Instead, micro.ts now names each entry literally in a benchmarks table, the same way it already named frames.ts. Since then main has added bandwidth and replay to the harness, and the table names them too. That makes drift.ts, ledger.ts, bandwidth.ts, replay.ts, and resume-history.ts modules of micro.ts, whose own call from run.sh is checked. resume-history.ts's error string named bench/browser/micro.ts, which made micro.ts look like a module, so nothing checked micro.ts either. The message now says bench/browser instead. It keeps the asynchronous gc that main adopted.
• Report key. Entry points reuse the script <path> key that test scripts use, so allowlist entries look the same for both.

Checks

On 94c4fd2442 (origin/main bf5e2b0b9b merged in):

• Inside nix develop via quest gate: just gh check (which includes python3 sh/gh/test-inventory.py on the real tree) and just gh test pass, and the checker's 16 tests pass. ruff format --check sh/gh and ruff check sh/gh are clean. bun biome check passes on the touched TS, as does tsc --noEmit for js/net/bench and bench/browser.
• quest check --base origin/main passes.
• Negative probe on the real tree (from an earlier head): replacing run.sh's bun micro.ts call flags script bench/browser/micro.ts.

Merge order

• moq-dev/moq#7440 (in the merge queue) adds quest/m2/test-inventory-followups.md. Its Plan bullet and ## Related entry both link to /quest/m2/js-bench-inventory.md, which this PR deletes. Whichever PR lands second drops that bullet and the ## Related section. Both are obsolete once this PR lands, because the bullet only hands JS bench scripts to this quest. quest check fails on the merge result until then.

Follow-ups

• Several open PRs add JS benches and Nightly entries in parallel. After this merges, any of them that adds a bench script without a CI caller will fail just gh check until a call is wired in. That is the intended effect.

Review

Codex (quest review), gpt-6.1-sol, medium effort, on head 94c4fd2442e8638efcf56dc83bd2e1e96b313147 against origin/main bf5e2b0b9b: round 1 (new session) reported no findings.

Automated findings since the last record:

• OpenAI review, P2 "Ignore line comments before collecting imports" (review, restated in review): fixed in a01b87680b, with a regression case (reply).
• Land fixer's resume-history.ts has no CI caller: fixed earlier by the literal benchmarks table in micro.ts. The Ruff formatting failure is fixed too.

Earlier rounds: no findings on e4f7491ffb and 4f7f15c657.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 22 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 48dc24c2-8da5-44bc-af7b-a4efcdde58fa

Commits

Reviewing files that changed from the base of the PR and between bf5e2b0 and 94c4fd2.

Files selected for processing: 6

• bench/browser/micro.ts
• js/net/bench/resume-history.ts
• quest/m2/README.md
• quest/m2/js-bench-inventory.md
• sh/gh/test-inventory.py
• sh/gh/test-inventory.test.py

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 4f7f15c65719fb09a63f2d5235e9f4464156de52.

• P2: Ignore line comments before collecting imports (sh/gh/test-inventory.py:904–910). graph.scripts() strips // comments internally, but IMPORT.findall(text) receives the original text. For example, a non-test sibling containing // import "./forward"; adds forward.ts to named, so an otherwise uncalled bench/forward.ts disappears from the inventory. Commenting out its only import can therefore leave the benchmark unrun without the gate noticing. Apply the same comment filtering before both scans and cover a commented-out import in the new negative test.

Overall direction is sound: reuse Nightly's existing literal loop and recipe binding rather than adding a second bench list or shell-indirection machinery. No public API or wire changes. Static review of the checker, tests, and calling recipes only; I did not run tests or the real-tree negative probes.

(Written by OpenAI)

[pullfrog]

✅ No new issues found.

Reviewed changes

• sh/gh/test-inventory.py — JS bench entry points. Adds JS/IMPORT, bench_entries(root, graph) (a /bench/ JS/TS file no other non-test source imports or names), drops the /bench/ exemption from drives_browser, threads a benches set through runs_tests, and reports each unreached entry as script <path>.
• sh/gh/test-inventory.py — comments/docstring. Block comments are stripped before scanning so a doc comment naming its loader does not turn a page into a module; the module docstring gains the new target kind.
• sh/gh/test-inventory.test.py — new test. Mirrors Nightly's for-loop plus javascript-helper "$@" shape; covers module vs entry, extension-less specifiers, a bundled page entry, a doc comment naming the bench that loads it, a test importing its subject, and orphaning by dropping a loop name or the browser-bench call.
• quest/m2/. Deletes the now-finished js-bench-inventory.md and its README.md link; no dangling references remain.

I verified bench_entries against the real tree standalone: 37 entries (34 under js/*/bench, including serve-browser.ts, plus bench/browser/{build,driver,micro}.ts), and every file it excludes is a genuine module, helper, or bundled page. js/net/bench/frames.ts is excluded because bench/browser/micro.ts names its path, which is also how it is reached, so there is no coverage gap there.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[moq-bot]

codex / Codex land fixer note:

The batch log identifies this PR's files: lines 40–41 say "unformatted: File would be reformatted" and "--> sh/gh/test-inventory.py:960:16"; lines 78–79 identify sh/gh/test-inventory.test.py; line 124 says "2 files would be reformatted, 161 files already formatted".

Merged base without conflicts and committed only Ruff formatting as 01c6ba56be3545c03b1044cf94984a352fe69f24 on pr-7541. Repository-wide ruff check --force-exclude and ruff format --check --force-exclude passed. With the locked PyYAML dependency available, python3 -m py_compile sh/gh/test-inventory.py sh/gh/test-inventory.test.py and python3 sh/gh/test-inventory.test.py passed (11 tests); quest check passed.

However, python3 sh/gh/test-inventory.py exits 1 with "script js/net/bench/resume-history.ts has no CI caller (js/net/bench/resume-history.ts)". The benchmark is already present on base. Selecting its CI coverage or an exception requires review beyond a mechanical formatting fix.

(written by codex / Codex)

[moq-bot]

codex / Codex land fixer: The logged Python formatting failure is corrected, but python3 sh/gh/test-inventory.py exits 1: script js/net/bench/resume-history.ts has no CI caller (js/net/bench/resume-history.ts); that script alr. Recommendation: Choose a CI caller or a reviewed inventory exception for the existing resume-history benchmark, then rerun the inventory check and its unit tests.

unformatted: File would be reformatted
   --> sh/gh/test-inventory.py:960:16
    |
959 |     script it tests does not make that script a module."""
    -     sources = [path for path in tracked(root, *(f"*{suffix}" for suffix in JS))
    -                if not is_test_script(str(path.relative_to(root)))]
960 +     sources = [
961 +         path
962 +         for path in tracked(root, *(f"*{suffix}" for suffix in JS))
963 +         if not is_test_script(str(path.relative_to(root)))
964 +     ]
965 |     named = set()
--------------------------------------------------------------------------------
971 |             base = (source.parent / specifier).resolve()
    -             named.update(path for path in (base, *(base.with_name(base.name + suffix) for suffix in JS),
    -                                            *(base.with_suffix(suffix) for suffix in JS),
    -                                            *(base / f"index{suffix}" for suffix in JS)) if path.is_file())
    -     return {path.resolve() for path in sources
    -             if "/bench/" in f"/{path.relative_to(root)}" and not path.name.endswith(".d.ts")
    -             and path.resolve() not in named}
972 +             named.update(
973 +                 path
974 +                 for path in (
975 +                     base,
976 +                     *(base.with_name(base.name + suffix) for suffix in JS),
977 +                     *(base.with_suffix(suffix) for suffix in JS),
978 +                     *(base / f"index{suffix}" for suffix in JS),
979 +                 )
980 +                 if path.is_file()
981 +             )
982 +     return {
983 +         path.resolve()
984 +         for path in sources
985 +         if "/bench/" in f"/{path.relative_to(root)}" and not path.name.endswith(".d.ts") and path.resolve() not in named
986 +     }
987 |
    |

unformatted: File would be reformatted
   --> sh/gh/test-inventory.test.py:281:20
    |
280 |         # Nightly's shape: a loop's literal names reach a recipe parameter through a helper's "$@".
    -         self.write("sh/nightly.sh", """
281 +         self.write(
282 +             "sh/nightly.sh",
283 +             """
284 |             #!/usr/bin/env bash
--------------------------------------------------------------------------------
293 |             javascript page just js page-bench
    -         """)
    -         self.write("js/justfile", """
294 +         """,
295 +         )
296 +         self.write(
297 +             "js/justfile",
298 +             """
299 |             bench name:
--------------------------------------------------------------------------------
306 |                 bun net/bench/gc.ts
    -         """)
307 +         """,
308 +         )
309 |         self.write("justfile", JUSTFILE.replace("    mod rs\n", "    mod js\n    mod rs\n") + "        sh/nightly.sh\n")
--------------------------------------------------------------------------------
317 |         self.write("js/net/bench/gc.ts", "")
    -         self.write("js/watch/bench/page.ts", 'import { chromium } from "playwright";\n'
    -                    'await Bun.build({ entrypoints: [`${import.meta.dir}/page-entry.ts`] });\n')
318 +         self.write(
319 +             "js/watch/bench/page.ts",
320 +             'import { chromium } from "playwright";\n'
321 +             "await Bun.build({ entrypoints: [`${import.meta.dir}/page-entry.ts`] });\n",
322 +         )
323 |         self.write("js/watch/bench/page-entry.ts", "")
--------------------------------------------------------------------------------
334 |         nightly = (self.root / "sh/nightly.sh").read_text()
    -         self.write("sh/nightly.sh", nightly.replace("in fills;", "in forward;")
    -                    .replace("javascript page just js page-bench\n", ""))
335 +         self.write(
336 +             "sh/nightly.sh",
337 +             nightly.replace("in fills;", "in forward;").replace("javascript page just js page-bench\n", ""),
338 +         )
339 |         orphans = self.orphans()
    |

2 files would be reformatted, 161 files already formatted

(written by codex / Codex)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: b859eb6cfdda8fbf7c03a1180cd488b0df241ab3, compared with the last reviewed 4f7f15c65719fb09a63f2d5235e9f4464156de52, separating upstream merges and formatting from the browser-harness change.

No new actionable bugs found in this update. The explicit entry/row table preserves the variant-specific bundling paths while making the existing browser benchmarks visible to the inventory; this is preferable to an inaccurate orphan allowlist entry.

Prior finding still open: P2: ignore line comments before collecting imports, now at sh/gh/test-inventory.py:968–970. IMPORT.findall(text) still scans line comments. Apply comment filtering before that scan and add the commented-out-import regression case; this is unchanged, not a new finding.

Static inspection of the substantive delta, checker and browser caller only. I did not run tests, Chromium benchmarks, or the real-tree negative probes.

[pullfrog]

OpenRouter returned an error, so this run stopped. If it names your key, plan or balance, fix that with the provider or pick another model.

Internal Server Error: {"error":{"message":"Internal Server Error","code":500}}

Pick another model →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS)}

[kixelated]

Fixed in a01b876: bench_entries now strips // line comments (after block comments) before both the graph.scripts and IMPORT scans, at sh/gh/test-inventory.py:991-994. test_a_js_bench_entry_point_needs_a_caller now gives fills.ts a trailing // import "./forward"; and a whole-line // import "./gc";, and still expects both forward.ts and gc.ts to be orphans. That case failed before the fix and passes after it.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 94c4fd2

The earlier comment-scanning finding remains partially open (sh/gh/test-inventory.py:994–996). A valid line such as run();// import "./forward"; bypasses the filter because // follows a semicolon rather than whitespace. The commented import can still hide an unrun benchmark from inventory. Recognize actual JavaScript line comments regardless of the preceding token while preserving strings and URLs, and add this no-whitespace case. The new regression fixes the whitespace-delimited case.

The added coverage is a useful step toward reliable inventory. Static GitHub review only; I did not run tests.

[pullfrog]

✅ No new issues found.

Reviewed changes

• Closed the commented-out-import gap. bench_entries now strips // comments after block comments and before both the script-name and IMPORT scans, so a commented-out import no longer makes its target a module; test_a_js_bench_entry_point_needs_a_caller gained the fills.ts regression case that fails without it.
• Named micro.ts's bench entries literally. bench/browser/micro.ts replaces the ROWS map and the templated js/net/bench/${benchmark}.ts path with a benchmarks table of full entry paths plus row counts, so the inventory sees the harness name each fixture; entry resolution and the per-benchmark row assertion are otherwise unchanged.
• Trimmed the resume-history.ts error message so it no longer spells bench/browser/micro.ts, which had made the harness look like a module and hidden it from the checker.
• Removed the finished quest (quest/m2/js-bench-inventory.md and its README link); no dangling references remain.

I re-ran bench_entries standalone against the real tree: 37 entries, with resume-history.ts, drift.ts, frames.ts, ledger.ts, bandwidth.ts, and replay.ts now named as modules of micro.ts, while micro.ts and serve-browser.ts stay entry points reached from bench/browser/run.sh and Nightly.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}