fix: clarify browser routing and repair relocated skill links
This commit is contained in:
+41
-13
@@ -26,11 +26,12 @@ const COMMANDS = new Set(["check", "sync", "prune"]);
|
||||
|
||||
function usage() {
|
||||
return (
|
||||
`Usage: node scripts/pi-links.mjs <check|sync|prune> [--dry-run]\n\n` +
|
||||
`Usage: node scripts/pi-links.mjs <check|sync|prune> [--dry-run] [--migrate-from /old/repo]\n\n` +
|
||||
` check Report missing, stale, or conflicting exposure without writing\n` +
|
||||
` sync Create or repair manifest-declared links and local package entries\n` +
|
||||
` prune Remove repository-owned links and local package entries not in the manifest\n` +
|
||||
` --dry-run Print sync/prune changes without applying them`
|
||||
` --dry-run Print sync/prune changes without applying them\n` +
|
||||
` --migrate-from Explicit old root for exact manifest-link repairs (check/sync only)`
|
||||
);
|
||||
}
|
||||
|
||||
@@ -384,7 +385,7 @@ async function readLocalPackages(settingsFile) {
|
||||
.filter((entry) => entry.resolved);
|
||||
}
|
||||
|
||||
async function inspectDesiredLink(link, repoRoot) {
|
||||
async function inspectDesiredLink(link, repoRoot, migrateFrom) {
|
||||
const stat = await statOrUndefined(link.destination);
|
||||
if (!stat) return { type: "create-link", link };
|
||||
if (!stat.isSymbolicLink()) {
|
||||
@@ -402,7 +403,10 @@ async function inspectDesiredLink(link, repoRoot) {
|
||||
);
|
||||
if ((await canonical(resolvedTarget)) === (await canonical(link.source)))
|
||||
return undefined;
|
||||
if (isInside(repoRoot, resolvedTarget))
|
||||
const expectedOldTarget = migrateFrom
|
||||
? path.join(migrateFrom, path.relative(repoRoot, link.source))
|
||||
: undefined;
|
||||
if (isInside(repoRoot, resolvedTarget) || resolvedTarget === expectedOldTarget)
|
||||
return { type: "replace-link", link, previousTarget: resolvedTarget };
|
||||
return {
|
||||
type: "conflict",
|
||||
@@ -451,6 +455,14 @@ async function discoverStaleLinks(state, desiredDestinations) {
|
||||
|
||||
export async function buildPlan(options = {}) {
|
||||
const state = await resolveDesiredState(options);
|
||||
let migrateFrom;
|
||||
if (options.migrateFrom !== undefined) {
|
||||
if (typeof options.migrateFrom !== "string" || !path.isAbsolute(options.migrateFrom))
|
||||
throw new Error("migrate-from must be an absolute old repository root");
|
||||
migrateFrom = path.resolve(options.migrateFrom);
|
||||
if (isInside(migrateFrom, state.repoRoot) || isInside(state.repoRoot, migrateFrom))
|
||||
throw new Error("migrate-from must be separate from the current repository root");
|
||||
}
|
||||
const desiredDestinations = new Set(
|
||||
state.links.map((link) => link.destination),
|
||||
);
|
||||
@@ -461,7 +473,7 @@ export async function buildPlan(options = {}) {
|
||||
const conflicts = [];
|
||||
|
||||
for (const link of state.links) {
|
||||
const result = await inspectDesiredLink(link, state.repoRoot);
|
||||
const result = await inspectDesiredLink(link, state.repoRoot, migrateFrom);
|
||||
if (result?.type === "conflict") conflicts.push(result);
|
||||
else if (result) operations.push(result);
|
||||
}
|
||||
@@ -545,7 +557,13 @@ function selectOperations(command, operations) {
|
||||
async function createOrReplaceLink(operation) {
|
||||
const { link } = operation;
|
||||
await mkdir(path.dirname(link.destination), { recursive: true });
|
||||
if (operation.type === "replace-link") await unlink(link.destination);
|
||||
if (operation.type === "replace-link") {
|
||||
const current = await statOrUndefined(link.destination);
|
||||
if (!current?.isSymbolicLink() ||
|
||||
path.resolve(path.dirname(link.destination), await readlink(link.destination)) !== operation.previousTarget)
|
||||
throw new Error(`${link.destination}: destination changed after planning; refusing replacement`);
|
||||
await unlink(link.destination);
|
||||
}
|
||||
const target = link.relative
|
||||
? path.relative(path.dirname(link.destination), link.source)
|
||||
: link.source;
|
||||
@@ -576,6 +594,9 @@ async function applyOperations(operations) {
|
||||
}
|
||||
|
||||
export async function run(command, options = {}) {
|
||||
if (!COMMANDS.has(command)) throw new Error("unknown link-manager command");
|
||||
if (command === "prune" && options.migrateFrom !== undefined)
|
||||
throw new Error("migrate-from is only supported for check/sync; it never authorizes pruning");
|
||||
const plan = await buildPlan(options);
|
||||
for (const conflict of plan.conflicts)
|
||||
console.error(`CONFLICT ${conflict.path}: ${conflict.reason}`);
|
||||
@@ -606,16 +627,23 @@ export async function run(command, options = {}) {
|
||||
return 0;
|
||||
}
|
||||
|
||||
async function main(argv = process.argv.slice(2)) {
|
||||
const command = argv.find((arg) => !arg.startsWith("-"));
|
||||
const unknown = argv.filter(
|
||||
(arg) => arg.startsWith("-") && arg !== "--dry-run",
|
||||
);
|
||||
if (!COMMANDS.has(command) || unknown.length > 0) {
|
||||
export async function main(argv = process.argv.slice(2)) {
|
||||
const [command, ...args] = argv;
|
||||
const options = {};
|
||||
let invalid = !COMMANDS.has(command);
|
||||
for (let index = 0; index < args.length; index++) {
|
||||
const arg = args[index];
|
||||
if (arg === "--dry-run" && options.dryRun === undefined) options.dryRun = true;
|
||||
else if (arg === "--migrate-from" && options.migrateFrom === undefined &&
|
||||
args[index + 1] && !args[index + 1].startsWith("--"))
|
||||
options.migrateFrom = args[++index];
|
||||
else invalid = true;
|
||||
}
|
||||
if (invalid) {
|
||||
console.error(usage());
|
||||
return 2;
|
||||
}
|
||||
return run(command, { dryRun: argv.includes("--dry-run") });
|
||||
return run(command, options);
|
||||
}
|
||||
|
||||
if (
|
||||
|
||||
@@ -11,7 +11,7 @@ import os from "node:os";
|
||||
import path from "node:path";
|
||||
import test from "node:test";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { buildPlan, run } from "./pi-links.mjs";
|
||||
import { buildPlan, main, run } from "./pi-links.mjs";
|
||||
|
||||
const repositoryRoot = path.resolve(
|
||||
path.dirname(fileURLToPath(import.meta.url)),
|
||||
@@ -295,6 +295,68 @@ test("normal exposure and package evidence retain the Slice 0 security boundary"
|
||||
);
|
||||
});
|
||||
|
||||
test("explicit relocation repairs only exact manifest links, including dangling targets", async () => {
|
||||
const options = await fixture();
|
||||
const oldRoot = path.join(options.root, "old-repo");
|
||||
const initial = await buildPlan(options);
|
||||
for (const link of initial.state.links) {
|
||||
await mkdir(path.dirname(link.destination), { recursive: true });
|
||||
const oldTarget = path.join(oldRoot, path.relative(options.repoRoot, link.source));
|
||||
await symlink(link.relative ? path.relative(path.dirname(link.destination), oldTarget) : oldTarget, link.destination);
|
||||
}
|
||||
assert.equal((await buildPlan(options)).conflicts.length, initial.state.links.length);
|
||||
const migration = { ...options, migrateFrom: oldRoot };
|
||||
const plan = await buildPlan(migration);
|
||||
assert.equal(plan.conflicts.length, 0);
|
||||
assert.equal(plan.operations.length, initial.state.links.length);
|
||||
assert.ok(plan.operations.every(op => op.type === "replace-link"));
|
||||
assert.equal(await run("sync", { ...migration, dryRun: true }), 0);
|
||||
assert.equal((await buildPlan(options)).conflicts.length, initial.state.links.length);
|
||||
assert.equal(await run("sync", migration), 0);
|
||||
assert.equal(await run("check", options), 0);
|
||||
});
|
||||
|
||||
test("relocation preserves wrong-path, unrelated, prefix-lookalike and regular-file collisions", async () => {
|
||||
for (const kind of ["wrong-path", "unrelated", "prefix", "file"]) {
|
||||
const options = await fixture();
|
||||
const oldRoot = path.join(options.root, "old-repo");
|
||||
const destination = path.join(options.homeDir, ".agents", "skills", "global-skill");
|
||||
await mkdir(path.dirname(destination), { recursive: true });
|
||||
const target = kind === "wrong-path" ? path.join(oldRoot, "skills/wrong")
|
||||
: kind === "prefix" ? `${oldRoot}-other/skills/group/global-skill`
|
||||
: path.join(options.root, "unrelated");
|
||||
if (kind === "file") await writeFile(destination, "keep");
|
||||
else await symlink(target, destination);
|
||||
const migration = { ...options, migrateFrom: oldRoot };
|
||||
assert.equal((await buildPlan(migration)).conflicts.length, 1);
|
||||
assert.equal(await run("sync", migration), 2);
|
||||
assert.equal(kind === "file" ? await readFile(destination, "utf8") : await readlink(destination), kind === "file" ? "keep" : target);
|
||||
}
|
||||
});
|
||||
|
||||
test("relocation never claims stale old-root links or local packages for pruning", async () => {
|
||||
const options = await fixture();
|
||||
const oldRoot = path.join(options.root, "old-repo");
|
||||
await run("sync", options);
|
||||
const stale = path.join(options.homeDir, ".agents", "skills", "old-excluded");
|
||||
await symlink(path.join(oldRoot, "skills/old-excluded"), stale);
|
||||
const settings = JSON.parse(await readFile(options.settingsFile, "utf8"));
|
||||
settings.packages.push(path.join(oldRoot, "extensions/old/index.ts"));
|
||||
await writeJson(options.settingsFile, settings);
|
||||
const plan = await buildPlan({ ...options, migrateFrom: oldRoot });
|
||||
assert.deepEqual(plan.operations, []);
|
||||
await assert.rejects(() => run("prune", { ...options, migrateFrom: oldRoot }), /never authorizes pruning/);
|
||||
assert.equal(await readlink(stale), path.join(oldRoot, "skills/old-excluded"));
|
||||
});
|
||||
|
||||
test("migration rejects invalid roots and malformed CLI options", async () => {
|
||||
const options = await fixture();
|
||||
for (const migrateFrom of ["relative", "", options.repoRoot, options.root, path.join(options.repoRoot, "nested")])
|
||||
await assert.rejects(() => buildPlan({ ...options, migrateFrom }), /migrate-from/);
|
||||
for (const args of [["sync", "--migrate-from"], ["sync", "unexpected"], ["sync", "--migrate-from", "/old", "--migrate-from", "/other"]])
|
||||
assert.equal(await main(args), 2);
|
||||
});
|
||||
|
||||
test("duplicate declared skill names fail before planning changes", async () => {
|
||||
const options = await fixture();
|
||||
await writeSkill(
|
||||
|
||||
Reference in New Issue
Block a user