fix(pi-plugin/dreamer): load pi-coding-agent from the running Pi, not a stale tree copy - #367
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the dreamer’s pi-coding-agent module resolution so it preferentially loads the version associated with the currently running Pi process (to avoid stale/mismatched extension-tree copies that can break retrospective / refresh-primers at runtime).
Changes:
- Reorders
defaultLoadersto prefer resolving from the running Pi entry before falling back to a bare import. - Replaces the previous
require.resolveapproach with a filesystem walk to find the owningpackage.jsonandimport()the ESM entry. - Adds a unit test asserting the default loader ordering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/pi-plugin/src/dreamer/pi-session-api.ts | Reorders loaders and implements filesystem-based resolution of @earendil-works/pi-coding-agent from the running Pi entrypoint. |
| packages/pi-plugin/src/dreamer/pi-session-api.test.ts | Adds an assertion that the “running Pi entry” loader is first and “Bare import” remains as a fallback. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
da65128 to
54e0a6d
Compare
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Thanks for this — the defect is real and your diagnosis is right. Confirmed independently: the extension tree can carry a stale peer copy of pi-coding-agent that the bare import wins before the running Pi is ever considered, and the old createRequire fallback cannot resolve Pi 0.83+ at all (those packages expose only an "import" export condition, so CJS resolution conditions never match). Moving running-Pi-first is the correct direction, and the realpath handling of the bin shim looks right. Review verdict is request-changes on four concrete items before this can merge (noting your head moved twice during review — if the newest commits already cover an item, say so and I'll re-verify at the current head):
The change is small and in good shape stylistically — with these four it's mergeable. Happy to re-review quickly on your next push. |
…i resolution Two issues flagged in review of cortexkit#367: 1. P2 - the bin-shim symlink test could pass without exercising the walk. A broken "Resolve from running Pi binary entry" loader would silently fall back to "Bare import", which loads the real installed pi-coding-agent whose listAll is also a function, so the old assertions gave false confidence. The fake copy now exports a unique marker (__piShimFakeMarker) and the test asserts it, making the fallback fail loudly instead of masking the regression. 2. P3 - when argv[1] is missing (packaged binary / CLI-arg run) the loader falls back to process.execPath, but the "Could not locate ... package.json from" error interpolated the undefined process.argv[1] (printing "from undefined"). The message now names the resolved entry actually walked, and a regression test covers the packaged-binary path. Tests: dreamer suite green (7 in this file; the two "installed package" tests require the devDependency tree). biome clean.
Address magic-alfonso's four request-changes on cortexkit#367: 1. Bundled/source layouts: detect compiled Bun binaries explicitly (execPath is the binary, argv[1] is a user CLI arg) and fall through cleanly; only accept script-like on-disk entries (Jiti virtual modules and CLI args now fall back to execPath); when the running entry is TypeScript (src/cli.ts source checkout), load the dist->src source counterpart and refuse to silently select stale build output. 2. Skip Pi's copied dist/package.json metadata: mirror Pi's own findNodePackageDir special case — when a matching manifest sits in a dist/ dir whose parent also owns a matching manifest, the parent is the package root (prevents dist/dist/index.js). 3. Decisive tests: fixtures now return unique markers through SessionManager.listAll, asserted end-to-end via the public loadDefaultPiSessionApi path so the bare-import fallback cannot green a broken resolver; add the core scenario — a stale extension-tree copy (mocked bare import) alongside a running-Pi fixture — plus coverage for the dist-metadata, traversal, and source-mode layouts. 4. Validate manifest entries like Node's export-target validation: "./"-prefix required for exports targets, no absolute paths, and a containment check so entries cannot escape the package root.
|
All four items addressed at 0186822 — thanks for the sharp review. 1. Bundled/source layouts — Compiled Bun binaries are now detected explicitly ( 2. Copied dist/package.json — Mirrored Pi's own 3. Test decisiveness — Fixtures now return unique markers through 4. Manifest path trust — Verification: 14/14 in pi-session-api.test.ts, full pi-plugin suite 820/820, |
|
Re-reviewed at 0186822 — three of the four are verified closed, and closed well: the compiled-binary detection with clean fall-through (never walking stale output from a user-controlled argv[1]), the src/cli.ts mapping that refuses stale dist when the source counterpart is absent, the dist/package.json parent-root skip with its fixture, and the stale-vs-running test whose assertion genuinely fails if running-Pi resolution loses (the mocked stale bare-import is fine for proving loader ordering — I'm not asking for a full FS-resolution integration fixture). One item remains open, and it's small: the ./-prefix validation is currently gated on fromExports, so a module/main target like "dist/index.js" (no ./ prefix) skips the relative-path requirement entirely. Apply the same validation to every manually joined manifest target regardless of which field supplied it, and add the negative tests for non-prefixed exports/module/main entries. Two adjacent gaps worth closing in the same pass since they can silently reintroduce the original bug via fall-through: the script whitelist misses extensionless and .tsx/.jsx entries, and the manifest parser only handles string exports["."] / exports["."].import — valid conditional/array export shapes fall back to bare import. Fall-through to bare import is the designed safety valve, but each unhandled-but-valid shape widens the stale-tree window the PR exists to close. With the ./ validation fixed + negative tests, and a green run of the head test file, this merges. Genuinely impressive turnaround on round one. |
fe5b54f to
12237a5
Compare
Done |
|
Round 3 verified at 12237a5 — the validation item is genuinely closed (./-requirement applies to every manually joined target with containment after resolve; and since a loader throw just falls through to bare import, the added strictness on module/main can only degrade to the pre-PR status quo, never break a working setup), and both adjacent gaps got real coverage: extensionless bin-style entries, .tsx source mode, and the conditional/array exports shapes with the default-condition fallback. One last one-liner, from running your head test file on macOS (21 pass / 1 fail): the bin-shim symlink test builds its expected path from tmpdir() unrealpathed, but the resolver (correctly) realpaths — on macOS /var is a symlink to /private/var, so expected Fix that line and this merges — no further review round needed on my side; I'll take it from the push. |
… a stale tree copy
The dreamer's defaultLoaders resolved @earendil-works/pi-coding-agent in an
order that could load a STALE or MISMATCHED copy from an extension tree
instead of the Pi binary that owns the live session format, breaking
retrospective / refresh-primers at runtime. Two problems:
1. Loader order. "Bare import" was first, so the dreamer loaded whatever
pi-coding-agent the extension tree happened to resolve to. Host peers in
the extension tree are not managed by `pi update --extensions`
(pi installs extensions with --omit=peer / --legacy-peer-deps so host
@earendil-works/pi-* peers are intentionally NOT solved), so a peer
auto-installed once can drift from the running Pi and never be updated.
When the stale copy's transitive imports hit a removed/renamed subpath
(e.g. highlight.js/lib/index.js before 0.84.x), the dreamer crashed with
"Failed to resolve ... via all strategies".
2. The fallback loader was broken. "Resolve from running Pi binary entry"
used `createRequire(process.argv[1]).resolve('<pkg>')`, but
pi-coding-agent ships ESM-only exports (only an "import" condition, no
"require" condition), so require.resolve threw ERR_PACKAGE_PATH_NOT_EXPORTED
and the fallback never worked — meaning a stale-tree bare import was the
ONLY path that could succeed, and when it loaded a mismatched version the
dreamer had no working escape hatch.
Fix:
- Put "Resolve from running Pi binary entry" FIRST so the dreamer always
loads the same pi-coding-agent that owns the live session format.
- Replace the broken require.resolve with a filesystem walk from
process.argv[1] up to the package.json whose name is
@earendil-works/pi-coding-agent, then import its ESM entry
(exports[''].import / module / main) directly. Handles ESM-only exports
and works regardless of whether the host package is installed via bun,
npm, or a managed/bundled layout.
- Keep "Bare import" as a second fallback for non-standard layouts.
Tests: add a default-loader-order assertion (running-Pi entry first) and
keep the existing ladder/memoization tests green (7 pass). Full pi-plugin
suite: 813 pass / 0 fail. typecheck + biome clean.
12237a5 to
e0b8f84
Compare
Done |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/pi-plugin/src/dreamer/pi-session-api.test.ts">
<violation number="1" location="packages/pi-plugin/src/dreamer/pi-session-api.test.ts:245">
P2: The "rejects a manifest entry that escapes the package root" test never reaches the path-containment guard it claims to cover. resolveManifestEntry first rejects any target without the "./" prefix, and "../../outside.js" starts with "..", so it throws the prefix error before the `resolved !== root` containment check runs. Use a target that is "./"-prefixed but still escapes the root (e.g. "./../../outside.js" or "./../escape.js") so the containment branch is actually exercised; as written, that security-relevant path remains untested.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }); | ||
| }, 30000); | ||
|
|
||
| it("rejects a manifest entry that escapes the package root", async () => { |
There was a problem hiding this comment.
P2: The "rejects a manifest entry that escapes the package root" test never reaches the path-containment guard it claims to cover. resolveManifestEntry first rejects any target without the "./" prefix, and "../../outside.js" starts with "..", so it throws the prefix error before the resolved !== root containment check runs. Use a target that is "./"-prefixed but still escapes the root (e.g. "./../../outside.js" or "./../escape.js") so the containment branch is actually exercised; as written, that security-relevant path remains untested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/dreamer/pi-session-api.test.ts, line 245:
<comment>The "rejects a manifest entry that escapes the package root" test never reaches the path-containment guard it claims to cover. resolveManifestEntry first rejects any target without the "./" prefix, and "../../outside.js" starts with "..", so it throws the prefix error before the `resolved !== root` containment check runs. Use a target that is "./"-prefixed but still escapes the root (e.g. "./../../outside.js" or "./../escape.js") so the containment branch is actually exercised; as written, that security-relevant path remains untested.</comment>
<file context>
@@ -52,6 +130,318 @@ describe("loadDefaultPiSessionApi", () => {
+ });
+ }, 30000);
+
+ it("rejects a manifest entry that escapes the package root", async () => {
+ const dir = mkdtempSync(join(tmpdir(), "pi-traversal-"));
+ const pkgRoot = join(dir, "pi-coding-agent");
</file context>
|
Merged — thanks for a model contribution arc: four review rounds, every item closed with real tests, and the final stale-tree discrimination test is the kind that keeps this fixed permanently (it fails against the pre-PR resolver by construction). One correction on my side for the record: my earlier hesitation over a "650-line rework" was my own misreading — the compare endpoint reports cumulative file totals against the shared parent, not the head delta. Your final push was exactly the promised one-line realpath fix. The v0.41 release will carry this; I'll note here when it ships. |
Problem
The dreamer's
defaultLoadersresolved@earendil-works/pi-coding-agentin an order that could load a stale or mismatched copy from an extension tree instead of the Pi binary that owns the live session format, breakingretrospective/refresh-primersat runtime with:Two compounding issues:
1. Loader order — stale tree copy wins
"Bare import" was first, so the dreamer loaded whatever
pi-coding-agentthe extension tree resolved to. Host peers in the extension tree are not managed bypi update --extensions: pi installs extensions with--omit=peer(bun) /--legacy-peer-deps(npm) so host@earendil-works/pi-*peers are intentionally NOT solved. A peer auto-installed once can drift from the running Pi and never be updated. When the stale copy's transitive imports hit a removed/renamed subpath (e.g.highlight.js/lib/index.jsbefore 0.84.x), the dreamer crashed.2. The fallback loader was broken
"Resolve from running Pi binary entry" used
createRequire(process.argv[1]).resolve('<pkg>'), butpi-coding-agentships ESM-only exports (only animportcondition, norequirecondition), sorequire.resolvethrewERR_PACKAGE_PATH_NOT_EXPORTEDand the fallback never worked. A stale-tree bare import was the ONLY path that could succeed; when it loaded a mismatched version the dreamer had no working escape hatch.Fix
pi-coding-agentthat owns the live session format.require.resolvewith a filesystem walk fromprocess.argv[1]up to thepackage.jsonwhosenameis@earendil-works/pi-coding-agent, thenimport()its ESM entry (exports['.'].import/module/main) directly. Handles ESM-only exports and works regardless of install layout (bun / npm / managed-bundle).Why this is the right layer
The dreamer runs in a separate child process and so can't use pi's in-process
VIRTUAL_MODULES/jiti aliases that other extensions use to resolve@earendil-works/pi-coding-agent. It must load a physical copy, and that copy must match the running Pi's session API version. Pointing it at the running Pi binary entry (rather than an unmanaged extension-tree peer) is the only way to stay correct acrosspi update --extensions.Tests
packages/pi-pluginsuite: 813 pass / 0 fail.typecheck+biomeclean.Repro environment
0.84.3(global, bun-managed), extension tree at~/.pi/agent/npmwith a leftover@earendil-works/pi-coding-agent@0.83.0host peer./ctx-dream→retrospectivefailed with "Failed to resolve ... via all strategies".Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Greptile Summary
The PR changes Dreamer’s shared Pi session-module resolver to prefer the package containing the running Pi entry rather than an extension-tree peer.
pi-coding-agentmanifest and imports its ESM entry directly.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Dreamer requests Pi session API] --> B{Compiled Bun binary?} B -->|Yes| F[Bare-import fallback] B -->|No| C[Resolve real running entry] C --> D[Walk upward to Pi package manifest] D --> E[Resolve and import ESM entry] D -->|Package not found| F E --> G[Expose session listing and parsing API] F --> GReviews (7): Last reviewed commit: "fix(pi-plugin/dreamer): load pi-coding-a..." | Re-trigger Greptile
Context used: