Skip to content

fjs/web: file an issue for the missing-index message - #1714

Merged
sergey-shandar merged 18 commits into
mainfrom
msg
Aug 26, 2026
Merged

fjs/web: file an issue for the missing-index message#1714
sergey-shandar merged 18 commits into
mainfrom
msg

Conversation

@sergey-shandar

@sergey-shandar sergey-shandar commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Files two issues for fjs web. The first is the one that prompted it: / on a root with no index.html says not found, which describes the file the server looked for rather than the request the client made. The second is a disclosure found while checking the first one's premise.

missing-index-message.md

  • Reachable from a clean checkout. index.html is a build artifact and is gitignored, so fjs web in a fresh tree answers not found at / until npm run index-html has run, and nothing in the response points at that.
  • The proposed sentence is a function of the URL, not the file system. A directory-form request is one the server answered by appending index.html, so no index.html in /fjs/ discloses nothing the client did not already send. Two constraints go with it: name the URL path rather than the resolved one — fileResponse already prefers errorSummary to errorMessage so a client is not handed the server's filesystem layout — and keep /fjs/ and /no-such-dir/ answering identically, since telling them apart needs a second stat and turns the response into a directory-existence oracle.
  • Echo the percent-encoded spelling, never percentDecode's output. Nothing on the way in excludes a control character, so /%1B%5B31m/ decodes to a real ANSI escape — and a body is not only read by browsers: curl and wget write it to a terminal, which acts on the sequence rather than displaying it. text/plain and nosniff bound what a browser does and say nothing about that. The encoded form cannot carry the problem, because a raw control byte never reaches this module: Node's parser answers 400 Bad Request on the request line before the listener runs, so such a character exists in the target only as the printable text %1B. That needs no escaping pass and no list of dangerous characters. Nothing echoes anything today, so this is a property to build in rather than a live bug.
  • Directory-form is not "ends in a slash". resolve asks segments.length === 0 || decoded.endsWith('/'), so /., /%2E, /a/.. and /fjs/.. qualify without ending in one; each answers 200 against a root with an index. A hidden path is the other way round: /.git/ is refused before isDirectory is computed, and keeps not found deliberately — the new sentence claims the server looked, and for a hidden path it refused without touching the disk.
  • The obstacle is that the isDirectory fact is already gone. resolve computes it and returns Result<string, Refusal> — a path and nothing else — so respond cannot tell an appended index.html from a requested one, and / and /index.html resolve to the identical string. Three options are laid out, recommending that resolve's return widen, since that keeps routing decisions in the one pure function that owns them.

notdir-status.md

  • A trailing slash onto an existing regular file answers 500 io error: ENOTDIR on POSIX, because isNotFound tests ENOENT alone. That is a hole in the otherwise-uniform 404, in exactly the directory-form shape the first issue triggers on: it tells an existing regular file from nothing.
  • It is platform-dependent. statSync('README.md/index.html') reports ENOENT on win32, so the same request is 404 on Windows and 500 on POSIX, and a proof written on one host cannot see the other's answer.
  • The test goes in fileResponse, not in isNotFound. Widening the shared predicate would break its two other callers, both of which document the opposite intent: fjs/cas's list answers ok([]) for an absent store precisely so a .cas that exists but cannot be read stays a surfaced error, and fjs/cas/evo's decodeReadRevision splits revision not found from failed to read revision so the former never denies a stored revision exists. Both would fail quietly, toward losing data.
  • Scoped to ENOTDIR on purpose. /locked/ (mode 000) and /loop1/ (symlink cycle) disclose the same way on POSIX and are knowingly left at 500: ENOTDIR fires on any ordinary file, while those are entries an operator placed. EISDIR needs no entry — stat succeeds with isFile false, so it is already notRegular404. All three are POSIX-only; on Windows none leak.

Test plan

  • npx tsc --noEmit clean
  • npm test — 3396/3396
  • npm run ci-update — no diff
  • Every response in both issues' tables reproduced against a live fjs web or a direct statSync before being written down, on win32; the POSIX rows for ENOTDIR, EACCES and ELOOP are from review on darwin, and the Windows non-reproduction of all three is verified here
  • The 400 Bad Request claim checked over a raw socket, since curl will not send a control byte in a request line — a raw 0x1B in the path never reaches the listener, while %1B%5B31m arrives as printable text

Changelog: none

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
functionalscript 1dc4fe8 Commit Preview URL

Branch Preview URL
Aug 26 2026, 04:21 PM

@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: 174a9bb396

ℹ️ 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 thread fjs/web/todo/missing-index-message.md Outdated
Comment on lines +80 to +81
- [ ] Answer a directory-form `404` with a sentence naming `index.html` and
the URL path, leaving every other `404` as it is.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid claiming a non-regular index is absent

When a directory-form URL resolves to an existing non-regular index.html (for example, a directory, FIFO, device, or socket), readBounded returns notRegular and fileResponse deliberately maps that to the same 404 as a missing path. Replacing every directory-form 404 with no index.html in … would therefore make a false claim; limiting the change to missing-file errors would instead disclose the distinction this proposal says to preserve. Specify neutral wording such as “no index page available” so the design covers both cases before implementation.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The core diagnosis holds and I reproduced it end to end. All five responses in the table are genuinely byte-identical — same md5 with Date stripped. The clean-checkout claim is exact: .gitignore:134 ignores index.html, it's untracked, / gives 404 in a fresh tree, npm run index-html makes it, / gives 200, rm and it's 404 again. The obstacle is real — module.f.mjs:284-285 computes isDirectory and discards it, respond holds only path, and / and /index.html both resolve to the identical string "./index.html", so it genuinely isn't recoverable downstream. The errorSummary/errorMessage constraint is exactly as described, the second-stat cost is real (nothing has statted <root>/fjs), the recommendation of option 1 matches types.ts's stated ownership, and every citation and link resolves. Gates match the body: npm test 3378/0, tsc exit 0, ci-update clean.

"Discloses nothing" survives adversarial probing — every candidate string is a pure function of the request URL, there are no redirects in the module, parse is lexical, and on case-insensitive macOS /FJS/ echoes the client's own casing.

Four accuracy items, one material:

The uniform 404 has a live hole, in exactly the shape this todo is about. A trailing slash onto an existing regular file answers 500, not 404:

/nope.md/                404 not found
/README.md/              500 io error: ENOTDIR
/fjs/web/module.f.mjs/   500 io error: ENOTDIR

isNotFound (fjs/effects/node/module.f.mjs:123) is ENOENT-only, so fileResponse falls past the 404 branch. That is present-tense enumeration through the directory-form shape your proposal triggers on — a trailing slash tells "an existing regular file lives here" from "nothing lives here". So the constraint sentence's "the enumeration the uniform 404 currently denies" isn't quite true, and your own task "prove /fjs/ and /no-such-dir/ still answer identically" would pass while /README.md/ keeps leaking. Same class as the already-filed name-too-long-status.md (ENAMETOOLONG500); ENOTDIR is filed nowhere under fjs/web/todo/. A table row plus a sibling todo, not a rewrite.

The directory predicate isn't "a path ending in /". module.f.mjs:284 is segments.length === 0 || decoded.endsWith('/'). So /., /fjs/.., /a/.. and /%2E all get index.html appended and none ends in a slash. The Proposal states the trailing slash as if it were the predicate — it's sufficient, not necessary. That under-specifies what to implement, and it leaves the wording unpinned for these: no index.html in /fjs/.. names a path that isn't directory-form by the todo's own description.

The servedHosts attribution overstates the 404's role. respond (module.f.mjs:525) answers 403 host not served before resolve runs, so the DNS-rebinding attacker never reaches a 404; the host check is what denies them enumeration. The uniform-404 argument is sound, but its support is the dot-prefixed-existence note in resolve (lines 248-252) and README "Deliberately absent", not servedHosts.

Which path string gets echoed is unspecified, and the candidates differ: raw target, decoded path and re-joined segments all disagree (/fjs%2F decodes to /fjs/; /fjs/./ re-joins to fjs). percentDecode rejects only NUL and invalid UTF-8, so /a%0Ab/ and /%1B%5B31m/ carry control characters into the message. Not XSS (text/plain + nosniff) and not a disclosure — worth pinning the string, though.

One note on my own probe: I ran /.git/ inside a git worktree, where .git is a file rather than a directory. Result is identical either way, since resolve refuses hidden segments at line 283 before touching disk.

Merge-base is 12 commits behind main, but git diff 6228f0e04..origin/main -- fjs/web is empty, so everything above holds against merged main.

Review findings, each reproduced first:

- The uniform 404 has a hole in exactly the directory-form shape this
  issue triggers on: a trailing slash onto an existing regular file
  answers 500 ENOTDIR, since isNotFound tests ENOENT alone. So the
  claim that the identical 404 "currently denies" enumeration was
  wrong, and the proof task would have passed while /README.md/ leaked.
  Filed as notdir-status.md and now a dependency of that task.

  It is also platform-dependent, which the report did not cover:
  statSync('README.md/index.html') is ENOENT on win32, so the same
  request is 404 on Windows and 500 on POSIX. Recorded in both files.

- Directory-form is `segments.length === 0 || endsWith('/')`, not a
  trailing slash: /., /%2E, /a/.. and /fjs/.. all get index.html
  appended and none ends in one. Verified 200 for each.

- servedHosts does not support the uniform-404 argument — respond
  answers 403 before resolve runs, so a rebound origin never reaches a
  404. Re-attributed to resolve's dot-prefixed note and the README.

- Which path string is echoed is now a stated constraint, including
  that percentDecode passes control characters through.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sergey-shandar

Copy link
Copy Markdown
Contributor Author

All four fixed in bc5cba9. The material one turned out to have a wrinkle worth having.

1. The ENOTDIR hole — confirmed, filed, and platform-dependent. You're right that it breaks the uniform-404 claim in exactly the shape this issue triggers on, and right that my proof task would have passed while /README.md/ kept leaking. That task now depends on the new file.

The wrinkle: it does not reproduce on Windows. Your three probes all answered 404 not found for me, and the reason is upstream of fjs/webstatSync('README.md/index.html') reports ENOENT on win32, so isNotFound matches and the 404 branch is taken. On POSIX the same call is ENOTDIR and falls through to the 500. So the same request is 404 on one host and 500 on another, and a proof written on either platform cannot see the other's answer. That is now the strongest argument in notdir-status.md for filing it apart from name-too-long-status, which has neither the disclosure nor the split. It also means "prove /README.md/ and /nope.md/ answer identically" has to name the host it runs on, which the task now does.

2. The directory predicate. Corrected — segments.length === 0 || decoded.endsWith('/'), with your four cases named and each verified at 200 against a root with an index, plus the note that a trailing slash is sufficient and not necessary. The wording question it opens is recorded too: no index.html in /fjs/.. names a path that does not read as directory-form, so echoing the parsed path is an alternative with its own cost — naming a directory the client never wrote.

3. servedHosts. Agreed, and the correction is a sharper claim than the original: respond answers 403 host not served before resolve runs, so a rebound origin never reaches a 404 at all. Re-attributed to resolve's dot-prefixed-existence note and README "Deliberately absent", with a parenthetical that the host check defends the socket while the uniform 404 defends what an answer says.

4. The echoed string. Now a third constraint, carrying your /fjs%2F and /fjs/./ disagreements and the control-character point — noted as harmless under text/plain + nosniff and not a disclosure, but needing one defined spelling.

Gates re-run after the edits: tsc clean, ci-update no diff. Still docs-only.

On your .git note — agreed it makes no difference, and for the reason you give: resolve refuses hidden segments before any disk access, so the worktree's .git being a file never comes up.

@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: bc5cba9300

ℹ️ 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 thread fjs/web/todo/notdir-status.md Outdated
Comment on lines +49 to +53
Where to put the test is the open question. Widening `isNotFound` to admit
`ENOTDIR` changes what *every* caller of it treats as absent, which is a
decision belonging to `fjs/effects/node` rather than to this module; a test
local to `fileResponse` keeps the blast radius here but leaves the next caller
to rediscover it. The platform split argues for the shared answer — `ENOENT`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep ENOTDIR handling local to web

If the implementer chooses the proposed shared isNotFound change, an ENOTDIR from fileCas.list's access(storePrefix)—for example, when the configured store path or one of its components is a regular file—will be converted into an empty store. fjs/cas/module.f.mjs explicitly uses this predicate to treat only a genuinely absent .cas as empty while surfacing corruption and other failures, so broadening it would silently mask a malformed store. Specify the web-local check here, or concretely include auditing and updating every existing caller, rather than leaving this behavior-changing decision open.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

@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: 4ef85a944d

ℹ️ 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 +64 to +65
- [ ] Report `ENOTDIR` from the virtual file system for a path descending
through a regular file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Link the existing virtual-filesystem issue

This task is already tracked in fjs/effects/node/virtual/todo/lexical-path-resolution.md lines 27–42, including the exact ENOTDIR behavior and the requirement to make that operation-wide change in its own PR. Leaving the implementation task here creates duplicate owners and encourages bundling a cross-cutting virtual-runner change into the web fix; mark this issue as blocked by that existing issue (or use the partialRun injection pattern from fjs/web/proof.f.mjs lines 321–325) and retain only the web-specific mapping and proof here.

AGENTS.md reference: AGENTS.md:L10-L11

Useful? React with 👍 / 👎.

Codex P2: the proposal left "shared predicate or web-local test" open,
and AGENTS.md §1 wants a concrete design before code. Checking the
other callers settles it — widening would be wrong, not merely broad.

isNotFound has two callers besides fjs/web, and both document in prose
an intent that admitting ENOTDIR would violate:

- fjs/cas's list answers ok([]) for an absent store precisely so that a
  .cas which exists but cannot be read stays a surfaced error; a store
  path with a regular file among its components would become an empty
  store.
- fjs/cas/evo's decodeReadRevision splits "revision not found" from
  "failed to read revision" so the former never denies a stored
  revision exists; it would start denying one.

Both fail quietly toward losing data. So the test goes in fileResponse,
beside its existing notRegular and tooLarge cases, and isNotFound is
left alone — with a note that a later caller wanting the same reading
should get a named predicate, not a broader isNotFound.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sergey-shandar

Copy link
Copy Markdown
Contributor Author

