* test(#2470): failing-first — pi extension must satisfy pi's auto-discovery filter pi auto-discovers extensions/ entries through isExtensionFile(), which accepts only .ts and .js. GSD installs its extension as gsd.cjs, so pi silently skips it: no /gsd command, no error, no log line. Encodes pi's discovery PREDICATE rather than a literal filename, so the contract keeps holding across future renames, and adds the migration-006 test matrix for retiring the stale gsd.cjs left in pre-fix installs. Red until the fix lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): install pi's extension as gsd.js so pi actually discovers it pi auto-discovers extensions/ entries via isExtensionFile(), which accepts only .ts and .js and skips everything else silently. capabilities/pi declared the dest as gsd.cjs, so the extension installed correctly and was then ignored forever: no /gsd command, no error, no log line. Install it as gsd.js. The in-repo source stays pi/gsd.cjs — tests require() it directly and .cjs is unambiguous CommonJS; only the installed name has to satisfy pi, and pi loads accepted files through jiti, which handles CJS and ESM alike. (The reporter's premise that ~/.pi/agent/package.json declares "type":"commonjs" does not hold — pi never writes that file.) Renaming an installed artifact requires a migration record, so add 006 to retire the stale gsd.cjs from pre-fix installs; without it the old path drops out of the manifest and uninstall can never remove it. The migration plans nothing for an unmanifested gsd.cjs: emitting remove-managed there would have the executor downgrade it to preserve-user and mark it blocked, failing the install for anyone who hand-placed their own file. Also pins body-parser >=2.3.0 (GHSA-v422-hmwv-36x6). The advisory reaches the production tree transitively via the Claude Agent SDK and fails the npm-integrity gate, blocking any PR; pinned via the existing overrides idiom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): address orthogonal review findings + register migration checksum Code review: - pi/gsd.cjs's install docstring still told readers to copy the file to extensions/gsd.cjs — the exact silently-broken state this PR fixes. Anyone following it recreated the bug. - Two stale extensions/gsd.cjs comments in install-minimal-hooks.test.cjs. Security review: - _installNativePluginIfDeclared confined nativePlugin.dir but joined nativePlugin.file onto the validated directory unchecked, so a descriptor whose file carried .., an absolute path, or a NUL byte would have written outside configHome. Not reachable in a shipped build (descriptors are first-party and compiled into the capability registry), but file is exactly the field this PR changes. Confine the full dest path instead; for a well-formed descriptor this resolves identically to the previous mkdir(dir) + join(dir, file). Covered by four new write-confinement tests. Also register migration 006 in the #670 EXPECTED_CHECKSUMS baseline — shipped migration bodies are locked to a committed checksum and a new migration fails CI until it is listed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): never dereference a symlinked managed path when snapshotting fs.copyFileSync follows symlinks, so a managed path replaced by a link had the REFERENT's bytes copied into the migration journal's rollback and backup trees — a gsd.cjs symlinked at a private key would land that key's contents under gsd-migration-journal/. Deletion was already safe (fs.rmSync unlinks the link, never the target); the copy was not. Nothing GSD installs is ever a symlink, so the faithful snapshot of a symlinked managed path is the link itself. copyPreservingSymlink recreates it, which keeps rollback fidelity (restore re-creates the same link) while never reading the referent. Scoped the pre-delete to the symlink branch only, so the regular-file path keeps copyFileSync's overwrite-in-place and a mid-restore failure cannot destroy the destination. The restore-side existence check moves to lstat, since existsSync follows a link whose target is gone and would silently skip the restore. This lives in the engine all six migrations share, so 000-005 are hardened too. Also regenerates the pi golden-parity hash: correcting pi/gsd.cjs's own install docstring changes the extension's content, which the golden suite caught. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): symlink-preserve the in-apply failure-recovery restore too The previous commit routed three copy sites through copyPreservingSymlink but missed a fourth: the catch block inside applyInstallerMigrationPlan, which replays rollback snapshots taken earlier in the SAME apply attempt. Those snapshots are symlinks precisely because of that commit, so the raw copyFileSync there dereferenced them and wrote the referent's bytes to the LIVE install path — worse than the journal-tree leak it was meant to fix, since it is user-visible and at a predictable location. Verified by experiment rather than assertion: with the pre-fix line restored, the managed path comes back as a REGULAR FILE containing the referent's bytes; with the fix it comes back as a symlink and the bytes appear nowhere. The accompanying test injects the failure by letting the delete succeed and then throwing once, modelling a later step failing after the delete. That ordering is load-bearing — an earlier draft injected before the delete, which leaves the live path in place, so the pre-fix copyFileSync hit a same-file collision and threw instead of leaking. That draft passed against the bug it was written to catch; this one fails against it. Adds the missing rollback() coverage as well: a restored symlinked managed path must come back as a link pointing at its original target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#2470): read the backup location from the journal, not the plan The new backup-content assertion read backupRelPath off result.plan.actions, where it is always null: the planner reserves the field and apply chooses the concrete location, recording it in the journal. The assertion therefore failed on "backup path must be recorded for the user" rather than on anything about the behavior it was written to check. Read it from the journal, which is the authoritative record. Verified by executing all four new test bodies in-process against the built engine — the backup file exists and holds the locally patched content. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#2470): backfill changeset pr number to 2478 * chore(#2470): backfill changeset pr number to 2478 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -1106,9 +1106,20 @@ function _installNativePluginIfDeclared(
|
||||
if (np && np.source) {
|
||||
const pluginSrc = path.join(src, np.source);
|
||||
if (fs.existsSync(pluginSrc)) {
|
||||
const destDir = runtimeArtifactInstallPlan.assertDestWithinConfigHome(configDir, np.dir);
|
||||
fs.mkdirSync(destDir, { recursive: true });
|
||||
fs.copyFileSync(pluginSrc, path.join(destDir, np.file));
|
||||
// Confine the FULL dest path (dir + file), not just the dir. Previously
|
||||
// only `np.dir` was validated and `np.file` was joined on unchecked, so a
|
||||
// descriptor whose `file` carried `..`, an absolute path, or a NUL byte
|
||||
// would have written outside configHome. Not reachable today — descriptors
|
||||
// are first-party and compiled into the capability registry at build time —
|
||||
// but `np.file` is exactly the field #2470 changes, and the guard costs
|
||||
// nothing. For a well-formed descriptor this resolves identically to the
|
||||
// previous mkdir(dir) + join(dir, file).
|
||||
const destPath = runtimeArtifactInstallPlan.assertDestWithinConfigHome(
|
||||
configDir,
|
||||
path.join(np.dir, np.file),
|
||||
);
|
||||
fs.mkdirSync(path.dirname(destPath), { recursive: true });
|
||||
fs.copyFileSync(pluginSrc, destPath);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -46,6 +46,38 @@ function sha256Text(value: string): string {
|
||||
return crypto.createHash('sha256').update(value).digest('hex');
|
||||
}
|
||||
|
||||
/**
|
||||
* Copy a managed path for the rollback snapshot or the user-facing backup,
|
||||
* WITHOUT dereferencing a symlink.
|
||||
*
|
||||
* `fs.copyFileSync` follows symlinks, so a managed path that has been replaced
|
||||
* by a link (tampering, or an unexpected user layout) would have had the
|
||||
* LINK TARGET's bytes copied into `gsd-migration-journal/…-backups/` — e.g. a
|
||||
* `gsd.cjs` symlinked at `~/.ssh/id_rsa` would land that key's contents in the
|
||||
* backup tree. Nothing GSD installs is ever a symlink, so the faithful snapshot
|
||||
* of a symlinked managed path is the link itself: recreating it preserves
|
||||
* rollback fidelity (restore re-creates the same link) while never reading the
|
||||
* referent. Deletion was already safe — `fs.rmSync` unlinks the link, never the
|
||||
* target.
|
||||
*
|
||||
* Windows note: `fs.symlinkSync` can throw EPERM for unprivileged users. That
|
||||
* surfaces as an apply failure and triggers the normal rollback path, which is
|
||||
* the correct outcome — refusing to proceed beats silently copying referent
|
||||
* bytes.
|
||||
*/
|
||||
function copyPreservingSymlink(srcPath: string, destPath: string): void {
|
||||
if (fs.lstatSync(srcPath).isSymbolicLink()) {
|
||||
// symlinkSync fails with EEXIST on an occupied path, so clear it first.
|
||||
// Scoped to this branch on purpose: the regular-file path below keeps
|
||||
// copyFileSync's overwrite-in-place, so a mid-restore failure cannot leave
|
||||
// the destination destroyed.
|
||||
fs.rmSync(destPath, { force: true });
|
||||
fs.symlinkSync(fs.readlinkSync(srcPath), destPath);
|
||||
return;
|
||||
}
|
||||
fs.copyFileSync(srcPath, destPath);
|
||||
}
|
||||
|
||||
function readJsonIfPresent(filePath: string, fallback: unknown): unknown {
|
||||
if (!fs.existsSync(filePath)) return fallback;
|
||||
try {
|
||||
@@ -627,9 +659,12 @@ function rollbackAppliedMigrationResult({ configDir, journal, journalPath, rollb
|
||||
const rollbackPath = path.join(configDir, action.rollbackRelPath as string);
|
||||
const dest = path.join(configDir, action.relPath as string);
|
||||
try {
|
||||
if (fs.existsSync(rollbackPath)) {
|
||||
// lstat-based existence check: a snapshot of a symlinked managed path is
|
||||
// itself a link, and existsSync() follows it — a link whose target is
|
||||
// gone would read as "missing" and silently skip the restore.
|
||||
if (fs.lstatSync(rollbackPath, { throwIfNoEntry: false })) {
|
||||
fs.mkdirSync(path.dirname(dest), { recursive: true });
|
||||
fs.copyFileSync(rollbackPath, dest);
|
||||
copyPreservingSymlink(rollbackPath, dest);
|
||||
}
|
||||
} catch (error) {
|
||||
failures.push({ relPath: action.relPath as string, error: (error as Error).message });
|
||||
@@ -743,7 +778,7 @@ function applyInstallerMigrationPlan({
|
||||
|
||||
const rollbackPath = path.join(rollbackRoot, normalized);
|
||||
fs.mkdirSync(path.dirname(rollbackPath), { recursive: true });
|
||||
fs.copyFileSync(fullPath, rollbackPath);
|
||||
copyPreservingSymlink(fullPath, rollbackPath);
|
||||
rollback.push({ relPath: normalized, rollbackPath });
|
||||
|
||||
if (action.type === 'rewrite-json') {
|
||||
@@ -765,7 +800,7 @@ function applyInstallerMigrationPlan({
|
||||
const backupRelPath = action.backupRelPath || path.posix.join(backupRootRelPath, normalized);
|
||||
const backupPath = path.join(configDir, backupRelPath);
|
||||
fs.mkdirSync(path.dirname(backupPath), { recursive: true });
|
||||
fs.copyFileSync(fullPath, backupPath);
|
||||
copyPreservingSymlink(fullPath, backupPath);
|
||||
journal.actions.push(journalAction(action, 'removed', {
|
||||
backupRelPath,
|
||||
rollbackRelPath: path.posix.join(rollbackRootRelPath, normalized),
|
||||
@@ -817,7 +852,12 @@ function applyInstallerMigrationPlan({
|
||||
const dest = path.join(configDir, entry.relPath);
|
||||
try {
|
||||
fs.mkdirSync(path.dirname(dest), { recursive: true });
|
||||
fs.copyFileSync(entry.rollbackPath, dest);
|
||||
// Symlink-preserving, same as the forward path: `entry.rollbackPath` is
|
||||
// itself a link whenever the managed path was one, so a raw copy here
|
||||
// would dereference it and write the referent's bytes back to the LIVE
|
||||
// install path — a worse leak than the journal-tree one, since it is
|
||||
// user-visible and at a predictable location.
|
||||
copyPreservingSymlink(entry.rollbackPath, dest);
|
||||
} catch (rollbackError) {
|
||||
rollbackFailures.push({
|
||||
relPath: entry.relPath,
|
||||
|
||||
129
src/installer-migrations/006-pi-extension-cjs-to-js.cts
Normal file
129
src/installer-migrations/006-pi-extension-cjs-to-js.cts
Normal file
@@ -0,0 +1,129 @@
|
||||
/**
|
||||
* Installer migration: retire pi's stale `extensions/gsd.cjs` after #2470
|
||||
* renamed the installed native extension to `extensions/gsd.js`.
|
||||
*
|
||||
* What old artifact is being retired?
|
||||
* `extensions/gsd.cjs` — the pre-#2470 dest filename for pi's native
|
||||
* extension. pi auto-discovers extensions by scanning `<agentDir>/extensions/`
|
||||
* and keeping only names accepted by its own predicate
|
||||
* (`isExtensionFile()` in @earendil-works/pi-coding-agent:
|
||||
* `name.endsWith(".ts") || name.endsWith(".js")`). A `.cjs` file is skipped
|
||||
* SILENTLY — no `/gsd` command, no error, no log line. The file is therefore
|
||||
* permanently inert, not merely redundant.
|
||||
*
|
||||
* How do we prove it is GSD-owned?
|
||||
* The installer records the native plugin in the install manifest as
|
||||
* `<nativePlugin.dir>/<nativePlugin.file>` (bin/install.js, the
|
||||
* `_hostBehaviors(runtime).nativePlugin` manifest block), so a pre-#2470 pi
|
||||
* install carries `extensions/gsd.cjs` as a manifest-managed entry. Only a
|
||||
* manifest-managed classification produces an action here; an unmanifested
|
||||
* `gsd.cjs` is treated as a user's own file and preserved.
|
||||
*
|
||||
* What happens if the user modified it?
|
||||
* `backup-and-remove` instead of `remove-managed`, so a patched extension is
|
||||
* recoverable from the backup rather than silently destroyed.
|
||||
*
|
||||
* What happens if it is missing?
|
||||
* No actions — fresh (post-#2470) installs and already-migrated installs both
|
||||
* plan empty, so the migration is idempotent.
|
||||
*
|
||||
* What runtime and scope does it affect?
|
||||
* pi only, global and local. No other runtime ever installed this path:
|
||||
* OpenCode and Kilo — the only other runtimes declaring
|
||||
* `hostBehaviors.nativePlugin` — both ship `plugins/gsd-core.js`.
|
||||
*
|
||||
* Is the action safe in non-interactive install?
|
||||
* Yes. Both emitted action types are non-interactive and journaled; neither
|
||||
* requires a user choice, and unknown files never produce an action.
|
||||
*
|
||||
* Why not `move-managed`? The installer materializes the new `extensions/gsd.js`
|
||||
* from the package payload in the same run, so moving the stale file onto that
|
||||
* path would just be overwritten. Retiring the old path is the accurate
|
||||
* description of the change.
|
||||
*
|
||||
* See docs/installer-migrations.md#shipped-migrations and the pi row of
|
||||
* docs/installer-migrations.md#runtime-configuration-contract-registry.
|
||||
*/
|
||||
|
||||
type ArtifactClassification = string;
|
||||
|
||||
interface ClassifiedArtifact {
|
||||
classification: ArtifactClassification;
|
||||
[key: string]: unknown;
|
||||
}
|
||||
|
||||
type ActionType = 'remove-managed' | 'backup-and-remove';
|
||||
|
||||
interface MigrationAction {
|
||||
type: ActionType;
|
||||
relPath: string;
|
||||
reason: string;
|
||||
ownershipEvidence: string;
|
||||
}
|
||||
|
||||
interface MigrationPlanContext {
|
||||
classifyArtifact(relPath: string): ClassifiedArtifact;
|
||||
}
|
||||
|
||||
interface InstallerMigration {
|
||||
id: string;
|
||||
title: string;
|
||||
description: string;
|
||||
introducedIn: string;
|
||||
runtimes: string[];
|
||||
scopes: string[];
|
||||
destructive: boolean;
|
||||
plan: (ctx: MigrationPlanContext) => MigrationAction[];
|
||||
}
|
||||
|
||||
/** Pre-#2470 dest filename for pi's native extension. */
|
||||
const STALE_PI_EXTENSION = 'extensions/gsd.cjs';
|
||||
|
||||
const OWNERSHIP_EVIDENCE =
|
||||
'pre-#2470 pi installs record the native extension at extensions/gsd.cjs in ' +
|
||||
'gsd-file-manifest.json (installer nativePlugin manifest entry)';
|
||||
|
||||
const REASON =
|
||||
'pi cannot auto-discover a .cjs extension (isExtensionFile accepts only .ts/.js), ' +
|
||||
'so this file is inert; superseded by extensions/gsd.js (#2470)';
|
||||
|
||||
const migration: InstallerMigration = {
|
||||
id: '2026-07-20-pi-extension-cjs-to-js',
|
||||
title: 'Retire pi\'s undiscoverable extensions/gsd.cjs',
|
||||
description:
|
||||
'Remove the stale extensions/gsd.cjs left by pre-#2470 pi installs, superseded by ' +
|
||||
'extensions/gsd.js — the suffix pi\'s extension auto-discovery actually accepts.',
|
||||
introducedIn: '1.7.1',
|
||||
runtimes: ['pi'],
|
||||
scopes: ['global', 'local'],
|
||||
destructive: true,
|
||||
plan: (ctx: MigrationPlanContext): MigrationAction[] => {
|
||||
const artifact = ctx.classifyArtifact(STALE_PI_EXTENSION);
|
||||
if (artifact.classification === 'managed-pristine') {
|
||||
return [
|
||||
{
|
||||
type: 'remove-managed',
|
||||
relPath: STALE_PI_EXTENSION,
|
||||
reason: REASON,
|
||||
ownershipEvidence: OWNERSHIP_EVIDENCE,
|
||||
},
|
||||
];
|
||||
}
|
||||
if (artifact.classification === 'managed-modified') {
|
||||
return [
|
||||
{
|
||||
type: 'backup-and-remove',
|
||||
relPath: STALE_PI_EXTENSION,
|
||||
reason: REASON,
|
||||
ownershipEvidence: OWNERSHIP_EVIDENCE,
|
||||
},
|
||||
];
|
||||
}
|
||||
// 'unknown' (never GSD-managed), 'missing', and 'managed-missing' all plan
|
||||
// nothing: unknown files are preserved by policy, and an absent file needs
|
||||
// no retirement.
|
||||
return [];
|
||||
},
|
||||
};
|
||||
|
||||
export = migration;
|
||||
Reference in New Issue
Block a user