emergent_testing: proof.f.mjs no longer constructs promises - #1747
emergent_testing: proof.f.mjs no longer constructs promises#1747sergey-shandar wants to merge 3 commits into
Conversation
Implements and deletes `todo/promises-in-proof-f-mjs.md`: the two `Promise.resolve` fixtures were the last construction of the `Promise` global in any authored `.f.mjs`. Neither promise was ever consumed, which is what made this a simplification rather than a move to `.mjs`. Every mock runner in this file intercepts the `test` *effect* and reads the `TestContext` as data, so `registerNoopCtx.test` was never invoked; it now panics if called -- enforcing "never called" and satisfying `TestFn` honestly at once, since a throwing body has type `never`, assignable to the `Promise<void>` the signature demands, with no cast. And `registerOne` routes every leaf through the `await` effect unconditionally while that proof's handler ignores the payload and answers `notImplemented`, so the awaited-leaf fixture reaches the same path with plain `undefined`. A grep for the `Promise` global across authored `.f.mjs` is now empty; the remaining matches are identifiers (`awaitIfPromise`, `awaitPromise`) and JSDoc types. `fjs t` 3478/3478, `npx tsc` clean. Local bun shows the same failure set with and without this diff (bun 1.3.11 here vs 1.4.0 in CI, which was green at the base commit), so none of it is this change's. Changelog: - `emergent_testing`: `proof.f.mjs` no longer constructs promises -- the last `Promise` construction in any authored `.f.mjs`. Both were unconsumed fixtures: the never-invoked `TestContext` stub now panics if called, and the awaited-leaf fixture reaches the `await` operation with a plain value
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 25a22f4 | Commit Preview URL Branch Preview URL |
Aug 28 2026, 12:51 AM |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a355054ae8
ℹ️ 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".
| @@ -0,0 +1,4 @@ | |||
| - `emergent_testing`: `proof.f.mjs` no longer constructs promises — the last | |||
There was a problem hiding this comment.
This commit changes only test fixtures and their documentation, without altering runtime behavior or the public API, so publishing this as a user-facing release note adds noise to the packaged changelog. Repository policy explicitly says test-only changes and internal refactors need no changelog entry; omit this file and the matching Changelog: section.
AGENTS.md reference: AGENTS.md:L100-L105
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and checked against the source rather than taken on trust. AGENTS.md, verbatim:
a PR that doesn't [have a user-visible change] — internal refactors, test-only changes, and PRs that only touch
todo/,AGENTS.md, or other documentation — needs neither.
This PR touches two proof fixtures and a todo/; nothing in the published package behaves differently. Entry file deleted, Changelog: section removed from the body, replaced with a line saying why there is none.
Worth a sentence on why I got it wrong, since it is the mirror image of a mistake from the previous PR: there I wrote an entry that said too much (rationale, a todo/ link); here I wrote one that should not exist. Both come from treating the changelog as part of the PR's story rather than as what it is — a release note for package users, who see nothing in this change.
Generated by Claude Code
AGENTS.md, verbatim: a PR that has no user-visible change -- "internal refactors, test-only changes, and PRs that only touch todo/" -- needs neither the entry file nor the `Changelog:` section. This one touches two proof fixtures and a todo; nothing in the published package behaves differently, so the entry was release-note noise.
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved. Verified at 25a22f4f5 on node v23.11.0: npm test 3478/3478 exit 0 and tsc --noEmit exit 0 on both sides. The headline -57 is mostly the deleted todo; proof.f.mjs itself is +16/-3 and the sorted list of exported names is unchanged — no leaf added or removed. I checked the two rewritten expressions are not load-bearing rather than assuming it: both go through the mock await effect unconditionally and the handler ignores the payload, so the promise value never carried anything the proofs pin.
The hang I raised on #1742 is now written down honestly. I reproduced it again — constructor replaced with a non-Promise function plus a no-op then, bounded with timeout 90 — and the run never prints a total; node exits 13 with "Detected unsettled top-level await". imports-promises-realms.md at this head has a section titled "fjs t still hangs on a promise whose constructor was replaced", says hangs rather than "reported as a failure", and marks it deferred with fjs t owes the same fix tracked against step 4. That is the right disposition for it.
One thing to be clear-eyed about, not blocking: the rule this rests on is convention only. I added a .f.mjs proof returning Promise.resolve(undefined) and ran every gate — tsc --noEmit exit 0, npm test picked it up and reported ok (3478→3479, exit 0), npm run website selected it into the browser bundle (137→138). Nothing rejects it. So "a .f.mjs constructing a promise is a defect, not an exception" is true as policy and unenforced in fact; the documents say so, and it is worth keeping visible until something checks it.
Changelog handling is right, and the PR self-corrected — an entry was added, then removed at 25a22f4f5 citing AGENTS.md's test-only/todo/-only exemption, which matches the rule verbatim.
3 participants
Implements and deletes
todo/promises-in-proof-f-mjs.md, filed in#1742: the two
Promise.resolvefixtures inproof.f.mjswere the last construction of thePromiseglobal in any authored.f.mjs. The rule inemergent_testing/README.md#scope— a.f.mjsthat breaks the subset is adefect to fix, not an exception to design around — and this closes the one known
case.
Neither promise was ever consumed
That is what made this a simplification rather than a move to
.mjs:registerNoopCtx.testis never invoked. Every mock runner in the fileintercepts the
testeffect; the context only travels as data. The stub nowpanics if called — which enforces "never called" and satisfies the type
honestly at once: a throwing body has type
never, assignable to thePromise<void>thatTestFndemands, with no cast and noPromise.registerOneroutesevery leaf through the
awaiteffect unconditionally, and that proof's mockhandler ignores the payload and answers
notImplementedregardless. Plainundefinedreaches the same path.A grep for the
Promiseglobal across authored.f.mjsis now empty; theremaining matches are identifiers (
awaitIfPromise,awaitPromise) and JSDoctypes. That makes the README's convention checkable by a one-line grep, should a
CI step ever want it.
No changelog entry: this is a test-only change with no user-visible behaviour,
which AGENTS.md says needs neither the entry file nor a
Changelog:section.Verification
npx tscclean,fjs t3478/3478,npm run covexit 0 (100% thresholds).Local bun shows an identical failure set with and without this diff — local bun
is 1.3.11 against CI's 1.4.0, which was green at the base commit — so none of it
belongs to this change.
🤖 Generated with Claude Code
https://claude.ai/code/session_016PyLwDNkPQM1uD1Tg5ApBg