Fixed in 836db52. You were right that it shouldn't stay open, and checking the callers made the answer stronger than "keep it local to be safe" — widening would be actively wrong.

There are two other callers, and both state in prose an intent that admitting ENOTDIR would violate:

  • fjs/cas's list answers ok([]) for an absent store precisely so that a .cas which exists but cannot be read stays a surfaced error — "a .cas that exists but cannot be read (permissions, corruption) is a genuine storage error and is surfaced, not masked as 'no hashes'". Your scenario is exactly right: a store path with a regular file among its components would start reporting as an empty store.
  • fjs/cas/evo's decodeReadRevision splits revision not found from failed to read revision, because "calling any of those 'not found' would deny a stored revision exists". It would start denying one.

Both fail quietly, and in the direction that loses data rather than the one that raises an error. So the Proposal now specifies the web-local test in fileResponse — beside its existing notRegular and tooLarge cases, which are the same kind of judgement about what this server answers as absent — and says isNotFound is left alone, with the note that a later caller wanting the same reading should get a named predicate rather than a broader isNotFound. The task no longer defers the choice, and both callers are listed under Related as what settles it.

Worth recording that this reverses my own reasoning: I had argued the platform split favoured the shared answer, since ENOENT and ENOTDIR are one fact wearing two names. That argument holds for fjs/web and does not survive contact with the callers who need the two names kept apart.

One unrelated check while here: the main merge in 4ef85a9 touched fjs/AGENTS.md, which both todo files cite as §1.2. Still accurate — §1.2 is "Proof coverage is mandatory".

Gates: tsc clean, ci-update no diff. Still docs-only.

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All three accuracy corrections are right and I verified them against the code, not just the prose. The directory predicate now quotes segments.length === 0 || decoded.endsWith('/') verbatim and its four examples all resolve to ./index.html as claimed; the wording question for /fjs/.. is explicitly deferred rather than dropped. The servedHosts attribution is gone and the replacement is correct — respond really does answer 403 host not served at :525 before resolve at :527, and the dot-prefixed-existence note and README section both say what's now cited. The ENOTDIR table matches reality exactly on POSIX, the causal claim holds at head, and I confirmed the scope is complete: stat and readFile each appear exactly once in fjs/web, both in the same chain feeding fileResponse, so no other site can produce it. Nothing the rewrite touched regressed — the five-row table, the clean-checkout reachability, the obstacle diagnosis and the second-stat cost all still check out.

Head moved to 836db529a while I was working; the extra commit is included above.

The fix is ENOTDIR-only, and two siblings still leak the same way. In a scratch root:

/locked/    (dir chmod 000, has index.html)  -> 500  io error: EACCES
/nolocked/  (nothing there)                  -> 404  not found
/loop1/     (symlink cycle)                  -> 500  io error: ELOOP
/dir/       (real dir, no index)             -> 404  not found

EISDIR is unreachable — stat succeeds and isFile is false, so it becomes notRegular → 404. But /locked/ vs /nolocked/ is exactly the existence oracle the todo argues against. I think scoping to ENOTDIR is defensible under your own client-caused/host-caused doctrine, since EACCES and ELOOP need an operator-placed entry while ENOTDIR fires on any ordinary file. Two consequences should be stated, though:

  • missing-index-message.md:121-122 calls notdir-status "the one directory-form request whose status already discloses". /locked/ is a second.
  • Task 3 ("prove /fjs/ and /no-such-dir/ still answer identically — and /README.md/ with them") will pass while /locked/ leaks. Same failure mode the correction was written to close, one code over.

Saying in notdir-status.md that EACCES/ELOOP are knowingly left at 500 would make the scoping read as a decision rather than an omission.

On the echoed string: it's recorded as a constraint but no spelling is chosen, so it isn't pinned. For a proposal doc that's arguably enough — noting it because my earlier point was about pinning. The facts are right, and control characters do still reach the resolved path.

The body fails the Changelog: gate. Changelog: none sits at line 15, followed by a --- and a four-bullet **Update.** section. Prose isn't a trailer block, so it has to move below. And the body now argues with itself: Summary bullet 3 still asserts all three pre-correction claims — "a path ending in / is one the server answered by appending index.html", "two constraints are recorded with it", and "the enumeration the uniform 404 currently denies under the DNS-rebinding reading in servedHosts" — which the Update at the bottom retracts. Both land in the squash commit. Test plan also still says 3378/3378; it's 3393/3393 now.

One thing I couldn't check: both files and the body state statSync('README.md/index.html') gives ENOENT on win32. I'm on darwin and can't reproduce it. Plausible — Windows returns ERROR_PATH_NOT_FOUND and libuv maps that to ENOENT — but it's unverified here.

Gates at head: npm test 3393/0, tsc --noEmit exit 0, ci-update no diff. Merge is clean w.r.t. fjs/webgit diff 6228f0e0..origin/main -- fjs/web is empty, and the branch adds exactly the two todo files.

@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: 0b0349df17

ℹ️ 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 +86 to +88
1. Widen what `resolve` returns, so the routing fact travels with the path.
Keeps every routing decision in the one pure function that owns them, at
the cost of a change to `Resolve` and its proofs.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Define the widened Resolve result before implementation

The preferred option changes the exported Resolve API, but the issue never specifies the new success payload or how respond consumes it; the task even leaves the transport decision open. Because types.ts currently promises Result<string, Refusal> and callers unwrap a string, an implementer must invent a public API rather than follow a concrete design. Record the exact result shape and its handling before this issue is ready for implementation.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Review: the fix is ENOTDIR-only and two siblings disclose the same way
on POSIX — /locked/ (mode 000, EACCES) and /loop1/ (symlink cycle,
ELOOP) both answer 500 where an absent path answers 404. So calling
notdir-status "the one" such request was wrong, and its proof task
would have passed while those two leaked.

Both are now named and left at 500 deliberately, with the reason: the
client-caused doctrine that makes ENOTDIR a 404 does not reach them,
since ENOTDIR fires on any ordinary file while a mode-000 directory or
a symlink cycle is an entry an operator placed. EISDIR needs no entry —
stat succeeds and isFile is false, so it is already notRegular -> 404.

Verified on win32 that none of the three leak there: chmod 000 does not
stop traversal, and a symlink cycle reports ENOENT rather than ELOOP.
With ENOTDIR arriving as ENOENT too, the whole oracle is POSIX-only, so
a proof of its absence has to run on a POSIX host. Both proof tasks now
say what they cover instead of implying directory-form requests
disclose nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sergey-shandar

Copy link
Copy Markdown
Contributor Author

All addressed in 1d19fee plus a body rewrite. Your /locked/ and /loop1/ finding was the important one, and probing it on Windows turned up something that changes how the scoping reads.

The two siblings — named, and left at 500 deliberately. You're right that "the one directory-form request whose status already discloses" was wrong, and that task 3 would have passed while /locked/ leaked. notdir-status.md now carries both in a table and says why they stay: ENOTDIR fires on any ordinary file — every served tree has thousands, so any client can ask — while a mode-000 directory or a symlink cycle is an entry an operator placed, and a 500 there is not obviously the wrong answer. Reopen them on their own evidence, not as a corollary. EISDIR is recorded as needing no entry, for the reason you give.

