fix(setup): derive Paseo skill bridge from a discovered source, not a name allowlist

docs/terminal-app-integrations.md

@@ -52,8 +52,14 @@ gjc setup paseo --remove         # roll back only what GJC itself created
 ```
 
 `--remove` deletes a key only when GJC's own provenance ledger recorded creating it *and* the value
-still matches what GJC wrote, so a hand-edited entry always survives. `~/.agents/skills` is treated as
-read-only.
+still matches what GJC wrote, so a hand-edited entry always survives. Paseo's skills directory is
+treated as read-only: GJC bridges `~/.agents/skills` when a CLI install materialized it, the Paseo.app
+bundle's `Contents/Resources/skills` on a desktop install, or an explicit `PASEO_SKILLS_DIR` — and
+derives the bridged skill names from whichever directory actually exists. When a Paseo release swaps
+skills, re-running `gjc setup paseo` converges the bridge and a follow-up `gjc setup paseo --check`
+verifies it — `--check` itself is read-only and never repairs. A source that disappears
+entirely is still reported as `missing-skills-directory` drift (exit 1) until Paseo is reinstalled or
+a trusted `PASEO_SKILLS_DIR` is supplied — the bridge is never faked with dangling links.
 
 ### Extra providers for model profiles
 
@@ -96,6 +102,7 @@ GJC owns, add to the provider's `env` entry and restart the Paseo daemon:
 | `gjc` reads `error` in `paseo provider ls` | daemon still holds the pre-install config | `paseo daemon restart` |
 | `gjc setup paseo --check` reports `stale` | config is correct, daemon has not reloaded | `paseo daemon restart` |
 | `gjc setup paseo --check` reports `drift` | the entry was edited by hand or by another tool | reconcile manually, or `--remove` then re-install |
+| `gjc setup paseo --check` reports `missing-skills-directory` | Paseo's skills moved or were uninstalled; the bridge is skipped, not faked | point `PASEO_SKILLS_DIR` at the real directory, or re-run `gjc setup paseo` after reinstalling Paseo |
 | `failed to create agent` in `~/.paseo/daemon.log` | `gjc` not resolvable from the daemon's PATH | re-run `gjc setup paseo` so the absolute path is rewritten |
 | Permission-gated tools never prompt | `GJC_ACP_PERMISSION_MODE` overridden | set it back to `prompt` in the provider `env` |
 

packages/coding-agent/src/setup/paseo/check.ts

@@ -21,10 +21,10 @@ import * as fs from "node:fs/promises";
 import { recoverIntent } from "./install-saga";
 import { PaseoPublishError, readTarget } from "./json-publisher";
 import { createOrchestrationSeed } from "./orchestration-preferences";
-import { readIntent } from "./paseo-ownership";
+import { readIntent, readProvenance } from "./paseo-ownership";
 import { buildProviderEntry, hasProviderConflict, providerKeyFor, resolveGjcCommand } from "./provider-config";
 import type { DriftReason, SetupCheckResult } from "./result-types";
-import { INSTALL_SKILL_NAMES, type PaseoSetupDependencies } from "./setup-deps";
+import { type PaseoSetupDependencies, resolvePaseoSkillsSource } from "./setup-deps";
 import { scanSkillsBridgeDrift } from "./skills-bridge";
 
 const PROBE_TIMEOUT_MS = 5_000;
@@ -44,7 +44,16 @@ async function collectL1(deps: PaseoSetupDependencies, options: CheckOptions): P
 
 	// An interrupted earlier run leaves a durable intent. Report it, never repair
 	// it here -- `--check` is read-only.
-	const intent = await readIntent(deps.paths.intentRecord);
+	const intent = await readIntent(deps.paths.intentRecord).catch((error: Error) => {
+		// A corrupt record is itself drift (#4644 review r10); the remaining
+		// checks still run, but recovery stays refused until it is resolved.
+		reasons.push({
+			code: "partial-install",
+			subject: deps.paths.intentRecord,
+			detail: error instanceof Error ? error.message : String(error),
+		});
+		return undefined;
+	});
 	if (intent) {
 		// `repair: false` keeps this read-only; the intent is reported, never settled.
 		const recovery = await recoverIntent(deps.paths.intentRecord, { repair: false });
@@ -139,18 +148,18 @@ async function collectL1(deps: PaseoSetupDependencies, options: CheckOptions): P
 		}
 	}
 
-	for (const name of INSTALL_SKILL_NAMES) {
-		const link = `${deps.paths.bridgeDir}/${name}`;
-		const linked = await fs
-			.lstat(link)
-			.then(stat => stat.isSymbolicLink())
-			.catch(() => false);
-		if (!linked) {
-			reasons.push({ code: "missing-bridge-link", subject: link, detail: "bridge symlink is missing" });
-		}
+	const source = await (deps.skillsSource?.() ?? resolvePaseoSkillsSource());
+	if (source === undefined) {
+		reasons.push({
+			code: "missing-skills-directory",
+			subject: "paseo skills source",
+			detail:
+				"no Paseo skills directory found; the skills bridge is skipped (supported: ~/.agents/skills, a Paseo.app bundle)",
+		});
 	}
 
-	reasons.push(...(await scanSkillsBridgeDrift(deps)));
+	const ledger = await readProvenance(deps.paths.provenanceLedger);
+	reasons.push(...(await scanSkillsBridgeDrift(deps, ledger.bridgeEntries)));
 	return reasons;
 }
 

packages/coding-agent/src/setup/paseo/install-saga.ts

@@ -114,6 +114,8 @@ export interface JsonStepInput {
 	readonly provenancePath: string;
 	readonly intentPath: string;
 	readonly ownedKeys: readonly string[];
+	/** Target identity the step's decisions were computed from; a mismatch refuses. */
+	readonly expectedPreflightIdentity?: string;
 	/** Mutates the parsed target in place. */
 	readonly mutate: (draft: Record<string, unknown>) => void;
 	/** Produces the ledger that must exist once this step commits. */
@@ -122,6 +124,10 @@ export interface JsonStepInput {
 	readonly revert: (draft: Record<string, unknown>) => void;
 	/** Produces the ledger that must exist once this step is undone. */
 	readonly revertLedger: (ledger: ProvenanceLedger) => ProvenanceLedger;
+	/** Durable artifacts this step's ledger references. Runs only after the target's CAS publish succeeded and before the ledger commit, so a refused or conflicting publish leaves no orphaned artifact. */
+	readonly persist?: () => Promise<void>;
+	/** Removes what {@link persist} created; runs after a successful undo. */
+	readonly unpersist?: () => Promise<void>;
 	readonly now: Date;
 }
 
@@ -141,6 +147,17 @@ export interface JsonStepOutput {
  */
 export async function runJsonStep(input: JsonStepInput): Promise<JsonStepOutput> {
 	const current = await readTarget(input.targetPath);
+	// Ownership and seed decisions were derived from the caller's preflight
+	// snapshot. A target that changed since must not be mutated under decisions
+	// computed from different bytes (a concurrent user edit between preflight
+	// and this step would be overwritten while provenance claims the stale
+	// pre-state): refuse and let the operator re-run against current bytes.
+	if (input.expectedPreflightIdentity !== undefined && current.identity !== input.expectedPreflightIdentity) {
+		throw new SagaStepError(
+			input.label,
+			`${input.targetPath} changed after setup inspected it; refusing to publish decisions computed from older bytes. Re-run gjc setup paseo.`,
+		);
+	}
 	const plan = planPublish(current, input.mutate);
 
 	const ledgerBefore = await readProvenance(input.provenancePath);
@@ -168,15 +185,71 @@ export async function runJsonStep(input: JsonStepInput): Promise<JsonStepOutput>
 	await writeIntent(input.intentPath, intent);
 
 	let backupPath: string | undefined;
+	let publishSucceeded = false;
+	// #4644 review r14: track whether the persist hook was ATTEMPTED, not
+	// only whether it resolved. `persist` can create the durable artifact and
+	// then throw (a post-creation validation failure); cleanup must remove
+	// the artifact on every post-publish failure, so "attempted" is the
+	// condition — an unattempted persist created nothing to remove.
+	let persistAttempted = false;
 	try {
 		const published = await publishPlan(input.targetPath, plan, {
 			expectedIdentity: current.identity,
 			backup: true,
 			now: input.now,
 		});
 		backupPath = published.backupPath;
+		publishSucceeded = published.published;
+		// Durable artifacts the ledger is about to reference are created only
+		// now, inside the same CAS boundary: a refused or conflicting publish
+		// (#4644 review r8) must leave no orphaned artifact behind.
+		if (input.persist) {
+			persistAttempted = true;
+			await input.persist();
+		}
 		await writeProvenance(input.provenancePath, ledgerAfter);
 	} catch (error) {
+		// The publish already succeeded, so any failure before the ledger
+		// commit must undo the publication AND remove the artifact this step
+		// created (#4644 reviews r8/r10): leaving the target carrying this
+		// step's write with no provenance would strand an unowned overwrite,
+		// and leaving a persisted sidecar behind would orphan a
+		// credential-bearing file nothing references. When the rollback itself
+		// fails, the intent record deliberately stays for recovery instead.
+		if (publishSucceeded) {
+			let reverted = false;
+			try {
+				const observed = await currentIdentity(input.targetPath);
+				if (observed === plan.expectedIdentity) {
+					const afterPublish = await readTarget(input.targetPath);
+					const undoPlan = planPublish(afterPublish, input.revert);
+					await publishPlan(input.targetPath, undoPlan, {
+						expectedIdentity: afterPublish.identity,
+						backup: false,
+						now: input.now,
+					});
+					reverted = true;
+				}
+				// A DIFFERENT identity means someone else changed the target
+				// after our publish: the rollback deliberately does not
+				// overwrite it, and that is NOT a successful rollback (#4644
+				// review r15). The published write is now unprovenanced and
+				// unrecoverable by us, so the intent record must SURVIVE for
+				// the next run's recovery classification and the persisted
+				// artifact must stay (the intent's ledger payload still
+				// references it). reverted stays false on this path.
+			} catch {
+				reverted = false;
+			}
+			if (reverted) {
+				// Any ATTEMPTED persist may have created the artifact before
+				// throwing (#4644 review r14): cleanup removes it whether or
+				// not the hook resolved, so no credential-bearing sidecar is
+				// ever left unreferenced.
+				if (persistAttempted && input.unpersist) await input.unpersist();
+				await clearIntent(input.intentPath);
+			}
+		}
 		throw new SagaStepError(input.label, error instanceof Error ? error.message : String(error), [
 			input.intentPath,
 			...(backupPath ? [backupPath] : []),
@@ -205,6 +278,7 @@ export async function runJsonStep(input: JsonStepInput): Promise<JsonStepOutput>
 					now: input.now,
 				});
 				await writeProvenance(input.provenancePath, input.revertLedger(await readProvenance(input.provenancePath)));
+				if (persistAttempted && input.unpersist) await input.unpersist();
 				return { status: "reverted" };
 			},
 		},
@@ -266,7 +340,14 @@ export async function recoverIntent(
 	intentPath: string,
 	options: RecoverIntentOptions = { repair: false },
 ): Promise<{ recovered: boolean; detail: string } | undefined> {
-	const intent = await readIntent(intentPath);
+	let intent: IntentRecord | undefined;
+	try {
+		intent = await readIntent(intentPath);
+	} catch (error) {
+		// A corrupt intent is an explicit refusal (#4644 review r10): never
+		// proceed as if the record were absent.
+		return { recovered: false, detail: error instanceof Error ? error.message : String(error) };
+	}
 	if (!intent) return undefined;
 	const recovery = await classifyIntent(intent);
 	if (recovery.action === "refuse") return { recovered: false, detail: recovery.detail };

packages/coding-agent/src/setup/paseo/json-publisher.ts

@@ -36,7 +36,8 @@ export const ABSENT_IDENTITY = "absent";
 export type PublishRefusal =
 	| { readonly reason: "parse-refusal"; readonly detail: string }
 	| { readonly reason: "format-drift"; readonly detail: string }
-	| { readonly reason: "cas-conflict"; readonly expected: string; readonly actual: string };
+	| { readonly reason: "cas-conflict"; readonly expected: string; readonly actual: string }
+	| { readonly reason: "sidecar-conflict"; readonly detail: string };
 
 export class PaseoPublishError extends Error {
 	readonly refusal: PublishRefusal;
@@ -58,6 +59,8 @@ function describeRefusal(targetPath: string, refusal: PublishRefusal): string {
 			return `Refusing to write ${targetPath}: ${refusal.detail}. GJC only edits files it can rewrite byte-for-byte, so it will not reformat a file it did not author.`;
 		case "cas-conflict":
 			return `Refusing to write ${targetPath}: the file changed while GJC was preparing its update. Re-run to pick up the current contents.`;
+		case "sidecar-conflict":
+			return `Refusing to preserve the replaced provider value at ${targetPath}: ${refusal.detail}. Inspect or remove the existing sidecar, then re-run.`;
 	}
 }
 
@@ -261,3 +264,125 @@ async function copyPrivately(from: string, to: string): Promise<void> {
 	// keeps its old permissions unless we set them explicitly.
 	await fs.chmod(to, mode);
 }
+/**
+ * Where a pre-`--force` provider value is preserved for a later restore.
+ *
+ * The replaced entry can carry credential-bearing `env` or argument values, so
+ * it must never be serialized into GJC's own provenance ledger or intent record.
+ * Instead it lives in a deterministic, mode-0600 sidecar beside Paseo's own
+ * config file -- the same directory and the same privacy rule the publish-step
+ * backups already use -- and the ledger records only the pointer.
+ *
+ * The name is INJECTIVE in the raw provider key (#4644 review r8): the visible
+ * part is sanitized for readability, and a digest of the exact key is appended
+ * so two distinct keys that sanitize identically (`a/b` and `a_b`) can never
+ * share one sidecar. A shared path would let the second `--force` rename over
+ * the first key's only preserved copy of the user's value.
+ */
+export function replacedProviderBackupPath(configJsonPath: string, providerKey: string): string {
+	const safeKey = providerKey.replace(/[^a-zA-Z0-9_-]/gu, "_");
+	const keyDigest = nodeCrypto.createHash("sha256").update(providerKey, "utf8").digest("hex").slice(0, 16);
+	return `${configJsonPath}.gjc-replaced-${safeKey}-${keyDigest}.json`;
+}
+
+/**
+ * Write the pre-`--force` value of one provider key into its private sidecar.
+ *
+ * Publication is no-clobber: the staged bytes are linked into place, so an
+ * existing sidecar is never replaced. A sidecar that already holds this key's
+ * exact value makes the write idempotent; anything else (a different value for
+ * the same key, a foreign or tampered file on the injective path) fails closed
+ * instead of destroying the only preserved copy of the user's value.
+ */
+export async function writeReplacedProviderBackup(
+	configJsonPath: string,
+	providerKey: string,
+	value: unknown,
+): Promise<ReplacedProviderBackupRef> {
+	const backupPath = replacedProviderBackupPath(configJsonPath, providerKey);
+	const valueSha256 = hashBytes(serializeJson(value));
+	const payload = serializeJson({ key: providerKey, value });
+	const temporary = `${backupPath}.${process.pid}.${nodeCrypto.randomUUID()}.tmp`;
+	const handle = await fs.open(temporary, "w", BACKUP_MODE);
+	try {
+		await handle.writeFile(payload, "utf8");
+		await handle.sync();
+	} finally {
+		await handle.close();
+	}
+	// `fs.open` honors the mode only on creation, so set it explicitly.
+	await fs.chmod(temporary, BACKUP_MODE);
+	try {
+		// `link` fails with EEXIST when the sidecar already exists: a rename
+		// would silently replace it, and the FIRST preserved value is the
+		// user's by contract.
+		await fs.link(temporary, backupPath);
+	} catch (error) {
+		if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error;
+		const existing = await readReplacedProviderBackup(backupPath, providerKey, valueSha256);
+		if (!existing.found) {
+			throw new PaseoPublishError(backupPath, {
+				reason: "sidecar-conflict",
+				detail: `a replaced-provider sidecar already exists at this path with different content for key ${providerKey}`,
+			});
+		}
+		// Idempotent: the existing sidecar already preserves exactly this value.
+	} finally {
+		await fs.rm(temporary, { force: true }).catch(() => undefined);
+	}
+	return { backupPath, valueSha256 };
+}
+
+/** Remove a sidecar this run created; a missing file is already clean. */
+export async function removeReplacedProviderBackup(backupPath: string): Promise<void> {
+	await fs.rm(backupPath, { force: true }).catch(() => undefined);
+}
+
+/** Pointer + integrity digest for one preserved pre-`--force` provider value. */
+export interface ReplacedProviderBackupRef {
+	readonly backupPath: string;
+	/** Hash of the preserved value exactly as serialized into the sidecar. */
+	readonly valueSha256: string;
+}
+
+/** Outcome of reading a replaced-provider sidecar: a `null` prior is a value too. */
+export type ReplacedProviderBackup = { readonly found: true; readonly value: unknown } | { readonly found: false };
+
+/**
+ * Read one provider key's preserved prior value. A missing, corrupt,
+ * key-mismatched, or CONTENT-ALTERED sidecar reports `found: false`, which
+ * callers must treat as a fail-closed condition rather than deleting content it
+ * was meant to restore. The ledger-recorded digest binds the sidecar's bytes to
+ * the record: substituting the sidecar (or swapping a symlink onto its path)
+ * cannot steer the value restoration.
+ */
+export async function readReplacedProviderBackup(
+	backupPath: string,
+	providerKey: string,
+	expectedSha256: string,
+): Promise<ReplacedProviderBackup> {
+	try {
+		// The read is fd-bound and symlink-rejecting (#4644 review r9): the path
+		// is opened with O_NOFOLLOW where the platform provides it, so a symlink
+		// swapped onto the sidecar path fails the open outright instead of
+		// redirecting restoration at attacker-controlled JSON; the regular-file
+		// check and the bytes then share one handle identity. Platforms without
+		// O_NOFOLLOW keep the fstat regular-file check on the same fd.
+		const nofollow = typeof fs.constants.O_NOFOLLOW === "number" ? fs.constants.O_NOFOLLOW : 0;
+		const handle = await fs.open(backupPath, fs.constants.O_RDONLY | nofollow);
+		let bytes: string;
+		try {
+			const stat = await handle.stat();
+			if (!stat.isFile()) return { found: false };
+			bytes = await new Response(await handle.readFile()).text();
+		} finally {
+			await handle.close();
+		}
+		const parsed = JSON.parse(bytes) as { key?: unknown; value?: unknown };
+		if (parsed.key !== providerKey) return { found: false };
+		if (hashBytes(serializeJson(parsed.value)) !== expectedSha256) return { found: false };
+		return { found: true, value: parsed.value };
+	} catch {
+		return { found: false };
+	}
+}