Repository navigation
Acquire REPL repositories inside the session workspace - #1068
Conversation
PR #1068: Acquire REPL repositories inside the session workspace7 files, +458 / -11 Scope🟡 469 lines changed. PRs under 400 receive more thorough review. Structural✅ No structural bloat detected. Slop✅ Slop indicators look low. Static AnalysisOxlint: 1 diagnostic across 1 file (1 rule) unbound-method (1): packages/cli/src/deno-repositories.ts CorrectnessFILE: packages/cli/src/deno-repositories.ts No extraneous code patterns detected. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Independent review — PR #1068 at 80c5ca8b
Verdict: no blocking findings. Correctness and minimalism hold for the accepted #1051 scope. Two non-blocking test observations are posted as comments. Posted as a COMMENT review because the posting account is the PR author.
Correctness
- The managed root is chosen from the direct placement input in
packages/cli/src/deno-repositories.ts; the only supplier ispackages/cli/src/repl/session.ts, which passes a workspace solely forworkspace: trueSubmit intents. Manual and unmarked entries keep the configured host root. The placement type has norootmember, so the spread cannot override the selection. - The provider still creates and canonicalizes the root once and keys leases per root, so slot identity and locking are per workspace. Replay uses each registration's recorded
managedRoot, so acknowledged external placements are neither moved nor revalidated; the cold-reopen row records zero new selections. - Creation-base reuse is preserved as accepted: the W4 row advances A→B with the whole-slot fingerprint unchanged, and the guidance distinguishes requested base from observed HEAD without inferring HEAD from the returned path.
- Repair commit
80c5ca8b:cliCommand([]).commandisprocess.execPathunder Deno, identical to the formerDeno.execPath(), andAPI.Env.operations.envis the existing contextual env read. Behaviour-preserving; it only removes the Deno globals from the Node typecheck. - The test extracts its programs from the shipped Entries chapter, so the published guidance is what passed. The new section sits after "Read the repository in an entry", outside the 975 journey's extraction bounds.
Minimalism
No new API, provider option, schema, selector policy or registry. One join import, consistent with sibling CLI modules. The repositories/repositories/<digest>/checkout address follows the existing root layout the accepted plan froze.
Performance
Not applicable. The change is a constant-cost path join evaluated once per entry installation; no interactive, hot-path, startup or cleanup path changed. No benchmark or profiler was run and no timing or memory assurance is claimed. The added journey reports about 12 s under Deno inside an existing Deno-only file; the corpus did not change.
Evidence used
Published diff against base b5cc8567, the repair commit alone, the Implementor's W1–W7 experiment record for two controlled repositories, Planner acceptance, the CI repair report, and exact-head CI green (run 41380043315).
| yield* ensureDir(saved); | ||
| yield* writeTextFile(join(saved, "acquire.xmd"), acquire); | ||
| yield* writeTextFile(join(saved, "read.xmd"), read); | ||
| const artifactRoot = yield* API.Env.operations.env("XMD_1051_ARTIFACTS"); |
There was a problem hiding this comment.
Non-blocking — minimalism. The artifact exporter (XMD_1051_ARTIFACTS lookup, receipt(), observations, submissions, sourceHash) is roughly 40 lines that never execute in CI; it exists only to write the experiment record the frozen plan asked for.
Consequence: the permanent suite carries evidence-capture machinery with no assertion value, and the W-row observations.push calls read as if they prove something.
Recommendation: now that the record is archived, remove the exporter and sourceHash, and either drop the observations.push calls or turn the facts they record (requested base vs observed HEAD, newSelections) into assertions. Keeping it as-is is acceptable; this does not block.
| yield* refused( | ||
| '<Repository name="refused" url="invalid://unsupported" as="invalid" />', | ||
| "invalid", | ||
| /locator|unsupported|url|repository/i, |
There was a problem hiding this comment.
Non-blocking — test coverage. The refusal matchers over JSON.stringify(terminal) are loose. /locator|unsupported|url|repository/i matches any managed-checkout refusal that contains "repository", so this row cannot tell the invalid-locator refusal from the creation-base or wrong-origin refusals; /checkout|repository|occupied/i at the occupied row has the same shape.
Consequence: a regression that routed the invalid locator into a different refusal would still pass the text check. The row still proves what W5 requires, because binding absence and slot-byte equality carry the discrimination.
Recommendation: anchor on a phrase of the settled diagnostic, e.g. /could not be used as a Git locator/i here and /no longer a readable Git checkout/i for the occupied row (the experiment report records both exact texts).
Acquire repositories beneath the workspace already owned by the REPL session, so later entries can read checkout files through workspace-relative paths. Cold reopening retains the checkout without another acquisition. Unmarked entries retain their existing host placement.
The change uses the existing Repository provider and ownership rules. The requested base identifies creation: if the owned checkout advances, acquisition preserves its current work. Guidance distinguishes that creation base from observed current HEAD. Occupied destinations, incompatible identities and filesystem permissions retain their protection; acknowledged older placements keep their recorded location.
Validation: the original Planner accepted the seven-row real-consumer matrix at feature commit 23010fe. Focused selection/lifecycle/managed suites passed (9 tests, 52 steps), workspace guidance passed (3 tests, 4 steps), and the saved-source experiment passed with two controlled repository inputs. Evidence checks actual origin/HEAD/location, later relative reads, cold reopen without reselection, independent sessions, preserved edits/conflicts and refusal/permission controls.
CI exposed three unresolved Deno references in the new test. The test-only correction 80c5ca8 uses existing contextual/test-support APIs; the previously failing Node project typecheck and affected 1051 journey pass. Candidate correspondence, signatures and initial failures are retained. Required CI is green at 80c5ca8.
Fable’s independent review found no blocking defects at 80c5ca8. Two optional test observations remain nonblocking. The reviewer found no affected performance-sensitive interaction requiring measurement; no benchmark, profiling or timing/memory assurance is claimed.
Fixes #1051.