Both proof tasks now state their coverage rather than reading as "no directory-form request discloses" — in notdir-status.md and in the dependent task in missing-index-message.md, and the Related line no longer says "the one".

All three are POSIX-only, which I could check. On win32: chmod 000 does not stop traversal, so stat('locked/index.html') simply succeeds; and a symlink cycle reports ENOENT, not ELOOP. With ENOTDIR arriving as ENOENT too, Windows has none of the three. So the oracle is a property of POSIX hosts, and a proof of its absence has to run on one — which is now stated, and is a sharper version of the platform note than the ENOTDIR row alone.

That also settles the item you flagged as unverifiable: statSync('README.md/index.html') really is ENOENT on win32, verified here rather than inferred from the libuv mapping. Symmetrically, I cannot reproduce your EACCES/ELOOP rows on this host — they are recorded on your evidence, and both files now say which platform each row came from.

The body is rewritten, not patched. Changelog: none is the last line with no prose after it, and the self-contradiction is gone: the retracted claims are no longer asserted anywhere, so there is nothing left for an Update section to retract. It is now two sections, one per issue file, which is also closer to what the squash commit should say. Test plan reads 3396/3396 — head moved again past your 3393.

On the echoed string: left as a constraint without a chosen spelling, deliberately. Picking one needs the /fjs/.. wording question decided too, and that is the kind of thing the implementing PR should settle with the code in front of 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: 1d19fee8f3

ℹ️ 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 +86 to +88
1. Widen what `resolve` returns, so the routing fact travels with the path.
Keeps every routing decision in the one pure function that owns them, at
the cost of a change to `Resolve` and its proofs.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Carry directory-form metadata through hidden refusals

The preferred flow only carries the routing fact alongside a successfully resolved path, but resolve returns the hidden-path 404 at fjs/web/module.f.mjs:283 before computing isDirectory at line 284. Consequently, /.git/ can never receive this metadata and would remain not found while /no-such-dir/ gets the new index-specific sentence, contradicting the proposal to handle directory-form 404s consistently. Specify how the fact is attached to this refusal too, and include a hidden-directory proof.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

sergey-shandar and others added 2 commits August 26, 2026 07:35
Codex P2: resolve refuses a dot-prefixed segment before computing
isDirectory, so /.git/ never reaches the index.html branch — and the
proposal never said what that means, since "answer a directory-form 404
with a sentence naming index.html" technically covers it.

Decided the other way from the suggestion: the hidden refusal keeps
"not found", and the ordering stays as it is. The new sentence claims
the server looked; for a hidden path it refused without touching the
disk, so "no index.html in /.git/" would describe work it did not do.

Non-disclosure does not decide this, and the file now says so rather
than leaving it implied: the hidden refusal never consults the disk, so
/.git/ and /.nonexistent/ answer identically under either sentence —
verified. What separates them from /foo/ is the dot the client wrote.

Adds the hidden-directory proof task that was asked for, pinning both
that /.git/ keeps the old sentence and that it matches a hidden path
that does not exist.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 2c3a5f0ffb

ℹ️ 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 thread fjs/web/todo/notdir-status.md Outdated

- [ ] Report `ENOTDIR` from the virtual file system for a path descending
through a regular file.
- [ ] Answer `404` for it from `fileResponse`, leaving `isNotFound` alone.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve ENOTDIR failures caused by the configured root

When the configured root is itself a regular file or has a non-directory ancestor (for example, fjs web README.md), stat('README.md/index.html') also returns ENOTDIR; a check inside fileResponse cannot distinguish that operator/configuration failure from /README.md/ descending through a file beneath a valid root. The proposed mapping would therefore turn every request to such a misconfigured server from the current 500 into a misleading 404. Specify a separate root validation or another way to retain 500 when the offending component is at or above the served root before implementing this task.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed and fixed in 08e106c — this is a genuine flaw in the design I committed to two rounds ago, not just an underspecified corner.

join('README.md', 'index.html') is README.md/index.html, so with a regular file as root every request stats a path descending through a file. Same errno, and nothing in fileResponse can separate it from /README.md/ under a good root. The blanket mapping would have answered 404 not found to every request against a misconfigured server — hiding the one case where the 500 was telling the operator something true.

Fixed by validating the root in main, before listen, the way an out-of-range port already is. Better than comparing the offending component against the root per request: no extra stat on the hot path, it fails at the moment the mistake was made rather than on some later visitor's request, and afterwards every ENOTDIR reaching fileResponse is client-caused by construction — which is exactly what the mapping assumes. It shares the replace-underneath window with stat-then-read and does not widen it, and that is now cross-linked.

One thing that fell out while checking: Windows already answers 404 to everything for such a root, silently. fjs web README.md starts happily there and 404s every request, because stat reports ENOENT rather than ENOTDIR — verified, after I first fooled myself with a stale server on a reused port and had to redo it against the serving README.md on … log line. So today the two platforms give two different misleading answers per request; the startup check makes both say the same true thing once.

@sergey-shandar

Copy link
Copy Markdown
Contributor Author

Fixed in d3f0978 — the gap was real, though I decided it the other way and want the reasoning on the record.

You're right that the proposal never covered this case. resolve refuses a dot-prefixed segment at :283, before isDirectory at :284, so /.git/ never reaches the branch that appends index.html — and "answer a directory-form 404 with a sentence naming index.html" technically covers it while the flow cannot deliver it. That ambiguity needed pinning.

But the fix is to keep not found there, not to carry the metadata through. The new sentence is a claim about having looked. For a hidden path the server refused without touching the disk, so no index.html in /.git/ would describe work it did not do. Making the refusal carry directory-form metadata would buy consistency of shape at the cost of the message being false — and the ordering that prevents it is the same ordering that keeps the hidden check cheap and disk-free.

Non-disclosure does not decide this, which is worth being explicit about since it is the axis the rest of the issue turns on. The hidden refusal never consults the disk, so /.git/ and /.nonexistent/ answer identically under either sentence — verified, both 404 not found, as do /.git and /.env. What separates them from /foo/ is that the client wrote a dot, which the client already knows. So neither choice leaks; only one is true.

The hidden-directory proof you asked for is added, and pins both halves: that /.git/ keeps the old sentence, and that it still matches a hidden path which does not exist — so a later change cannot make the refusal disclose by moving it after the new branch. The implementation task now says to key off the index.html-appending branch specifically, rather than off directory-form in the abstract.

Gates: tsc clean, ci-update no diff. Docs-only.

@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: d3f0978f18

ℹ️ 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 thread fjs/web/todo/missing-index-message.md Outdated
the decoded path, and the re-joined segments differ for `/fjs%2F` (decodes
to `/fjs/`) and `/fjs/./` (re-joins to `fjs`) — and `percentDecode` rejects
only NUL and invalid UTF-8, so `/a%0Ab/` and `/%1B%5B31m/` carry control
characters through. Harmless in a `text/plain` body served `nosniff`, and no

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Escape control characters before echoing URL paths

