Repository navigation
Enable existing assertions in ordinary REPL entries - #1067
Conversation
PR #1067: Enable existing assertions in ordinary REPL entries6 files, +293 / -42 Scope✅ PR scope looks good. StructuralOxlint structural signals:
Slop✅ Slop indicators look low. Static AnalysisOxlint: 2 diagnostics across 1 file (2 rules) no-array-sort (1): packages/cli/src/repl-profile.ts CorrectnessFILE: packages/cli/src/repl-profile.ts FILE: packages/cli/src/repl-profile.ts FILE: packages/cli/src/repl-profile.ts |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Verdict: request changes (one finite correction). Posted as a COMMENT-event review because this account authored the PR and GitHub refuses REQUEST_CHANGES from the author; the verdict is this line.
Independent review of head d2edf309df5f9fd3513d443cb4cb55fa3490c136 against base 2e67bab7 (#1065) for #1066: correctness, minimalism, and the two affected timing boundaries.
Correctness. The production change is packages/cli/src/repl-profile.ts:222-223: register the original Assert* entries of TESTING_REGISTRATIONS in the command/profile scope and contribute the existing testing documentation. Traced through the real callers:
- Registration is
defaultkind, so a repositorycomponents/Assert.mdstill wins and removing it restores the built-in (A5). - The headless child and
REPL.Test'suseTestingchild re-register in their own scopes; the collision index is own-scope and the registry mergesat: "min", so no collision. Both documentation contributions are value-identical and fold incapturedDocumentation, so no duplicate-documentation refusal. - No activation guard,
Staging,TestingorTestAgentis installed, so a bare<Test>keeps its "requires a testing package" refusal and<TestAgent>stays unresolved. - Failure path: the
AssertionErrorends the entryerrwith both operands, the later<File>never reaches the write seam (the sameAPI.Files.aroundseam W1 in this file proves positive), the gated cleanup joins exactly once, successor admission refuses while held, and the same session runs the next entry. The red witness is retained (expectedok, receivederrbefore the profile change).
Finding (correction, inline at repl-profile.ts:367). packages/cli/src/documents/sidekick/entries.md lines 18–21, the <Guide.Entries /> chapter the Sidekick reads before preparing an entry, still says built-in assertions are absent from the ordinary entry profile and must go through REPL.Test. #1053 wrote that; this PR makes it untrue, and it now contradicts the new REPL.Syntax footer and the contract clause that named documentation agrees with admission.
Minimalism. Smallest working composition of existing exports; no new API, state or lifecycle. Two non-blocking nits inline.
Performance (measured on this host, head d2edf30, Deno 2.9.7, 300 samples after 20 warmup, scoped iterations).
| boundary | before | after |
|---|---|---|
per entry, capturedDocumentation p50 |
0.13 ms | 0.21 ms |
per command, registerComponents(Assert*) p50 |
— | 20.1 ms, paid twice (command scope + headless child) |
The per-command cost is compilePropsSchema at ~1.3 ms per declaration, the same pre-existing core cost xmd run pays for the full list (27.8 ms p50). No memory measured; total REPL startup not measured, so no proportion is claimed. Probe scripts and raw samples are in the review attempt's perf/ directory; nothing in the checkout changed.
Not covered here: CI, live model, terminal run, signature verification. Main health and required checks remain merge gates.
| return ( | ||
| renderSyntaxMarkdown(yield* Syntax.operations.symbols) + | ||
| "\nMarkdown test components are documented by name with REPL.Syntax and run through REPL.Test.\n" + | ||
| "\nAssertions are available in ordinary entries. Markdown test helpers are documented by name with REPL.Syntax and run through REPL.Test.\n" + |
There was a problem hiding this comment.
Correction (required): the Sidekick's own entry guide now contradicts this footer.
packages/cli/src/documents/sidekick/entries.md:18-21 (<Guide.Entries />, which the Sidekick is told to read before preparing an entry) still says:
Built-in assertions absent from the ordinary entry profile are Markdown-test components. Named lookup explains their test context and
REPL.Testinvocation. Save the test document and request it withREPL.Test; submitting assertions as an ordinary entry does not activate testing.
After this PR the built-in assertions are present in the ordinary entry profile. Consequence: the chapter steers the agent to write a .test.md and call REPL.Test for exactly the check this story exists to make an ordinary entry, and it disagrees with this footer and with REPL.Syntax names={["Assert"]} answering "Available in this evaluation: yes". The accepted contract requires named documentation to agree with admission.
Evidence: git log -L18,21:packages/cli/src/documents/sidekick/entries.md shows the bullet was last written by #1065 (#1053) for the pre-#1066 state; grep -rn "absent from the ordinary entry profile" packages scripts specs finds no test freezing the text, so this is a prose-only edit.
Recommendation: rewrite the bullet to state the new availability and keep the distinction that still holds, e.g.: "Built-in assertions (Assert, AssertEquals, AssertThrows, …) run in ordinary entries; a failing assertion fails that entry and stops its later effects. Test, Testing and the nested harness remain Markdown-test components: named lookup explains their test context and REPL.Test invocation, and a bare <Test> submitted as an ordinary entry refuses rather than activating testing." No other Sidekick chapter mentions assertions (environment.md only names REPL.Test for real tests).
| plugins: CommandPlugins, | ||
| facts: ReplProfileFacts = {}, | ||
| ): Operation<ReplProfileAssembly> { | ||
| yield* registerComponents(TESTING_REGISTRATIONS.filter(({ name }) => name.startsWith("Assert"))); |
There was a problem hiding this comment.
Non-blocking. Selecting by name prefix is the smallest composition available without a new export, and the plan ruled out a new API, so this is fine as written. Note the brittleness for whoever next touches TESTING_REGISTRATIONS: any future entry whose name starts with Assert (not necessarily an assertion) is registered in ordinary entries silently. No action required for this PR.
| testingDocumentation, | ||
| useTestingComponents, | ||
| } from "@executablemd/testing"; | ||
| import { registerComponents, contributeDocumentation } from "@executablemd/core"; |
There was a problem hiding this comment.
Nit, non-blocking: this file already imports from @executablemd/core on line 36; registerComponents and contributeDocumentation can join that import instead of a second statement.
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Verdict: pass for the required correction (comment 4437958698 of review 5730949706). Posted as a COMMENT-event review because this account authored the PR; the verdict is this line.
Verified only the correction at head b07fb07c5399f176d300cab3bc72e0a0dd481e0c:
git diff --stat d2edf309..b07fb07ctouches one file,packages/cli/src/documents/sidekick/entries.md(+6 −4). No TypeScript, test or spec changed, so the behavioural and performance evidence from my review atd2edf309stands unchanged; nothing was re-run and no new timing or test claim is made here.- The
<Guide.Entries />bullet now states that<Assert>,<AssertEquals>and<AssertThrows>run directly in ordinary entries, that a failing assertion fails that entry and stops later ordinary effects, and that assertions do not activate testing. That matches the admitted/executed behaviour the owning tests prove. - A second bullet keeps the boundary:
Test,Testingand the nested harness belong in Markdown test documents requested through<REPL.Test />, and a bare<Test>in an ordinary entry refuses without running its body or activating testing. That matches the retained refusal. - The obsolete "absent from the ordinary entry profile" advice is gone; the rest of the chapter is unchanged and still accurate.
The two optional inline nits from the original review (prefix filter, duplicate import) remain non-blocking and were correctly left out of this correction. Original review: #1067 (review). Main health and required CI remain merge gates.
Use existing assertion components directly in ordinary REPL entries. Passing assertions continue execution. An unhandled failure reports its diagnostic, prevents later effects and joins cleanup; the same session can then run another entry.
The ordinary profile registers the original assertion definitions and documentation without activating testing or installing a TestAgent. Syntax discovery and Guide.Entries describe the same availability. Component overrides, permissions, entry identity and journal ownership retain their existing behavior. Explicit Markdown tests and ordinary run assertions keep their boundaries; a bare Test in an ordinary entry still refuses without executing its body.
Validation at feature head d2edf30: six ordinary REPL journey steps, three explicit REPL.Test steps and 44 assertion/CLI steps pass. Evidence covers the advertised source, failure diagnostics and a suppressed file effect, held cleanup and successor admission, AssertThrows forms, invalid props, overrides and explicit test boundaries. The original Planner accepted that exact signed head.
Fable’s full review requested one guide correction. The guide-only child b07fb07 closes it, confirmed by the same reviewer’s targeted follow-up. The full review includes measured registration/documentation costs and their limits; neither the review, benchmarks nor product tests were repeated for the prose correction. Optional prefix/import nits remain nonblocking. Required CI is green at b07fb07.
Fixes #1066.