fix(producer): pin lint entry reads to checked descriptors - #3734
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
The input boundary is correct at this head.
packages/producer/src/services/hyperframeLint.ts:46-69 opens each candidate read-only/nonblocking, checks and reads that descriptor, and closes on regular, nonregular, stat-failure, and read-failure paths. :85-111 preserves preferred → index → src/index order and uses html !== null, so an empty preferred file remains authoritative. The relative/separator/absolute containment test closes the sibling-prefix escape on POSIX and Windows without changing the documented project/entry symlink behavior.
hyperframeLint.file-race.test.ts:89-164 is substantive: all three candidate replacements, every fallback class, FIFO, project/entry links, the sibling escape, prior errors, descriptor cleanup, and inline/files payload precedence are pinned. The workflow addition runs this exact focused file in the existing Windows lane, and classification places it in producer integration. I independently ran 15/15 regressions, classification, producer typecheck, and changed-file lint/format; all pass. Exact-head CI has no failures, with Build/integration/Windows/regressions and JavaScript CodeQL still running and remaining landing gates. The disclosed audioPadTrim LUFS failure is unrelated to these four files and is not present in the focused path.
Scope note: containment remains intentionally lexical—an in-project entry symlink may resolve outside the project, and the test explicitly preserves that contract. The sibling-prefix fix should not be described later as a realpath sandbox.
Verdict: APPROVE
Reasoning: Every filesystem candidate now binds validation and bytes to one descriptor, the prefix escape is closed cross-platform, and tests preserve the selection, fallback, symlink, payload, and error contracts.
— Magi
|
Windows exposed a fixture portability issue: renaming a replacement over the open target returned EPERM in all three replacement cases. Revised in 0e27234: move the checked file to a fresh name, then write replacement bytes at its previous pathname, matching the existing file-server and Studio descriptor-race fixtures that passed Windows. Production code is unchanged. All 15 focused tests and signed hooks pass locally, and the revised three replacement witnesses still fail against the main reader. No assertions are weakened or skipped; only the existing POSIX FIFO case remains Windows-skipped. Fresh Windows/CI/CodeQL and exact-head re-review are required before merge. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at 0e27234984779dcaa8c5f73ca550049685fda8c2: the delta from my approved head is exactly one test file, +4/−3; production code and assertions are unchanged.
hyperframeLint.file-race.test.ts:89-99 now models replacement portably: after the real fstat, it renames the checked inode to a fresh path and writes different bytes at the original pathname. The checked descriptor still reads the moved inode. The prior implementation's readFileSync(absoluteEntryPath) instead reads the recreated pathname, so all three preferred/index/src-index witnesses remain non-vacuous; no skip or expectation was weakened.
I reran the focused suite (15/15) and the changed test's lint/format checks; all pass. Exact-head check-runs currently have no failures. Fresh Windows, full CI, and JavaScript CodeQL remain landing gates.
Verdict: APPROVE
Reasoning: The test-only revision removes Windows' replace-over-open operation while preserving the exact descriptor-versus-path identity distinction the regression is meant to prove.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at 0e27234984779dcaa8c5f73ca550049685fda8c2: the delta from my approved head is exactly one test file, +4/−3; production code and assertions are unchanged.
hyperframeLint.file-race.test.ts:89-99 now models replacement portably: after the real fstat, it renames the checked inode to a fresh path and writes different bytes at the original pathname. The checked descriptor still reads the moved inode. The prior implementation's readFileSync(absoluteEntryPath) instead reads the recreated pathname, so all three preferred/index/src-index witnesses remain non-vacuous; no skip or expectation was weakened.
I reran the focused suite (15/15) and the changed test's lint/format checks; all pass. Exact-head check-runs currently have no failures. Fresh Windows, full CI, and JavaScript CodeQL remain landing gates.
Verdict: APPROVE
Reasoning: The test-only revision removes Windows' replace-over-open operation while preserving the exact descriptor-versus-path identity distinction the regression is meant to prove.
— Magi
The producer's project-directory lint input checked an HTML pathname and then reopened that pathname to read it. A replacement between those operations could make lint consume a different file. The reader now opens read-only/nonblocking, checks and reads the same descriptor, and closes it on every path. This addresses CodeQL #281.
The same input boundary now rejects sibling directories sharing the project's name prefix. Preferred/index/src-index ordering, empty HTML, project and entry symlinks, inline/files payloads, and existing error responses remain supported. Missing and nonregular candidates still fall through; nonblocking opens keep named pipes from hanging. The new regression file is registered in the producer integration lane and runs explicitly in the existing Windows lane.
Validation: 15 new cases (three replacement witnesses plus the sibling escape fail on main), 26 focused input/API tests, 649 Bun unit tests, all 548 lint-package tests, producer typecheck, dependency builds, classification, lint/format and signed hooks pass. The broader producer Vitest unit lane passed 644 assertions with one pre-existing local audio loudness failure:
audioPadTrim.integration.test.tsmeasured a 3.9 LUFS delta against a limit of 3. It fails identically with the unchanged main lint reader; this PR does not touch the audio path. Required CI, Windows and CodeQL remain merge gates.