For a request such as /%1B%5B31m/, percentDecode turns the escape into a literal control character that the proposed response would reflect. text/plain and nosniff constrain browser interpretation only; terminal clients such as curl or wget can still pass ANSI/OSC sequences to the user's terminal, altering its state or triggering terminal-specific actions. Specify an escaped or percent-encoded representation rather than declaring these characters harmless, so this non-trivial issue has a safe concrete design.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f306a7c, and you're right that "harmless" was wrong — I had reasoned about browsers and stopped there. text/plain and nosniff bound what a browser does with a body; curl and wget write it to a terminal, where the sequence /%1B%5B31m/ decodes to is acted on rather than displayed. A message quoting the client would hand attacker-chosen escapes to whoever ran the request.

Fixed by choosing a representation rather than declaring the characters safe: echo the percent-encoded spelling, never percentDecode's output.

What makes that more than a rule to remember is that the encoded form cannot carry the problem. A raw control byte never reaches this module — Node's parser answers 400 Bad Request on the request line before the listener runs, which I checked over a socket rather than assuming, since curl would not send one. So in the target such a character exists only as the printable text %1B. Echoing the target's path as received therefore needs no escaping pass and no list of dangerous characters — the version of this that cannot rot as the list ages. A normalized path is equally fine on the same terms provided it is re-encoded on the way out, so the /fjs/.. wording question stays open on its own merits rather than being decided by this.

Two things worth recording alongside it:

  • Nothing echoes anything today. The current answer is the constant not found — confirmed against a live server for /%1B%5B31m/ — so this is a property to build in, not a bug to fix. The issue now says so, so a reader does not go looking for a live vulnerability.
  • Added the proof task: /%1B%5B31m/ must echo %1B%5B31m and not the escape it names, so no answer this server writes can drive a terminal.

Gates: tsc clean, ci-update no diff. Docs-only.

Codex P2, and the "harmless" claim was wrong. text/plain and nosniff
bound what a browser does with a body; curl and wget write it to a
terminal, where the ANSI or OSC sequence /%1B%5B31m/ decodes to is
acted on rather than displayed. A message quoting the client would
hand attacker-chosen escape sequences to whoever ran the request.

Fixed by choosing a representation instead of declaring the characters
safe: echo the percent-encoded spelling, never percentDecode's output.
The encoded form cannot carry the problem, because a raw control byte
never reaches this module — Node's parser answers 400 on the request
line first, verified over a socket — so such a character exists in the
target only as the printable text %1B. That needs no escaping pass and
no list of dangerous characters, which is the version that cannot rot.
A normalized path is equally fine if re-encoded on the way out, so the
/fjs/.. wording question stays open on its own merits.

Nothing echoes anything today — the answer is the constant "not found",
confirmed — so this is a property to build in, not a bug to fix. Adds
the proof task pinning it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: f306a7ce47

ℹ️ 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 thread fjs/web/todo/notdir-status.md Outdated
Comment on lines +112 to +113
- [ ] Prove `/README.md/` and `/nope.md/` answer identically, on a host whose
`stat` distinguishes them — a Windows run cannot see the difference.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep the ENOTDIR proof platform-independent

The proposal explicitly adds ENOTDIR to the virtual filesystem, and the existing web proofs execute respond through that virtual runner, so a Windows test run can exercise the distinction without relying on the host filesystem. Requiring “a host whose stat distinguishes them” may lead the implementation to skip this proof on Windows and leave the new response branch uncovered there; specify a platform-independent virtual/injected ENOTDIR proof, with any POSIX host check kept as separate integration validation.

AGENTS.md reference: fjs/AGENTS.md:L27-L35

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, and the task contradicted this file's own plan — fixed in 08e106c.

The proposal already teaches the virtual file system to report ENOTDIR, and for exactly this reason: fjs/AGENTS.md §1.2 rejects an unreachable branch, so the error has to be producible without a host. fjs/web/proof.f.mjs already drives respond through virtual(...). So requiring "a host whose stat distinguishes them" would have made the branch uncovered on Windows while the file was simultaneously arranging for it to be coverable anywhere — my wording, carried over from the platform-split paragraph where it did belong.

The task is now virtual and unconditioned on the host. I kept a one-off POSIX check as a separate task rather than dropping it, and said why: the virtual file system models what a host does, and this is the issue where that model turned out to be wrong on two platforms at once — ENOTDIR on POSIX, ENOENT on Windows. A virtual proof pins the branch; it cannot notice the model drifting from either host. Keeping them as two tasks says which one answers which question.

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both of my findings are properly closed, and the last commit closes the third one better than I asked.

The ENOTDIR scoping now reads as a decision: notdir-status.md:75-99 has its own table and a coherent rationale — ENOTDIR fires on any ordinary file, EACCES and ELOOP need an operator-placed entry — with EISDIR given an explicit row. I rebuilt the scratch root and re-measured; every row matches. And task 3 now names /locked/ and /loop1/ and requires the proof to state its scope, which is the part that actually prevents it reading as "no directory-form request discloses".

On the echoed string: deciding the encoding axis rather than the raw-vs-normalized candidate is the right decomposition. The raw/normalized choice was always cosmetic and both are pure functions of the request URL; the safety property lives entirely on whether percentDecode's output is quoted. Retracting "harmless in a text/plain body served nosniff" is also correct — nosniff bounds browsers, not curl writing to a terminal.

I reproduced the load-bearing claim on darwin, Node v23.11.0, over raw sockets: a raw 0x1B or 0x07 in the path gets HPE_INVALID_URL and the listener never runs, while %1B%5B31m and %0A reach it as printable text. With no clientError handler Node's default is exactly HTTP/1.1 400 Bad Request\r\nConnection: close\r\n\r\n. So the wording holds and the encoded form needs no escaping pass.

One thing better than the file claims: respond already calls parseTarget(url) at :523 for the host check, so target.path is in scope at the point the message would be built. Nothing new needs plumbing for this constraint — only the separate isDirectory problem does.

/fjs/.. stays deferred and is now explicitly orthogonal ("a normalized path is re-encoded on the way out"), which is right.

The body is now stale about this very commit. Line 5 still describes the third constraint as open — "pin which string is echoed, given that /fjs%2F and /fjs/./ disagree and percentDecode passes control characters through" — when the commit's whole point is that it is pinned. The body also carries none of the terminal-escape reasoning or the retraction. It becomes the squash commit, so it would land describing an undecided constraint the branch decided. The test-plan bullet also doesn't mention the new 400 Bad Request verification. Everything else about the body is fine: Changelog: none last, no ---, no duplicate trailers, counts current.

Small carry-over: :89 now says "percentDecode rejects just NUL and invalid UTF-8" — still not quite right. NUL is rejected by resolve at :278, not by percentDecode, and percentDecode also rejects malformed escapes at :124. Conclusion unaffected. That phrasing traces to my own earlier comment, so it's mine to correct as much as yours.

Three Windows claims are now author-attested and unverifiable from here (the original statSync one plus mode-000 traversal and the symlink cycle reporting ENOENT). Noting as attested, not as a finding.

Gates: npm test 3396/0, tsc --noEmit exit 0, ci-update no diff, still docs-only — the branch adds exactly the two todo files, +296/−0.

Review carry-over: the encoding constraint said percentDecode rejects
"just NUL and invalid UTF-8". Neither half held. percentDecode rejects
a malformed escape and invalid UTF-8; NUL is rejected separately by
resolve. The conclusion is unchanged — none of those excludes a control
character — but the sentence now names the right guard for each.

