Repository navigation
Mount external directories in the REPL workspace - #991
Conversation
PR #991: Mount external directories in the REPL workspace51 files, +4011 / -426 Scope🔴 PR has 4437 lines changed. Split into focused PRs. 🟡 4437 lines changed. PRs under 400 receive more thorough review. 🟡 51 files changed. Are all changes related? 🟡 Changes span 8 directories. 🟡 Large PR with no linked issue. 🟡 PR mixes config and source changes. 🟡 package.json changed without dependency justification. StructuralOxlint structural signals:
SlopOxlint slop signals:
Static AnalysisOxlint: 210 diagnostics across 19 files (24 rules) no-shadow (41): packages/cli/src/repl/mount-records.ts, packages/cli/src/repl/repositories/deno/provider.ts, packages/cli/src/repl/file-recorder.ts (+8) CorrectnessFILE: packages/cli/src/repl/mount-records.ts FILE: packages/cli/src/repl/repositories/deno/provider.ts FILE: packages/core/src/execute.ts FILE: packages/cli/src/repl/program.ts FILE: packages/cli/src/repl/session.ts FILE: packages/core/src/expand.ts No extraneous code patterns detected. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Fable 5.1 review of PR #991 at head 8d9b6a1d43fa424c7d8bc1ad3f71ee96b442cf56 against main (COMMENT, not an approval or a block).
Scope. The full diff (51 files) plus the surrounding source it changes: expand.ts, execute.ts, invocation-identity.ts, Dir.ts/File.ts, fixed-filesystem.ts, the runtime Files composer, and the REPL recorder/mount modules. CI run 40255186796 is green on this head; I ran no tests and changed nothing.
What holds. The repeatable --mount SOURCE:TARGET grammar and frozen table; canonical dispatch still authenticates through runSelectedForm, and the printErrors mark is read from the dispatcher, so the direct read body is covered; ensure precedes binding and only the invocation's own Unit admits children; the inline Files composer walks scope.contexts exactly as the pinned context-api 0.6.0 does (checked against the vendored copy); legacy repl_repository_local records keep physical matching; a saved mapped read replays without a current pair; mounts grant no permission.
Findings (details inline):
- F1 · correctness · medium —
coverage.targetis realpath-canonical while the association'sdestination.actualis lexical. A covered mapped read or ensure under a symlinked or case-differing SOURCE silently drops to a physical-only v1 record and then refuses on the accepted reassignment cases (A3/A5) instead of replaying.packages/cli/src/repl/mount-files.tsline 303. - F2 · correctness · low — the direct
<Dir>projection copies the narrowed-syntax block, notcontent(), so it omits theDocumentationError→ContentExpansionFailurewrapping and the rendered-prefix handling inoutput/throwmodes.packages/core/src/expand.tsline 3884. - F3 · maintainability · optional — a
packages/coretest now importspackages/cli/src; relative../../test-support/imports bypass the@executablemd/test-support/*mapping. - F4 · maintainability · optional — eleven
describeblocks titledadditional focused boundary N, imports between describe blocks. - F5 · maintainability · optional — the mount installation's admission condition encodes an ordering that cannot occur and would double-admit if it did.
- F7 · plan conformance · low — the overlap error names one target; the plan asks for both.
Style, no action unless the files are touched again. Several files gained imports above their module doc comment (cli.ts, fixed-filesystem.ts, execute.ts, expand.ts, generated-xmd.ts, repositories/deno/files.ts, repositories/deno/provider.ts, session.ts, repl-entry-installation.ts, repl-profile.ts), and Dir.ts, File.ts, fixed-filesystem.ts and deno/files.ts use inline import("…") type references where the file's own convention is a top-level import type.
Note, no action. packages/runtime/files-api.ts reads Effection's scope.contexts. That is the same internal the pinned context-api 0.6.0 reads, so an Effection upgrade breaks both together rather than this file alone; a one-line comment naming that parity would spare the next reader the comparison.
| if ( | ||
| binding === undefined || | ||
| (a.operation === "dir-ensure" && coverage.kind !== "existing") || | ||
| (coverage.kind === "existing" && coverage.target !== binding.destination.actual) |
There was a problem hiding this comment.
F1 · correctness · medium — coverage.target and binding.destination.actual are compared as strings, but they come from different coordinate systems.
routed() in repositories/deno/files.ts computes target through targetOf() (line 243), which realpaths the request cwd before joining. binding.destination.actual comes from admitDocumentHostPath, a pure lexical join of SOURCE and the checked components. When SOURCE has a symlinked component (/tmp → /private/tmp, /var on macOS) or differs in case from the on-disk spelling (case-insensitive APFS; realpath returns the stored case), the two never match.
Impact: every mapped read or ensure that lands inside existing checkout/session coverage then silently takes the recordedLocalOperation branch and is retained as physical-only v1. Nothing warns at record time, and the entry works while the pair is unchanged. On the accepted reassignment cases the v1 name no longer matches the new physical request: an ensure hits the MountRecordError throw in replay(), and a read falls through to a repl_mount_file/v1 durable operation landing on a pending repl_repository_local slot, which is an engine divergence. Either way A3/A5 refuse instead of restoring the saved text or Unit. The PTY journey avoids this by realpathing its temp root first (support/repl-mounts-journey.ts line 15), so the suite does not see it.
Smaller correct alternative: have the router hand retain() the lexical effective path (isAbsolute(request.path) ? request.path : resolve(request.cwd, request.path)) as coverage.target and keep the canonical target for routing only; then parseMountRecord's within(root, actual) either compares against a root in the same coordinates or is dropped, since the plan says coverage root/target confer no authority. Add one T5 row whose SOURCE goes through a symlink (fs.symlink in the fixture) asserting that a repl_mount_file/v1 record is written and that the reassigned run replays with zero ensures.
| : invokeFixedFilesystem(fixed, validatedProps, issued.invocation, { | ||
| filesystem, | ||
| occurrence: expansion.id, | ||
| project: function* (frame) { |
There was a problem hiding this comment.
F2 · correctness · low — This projection copies the narrowed-syntax block above rather than content() (lines 3716–3735). content() wraps a DocumentationError into ContentExpansionFailure([segment], failure, rendered) so the catch at line 3996 can keep the text the region rendered before stopping and can return an output-decided failure as segments. Here the raw DocumentationError is rethrown, so under an output or throw error mode a mounted <Dir> loses that handling while the same document under an ordinary host keeps it. print mode is unaffected, which is why the suite passes.
Alternative: extract one projectSlot(handle, siteErrorMode) helper from content() and use it in all three places (ordinary, narrowed, direct). That removes the third copy and makes the direct path equal by construction. One test row: a mounted <Dir> whose content fails under ErrorMode output, asserting the rendered prefix survives as it does without the mount.
| import { collect, inlineSource } from "@executablemd/core"; | ||
| import { Component } from "../src/component-api.ts"; | ||
| import { executeInstalled } from "../host.ts"; | ||
| import { workspaceMountInstallation } from "../../cli/src/repl/workspace-mount-installation.ts"; |
There was a problem hiding this comment.
F3 · maintainability · optional — This is the only test under packages/core/tests that imports packages/cli/src. Core's suite now depends on the CLI package (workspace-mount-installation.ts, mount-files.ts, evaluation-profile.ts) to exercise core's own expansion frame, inverting the layering the rest of the suite keeps. The R1 row below already shows the alternative: deliver a test-local FilesystemHost through ExecutionInstallation.filesystem. Move the rows that need the real policy to packages/cli/tests, or keep them here on a test-support host.
Same note for ../../test-support/mount-fixture.ts (line 4 here and in retained-mount-records.test.ts): the root deno.json maps @executablemd/test-support/*; add mount-fixture to that map instead of a relative cross-package path.
| import { API, filesFailure } from "@executablemd/runtime"; | ||
| import { Err, Ok, scoped, spawn, suspend, withResolvers, ensure } from "effection"; | ||
| import { DurablePersistenceError } from "@executablemd/durable-streams"; | ||
| describe("additional focused boundary 1", () => { |
There was a problem hiding this comment.
F4 · maintainability · optional — Eleven describe blocks across this file, workspace-mounts.test.ts and runtime/tests/files-call.test.ts are titled additional focused boundary N. deno task test --filter matches the describe title and a failure report prints it, so these titles carry no information. The it sentences already say what each proves; lift them into the describe (append failure, cancellation before child, composed refusal replay, …) or fold the single-it blocks into the describe above. The same files place import statements between describe blocks; hoisting makes it work, but grouping them at the top matches every other test in the repository.
| return { | ||
| admissions: [ | ||
| function* (events: readonly DurableEvent[]) { | ||
| if (access === undefined || entry === undefined) { |
There was a problem hiding this comment.
F5 · maintainability · optional — This reads as if an admission may run before filesystem() delivers. It cannot: invoke() runs the filesystem factory right after Execution.init, and admissions run inside guardedJournal in executeDocument, so with access set entry is always defined here. With access undefined the portable path does need admitReplRepositoryHistory; with access set the REPL plugin already registers it (repositories/plugin.ts line 51), so the entry === undefined leg would only ever double-admit. Spell the two paths explicitly (access === undefined ? admitReplRepositoryHistory(events) : entry.policy.admit(events)) so the next reader does not re-derive the ordering.
| ) | ||
| ) { | ||
| return Err( | ||
| new Error(`--mount TARGET ${JSON.stringify(destination)} overlaps another TARGET.`), |
There was a problem hiding this comment.
F7 · plan conformance · low — The plan asks the overlap refusal to name the conflicting targets. This names only the new one; include the existing pair.target it collided with ("/a/b" overlaps "/a"), which is what someone with several --mount flags needs to fix the line.
Map external directories to explicit document paths with repeatable arguments:
Existing Dir, File, Glob and foreground commands use the mapped filesystem placement and process cwd. Authored parent navigation remains consistent. Acknowledged reads keep their saved values; genuinely new reads follow the current mapping, including reassigned or omitted mappings. Permissions and ordinary host retention keep their existing behavior.
Validation:
deno task test packages/cli/tests/retained-mount-files.test.ts packages/cli/tests/retained-mount-records.test.ts packages/cli/tests/retained-repository-local.test.ts packages/core/tests/filesystem-placement.test.ts— 19 tests, 60 steps passed.deno task test packages/cli/tests/repl-mounts-pty.test.ts— real CLI journey, 1 test and 1 step passed.Full CI, compiled journey execution and runner weight measurement remain pending.