Also records what review found in the code: respond already binds
parseTarget(url) for the host check, so target.path is in scope where
the message would be built. The encoding constraint needs no plumbing;
only the isDirectory fact does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sergey-shandar

Copy link
Copy Markdown
Contributor Author

Both fixed in 8de8048 and a body rewrite. Thanks for reproducing the socket claim independently on darwin — that one was the load-bearing step and I could only test it on one platform.

The body no longer describes a constraint the branch decided. Rewritten rather than patched: the third bullet now states the encoding decision and carries the reasoning that was only in the commits — the terminal-escape point, the retraction of "harmless in a text/plain body served nosniff", and the 400 fact that makes the encoded form safe without an escaping pass. The hidden-path decision was missing from the body too, so it is in now, folded into the directory-form bullet. Test plan gains the raw-socket verification and why curl could not do it.

The percentDecode carry-over is corrected, and both halves were wrong rather than one: it rejects a malformed escape and invalid UTF-8, while NUL is rejected separately in resolve. The sentence now names the right guard for each. Conclusion unchanged — none of them excludes a control character — but the file should not be teaching the wrong division of labour. No need to split the credit; it was mine to check before repeating it.

respond already binding parseTarget(url) is recorded — that is a better fact than the file had, and it narrows the plumbing question usefully: the encoding constraint needs nothing new, only the isDirectory fact has to travel. Now stated where the constraint is, so the implementing PR does not go looking.

On the three Windows claims: attested is the right label, and both files say which platform each row came from for exactly this reason. The statSync one is the only load-bearing one — it is what makes ENOTDIR platform-dependent — and if it ever needs a second pair of eyes, node -e "try{require('fs').statSync('README.md/index.html')}catch(e){console.log(e.code)}" in a repo root is the whole test.

Gates: tsc clean, ci-update no diff, npm test 3396/3396. Still docs-only, two files.

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. The attribution is now correct, and I confirmed it with a discriminating experiment rather than by reading — all three rejections share one message, so resolve outputs alone cannot tell them apart. Deleting only the || decoded.includes(nul) half of :278 leaves /a%zz/ and /%FF/ at 400 while /%00/ becomes ok. That pins it exactly as the new sentence says: percentDecode rejects the malformed escape and the invalid UTF-8, NUL survives it and is caught only by resolve.

No over-correction — it doesn't claim percentDecode rejects nothing or that NUL goes unchecked. The surrounding argument still holds: %1B and %0A resolve to real control characters in the decoded path, so "echo the encoded spelling, never percentDecode's output" is unaffected. No "just NUL" text remains under fjs/web/. The commit's other addition checks out too — respond binds parseTarget(url) at :523, before resolve, so target.path is in scope where the message would be built.

Gates: npm test 3396/0, tsc --noEmit exit 0, purge matched nothing. Body well-formed and not stale.

Two codex findings on notdir-status, both real and both missed in
earlier rounds.

ENOTDIR does not only arise below a valid root. `fjs web README.md`
serves a regular file, so join produces README.md/index.html and every
request stats a path descending through a file — same errno, and
fileResponse cannot tell it from /README.md/ under a good root. The
blanket mapping would have answered 404 to every request against a
misconfigured server, hiding the one case where the 500 was telling the
operator something true. So the root is now checked once in main before
listen, the way an out-of-range port already is: no per-request stat, it
fails where the mistake was made, and afterwards every ENOTDIR reaching
fileResponse is client-caused by construction. Windows already answers
404 to everything for such a root, silently — verified — so the check
makes both hosts say the same true thing at startup.

The proof task required "a host whose stat distinguishes them", which
contradicted this file's own plan: the fix teaches the virtual file
system to report ENOTDIR precisely so the branch is coverable, and
proof.f.mjs already drives respond through virtual(). Conditioning on
the host would have left the new branch uncovered on Windows. Now
virtual, with a one-off POSIX check kept as separate validation, since
this is the issue where the model was wrong on two platforms at once.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 08e106c764

ℹ️ 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 thread fjs/web/todo/notdir-status.md Outdated
Comment on lines +84 to +85
So the root is checked once, in `main`, before `listen`: if it is not a
directory, `errorExit` the way an out-of-range port already does. That is

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Define a directory-capable root check

The proposed validation cannot distinguish a directory with the existing stat API: FileStat in fjs/effects/node/types.ts exposes only size and isFile, so isFile === false also covers FIFOs, devices, sockets, and the virtual runner's JsModule. Implementing this task therefore requires an unspecified API change or a different operation such as readdir, with different error and permission behavior. Specify that mechanism and its failure handling so the non-trivial issue has a concrete implementable design.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, and it lands squarely on the fix from the previous commit — that root check was not implementable as written. Specified in 14e9fdf.

FileStat is { size, isFile }, so isFile === false is not "is a directory": it covers a FIFO, a device, a socket, and the virtual runner's JsModule, whose _Entity is readonly Vec[] | Dir | JsModule. Serving any of those as a root is the same operator error as serving a regular file, so the check has to name what it wants rather than infer it from a negation.

Specified: add isDirectory to FileStat, in the node runner and the virtual one together, and reject a root that is not one — including the case where both flags are false. Failure handling covers two cases and uses the same errorExit for both: stat failing outright (a root that does not exist), and a root that exists and is not a directory. Both are the command line being wrong, which main already reports that way for an out-of-range port.

Not readdir(root), which is the obvious alternative and needs no new API — I recorded why, since a later reader will reach for it. It answers a different question: a directory may be traversable without being listable, so mode --x permits opening a known path under it while readdir fails EACCES, and a root this server can serve perfectly well would be refused at startup. Reading a whole directory only to discard it is the smaller objection.

The task list now separates the API change from its use, and FileStat is cross-referenced under Related.

Codex P2 on the fix from the previous commit: "reject a non-directory
root" is not expressible today. FileStat is { size, isFile }, so
isFile === false is not "is a directory" — it also covers a FIFO, a
device, a socket, and the virtual runner's JsModule, whose _Entity is
readonly Vec[] | Dir | JsModule. Serving any of those as a root is the
same operator error, so the check has to say what it wants.

Specified: add isDirectory to FileStat, in the node runner and the
virtual one together, and reject a root that is not one — including
when both flags are false, and when stat fails outright because the
root does not exist. Both are the command line being wrong, which main
already reports with errorExit for a port.

Recorded why not readdir(root), which needs no new API and is the
obvious alternative: it answers a different question. A directory may
be traversable without being listable — mode --x permits opening a
known path under it while readdir fails EACCES — so a root this server
can serve perfectly well would be refused at startup.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 14e9fdfc59

ℹ️ 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 thread fjs/web/todo/notdir-status.md Outdated
Comment on lines +84 to +85
So the root is checked once, in `main`, before `listen`: if it is not a
directory, `errorExit` the way an out-of-range port already does. That is

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Revalidate the root before mapping ENOTDIR to 404

When the served directory is renamed or replaced with a regular file after startup, this one-time check no longer establishes the stated invariant: every subsequent stat(root/...) can return ENOTDIR, which fileResponse would misclassify as a client-caused 404 instead of the operator-facing 500. Unlike the request-local stat/read race, this window lasts for the server's remaining lifetime; recheck the root when handling ENOTDIR or otherwise preserve the error in this case. The fresh scenario beyond the earlier configured-root comment is replacement after the startup validation has succeeded.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, and the part worth conceding explicitly is the sentence you're contradicting: the previous commit said the startup check "leaves the same replace-underneath window as stat-then-read, and no more". That was wrong, and not by a little. stat-then-read races two calls microseconds apart; this window opens once and stays open for the life of the process, turning the operator's mistake into a lie told to every visitor. Fixed in 84f3386, and the file now says the earlier claim was wrong rather than quietly dropping it.

The mapping re-checks the root on ENOTDIR and answers 404 only if it is still a directory, otherwise 500 — which is again the true answer. Your placement is the right one: the cost sits on the rare path, and the serving path is untouched, so it does not reintroduce the per-request stat that made the component-comparison alternative unattractive.

What remains after that is a genuine race between the re-check and the stat that produced the error — but it is request-local, so it degrades to exactly the class stat-then-read already covers: a wrong status in a vanishing window rather than a wrong status forever. The Related entry now says that instead of claiming equivalence.

The startup check stays, for the reason it was added: it turns a mistyped root into immediate feedback rather than a 500 that waits for a visitor to discover. The two do different jobs — one reports the mistake, the other keeps the mapping honest afterwards — and the task list now separates them.

Not reproducible on this host, for the standing reason: Windows reports ENOENT rather than ENOTDIR, so the misclassification never arises there. Reasoned from the code, not measured.

Codex P2: the startup check establishes the invariant only at startup.
Rename the root, or replace it with a regular file, and every later
stat(root/...) is ENOTDIR from the root itself — which fileResponse
would report as a client-caused 404 for the rest of the process's life.

The previous commit called that "the same replace-underneath window as
stat-then-read, and no more". Wrong: that one is two calls racing
microseconds apart, this one opens once and stays open, turning the
operator's mistake into a lie told to every visitor. The file now says
so rather than quietly dropping the claim.

So the mapping re-checks the root on ENOTDIR and answers 404 only if it
is still a directory. The cost is on the rare path, not the serving
path. What is left is a genuine race between the re-check and the stat
that produced the error — request-local, and the kind stat-then-read
already covers.

The startup check stays: it turns a mistyped root into immediate
feedback rather than a 500 waiting for a visitor.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 84f3386eed

ℹ️ 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 +91 to +92
needs.** Rename the root, or replace it with a regular file, and every later
`stat(root/…)` is `ENOTDIR` from the root itself — which `fileResponse` would

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Recheck vanished roots on ENOENT

When the served directory is renamed or deleted after startup, stat(root/…) returns ENOENT, not ENOTDIR; the existing fileResponse maps that directly to 404, while the proposed root recheck runs only for ENOTDIR. The server would therefore still report a permanent operator failure as client-caused absence. The fresh evidence beyond the earlier revalidation comment is this distinct errno for root disappearance, so the design must also validate the root before accepting ENOENT as a 404 (or otherwise retain a stable root reference).

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed: a deleted or renamed root fails ENOENT, takes the existing isNotFound branch, and reports the same permanent operator failure as a client-caused 404 — the ENOTDIR re-check does not see it. Addressed in 3496001, though by scoping rather than by extending the re-check, and I want the reasoning on the record since it is a decline.

Validating the root before accepting ENOENT is declined on cost. An ENOENT 404 is the most common answer a static server gives; adding a root stat there puts a second syscall on the hot path to improve a diagnostic. The ENOTDIR re-check pays nothing on that path precisely because ENOTDIR is rare. The file now says this is a trade rather than a principle, so nobody later reads the asymmetry as a claim that the two cases differ in kind.

One thing worth being precise about, which sharpens what is actually lost: 404 is not false in either case. With the root gone or replaced by a file, nothing beneath it exists, so "not found" is a true statement about the requested resource. What the 500 buys is telling the operator which mistake they made. So the asymmetry costs diagnostic reach, not correctness — that framing is now in the file, and it is the honest version of the argument I was making two rounds ago for ENOTDIR.

The design that answers both, with no re-check at all, is holding the root open and resolving beneath the handle, so it cannot be swapped underneath the server at any point. That is exactly the effect stat-then-read is blocked on, and it is now cross-referenced as a second reason to want it — which is a better outcome than this issue growing a third mitigation.

Comment thread fjs/web/todo/missing-index-message.md Outdated
Comment on lines +98 to +102
The encoded form cannot carry the problem. A raw control byte never reaches
this module — Node's parser answers `400 Bad Request` on the request line
before the listener runs, verified — so in the target such a character
exists only as the printable text `%1B`. Echoing the target's path as
received therefore needs no escaping pass and no list of dangerous

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Escape raw controls for direct respond calls

When exported respond is invoked directly, through the virtual runner, or by a non-Node runner with a URL such as '/\u001b[31m/', Node's request parser is never involved; IncomingMessage.url is typed as an unrestricted string. Echoing target.path as received would therefore place the terminal control sequence in the response despite this claim. The fresh evidence beyond the earlier control-character thread is this supported direct-call path, so raw controls still need escaping or an enforced input invariant even while percent-encoded spelling is preserved.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct — the invariant was one caller's property, not respond's. Fixed in 3496001.

respond is exported, IncomingMessage.url is an unrestricted string, and fjs/web/proof.f.mjs's answer already calls respond('.')(request(method, url)) straight through the virtual runner with whatever URL a case names. So "echoing the target as received needs no escaping pass" held only behind Node's parser, and put the invariant outside the function that depends on it — the arrangement that quietly stops holding when someone adds a caller.

The message now percent-encodes what it echoes rather than trusting what it was handed. That is total and still needs no list of dangerous characters: encode everything outside the safe set, and %1B survives as %1B whether it arrived encoded or raw. The parser fact stays in the file, demoted to what it is — true of the HTTP path, and not a licence to skip the pass.

It also has a side benefit worth noting: with an encoding pass on the way out, the raw-versus-normalized choice becomes free of safety consequences, so that question stays open purely on wording grounds, which is where it belonged.

The proof task now covers the direct-call path: a raw control character passed to respond the way proof.f.mjs already calls it, alongside the %1B%5B31m case. Without that, the proof would only ever exercise inputs the parser had already filtered — which is precisely how this would have regressed unnoticed.

Two codex findings, both on claims the previous rounds made.

The encoding constraint leaned on Node's parser rejecting a raw control
byte. That is a property of one caller, not of respond: respond is
exported, IncomingMessage.url is an unrestricted string, and
proof.f.mjs already calls it directly through the virtual runner with
whatever URL a case names. So "needs no escaping pass" was wrong, and
the invariant sat outside the function depending on it. The message now
percent-encodes what it echoes — total, no list of dangerous characters,
and %1B survives as %1B whether it arrived encoded or raw. The proof
task gains a raw-control case exercising the direct-call path.

The ENOTDIR re-check does not cover a root that is deleted rather than
replaced: that fails ENOENT, takes the existing isNotFound branch, and
reports the same permanent operator failure as a 404. Declined
deliberately, with the reason recorded: an ENOENT 404 is the most common
answer a static server gives, so validating the root there puts a second
stat on the hot path for a diagnostic, where the ENOTDIR re-check pays
nothing on it. Recorded as a trade rather than a principle, along with
the note that 404 is not false in either case — what the 500 buys is
telling the operator which mistake they made. The version answering both
is holding the root open, which is the effect stat-then-read is blocked
on, and this is a second reason to want it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 349600156d

ℹ️ 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 thread fjs/web/todo/notdir-status.md Outdated
Comment on lines +100 to +101
So the mapping re-checks: on `ENOTDIR`, `stat` the root, and answer `404` only
if it is still a directory — otherwise `500`, which is again the true answer.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Normalize the empty root before validating it

When fjs web '' is used, the supported empty-root argument is normalized by served to .; the existing emptyRoot proof covers this behavior. If either proposed root check applies stat to the raw argument as written here, stat('') returns ENOENT, so startup rejects a valid invocation and the request-time recheck misclassifies ENOTDIR beneath respond(''). Specify that both checks use served(root) (or an equivalently normalized root) before this design is implemented.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed and fixed in 2b01b11. Both checks said "stat the root", and the raw argument is not always the served directory.

served maps '' to ., and fjs web '' is supported with two emptyRoot proofs behind it — one on resolve(''), one driving main with args: [''] and asserting it prints serving . on http://127.0.0.1:8080/. statSync('') is ENOENT, verified, so the startup check would have rejected a valid invocation and the re-check would have misjudged every ENOTDIR under respond('').

Both now say served(root) explicitly, and the task points at the proof that pins it — the existing emptyRoot case would have caught the startup half, but only for an implementer who ran it before wondering why, and the re-check half it would not have caught at all.

Worth noting this is the second time served's normalization has mattered to a design that did not mention it: its own JSDoc records that fjs web '' would otherwise serve /etc/passwd, because join('', 'etc') reads as absolute. An empty root is a value the argument default cannot catch, and evidently one a proposal has to name rather than assume away.

Codex P2: both root checks said "stat the root", and the raw argument
is not always the served directory. served maps '' to '.', and
`fjs web ''` is supported with two emptyRoot proofs — one on resolve(''),
one on main asserting it prints "serving . on ...". Statting '' fails
ENOENT, so the startup check would have rejected a valid invocation and
the re-check would have misjudged every ENOTDIR under respond('').

Both checks now say served(root) explicitly, and the task points at the
proof that pins it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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: 2b01b113fb

ℹ️ 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 thread fjs/web/todo/missing-index-message.md Outdated
Comment on lines +108 to +110
An encoding pass over the echoed path is total and needs no list of
dangerous characters: encode everything outside the safe set, and `%1B`
survives as `%1B` whether it arrived encoded or raw. It also makes the

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Specify how validated escapes survive encoding

For an already encoded request such as /%1B%5B31m/, ordinary URI encoders encode the percent signs and produce /%251B%255B31m/, contradicting both this claim and the required proof at lines 155–159; preserving % indiscriminately is not a complete encoding rule either. The fresh problem after adding the encoding pass is that the design still needs to define the safe set and explicitly say how validated %XX triplets are preserved while raw controls are encoded, rather than leaving the implementer to invent the security-sensitive transformation.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right — "encode everything outside the safe set" left the security-sensitive half to the implementer, and the obvious reading of it contradicts this file's own proof task. Fixed in 6529b19.

Your counterexample is the decisive one: a general-purpose URI encoder escapes the % as well, so /%1B%5B31m/ comes back /%251B%255B31m/ — unreadable, and not what the proof asks for. And preserving every % is not a rule, since a lone one is not an escape. The pass is now three cases:

  • a %XX triplet passes through verbatim — isEscape, the predicate percentDecode already uses, so the rule reuses the module's own notion rather than inventing a second one;
  • unreserved characters and / pass through;
  • every other byte is percent-encoded, which is what catches a raw control character.

Preserving triplets is total rather than a special case, which is the part that makes this safe to state so briefly: by the time the message is built, resolve has accepted the path and percentDecode rejects a malformed escape, so every % present already begins a valid triplet. The direct-call path you raised earlier does not weaken that — it takes the same route through resolve, so it cannot smuggle a lone % past; what it can carry is a raw control byte, and that is exactly what the third rule encodes. So the two findings close against each other rather than needing separate guards.

Comment on lines +105 to +107
So the mapping re-checks: on `ENOTDIR`, `stat` `served(root)`, and answer
`404` only if it is still a directory — otherwise `500`, which is again the
true answer.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Put the root recheck in an implementable effect chain

The fresh issue introduced by the revalidation requirement is that the current fileResponse is a synchronous (path) => (Result) => ServerResponse mapper with neither root nor the ability to execute stat, while respond invokes it inside resultMapStep (fjs/web/module.f.mjs:483-535). Thus the later instruction to perform this check “from fileResponse” cannot be implemented as written without redesigning the effect chain; specify whether the ENOTDIR branch moves into respond, or exactly how fileResponse becomes effectful and its nested effect is flattened.

AGENTS.md reference: AGENTS.md:L47-L49

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed against the code, and fixed in 6529b19 — "re-check from fileResponse" was not implementable as written.

fileResponse is (path) => (Result) => ServerResponse: pure, and holding neither root nor any way to run a stat. I had carried "keep it local to fileResponse" forward from the round where the question was which predicate to widen, where it was the right answer, without re-checking that it still fit once the decision needed an effect.

The branch goes in respond, which has both root and the effect chain. The change is smaller than redesigning anything: respond already ends in resultMapStep(bytes, r => ok(fileResponse(path)(r))), and resultMapStep is by definition resultStep over a pure function. Dropping to resultStep is the whole edit — the ENOTDIR case becomes a step(stat(served(root)), …) deciding 404 or 500, every other case stays fileResponse(path)(r) exactly as today, and fileResponse keeps its signature and its purity.

That also preserves what the original "local, not isNotFound" decision was actually protecting: the reading stays inside fjs/web, and the two fjs/cas callers are untouched. Only the placement within the module moves.

sergey-shandar and others added 3 commits August 26, 2026 09:18
Two codex findings, both on mechanisms the last two commits added
without specifying.

The encoding pass said "encode everything outside the safe set", which
leaves the security-sensitive part to the implementer — and the obvious
reading is wrong: a general URI encoder escapes the % too, turning
/%1B%5B31m/ into /%251B%255B31m/, contradicting this file's own proof
task. Preserving every % is not a rule either, since a lone one is not
an escape. Now stated as three cases: a %XX triplet passes verbatim
(isEscape, the predicate percentDecode already uses), unreserved and /
pass, everything else is encoded. Preserving triplets is total rather
than a special case, because resolve has already accepted the path and
percentDecode rejects a malformed escape, so every % present begins a
valid triplet — including on the direct-call path, which can carry a raw
control byte but not a lone %.

"Re-check from fileResponse" was not implementable: fileResponse is
(path) => (Result) => ServerResponse, pure, holding neither root nor any
way to stat. respond has both and ends in resultMapStep, which is by
definition resultStep over a pure function. So the branch goes in
respond and the change is dropping to resultStep: the ENOTDIR case
becomes step(stat(served(root)), ...), everything else stays as it is,
and fileResponse keeps its signature.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit eb8da19 Aug 26, 2026
19 checks passed
@sergey-shandar
sergey-shandar deleted the msg branch August 26, 2026 18:16
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.

2 participants