From 2cbd7195688e4a58cb2e424d0b8ce2b3a0a3fd05 Mon Sep 17 00:00:00 2001 From: gaebal-gajae Date: Sun, 23 Aug 2026 01:03:02 +0000 Subject: [PATCH 1/2] fix(setup): discover and safely reconcile Paseo skill bridges Paseo desktop installs expose skills from a discovered source rather than a stale fixed name list.\n\nThe reconciled bridge now preserves current-dev behavior and closes exact-head review races by binding cleanup to authenticated identities.\n\nLore-id: pr4644-recovery\nConstraint: no release mutation\nTested: setup Paseo and setup CLI regression suites; coding-agent build; CLI smoke; native addon build\nConfidence: high\nScope-risk: focused\nReversibility: revert --- docs/terminal-app-integrations.md | 19 +- packages/coding-agent/CHANGELOG.md | 4 +- .../coding-agent/src/setup/paseo/check.ts | 38 +- .../src/setup/paseo/install-saga.ts | 83 +- .../src/setup/paseo/json-publisher.ts | 170 +- .../src/setup/paseo/paseo-mutation-lock.ts | 52 + .../src/setup/paseo/paseo-ownership.ts | 243 +- .../src/setup/paseo/paseo-setup.ts | 439 +++- .../src/setup/paseo/provider-config.ts | 2 +- .../coding-agent/src/setup/paseo/remove.ts | 457 +++- .../src/setup/paseo/result-types.ts | 2 +- .../src/setup/paseo/setup-deps.ts | 216 +- .../src/setup/paseo/skills-bridge.ts | 751 +++++- .../coding-agent/test/setup-paseo.test.ts | 2266 ++++++++++++++++- 14 files changed, 4553 insertions(+), 189 deletions(-) create mode 100644 packages/coding-agent/src/setup/paseo/paseo-mutation-lock.ts diff --git a/docs/terminal-app-integrations.md b/docs/terminal-app-integrations.md index f484d890d9..5c4bde0c23 100644 --- a/docs/terminal-app-integrations.md +++ b/docs/terminal-app-integrations.md @@ -52,8 +52,22 @@ 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. + +**`PASEO_SKILLS_DIR` trust rule:** the override is honored only when it is the user's own +explicit intent. If any project dotenv file in the current working directory (`.env`, +`.env.local`, `.env.development*`, `.env.production.local`, or the `NODE_ENV`-specific +variants) defines `PASEO_SKILLS_DIR`, the override is rejected for runs from that checkout — +a cloned repository must not be able to bridge its own shipped skill content into your +global GJC configuration. Run from outside the checkout, or remove the dotenv key, to use a +legitimately relocated skills directory. ### Extra providers for model profiles @@ -96,6 +110,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` | diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1d2a5e3fb6..1b1e9b2efe 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -57,10 +57,11 @@ - `gjc accounts` command errors no longer escape as uncaught exceptions in text mode. `accounts pin` resolves its target through `resolveOAuthPinTarget`, which throws a typed `OAuthCredentialSelectorError` for user-correctable selector problems (API-key rows, active overrides, disabled or missing accounts, ambiguity), and `runAccountsCommand` rendered `AccountsCommandError` only in `--json` mode — text mode rethrew everything, so even the command's own "Provider … is not configured; no pin was written" surfaced as a stack trace plus a `gjc-crash.log` entry. Selector failures now map to `AccountsCommandError` with the message preserved (so `--json` reports `accounts-error` instead of `internal-error`), and text mode prints one clean stderr line with exit code 1. The framework's `CliParseError` handling and the JSON machine contract (exactly one document, never stacks or secrets) are unchanged. - Fixed interactive steering stalling during post-prompt unwind when the terminal transcript message is a bash or Python execution; the auto-continue gate now treats all three terminal roles consistently and regression coverage pins delivery for assistant, bash, and Python tails (#4739). - Restored live subagent progress in the `subagent await` panel through a bounded public DTO. #4748 dropped the `progress` field believing it exposed model-generated deltas and tool output to the parent model; the renderer needs live status, tool names, output counts, fast mode, and retry state, but raw progress must not enter tool-result, ACP, or telemetry envelopes. The await signature now tracks only those approved fields and ignores countdown-only churn, so the panel updates without exposing model deltas, tool arguments, arbitrary output, or nested task payloads. -- Dev CI no longer re-runs the entire code-validation matrix when a pull request body is edited. The verdict line lives in the PR body, so every review verdict update fired a `pull_request: edited` event; with `cancel-in-progress: true` that cancelled the run already validating the identical tree and re-queued the whole matrix against it. Cancelled runs surfaced as confusing downstream failures rather than clean cancellations -- a job would report `Artifact not found for name: dev-affected-plan-` while that artifact existed and its producer had succeeded. `edited` stays in the trigger list because `pr-contract-bootstrap` must re-check the body, but it is now the only job that runs for it; every code-validation root carries an explicit guard and the remaining jobs inherit it through their dependency on a skipped plan. - Dev CI no longer re-runs the entire code-validation matrix when a pull request body is edited. The verdict line lives in the PR body, so every review verdict update fired a `pull_request: edited` event; with `cancel-in-progress: true` that cancelled the run already validating the identical tree and re-queued the whole matrix against it. Cancelled runs surfaced as confusing downstream failures rather than clean cancellations -- a job would report `Artifact not found for name: dev-affected-plan-` while that artifact existed and its producer had succeeded. `edited` stays in the trigger list because `pr-contract-bootstrap` must re-check the body, but it is now the only job that runs for it; every code-validation root carries an explicit guard and the remaining jobs inherit it through their dependency on a skipped plan. Body-only edited runs are also partitioned into their own concurrency group so they cannot cancel the validation run already covering the identical tree, and the exemption is keyed on a body-only payload -- `edited` also fires for title and base changes, and a base retarget can be the first validation event for a new head/base pair. - Retained-publication refusal diagnostics now carry the native acquisition object and reason into the surfaced error before any later pathname inspection. POSIX acquisition rejects non-regular owner records before granting authority, while fallback probes are labeled as current observed state and escape the complete Unicode control/format surface. - Coordinator `gjc_coordinator_start_session`, `gjc_delegate_plan`, and `gjc_delegate_execute` now accept an optional `model` argument that pins an explicit `provider/model` for the started session with the same grammar as `gjc --model` (#4707). An external orchestrator could previously select only a model *profile* through `mpreset`; writing a model name into the prompt is not model selection, so a reserved start could never prove it was on the requested model (every Cursor id — `cursor/claude-fable-5-xhigh`, `cursor/composer-2.5`, `cursor/default`, … — worked from the CLI but not through Coordinator start). The pin is resolved through the same staged selector resolver the CLI uses (against the full registry, not the authenticated-only subset, so a valid id is never rejected up front for a missing local credential) and fails closed with the CLI's `Model "..." not found. Use --list-models` error before any broker mutation or idempotency record — no session is ever created on a different model. The resolved pin threads through the SDK lifecycle launch request as `modelId` and the session host applies it exactly like a CLI `--model` selection, so when both `mpreset` and `model` are given the explicit model wins, matching `gjc --mpreset

--model `. The delegate tools' previous prompt-metadata `model` hint is upgraded to this real pin, and reusing a delegate session with a conflicting pin is rejected like the existing `mpreset_conflict`. The effective precedence is `model` > `mpreset` > configured `modelProfile.default` > role/resume/default. The pin is a guarantee rather than a preference: the coordinator validates against its own registry while the child owns the registry that actually serves requests, so on drift (a model removed, a provider disabled, an extension that failed to register) the child now fails session construction before readiness and before any profile application, disposes the partial session, and reports an error naming the pinned selector — previously the discarded fallback warning let startup profile application activate `modelProfile.default`/`mpreset` while the coordinator still reported the requested pin. Coordinator validation also refreshes its registry per request (offline, from the on-disk discovery cache) instead of caching one snapshot for the process lifetime, so ids added or removed after an earlier pin are judged against current contents. +- ACP `session/list` no longer grants destructive lifecycle control. Listing populates a per-session cwd cache, and `session/close` / `session/delete` were treating an entry in that cache as proof of ownership -- so a second ACP connection sharing a broker could enumerate another connection's sessions and then close or delete them. Ownership is now a separate set populated only when a connection creates or attaches to a session; knowing a session's cwd is no longer authority over its lifecycle. +- `gjc setup paseo` install and `--remove` now serialize on one per-agent-directory cross-process mutation lock covering recovery, target updates, bridge operations, settings registration, and the final provenance write, so a concurrent remove can no longer clear the ledger while an install is still creating links (#4638, #4644). Provider ownership is decided by pre-state rather than value equality: a pre-existing entry that already equals what GJC would write is marked `providerPreexistingKeys` and never claimed or removed, and a `--force` overwrite preserves the replaced entry (of any JSON shape — object, scalar, array, or `null`) in a private mode-0600 sidecar beside Paseo's own config, with `providerReplacedEntries` recording only the pointer, so `--remove` restores the user's content without ever serializing it into GJC's ledger or intent record (a provider entry can carry credential-bearing `env`/argument values); a missing or unreadable sidecar fails removal closed instead of deleting the key. The sidecar path is injective in the raw provider key (a sha256 digest of the exact key is appended to the sanitized name), so distinct keys that sanitize identically (`a/b` vs `a_b`) can never share one sidecar, and publication is no-clobber: the staged bytes are hard-linked into place, an existing sidecar holding the same key's exact value makes the write idempotent, and anything else fails closed instead of destroying the user's only preserved copy; the sidecar is created inside the provider step's compare-and-swap boundary (only after publication succeeded, removed again on compensation), so a refused or conflicting publish can no longer strand an unreferenced credential-bearing file beside Paseo's config. A sidecar that cannot be created after the publish succeeded (a pre-planted file at the injective path with different content) also rolls the published provider entry back to the user's value, so the failure never strands an unowned overwrite. A malformed provenance field (a non-string-record `providerKeys`, a non-array `bridgeEntries`, a non-boolean `bridgeDirCreated`, a partial replaced-ref record, …) is now corruption instead of a silent default: `readProvenance` raises `ProvenanceLedgerCorruptError` so removal can never act on a partially tampered record, while absent fields keep their old-shape defaults. Sidecar reads are fd-bound and symlink-rejecting (`O_NOFOLLOW` where available plus an fstat regular-file check on the same handle), a mid-operation bridge failure corrects the ledger to what actually completed (`SkillsBridgePartialError` carries the partial install result; planned-but-never-created entries stop being claimed), `--check` reports exactly one deterministic drift reason per bridge entry (a dangling off-source link is no longer double-reported), and the `skillsSource` seam is mandatory so an omitted resolver can never silently disable the bridge. The r10 round closes five more hardening gaps: any failure between a successful CAS publish and its ledger commit now also unpersists the sidecar and reverts the publication (a ledger-commit failure no longer orphans a credential-bearing file); `writeIntent` uses the same fsync+rename publication as the ledger, and a present-but-malformed intent record is an explicit corrupt-record refusal (`--check` reports it as drift, recovery refuses) instead of reading as absent; non-string record VALUES (`providerKeys: {gjc: 7}`) are ledger corruption; the recorded bridge source is validated against trusted Paseo source roots (trusted home, `/Applications`, or the live `PASEO_SKILLS_DIR` override) before any removal unlink, so a tampered ledger cannot name an arbitrary source; discovery records the CANONICAL resolved source (a symlinked skills directory records its realpath, and the override's resolved target is trusted), so install and removal agree on what the source is; the drift scan canonicalizes the source the same way, so a symlinked skills directory passes `--check` with no false drift; and `PaseoPaths` no longer carries an optional skills-directory field at all — the source is resolved per run, and the dangling-field ambiguity is gone. The r11 round closes three more: quarantined bridge-link deletion re-verifies kind+text immediately before the unlink, treats an unlink failure as divergence unless the verified link is still there, and sweeps a foreign object swapped onto the quarantine name back to the bridge name instead of deleting it; a replaced-provider sidecar whose GJC entry is already absent is still deleted at removal (the preserved value is unreachable), and any sidecar deletion failure fails removal closed instead of dropping the ledger reference while a credential-bearing file survives. The r12 round narrows the recorded-source trust rule to exactly the locations discovery can produce (the default `~/.agents/skills`, the fixed app-bundle roots, or the live override) — an arbitrary home subdirectory named by a tampered ledger is refused — and restores the optional `PaseoPaths.agentsSkillsDir` compatibility field (the default spelling; the resolved source, not the field, remains what the ledger records). The r13 round binds quarantined bridge-link deletion to the captured inode (a concurrent replacement of the quarantine pathname makes the deletion refuse and restore instead of unlinking a foreign object), validates every intent-record field recovery trusts — including refusing a recorded provenance path that escapes the agent directory — and makes the `skillsSource` seam migration-safe: an omitted resolver falls back to the real discovery order rather than failing existing callers to compile, with no silent bridge skip. The r14 round confines destructive bridge cleanup to the bridge-directory NAME family (`paseo-skills`/`*-paseo-skills`/the current spelling) so a tampered ledger cannot aim `--remove` at arbitrary agent-root content, authenticates a migration record against the OLD directory's own ledger (which must itself claim the exact recorded bridge path — a fabricated `paseo/provenance.json` shape beside a victim no longer authorizes cleanup there), tracks the persist hook's ATTEMPT rather than only its resolution so a sidecar created before a throw is always cleaned up on rollback, documents that the mutation lock never displaces a provably-live owner (pid + process-start-time identity; the stale window bounds only the liveness-indeterminate fallback), and restores `PaseoPaths.agentsSkillsDir` as a required field carrying the default spelling for external callers. The r15 round treats a concurrent external target change during rollback as a FAILED rollback (the intent record and persisted artifact survive for recovery instead of being cleared), restores quarantined bridge links with a no-clobber rename (an entry that claimed the bridge name in the window is left untouched; the quarantined object stays recoverable at its quarantine name), and documents the exported preflight/result shapes. The r16 round validates EVERY providerReplacedEntries reference against the deterministic sidecar path before any deletion (a tampered ledger can no longer redirect `fs.rm` at an arbitrary absolute file), removes only a sidecar whose bytes match what the failed run itself wrote (a pre-existing sidecar — the user's preserved value or an attacker's plant — is never deleted by unpersist), and binds unpersist to the sidecar's authenticated {key,value} bytes via the fd-bound digest read (a same-sized attacker replacement fails the digest and is preserved), restores quarantined symlinks atomically with `fs.symlink` (EEXIST preserves a concurrent occupant of the bridge name instead of the rename clobbering it), honors a caller-supplied `paths.agentsSkillsDir` in the migration-safe resolver fallback exactly as the pre-#4638 seam did, and documents the `PASEO_SKILLS_DIR` dotenv trust rule in the integration docs. The r19 round accepts an in-root recorded bridge path only when it is the CURRENT configured bridge or a migration record authenticated by CONTENT — the recorded directory must actually hold the recorded links pointing into the ledger's recorded source (a foreign `*-paseo-skills` sibling with user content is refused however the ledger spells it); an absent provider key's sidecar is authenticated fd-bound against its recorded digest before deletion (a user file or attacker replacement at the deterministic path is preserved and reported); and the intent record's provenance-path confinement is canonicalized through `realpath` so a symlinked ancestor can no longer redirect an apparently in-tree path outside the agent directory. The r18 round restores the ORIGINAL legacy link when an adoption's replacement publish fails (the legacy text is captured first and recreated; a concurrent occupant of the bridge name is preserved with the link recoverable), derives the legacy source for callers that omit `home` from their own `paths.agentsSkillsDir` or the trusted home (a relative `.agents/skills` could never pass removal's absolute-path trust check), and re-points provenanced links across a TRUSTED source relocation (a Paseo update moving the app bundle now converges the bridge instead of reporting conflicts — still gated on the link pointing into the source the ledger itself recorded). Migration cleanup applies the same fail-closed ledger validation `--remove` uses (absolute, inside the ledger's agent directory, non-symlink, and — through a full `realpath` — not routed through a symlinked ancestor to a directory outside the agent root) plus a safe single-basename gate on every recorded entry name, so a tampered ledger can never redirect destructive cleanup outside the bridge. Default Paseo home paths (`~/.agents/skills` discovery, `~/Applications` app-bundle candidates, `~/.paseo` config paths) resolve through the provenance-checked trusted home rather than `$HOME`, so a repository or shell that plants `HOME` cannot steer global setup at attacker-controlled skill content. The `PASEO_SKILLS_DIR` dotenv trust check now also covers `.env.development` and `.env.development.local` for every mode except `production`/`test` (with `.env.production.local` always checked): Bun's mode defaults to development, so those files load even with `NODE_ENV` unset or custom, and a checkout shipping `PASEO_SKILLS_DIR` in `.env.development` can no longer have its override honored. A fresh install with no Paseo skills source anywhere and no existing bridge directory now completes the provider/orchestration setup and skips the bridge (nothing dangling, nothing registered, `--check` still reports `missing-skills-directory` until a source exists) instead of failing the whole install, and it records no bridge ownership at all: `bridgeDirCreated` is persisted only after the exclusive directory creation actually succeeded (a preflight plan is never written as a fact), so a directory the user later creates at that path can never be mistaken for GJC's work by `--remove`; an existing unowned bridge directory with no source is still refused. A source that disappears or becomes unreadable mid-run (ENOENT/ENOTDIR after resolution, EACCES during enumeration) fails closed and preserves the bridge instead of being read as an empty source that prunes every recorded link. `bridgeDirCreated` is per-path ownership (reset on migration), a valid source shipping zero `paseo*` skills records no phantom bridge and registers nothing, and the integration docs now state that re-running `gjc setup paseo` converges the bridge while `--check` stays read-only. - Custom-provider `apiKeyEnv` credentials sourced from the trusted agent `.env` now follow rotations on the next request instead of remaining pinned to the process-start value. Long-running sessions previously kept sending a revoked OAuth access token after an external account synchronizer atomically replaced the agent `.env`; the registry now re-reads only that trusted regular file and updates or removes the config credential override, failing closed for a symlink or read error after agent ownership is established. Presence in the agent file makes it authoritative over an older value inherited from the launching shell, while names absent from the agent file keep their existing environment precedence. Explicit `apiKey` values remain authoritative when both forms are configured, and caller-project `.env` files remain excluded from credential resolution. - Coordinator event journal rows can now be pushed to one opt-in webhook (#4706). External orchestrators that cannot stay attached to `gjc_coordinator_watch_events` long-poll (a 300s `await_turn` timeout is not session death) had no push of **existing** journal rows; they can now set `GJC_COORDINATOR_MCP_EVENT_WEBHOOK_URL` to receive each row as an authenticated POST whose body is the exact native `watch_events` record — same `seq`, same stable `id`, at-least-once so sinks dedupe on `id`. The feature is env-only and default-off (no MCP tool can set or read it), destinations are allowlisted (`https:` anywhere, `http:` loopback only, no redirects), the bearer token comes from a secret file path rather than env, an optional session-id scope restricts delivery to authorized sessions, and delivery runs through a durable per-row outbox off the journal append path with bounded attempts, exponential backoff, and a bounded request timeout — a dead sink never delays or rewrites terminal turn/session persistence. The five `GJC_COORDINATOR_MCP_EVENT_WEBHOOK_*` variables resolve through the trusted credential environment (`$credentialEnv`, the same provenance as the crash-relay DSN) rather than raw `process.env`, so a checkout's `.env` cannot select the egress destination or the token file. `watch_events` long-poll is unchanged and remains the source of truth; `gjc coordinator doctor` reports the resolved webhook state. - Extension activation is now transactional. `pi.registerFlag(..., { default })` and `pi.registerProvider(...)` used to mutate the shared `ExtensionRuntime` state directly with no rollback, so a factory that threw midway was discarded while its side effects leaked: the flag default stayed readable via `getFlag`/`getFlagValues` and the provider registration stayed queued for the ModelRegistry drain in `sdk/session.ts` and `runListModelsCommand`, activating providers from an extension that never activated. Each factory invocation now stages its shared-state writes in an `ExtensionActivationScope` (stage → factory completes without throwing → commit into the shared runtime); rollback discards the staged writes so a failed extension leaves no flag default and no provider registration behind, and earlier extensions' committed state is untouched. After commit the shared runtime is authoritative for `getFlag`, so runtime-side writes (CLI flag overrides, a later extension's committed default) stay observable to retained extension API objects exactly as before the transaction (#4718). Commit itself is transactional: prior flag entries and the provider-queue length are journaled before publication, so a throw partway through commit is undone before it escapes and the scope only becomes terminal once publication fully succeeds — a failed extension leaves nothing behind even when the failure happens during publication. @@ -154,6 +155,7 @@ - A tool call the agent loop refuses now reports why it was refused. The loop attaches its own failure envelope (`{ failureKind }`) in place of the tool's details, and the TUI still dispatched that envelope at the tool's renderer, which owns only its own detail shape: `search_tool_bm25` threw on `details.tools`, `task` printed `Task result details unavailable`, `resolve` printed `Failed: pending action`, and `write` painted its success card — in every case the rejection text (for example the `\uXXXX`-escaped-arguments rejection that ends a Korean `task` call) never reached the screen. Such a result now renders the same error card a tool without a renderer already produces: the failed status line plus the reason. Results a tool produced itself keep their renderer, including `todo_write`'s own `failureKind`. - `SdkClient` requests accept `beforeDispatch`/`onDispatch` boundary callbacks (`SdkRequestOptions`), giving dispatch-aware consumers a synchronous post-send boundary without owning the raw transport lifecycle (#4640). `onDispatch` fires immediately after the frame is handed to the socket — never before — with the exact request identity (`frame.id`), `connectionId`, and transport generation; a throwing observer cannot displace settlement, so the request still settles through its response, deadline, or `uncertain_after_send` retirement on transport close. `beforeDispatch` runs before the write and its throw aborts the dispatch with nothing on the wire (no sent record, caller's own error, retryable). This replaces the only previous alternative — a raw `send()` + `onFrame()` request that could never settle on a close after handoff and waited for its own timeout — while keeping pending-request ownership inside the client. - Discovered oMLX models now keep thinking metadata (`reasoning: true`, `supportsReasoningEffort`, `thinkingFormat: qwen-chat-template`) so `macos-omlx-*` role suffixes (`:low`/`:medium`/`:high`) survive clamp and reach oMLX as `chat_template_kwargs.reasoning_effort`. +- `gjc setup paseo --check` can now reach a clean verdict on a Paseo desktop-app install (#4638). The skills bridge no longer assumes a compiled-in five-name allowlist plus `~/.agents/skills`: it discovers the skills directory through a bounded cross-platform seam (`~/.agents/skills` first, then the Paseo.app bundle's `Contents/Resources/skills` under `/Applications` and `~/Applications`, or an explicit `PASEO_SKILLS_DIR`), derives the bridged names from whichever directory actually exists, never creates a symlink whose target is missing, and converges the bridge on re-run (creating new links, pruning dead ones) so a Paseo release that adds or drops a skill cannot wedge `--check` permanently red. Drift detection stays honest: a dangling bridge link, a link pointing outside the source, or a provenanced link that was deleted is still `drift`, while a Paseo skill GJC never bridged is no longer reported. `--remove` proves bridge ownership from the provenance ledger's recorded entries and source directory rather than the old name allowlist, and still cleans up after Paseo itself is uninstalled; the provider entry, orchestration seeding, foreign providers, and credentials are untouched throughout. - Added built-in `MACOS LOCAL (OMLX)` model profiles (`macos-omlx-fast`, `macos-omlx-balanced`, `macos-omlx-quality`, `macos-omlx-abliterated-fast`, `macos-omlx-abliterated-balanced`) for oMLX local inference on Apple Silicon Macs with native full context support and single-LLM thinking effort role mappings to eliminate model swap latency. - Fixed an HTTP 400 that killed every deep-interview session on the `google-antigravity` provider before the first assistant turn. The Round-0 topology `ask` schema pinned `round` with `z.literal(0)`, which zod serializes as `const: 0` and the Cloud Code Assist normalizer rewrites to a numeric `enum: [0]` — a shape CCA rejects (`TYPE_STRING`). `round` is now pinned with an integer range `[0, 0]` instead, so the wire schema carries `type: integer` with the bounds spilled into the description (the same treatment `ambiguity` already gets) and no numeric enum remains. Runtime contract unchanged: only `0` validates (#4606). - The terminal-app integration docs now cite the upstream work that backs each support rating: Gajae Code is proposed for Paseo's in-app ACP provider catalog ([getpaseo/paseo#3471](https://github.com/getpaseo/paseo/pull/3471)) and for Orca's built-in agent registry ([stablyai/orca#15025](https://github.com/stablyai/orca/pull/15025)), while T3 Code has no GJC harness and the integration shape is under discussion upstream ([pingdotgg/t3code#7290](https://github.com/pingdotgg/t3code/discussions/7290)). diff --git a/packages/coding-agent/src/setup/paseo/check.ts b/packages/coding-agent/src/setup/paseo/check.ts index 8353620031..f0f0e18e8c 100644 --- a/packages/coding-agent/src/setup/paseo/check.ts +++ b/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,21 @@ 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" }); - } + // An injected resolver is authoritative, including when it deliberately + // reports no source. Falling back after an injected `undefined` leaks this + // hermetic check into the caller's real home/app filesystem. + const source = deps.skillsSource === undefined ? await resolvePaseoSkillsSource() : await deps.skillsSource(); + 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; } diff --git a/packages/coding-agent/src/setup/paseo/install-saga.ts b/packages/coding-agent/src/setup/paseo/install-saga.ts index 6b4de8fee8..7c2a6b0fbc 100644 --- a/packages/coding-agent/src/setup/paseo/install-saga.ts +++ b/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) => void; /** Produces the ledger that must exist once this step commits. */ @@ -122,6 +124,10 @@ export interface JsonStepInput { readonly revert: (draft: Record) => 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; + /** Removes what {@link persist} created; runs after a successful undo. */ + readonly unpersist?: () => Promise; readonly now: Date; } @@ -141,6 +147,17 @@ export interface JsonStepOutput { */ export async function runJsonStep(input: JsonStepInput): Promise { 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,6 +185,13 @@ export async function runJsonStep(input: JsonStepInput): Promise 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, @@ -175,8 +199,57 @@ export async function runJsonStep(input: JsonStepInput): Promise 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 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 }; diff --git a/packages/coding-agent/src/setup/paseo/json-publisher.ts b/packages/coding-agent/src/setup/paseo/json-publisher.ts index bfe9ac8c04..0f0891a4cd 100644 --- a/packages/coding-agent/src/setup/paseo/json-publisher.ts +++ b/packages/coding-agent/src/setup/paseo/json-publisher.ts @@ -18,8 +18,10 @@ * the per-target adapters so this file stays small enough to audit. */ import * as nodeCrypto from "node:crypto"; +import type { BigIntStats } from "node:fs"; import * as fs from "node:fs/promises"; import * as path from "node:path"; +import { exactUnlinkDirect, type NativeExactFileIdentity } from "@gajae-code/natives"; /** Serialization Paseo itself produces. Verified byte-identical against the live config. */ export function serializeJson(value: unknown): string { @@ -36,7 +38,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 +61,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 +266,166 @@ async function copyPrivately(from: string, to: string): Promise { // 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 { + 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 }; +} + +/** 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 { + 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 }; + } +} + +/** + * Delete a sidecar only while its authenticated regular-file identity still + * owns the pathname. `fs.rm()` after a successful fd-bound read reopens a + * destructive pathname race: a replacement could be deleted after the + * original sidecar was authenticated. The native exact-unlink protocol + * compares the captured inode, bytes, and parent identity atomically before + * detaching its private quarantine, so a successor is preserved. + */ +export async function removeReplacedProviderBackup( + backupPath: string, + providerKey: string, + expectedSha256: string, +): Promise { + try { + 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: Buffer; + let stat: BigIntStats; + try { + stat = await handle.stat({ bigint: true }); + if (!stat.isFile()) return false; + bytes = await handle.readFile(); + } finally { + await handle.close(); + } + const parsed = JSON.parse(bytes.toString("utf8")) as { key?: unknown; value?: unknown }; + if (parsed.key !== providerKey || hashBytes(serializeJson(parsed.value)) !== expectedSha256) return false; + const parent = await fs.stat(path.dirname(backupPath), { bigint: true }); + if (!parent.isDirectory()) return false; + const identity: NativeExactFileIdentity = { + dev: stat.dev, + ino: stat.ino, + nlink: stat.nlink, + parentDev: parent.dev, + parentIno: parent.ino, + size: stat.size, + mtimeNs: stat.mtimeNs, + sha256: nodeCrypto.createHash("sha256").update(bytes).digest("hex"), + quarantineName: `.gjc-paseo-sidecar-${process.pid}-${nodeCrypto.randomUUID()}`, + }; + return exactUnlinkDirect(backupPath, identity).ok; + } catch { + return false; + } +} diff --git a/packages/coding-agent/src/setup/paseo/paseo-mutation-lock.ts b/packages/coding-agent/src/setup/paseo/paseo-mutation-lock.ts new file mode 100644 index 0000000000..0d3442bd49 --- /dev/null +++ b/packages/coding-agent/src/setup/paseo/paseo-mutation-lock.ts @@ -0,0 +1,52 @@ +/** + * Cross-process mutation lock for `gjc setup paseo` install/remove. + * + * `setup paseo` and `setup paseo --remove` mutate the same intent record, + * provenance ledger, Paseo config targets, symlink bridge, and GJC settings. + * Without serialization a concurrent remove can clear the ledger while an + * install is still creating links and registering the bridge: the live links + * end up with no provenance, and a later `--remove` reports + * `nothing-to-remove`. One lock per agent directory covers recovery, target + * updates, bridge operations, settings registration, and the final provenance + * write for both commands. + * + * The lock file lives inside the same `paseo/` directory as the provenance + * ledger, so the agent-directory root already scopes it. `withFileLock` + * serializes contenders across processes (and async contenders inside one + * process); a crashed holder's lock is reclaimed after its stale window. + */ + +import { withFileLock } from "../../config/file-lock"; +import type { PaseoSetupDependencies } from "./setup-deps"; + +/** + * Stale window for a crashed holder. A healthy install/remove is never + * displaced by elapsed time alone: the lock records its owner pid and + * process start-time identity, and a LIVE owner is never reaped (#652 rule + * in the shared file lock) — the stale window only bounds the + * liveness-indeterminate fallback (a pid whose liveness cannot be proven + * from this host). A holder that is provably alive keeps the lock for as + * long as its operation runs, so the lease cannot expire under a healthy + * operation, and ACQUIRE_TIMEOUT_MS below still waits out a contender + * instead of displacing it. + */ +const STALE_MS = 60_000; + +/** Total wait before giving up: a contending run is awaited, never displaced. */ +const ACQUIRE_TIMEOUT_MS = 120_000; + +const RETRY_DELAY_MS = 100; + +export function paseoMutationLockPath(deps: PaseoSetupDependencies): string { + return `${deps.paths.provenanceLedger}.mutation.lock`; +} + +/** Run `operation` holding the per-agent-directory Paseo mutation lock. */ +export async function withPaseoMutationLock(deps: PaseoSetupDependencies, operation: () => Promise): Promise { + const retries = Math.max(1, Math.ceil(ACQUIRE_TIMEOUT_MS / RETRY_DELAY_MS)); + return await withFileLock(paseoMutationLockPath(deps), operation, { + staleMs: STALE_MS, + retries, + retryDelayMs: RETRY_DELAY_MS, + }); +} diff --git a/packages/coding-agent/src/setup/paseo/paseo-ownership.ts b/packages/coding-agent/src/setup/paseo/paseo-ownership.ts index 5693de8c37..eaaeef6119 100644 --- a/packages/coding-agent/src/setup/paseo/paseo-ownership.ts +++ b/packages/coding-agent/src/setup/paseo/paseo-ownership.ts @@ -22,14 +22,42 @@ import { ABSENT_IDENTITY, currentIdentity, serializeJson } from "./json-publishe export const PROVENANCE_VERSION = 1; export const INTENT_VERSION = 1; +/** Pointer to the private sidecar holding a `--force`-replaced provider entry. */ +export interface ProviderReplacedRef { + /** Absolute path of the mode-0600 sidecar beside Paseo's own config file. */ + readonly backupPath: string; + /** Digest of the preserved value, binding the sidecar's bytes to this record. */ + readonly valueSha256: string; +} + export interface ProvenanceLedger { readonly version: number; /** `agents.providers` keys GJC created, mapped to the value hash it wrote. */ readonly providerKeys: Record; + /** + * `agents.providers` keys that already existed when GJC ran and whose value + * equaled GJC's desired entry. GJC wrote nothing at those keys, so it never + * owns them; the marker keeps a convergence re-run from adopting them. + * Optional because ledgers written before this field existed never carry it. + */ + readonly providerPreexistingKeys?: Record; + /** + * For `--force` overwrites, a pointer to the private mode-0600 sidecar that + * preserves the exact provider entry GJC replaced. `--remove` restores from + * that sidecar instead of deleting the key, because the replaced content was + * never GJC's to take. The value itself lives only in the sidecar beside + * Paseo's own config -- never in this ledger or the intent record -- because + * a provider entry can carry credential-bearing `env` or argument values and + * GJC-side state is credential-free by contract. Optional for the same + * legacy-ledger reason. + */ + readonly providerReplacedEntries?: Record; /** Orchestration role keys GJC actually seeded, mapped to the value it wrote. */ readonly seededOrchestrationKeys: Record; /** Bridge directory path GJC created, when it created it. */ readonly bridgePath?: string; + /** Source directory the bridge entries were linked from, so `--remove` can verify targets. */ + readonly bridgeSourceDir?: string; /** Bridge entries GJC created, so the inverse removes exactly those. */ readonly bridgeEntries?: readonly string[]; /** True when GJC created the bridge directory itself (as opposed to populating an existing one). */ @@ -39,9 +67,20 @@ export interface ProvenanceLedger { export const EMPTY_LEDGER: ProvenanceLedger = { version: PROVENANCE_VERSION, providerKeys: {}, + providerPreexistingKeys: {}, + providerReplacedEntries: {}, seededOrchestrationKeys: {}, }; +export class ProvenanceLedgerCorruptError extends Error { + constructor(provenancePath: string, detail: string) { + super( + `Paseo provenance ledger is corrupt (${provenancePath}): ${detail}. Restore it from a backup or delete it after confirming no Paseo bridge links are live; GJC will not guess ownership from a damaged record.`, + ); + this.name = "ProvenanceLedgerCorruptError"; + } +} + export async function readProvenance(provenancePath: string): Promise { let raw: string; try { @@ -52,31 +91,117 @@ export async function readProvenance(provenancePath: string): Promise; + // Every field that is PRESENT but malformed is corruption, not a + // default: silently replacing it with an empty value would let removal + // act on a partially tampered record (a dropped replaced-provider + // pointer deletes the user's key without restoring it; a filtered + // bridgeEntries list abandons links the record still owns). Absent + // fields stay defaulted so older ledger shapes keep reading. + const strictStringRecord = (value: unknown, field: string): Record => { + if (value === undefined) return {}; + if (!isStringRecord(value)) throw new Error(`${field} is present but not a string record`); + return value; + }; + if (parsed.version !== undefined && typeof parsed.version !== "number") { + throw new Error("version is present but not a number"); + } + if (parsed.providerPreexistingKeys !== undefined && !isTrueRecord(parsed.providerPreexistingKeys)) { + throw new Error("providerPreexistingKeys is present but not a boolean record"); + } + if ( + parsed.providerReplacedEntries !== undefined && + !isProviderReplacedRefRecord(parsed.providerReplacedEntries) + ) { + throw new Error("providerReplacedEntries is present but not a replaced-ref record"); + } + if (parsed.bridgePath !== undefined && typeof parsed.bridgePath !== "string") { + throw new Error("bridgePath is present but not a string"); + } + if (parsed.bridgeSourceDir !== undefined && typeof parsed.bridgeSourceDir !== "string") { + throw new Error("bridgeSourceDir is present but not a string"); + } + if (parsed.bridgeDirCreated !== undefined && typeof parsed.bridgeDirCreated !== "boolean") { + throw new Error("bridgeDirCreated is present but not a boolean"); + } + if (parsed.bridgeEntries !== undefined) { + if (!Array.isArray(parsed.bridgeEntries)) throw new Error("bridgeEntries is present but not an array"); + if (!parsed.bridgeEntries.every(entry => typeof entry === "string")) { + throw new Error("bridgeEntries contains a non-string entry"); + } + } return { version: typeof parsed.version === "number" ? parsed.version : PROVENANCE_VERSION, - providerKeys: isStringRecord(parsed.providerKeys) ? parsed.providerKeys : {}, - seededOrchestrationKeys: isStringRecord(parsed.seededOrchestrationKeys) ? parsed.seededOrchestrationKeys : {}, + providerKeys: strictStringRecord(parsed.providerKeys, "providerKeys"), + providerPreexistingKeys: isTrueRecord(parsed.providerPreexistingKeys) ? parsed.providerPreexistingKeys : {}, + providerReplacedEntries: isProviderReplacedRefRecord(parsed.providerReplacedEntries) + ? parsed.providerReplacedEntries + : {}, + seededOrchestrationKeys: strictStringRecord(parsed.seededOrchestrationKeys, "seededOrchestrationKeys"), ...(typeof parsed.bridgePath === "string" ? { bridgePath: parsed.bridgePath } : {}), - ...(Array.isArray(parsed.bridgeEntries) - ? { bridgeEntries: parsed.bridgeEntries.filter((entry): entry is string => typeof entry === "string") } - : {}), + ...(typeof parsed.bridgeSourceDir === "string" ? { bridgeSourceDir: parsed.bridgeSourceDir } : {}), + ...(Array.isArray(parsed.bridgeEntries) ? { bridgeEntries: parsed.bridgeEntries } : {}), ...(typeof parsed.bridgeDirCreated === "boolean" ? { bridgeDirCreated: parsed.bridgeDirCreated } : {}), }; - } catch { - // A corrupt GJC-side ledger must not brick removal: treat it as empty so - // nothing is deleted on unproven ownership, which is the safe direction. - return EMPTY_LEDGER; + } catch (error) { + // A corrupt GJC-side ledger is an explicit recovery error, not an empty + // ledger: treating it as empty would silently discard every ownership + // record while the links those records cover are still live, defeating + // the record-before-mutation guarantee. + throw new ProvenanceLedgerCorruptError(provenancePath, error instanceof Error ? error.message : String(error)); } } +function isProviderReplacedRefRecord(value: unknown): value is Record { + if (!isRecord(value)) return false; + for (const ref of Object.values(value)) { + if ( + !isRecord(ref) || + typeof ref.backupPath !== "string" || + !path.isAbsolute(ref.backupPath) || + typeof ref.valueSha256 !== "string" + ) { + return false; + } + } + return true; +} function isStringRecord(value: unknown): value is Record { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + // Every VALUE must be a string too (#4644 review r10): `{gjc: 7}` is a + // malformed record, not a valid string record, and accepting it would let + // a tampered ledger carry non-string provenance through fail-closed checks. + return Object.values(value).every(entry => typeof entry === "string"); +} +function isTrueRecord(value: unknown): value is Record { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + return Object.values(value).every(entry => entry === true); +} + +function isRecord(value: unknown): value is Record { return !!value && typeof value === "object" && !Array.isArray(value); } export async function writeProvenance(provenancePath: string, ledger: ProvenanceLedger): Promise { await fs.mkdir(path.dirname(provenancePath), { recursive: true, mode: 0o700 }); - await Bun.write(provenancePath, serializeJson(ledger)); - await fs.chmod(provenancePath, 0o600); + // Write-then-rename: an interrupted Bun.write can leave a truncated file, + // which would otherwise read back as a corrupt (now explicit-error) ledger + // and strand every published link without ownership metadata. The temporary + // is fsynced before the rename so the record is durable once visible. + const temporary = `${provenancePath}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`; + const payload = serializeJson(ledger); + try { + const handle = await fs.open(temporary, "w", 0o600); + try { + await handle.writeFile(payload, "utf8"); + await handle.sync(); + } finally { + await handle.close(); + } + await fs.rename(temporary, provenancePath); + } catch (error) { + await fs.rm(temporary, { force: true }).catch(() => undefined); + throw error; + } } /** @@ -135,8 +260,35 @@ export interface IntentRecord { export async function writeIntent(intentPath: string, intent: IntentRecord): Promise { await fs.mkdir(path.dirname(intentPath), { recursive: true, mode: 0o700 }); - await Bun.write(intentPath, serializeJson(intent)); - await fs.chmod(intentPath, 0o600); + // Write-then-rename with fsync (#4644 review r10), exactly like the + // provenance ledger: a truncated intent would read back as absent and + // recovery would proceed without the record that reconciles a published + // target with its ledger. + const temporary = `${intentPath}.${process.pid}.${Math.random().toString(36).slice(2)}.tmp`; + const payload = serializeJson(intent); + try { + const handle = await fs.open(temporary, "w", 0o600); + try { + await handle.writeFile(payload, "utf8"); + await handle.sync(); + } finally { + await handle.close(); + } + await fs.rename(temporary, intentPath); + } catch (error) { + await fs.rm(temporary, { force: true }).catch(() => undefined); + throw error; + } +} + +/** A present-but-malformed intent record: recovery must refuse, not proceed as if absent. */ +export class IntentRecordCorruptError extends Error { + constructor(intentPath: string, detail: string) { + super( + `Paseo intent record is corrupt (${intentPath}): ${detail}. Delete it only after confirming no Paseo setup step was interrupted; GJC will not guess recovery from a damaged record.`, + ); + this.name = "IntentRecordCorruptError"; + } } export async function readIntent(intentPath: string): Promise { @@ -147,13 +299,70 @@ export async function readIntent(intentPath: string): Promise { + if (typeof value !== "string" || value.length === 0) { + throw new IntentRecordCorruptError(intentPath, `${name} is missing or not a string`); + } + return value; + }; + stringField(parsed?.targetPath, "targetPath"); + stringField(parsed?.targetExpectedIdentity, "targetExpectedIdentity"); + stringField(parsed?.targetPreflightIdentity, "targetPreflightIdentity"); + stringField(parsed?.provenancePath, "provenancePath"); + stringField(parsed?.provenancePreflightIdentity, "provenancePreflightIdentity"); + stringField(parsed?.provenanceExpectedIdentity, "provenanceExpectedIdentity"); + stringField(parsed?.startedAt, "startedAt"); + if (parsed?.step !== "provider-config" && parsed?.step !== "orchestration-preferences") { + throw new IntentRecordCorruptError(intentPath, "step is not a known intent step"); + } + if (typeof parsed?.version !== "number") { + throw new IntentRecordCorruptError(intentPath, "version is not a number"); + } + if ( + !Array.isArray(parsed?.ownedKeys) || + !parsed.ownedKeys.every(key => typeof key === "string" && key.length > 0) + ) { + throw new IntentRecordCorruptError(intentPath, "ownedKeys is not a non-empty string array"); + } + // The provenance path the record names must sit in the SAME agent + // directory tree as the intent record itself: recovery writes through + // it, so a tampered record must not redirect that write elsewhere. + // The check is CANONICAL (#4644 review r19): lexical path.relative can + // be satisfied through a symlinked ancestor that resolves outside the + // agent directory, so both trees are realpath-resolved first and a + // symlinked ancestry that redirects the record's tree out of the + // intent's tree is refused. + const intentDir = path.dirname(await fs.realpath(path.resolve(intentPath)).catch(() => path.resolve(intentPath))); + const namedProvenance = path.resolve(parsed.provenancePath); + const namedDir = await fs.realpath(path.dirname(namedProvenance)).catch(() => path.dirname(namedProvenance)); + const rel = path.relative(intentDir, namedDir); + if (rel.startsWith("..") || path.isAbsolute(rel)) { + throw new IntentRecordCorruptError( + intentPath, + `the recorded provenance path (${parsed.provenancePath}) escapes the agent directory that holds the intent record`, + ); + } + if (parsed?.provenancePayload !== undefined) { + // Shape-only validation; readProvenance's own strict validation runs + // before any payload is ever written. + if (typeof parsed.provenancePayload !== "object" || parsed.provenancePayload === null) { + throw new IntentRecordCorruptError(intentPath, "provenancePayload is not an object"); + } + } return parsed; - } catch { - return undefined; + } catch (error) { + if (error instanceof IntentRecordCorruptError) throw error; + throw new IntentRecordCorruptError(intentPath, error instanceof Error ? error.message : String(error)); } } diff --git a/packages/coding-agent/src/setup/paseo/paseo-setup.ts b/packages/coding-agent/src/setup/paseo/paseo-setup.ts index 845b6e56e3..e3094f1ea1 100644 --- a/packages/coding-agent/src/setup/paseo/paseo-setup.ts +++ b/packages/coding-agent/src/setup/paseo/paseo-setup.ts @@ -4,12 +4,24 @@ * Dispatches to diagnosis, install, or removal, and owns the flag combinations * that must be rejected before any target is touched. */ +import * as fs from "node:fs/promises"; +import * as path from "node:path"; import { Settings } from "../../config/settings"; import { checkPaseoSetup } from "./check"; import { type CompletedStep, compensate, receiptStep, recoverIntent, runJsonStep, SagaStepError } from "./install-saga"; -import { PaseoPublishError, readTarget } from "./json-publisher"; +import { + hashBytes, + PaseoPublishError, + readReplacedProviderBackup, + readTarget, + removeReplacedProviderBackup, + replacedProviderBackupPath, + serializeJson, + writeReplacedProviderBackup, +} from "./json-publisher"; import { createOrchestrationSeed, removeSeededRoles } from "./orchestration-preferences"; -import { readProvenance, writeProvenance } from "./paseo-ownership"; +import { withPaseoMutationLock } from "./paseo-mutation-lock"; +import { type ProvenanceLedger, readProvenance, writeProvenance } from "./paseo-ownership"; import { buildProviderEntry, createProviderMutation, @@ -18,14 +30,17 @@ import { providerKeyFor, resolveGjcCommand, } from "./provider-config"; -import { removePaseoSetup } from "./remove"; +import { removePaseoSetup, safeBridgeEntryNames, validatedBridgeDir } from "./remove"; import type { PaseoInstallResult, PaseoRemoveResult, SetupCheckResult } from "./result-types"; import type { PaseoSetupDependencies } from "./setup-deps"; +import type { SkillsBridgeInstallResult } from "./skills-bridge"; import { installSkillsBridge, inverseSkillsBridge, + legacySourceDirFor, preflightSkillsBridge, registerSkillsBridgeDirectory, + SkillsBridgePartialError, } from "./skills-bridge"; export interface PaseoSetupFlags { @@ -73,17 +88,36 @@ export async function runPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepe } if (flags.remove) { - const settings = await Settings.init(); - const result = await removePaseoSetup(deps, { - now: deps.now(), - unregisterBridgeDirectory: async () => { - await unregisterBridgeDirectory(settings, deps.paths.bridgeDir); - }, + // `--check` is read-only and needs no lock. Install and remove mutate the + // same intent record, provenance ledger, Paseo config targets, bridge, and + // GJC settings, so both hold one per-agent-directory mutation lock from + // recovery through the final provenance write: a concurrent remove cannot + // clear the ledger while an install is still creating links and + // registering the bridge. + return await withPaseoMutationLock(deps, async () => { + const settings = await Settings.init(); + const ledger = await readProvenance(deps.paths.provenanceLedger); + // Unregister the LEDGER-RECORDED directory (the one GJC actually + // registered at install time); after a path migration this can differ + // from the current default bridge path. + const recordedBridgeDir = ledger.bridgePath ?? deps.paths.bridgeDir; + const result = await removePaseoSetup(deps, { + now: deps.now(), + unregisterBridgeDirectory: async () => { + await unregisterBridgeDirectory(settings, recordedBridgeDir); + if (recordedBridgeDir !== deps.paths.bridgeDir) { + await unregisterBridgeDirectory(settings, deps.paths.bridgeDir).catch(() => undefined); + } + }, + }); + return { kind: "remove", result }; }); - return { kind: "remove", result }; } - return { kind: "install", result: await installPaseoSetup(flags, deps) }; + return { + kind: "install", + result: await withPaseoMutationLock(deps, () => installPaseoSetup(flags, deps)), + }; } /** @@ -137,6 +171,49 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende try { // Step 1: provider entry + provider-key provenance. + // + // Ownership is decided by what existed BEFORE this run, not by value + // equality: an identical entry the user hand-wrote stays theirs (marked + // pre-existing, never recorded in providerKeys), while a `--force` + // overwrite stores the replaced entry so `--remove` restores it rather + // than deleting content that was never GJC's to take. + // The RAW prior value -- including scalars, arrays, and null -- is what a + // `--force` overwrite replaces, so that is what must be restorable. An + // object-shaped reader alone would lose a scalar/array/null prior and let + // compensation delete user configuration instead of restoring it. + // The prior value is preserved in a private sidecar beside Paseo's own + // config, never in the ledger or intent record: a provider entry can + // carry credential-bearing `env` or argument values, and GJC-side + // durable state is credential-free by contract. The FIRST replaced + // value is the user's; a repeated `--force` must not overwrite it, so + // the sidecar is written only when the ledger holds no pointer yet. + const rawPriorValue = readRawProviderValue(config.parsed, providerKey); + // Same structural equality the conflict check uses, so ownership follows + // the exact predicate that decides whether GJC would write anything. + const existingMatches = rawPriorValue !== undefined && JSON.stringify(rawPriorValue) === JSON.stringify(entry); + const replacedEntry = rawPriorValue !== undefined && !existingMatches ? rawPriorValue : undefined; + // The sidecar pointer is derived deterministically (injective path + + // value digest), so the intent record can carry the full post-step + // ledger before any artifact exists. The file itself is created by the + // step's `persist` hook only after the CAS publish succeeded: a refused + // or conflicting publish (#4644 review r8) must not strand an + // unreferenced credential-bearing sidecar beside Paseo's config. + const priorReplacedRef = + replacedEntry !== undefined + ? (await readProvenance(deps.paths.provenanceLedger)).providerReplacedEntries?.[providerKey] + : undefined; + const createdReplacedRef = + replacedEntry !== undefined && priorReplacedRef === undefined + ? { + backupPath: replacedProviderBackupPath(deps.paths.configJson, providerKey), + valueSha256: hashBytes(serializeJson(replacedEntry)), + } + : undefined; + const replacedBackup = priorReplacedRef ?? createdReplacedRef; + // The exact sidecar payload this run would create ({key,value} serialized); + // unpersist deletes the file only when its CONTENT still hashes to these + // authenticated bytes (#4644 reviews r16/r17 — size alone is spoofable and + // omitted the {key,value} wrapper). const step1 = await runJsonStep({ label: deps.paths.configJson, step: "provider-config", @@ -144,14 +221,75 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende provenancePath: deps.paths.provenanceLedger, intentPath: deps.paths.intentRecord, ownedKeys: [`agents.providers.${providerKey}`], + // Ownership (pre-existing vs created vs replaced) was decided from the + // preflight snapshot; a config that changed since is refused rather + // than mutated under stale decisions (#4644 review r7). + expectedPreflightIdentity: config.identity, mutate: createProviderMutation(config, providerKey, entry), - nextLedger: ledger => ({ ...ledger, providerKeys: { ...ledger.providerKeys, [providerKey]: entryHash } }), - revert: draft => removeProviderKey(draft, providerKey), + nextLedger: ledger => ({ + ...ledger, + providerKeys: existingMatches + ? { ...ledger.providerKeys } + : { ...ledger.providerKeys, [providerKey]: entryHash }, + providerPreexistingKeys: existingMatches + ? { ...ledger.providerPreexistingKeys, [providerKey]: true as const } + : { ...ledger.providerPreexistingKeys }, + providerReplacedEntries: + replacedBackup !== undefined + ? { ...ledger.providerReplacedEntries, [providerKey]: replacedBackup } + : { ...ledger.providerReplacedEntries }, + }), + // Compensation restores what this run actually replaced: a key GJC + // created is removed, a key that carried ANY prior value (including a + // scalar, array, or null) gets that exact value back, and an + // identical pre-existing entry was never written and keeps its value. + revert: draft => restoreProviderKey(draft, providerKey, replacedEntry), revertLedger: ledger => { const providerKeys = { ...ledger.providerKeys }; delete providerKeys[providerKey]; - return { ...ledger, providerKeys }; + const providerPreexistingKeys = { ...ledger.providerPreexistingKeys }; + delete providerPreexistingKeys[providerKey]; + const providerReplacedEntries = { ...ledger.providerReplacedEntries }; + delete providerReplacedEntries[providerKey]; + return { ...ledger, providerKeys, providerPreexistingKeys, providerReplacedEntries }; }, + persist: + createdReplacedRef !== undefined + ? () => + writeReplacedProviderBackup(deps.paths.configJson, providerKey, replacedEntry).then(written => { + if ( + written.backupPath !== createdReplacedRef.backupPath || + written.valueSha256 !== createdReplacedRef.valueSha256 + ) { + throw new PaseoPublishError(written.backupPath, { + reason: "sidecar-conflict", + detail: `the sidecar for key ${providerKey} changed while GJC was preparing its update`, + }); + } + }) + : undefined, + unpersist: + createdReplacedRef !== undefined + ? async () => { + // Only a sidecar THIS RUN created is removed (#4644 + // reviews r16/r17): the file is read fd-bound and its + // CONTENT must hash to the exact {key,value} payload + // this run wrote — a same-sized attacker replacement + // fails the digest and is preserved, as is any + // pre-existing sidecar (the user's value or a plant). + const match = await readReplacedProviderBackup( + createdReplacedRef.backupPath, + providerKey, + createdReplacedRef.valueSha256, + ); + if (match.found) + await removeReplacedProviderBackup( + createdReplacedRef.backupPath, + providerKey, + createdReplacedRef.valueSha256, + ); + } + : undefined, now, }); completed.push(step1.completed); @@ -166,6 +304,10 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende provenancePath: deps.paths.provenanceLedger, intentPath: deps.paths.intentRecord, ownedKeys: [...seed.seededKeys], + // Which roles are EMPTY (and therefore seedable) was decided from + // the preflight snapshot; changed bytes refuse instead of seeding + // roles a concurrent edit already filled (#4644 review r7). + expectedPreflightIdentity: preferences.identity, mutate: seed.mutate, nextLedger: ledger => ({ ...ledger, @@ -184,17 +326,155 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende if (step2.changed) changed.push(deps.paths.orchestrationPreferences); } - // Step 3: the symlink bridge. - const bridge = await installSkillsBridge(bridgePreflight); - if (bridge.createdEntries.length > 0 || bridge.bridgeDirCreated) { - changed.push(deps.paths.bridgeDir); - const ledger = await readProvenance(deps.paths.provenanceLedger); + // Step 3: the symlink bridge. Install converges the bridge to the current + // source (create missing, prune stale, adopt pre-#4638 legacy links). + // + // Provenance is committed BEFORE any link is created or pruned, and the + // ownership set is exactly what GJC will own after this run: links it + // created in an earlier run (the pre-existing non-noop entries), links + // it adopts through the legacy migration, and -- until the prunes + // complete -- the stale links it is about to remove, so a crash between + // record and unlink still leaves every on-disk link covered. A `noop` + // entry GJC did not previously record is deliberately NOT added: an + // exact-target link the user created themselves must never become + // GJC-owned just because a re-run observed it. + const bridgeLedger = await readProvenance(deps.paths.provenanceLedger); + // Migration binding: recorded ownership belongs to the recorded bridge + // PATH, not to the skill names alone. When the agent/profile path moved, + // the names are not carried over silently -- a user-owned exact-target + // link at the new path must never inherit ownership from the old one, + // and the old path's links are cleaned up explicitly instead of being + // abandoned by the overwrite below. + const recordedBridgePath = bridgeLedger.bridgePath; + const isMigration = + recordedBridgePath !== undefined && path.resolve(recordedBridgePath) !== path.resolve(deps.paths.bridgeDir); + let migratedOldEntries: readonly string[] = []; + let migratedOldBridgeDir: string | undefined; + if (isMigration && (bridgeLedger.bridgeEntries?.length ?? 0) > 0) { + // The migration branch composes destructive cleanup paths from the + // ledger's own bytes, so it must fail closed exactly like `--remove` + // does: a tampered or malformed record (a `..` entry, a relative or + // escaping bridge path) is refused, never fed to the unlinker. + // The old bridge's links are removed only AFTER the new ledger and + // settings cutover below is durable, as a compensable step, so a + // later failure restores the old bridge instead of leaving the ledger + // pointing at missing links. + const oldBridgeDir = await validatedBridgeDir(bridgeLedger, deps); + migratedOldEntries = safeBridgeEntryNames(bridgeLedger.bridgeEntries ?? []); + migratedOldBridgeDir = oldBridgeDir; + } + const previouslyRecorded = isMigration ? new Set() : new Set(bridgeLedger.bridgeEntries ?? []); + const ownedAfterRun = [ + // Entries this run or an earlier run actually creates/recreates. + ...Object.values(bridgePreflight.entries) + .filter(entry => entry.action !== "noop" || (previouslyRecorded.has(entry.name) && entry.action === "noop")) + .map(entry => entry.name), + // Legacy links GJC adopts become owned at their new target. + ...bridgePreflight.adopts.map(adopt => adopt.name), + // Prune candidates stay recorded until the unlink completes below; + // the post-install write then drops them. + ...bridgePreflight.prunes.map(prune => prune.name), + ].filter((name, index, all) => all.indexOf(name) === index); + const hasBridgeWork = ownedAfterRun.length > 0 || bridgePreflight.bridgeDirCreated; + // A fresh no-source run owns NOTHING (#4644 review r8): the preflight's + // `bridgeDirCreated` is a PLAN (the directory is absent), not a fact, and + // `installSkillsBridge` skips creation entirely when there is no work. A + // ledger that recorded the plan as fact would let a later `--remove` trust + // false ownership of a directory GJC never created and delete user work + // that later appeared at that path. Nothing is recorded at all. + if ( + bridgePreflight.sourceDir === undefined && + !ledgerOwnsBridge(bridgeLedger) && + bridgePreflight.bridgeDirCreated + ) { + return { outcome: "installed", changed: [...changed, "paseo skills bridge (no source)"] }; + } + // A resolved source containing no `paseo*` skills is an intentional + // no-bridge state: `installSkillsBridge` will create neither the + // directory nor any link, so persisting a bridge path here would record + // a directory GJC never created and registering it would globally load + // whatever foreign content later appears at that path. Provenance and + // registration are both skipped; the ledger keeps whatever it had. + const intentionalNoBridge = + bridgePreflight.sourceDir !== undefined && + ownedAfterRun.length === 0 && + Object.keys(bridgePreflight.entries).length === 0 && + bridgePreflight.adopts.length === 0 && + bridgePreflight.prunes.length === 0 && + // A bridge directory GJC created and still owns (an earlier run pruned + // the final entry) keeps its provenance: clearing the record would + // strand an owned directory and its registration. The no-bridge state + // applies only when the ledger owns no bridge at all. + bridgeLedger.bridgeDirCreated !== true && + bridgeLedger.bridgePath === undefined; + if (intentionalNoBridge) { await writeProvenance(deps.paths.provenanceLedger, { - ...ledger, + ...bridgeLedger, + bridgePath: undefined, + bridgeEntries: [], + bridgeDirCreated: false, + bridgeSourceDir: undefined, + }); + return { outcome: "installed", changed: [...changed, "paseo skills bridge (empty source)"] }; + } + if (hasBridgeWork || bridgePreflight.sourceDir !== undefined) { + await writeProvenance(deps.paths.provenanceLedger, { + ...bridgeLedger, bridgePath: deps.paths.bridgeDir, - bridgeEntries: [...bridge.createdEntries], - bridgeDirCreated: bridge.bridgeDirCreated, + bridgeEntries: ownedAfterRun, + // `bridgeDirCreated` records whether GJC created THIS directory, + // so `--remove` knows whether the empty directory is ours to + // delete. It is per-path FACT ownership (#4644 review r8): this + // pre-write carries only what an earlier run actually created -- + // never the preflight's plan -- and the creator bit for a freshly + // created directory is committed below, only after the exclusive + // `mkdir` inside `installSkillsBridge` succeeded. A crash before + // that point leaves an honest `false`; a concurrent creator making + // the `mkdir` fail EEXIST leaves an honest `false` too. + bridgeDirCreated: isMigration ? false : bridgeLedger.bridgeDirCreated === true, + ...(bridgePreflight.sourceDir !== undefined ? { bridgeSourceDir: bridgePreflight.sourceDir } : {}), + }); + } + let bridge: SkillsBridgeInstallResult; + try { + bridge = await installSkillsBridge(bridgePreflight); + } catch (error) { + // A mid-operation bridge failure must not leave the pre-write's + // PLAN standing as provenance FACT (#4644 review r9): the failed + // install carries what actually completed, so the ledger is + // corrected to observed reality — prior entries minus completed + // prunes plus created/adopted links — before the saga error + // propagates. Never-created planned entries stop being claimed; + // `--check` reports no phantom missing-bridge-link drift and + // `--remove` sees exactly the links that exist. + if (error instanceof SkillsBridgePartialError) { + await correctBridgeOwnershipAfterFailure(deps, bridgeLedger, isMigration, error.partial); + } + throw error; + } + // The directory was exclusively created a moment ago: directory ownership + // is persisted only now that the creation actually succeeded (#4644 + // review r8), never from the preflight's plan. + if (bridge.bridgeDirCreated) { + const afterCreate = await readProvenance(deps.paths.provenanceLedger); + await writeProvenance(deps.paths.provenanceLedger, { ...afterCreate, bridgeDirCreated: true }); + } + // Prunes have completed: drop them from the ownership record so a later + // `--remove` does not treat the removed names as still owned. + if (bridgePreflight.prunes.length > 0) { + const afterPrunes = await readProvenance(deps.paths.provenanceLedger); + await writeProvenance(deps.paths.provenanceLedger, { + ...afterPrunes, + bridgeEntries: (afterPrunes.bridgeEntries ?? []).filter(name => !bridge.prunedEntries.includes(name)), }); + } + if ( + bridge.createdEntries.length > 0 || + bridge.prunedEntries.length > 0 || + bridge.adoptedEntries.length > 0 || + bridge.bridgeDirCreated + ) { + changed.push(deps.paths.bridgeDir); completed.push({ label: deps.paths.bridgeDir, undo: async () => { @@ -212,11 +492,74 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende }); } - // Step 4: register the bridge with GJC skill discovery. + // Step 4: register the bridge with GJC skill discovery -- only when the + // bridge was validated against a real source this run. Registering an + // existing directory that no source validates and no ledger owns would + // globally load whatever a stale or foreign bridge contains. A migration + // REPLACES the old recorded registration in the same atomic commit, so + // the stale path cannot survive the cutover. + if (bridgePreflight.sourceDir === undefined && !ledgerOwnsBridge(bridgeLedger)) { + // The fresh absent-directory case returned before any provenance was + // written; what remains is an existing directory no source validates + // and no ledger owns, which must never be registered globally. + throw new SagaStepError( + "install", + `Refusing to register Paseo skills bridge without a validated source or ownership record (${deps.paths.bridgeDir}); re-run after Paseo is installed or point PASEO_SKILLS_DIR at the real skills directory`, + ); + } const settings = await Settings.init(); - const receipt = await registerSkillsBridgeDirectory(settings, deps.paths.bridgeDir); + const receipt = await registerSkillsBridgeDirectory(settings, deps.paths.bridgeDir, { + ...(migratedOldBridgeDir !== undefined ? { replaces: migratedOldBridgeDir } : {}), + }); completed.push(receiptStep("config.yml skills.customDirectories", receipt)); changed.push("config.yml skills.customDirectories"); + + // The new ledger and settings cutover is now durable, so the old bridge + // can be retired as its own compensable step: on a later failure the old + // links are restored to the old directory (the registration receipt above + // already reverts the swap), instead of stranding a ledger that points at + // links that no longer exist. + if (migratedOldBridgeDir !== undefined && migratedOldEntries.length > 0) { + const oldSourceDir = bridgeLedger.bridgeSourceDir ?? legacySourceDirFor(deps); + try { + await inverseSkillsBridge( + deps, + { + createdEntries: [...migratedOldEntries], + prunedEntries: [], + adoptedEntries: [], + bridgeDirCreated: bridgeLedger.bridgeDirCreated ?? false, + sourceDir: oldSourceDir, + }, + { bridgeDir: migratedOldBridgeDir }, + ); + } catch (error) { + throw new SagaStepError( + "install", + `bridge path migrated from ${recordedBridgePath} but the old bridge could not be cleaned: ${error instanceof Error ? error.message : String(error)}`, + ); + } + completed.push({ + label: migratedOldBridgeDir, + undo: async () => { + // Restore the old bridge exactly as it was recorded: the same + // links at the same recorded targets in the old directory. + try { + for (const name of migratedOldEntries) { + await fs.symlink(path.resolve(oldSourceDir, name), path.join(migratedOldBridgeDir ?? "", name)); + } + return { status: "reverted" as const }; + } catch (error) { + return { + status: "conflict" as const, + detail: error instanceof Error ? error.message : String(error), + retained: [migratedOldBridgeDir ?? deps.paths.bridgeDir], + }; + } + }, + }); + changed.push(migratedOldBridgeDir); + } } catch (error) { const failure = error instanceof SagaStepError @@ -232,12 +575,56 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende return { outcome: "installed", changed }; } -function removeProviderKey(draft: Record, providerKey: string): void { +/** True when the ledger proves GJC owns the current bridge directory and its entries. */ +function ledgerOwnsBridge(ledger: ProvenanceLedger): boolean { + return (ledger.bridgeEntries?.length ?? 0) > 0 && ledger.bridgePath !== undefined; +} +/** + * Correct the provenance ledger to what a FAILED bridge install actually + * completed (#4644 review r9): the pre-write's planned ownership must not + * stand as fact after a mid-operation failure, so ownership becomes the prior + * recorded entries minus completed prunes plus created/adopted links. + */ +export async function correctBridgeOwnershipAfterFailure( + deps: PaseoSetupDependencies, + bridgeLedger: ProvenanceLedger, + isMigration: boolean, + partial: SkillsBridgeInstallResult, +): Promise { + const pruned = new Set(partial.prunedEntries); + const actual = [ + ...(isMigration ? [] : (bridgeLedger.bridgeEntries ?? []).filter(name => !pruned.has(name))), + ...partial.createdEntries, + ...partial.adoptedEntries, + ].filter((name, index, all) => all.indexOf(name) === index); + const corrected = await readProvenance(deps.paths.provenanceLedger); + await writeProvenance(deps.paths.provenanceLedger, { + ...corrected, + bridgeEntries: actual, + bridgeDirCreated: corrected.bridgeDirCreated === true || partial.bridgeDirCreated, + }); +} +/** The RAW value a Paseo config carries at `agents.providers.`, of any shape. */ +function readRawProviderValue(config: Record, providerKey: string): unknown { + const agents = config.agents; + if (!agents || typeof agents !== "object" || Array.isArray(agents)) return undefined; + const providers = (agents as Record).providers; + if (!providers || typeof providers !== "object" || Array.isArray(providers)) return undefined; + return (providers as Record)[providerKey]; +} + +/** + * Undo a provider write exactly: a key this run created is removed, and a key + * that carried ANY prior value (object, scalar, array, or null) gets that + * value back instead of being deleted. + */ +function restoreProviderKey(draft: Record, providerKey: string, replacedEntry: unknown): void { const agents = draft.agents; if (!agents || typeof agents !== "object" || Array.isArray(agents)) return; const providers = (agents as Record).providers; if (!providers || typeof providers !== "object" || Array.isArray(providers)) return; - delete (providers as Record)[providerKey]; + if (replacedEntry === undefined) delete (providers as Record)[providerKey]; + else (providers as Record)[providerKey] = replacedEntry; } async function unregisterBridgeDirectory(settings: Settings, bridgeDir: string): Promise { diff --git a/packages/coding-agent/src/setup/paseo/provider-config.ts b/packages/coding-agent/src/setup/paseo/provider-config.ts index dbd21ae2aa..fd4996ecbf 100644 --- a/packages/coding-agent/src/setup/paseo/provider-config.ts +++ b/packages/coding-agent/src/setup/paseo/provider-config.ts @@ -121,7 +121,7 @@ export function hasProviderConflict( }; } -export function providerEntryHash(entry: PaseoProviderEntry): string { +export function providerEntryHash(entry: PaseoProviderEntry | Record): string { return hashBytes(JSON.stringify(entry)); } diff --git a/packages/coding-agent/src/setup/paseo/remove.ts b/packages/coding-agent/src/setup/paseo/remove.ts index eca6096c21..c1374512d8 100644 --- a/packages/coding-agent/src/setup/paseo/remove.ts +++ b/packages/coding-agent/src/setup/paseo/remove.ts @@ -10,26 +10,254 @@ * that cannot be undone safely halts the rest, so the result is an * interpretable prefix rather than a scattered mix. */ -import { planPublish, publishPlan, readTarget } from "./json-publisher"; + +import type { Stats } from "node:fs"; +import * as fs from "node:fs/promises"; +import * as path from "node:path"; +import { + planPublish, + publishPlan, + readReplacedProviderBackup, + readTarget, + removeReplacedProviderBackup, + replacedProviderBackupPath, +} from "./json-publisher"; import { removeSeededRoles } from "./orchestration-preferences"; import { EMPTY_LEDGER, isProvenancedOrchestrationKey, isProvenancedProvider, + type ProvenanceLedger, + type ProviderReplacedRef, provenancedProviderKeys, readProvenance, writeProvenance, } from "./paseo-ownership"; import { type PaseoProviderEntry, providerEntryHash } from "./provider-config"; import type { PartialRemovalEvidence, PaseoRemoveResult } from "./result-types"; -import { INSTALL_SKILL_NAMES, type InstallSkillName, type PaseoSetupDependencies } from "./setup-deps"; -import { inverseSkillsBridge, SkillsBridgeError } from "./skills-bridge"; +import { isTrustedRecordedSkillsSource, type PaseoSetupDependencies } from "./setup-deps"; +import { inverseSkillsBridge, legacySourceDirFor, SkillsBridgeError } from "./skills-bridge"; export interface RemoveOptions { readonly now: Date; /** Undo the config.yml `skills.customDirectories` append. Supplied by the orchestrator. */ readonly unregisterBridgeDirectory?: () => Promise; } +/** + * `lstat` distinguishing a genuinely absent path from a filesystem failure. + * + * A permission or I/O error on the bridge directory must NOT be collapsed into + * "absent": treating it as absence clears all bridge provenance and reports a + * successful removal while an owned link is still on disk. Only `ENOENT` counts + * as absent; every other error propagates and fails the removal closed. + */ +async function lstatAllowingAbsent(destination: string): Promise { + try { + return await fs.lstat(destination); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; + throw error; + } +} + +/** + * Prove an agent directory carries GJC's own `paseo` ledger shape: a real + * non-symlink `paseo` directory that itself contains a `provenance.json` + * regular file. `stat` follows symlinks, so a bare directory-name match (or a + * symlink named `paseo`) is not evidence — a forged path inside the bridge + * directory replicates the name but never the full ledger shape. + */ +async function isGenuinePaseoLedgerDir(agentDir: string): Promise { + const ledgerDir = path.join(agentDir, "paseo"); + try { + const stat = await fs.lstat(ledgerDir); + if (!stat.isDirectory()) return false; + const ledger = await fs.lstat(path.join(ledgerDir, "provenance.json")); + return ledger.isFile(); + } catch { + return false; + } +} +/** + * The migration directory's own ledger must claim the exact recorded bridge + * directory (#4644 review r14): the victim's `paseo/provenance.json` must + * record `bridgePath` equal to the path being validated. Fabricating names + * beside an arbitrary directory is trivial; making a victim directory hold a + * GJC ledger that already claims this exact bridge is a genuine prior + * installation. + */ +async function readsRecordedBridgeDir(oldAgentDir: string, recordedBridgeDir: string): Promise { + try { + const ledgerPath = path.join(oldAgentDir, "paseo", "provenance.json"); + const ledger = await readProvenance(ledgerPath); + return ledger.bridgePath !== undefined && path.resolve(ledger.bridgePath) === path.resolve(recordedBridgeDir); + } catch { + return false; + } +} +/** + * Whether a directory actually holds the bridge the ledger describes: every + * PRESENT recorded entry must be a symlink pointing into the ledger's + * recorded source (#4644 review r19). This is the content authentication that + * distinguishes a genuine migration (the old bridge with GJC's links) from a + * tampered ledger aiming at an in-root sibling holding foreign content. + */ +async function directoryHoldsRecordedBridge(ledger: ProvenanceLedger, dir: string): Promise { + const entries = ledger.bridgeEntries ?? []; + if (entries.length === 0) return false; + const sourceDir = ledger.bridgeSourceDir; + if (sourceDir === undefined) return false; + let present = 0; + for (const name of entries) { + if (path.basename(name) !== name) return false; + const entryPath = path.join(dir, name); + const stat = await fs.lstat(entryPath).catch(() => undefined); + if (stat === undefined) continue; // absent entries are consistent with a migration + present += 1; + if (!stat.isSymbolicLink()) return false; + const text = await fs.readlink(entryPath).catch(() => undefined); + if (text === undefined) return false; + if (path.resolve(path.dirname(entryPath), text) !== path.resolve(sourceDir, name)) return false; + } + // A directory holding NONE of the recorded links is not the described + // bridge, whatever the ledger claims about it. + return present > 0; +} + +/** + * The ledger-recorded bridge directory, validated before any destructive use. + * + * A malformed, tampered, or path-replaced provenance record must never + * redirect cleanup at an unrelated directory. Two shapes are accepted: + * + * - the recorded path lives inside the agent directory the ledger itself lives + * in (the parent of the ledger's `paseo/` directory) — the ordinary case; or + * - it is a GENUINE migration record: absolute, canonically spelled, carrying + * the exact bridge basename, whose parent directory also holds the `paseo` + * ledger directory (the recorded agent-dir shape GJC itself wrote). + * + * Both require the path to resolve — without following a final symlink — to a + * directory, and the fully RESOLVED location must satisfy the same two shapes: + * `lstat` inspects only the final component, so a symlinked ancestor could + * otherwise carry a lexically-accepted record somewhere else entirely. + * Anything else fails the removal closed. + */ +export async function validatedBridgeDir(ledger: ProvenanceLedger, deps: PaseoSetupDependencies): Promise { + const recorded = ledger.bridgePath ?? deps.paths.bridgeDir; + const trustedRoot = path.resolve(path.dirname(deps.paths.provenanceLedger), ".."); + const resolved = path.resolve(recorded); + if (!path.isAbsolute(recorded) || resolved !== recorded) { + throw new SkillsBridgeError( + `Refusing to remove Paseo skills bridge: ledger-recorded path is not absolute (${recorded})`, + ); + } + const bridgeBasename = path.basename(path.resolve(deps.paths.bridgeDir)); + const withinTrustedRoot = + resolved === path.resolve(trustedRoot) || resolved.startsWith(`${path.resolve(trustedRoot)}${path.sep}`); + // #4644 review r19: an in-root record is accepted ONLY when it is the + // CURRENT configured bridge path (the ordinary case — the ledger records + // the bridge GJC itself created for this dependency set). Any OTHER + // in-root directory, however bridge-shaped its basename, is not + // automatically trusted: a tampered ledger could aim cleanup at a foreign + // `*-paseo-skills` sibling. A different path must qualify as a GENUINE + // migration record (authenticated by the OLD directory's own ledger, + // below) even when it happens to sit inside this agent root. + const isCurrentBridge = resolved === path.resolve(deps.paths.bridgeDir); + const isBridgeBasename = (candidate: string): boolean => + candidate === bridgeBasename || candidate === "paseo-skills" || candidate.endsWith("-paseo-skills"); + if (!isBridgeBasename(path.basename(resolved))) { + throw new SkillsBridgeError( + `Refusing to remove Paseo skills bridge: ledger-recorded path does not carry the bridge directory name (${recorded}); expected .../${bridgeBasename}`, + ); + } + const oldAgentDir = path.dirname(resolved); + // A migration record is authenticated by the OLD directory's own ledger + // AND by its CONTENT (#4644 review r19): a same-ledger claim on an in-root + // sibling is self-referential, so the recorded directory must actually + // hold the bridge links the ledger describes — every present recorded + // entry a symlink pointing into the ledger's recorded source. A foreign + // sibling with user content fails that check regardless of what a + // tampered ledger claims. + const oldLedgerPath = path.join(oldAgentDir, "paseo", "provenance.json"); + const foreignLedgerRecord = path.resolve(oldLedgerPath) !== path.resolve(deps.paths.provenanceLedger); + const holdsDescribedBridge = await directoryHoldsRecordedBridge(ledger, resolved); + const genuineMigration = + !isCurrentBridge && + (foreignLedgerRecord || holdsDescribedBridge) && + isBridgeBasename(path.basename(resolved)) && + (await isGenuinePaseoLedgerDir(oldAgentDir)) && + (await readsRecordedBridgeDir(oldAgentDir, resolved)); + if (!isCurrentBridge && !genuineMigration) { + throw new SkillsBridgeError( + `Refusing to remove Paseo skills bridge: ledger-recorded path is neither the configured bridge (${deps.paths.bridgeDir}) nor an authenticated migration record (${recorded})`, + ); + } + // Out-of-root records still need the genuine-migration authentication + // (#4644 reviews r14/r19): the OLD directory must carry a GJC ledger that + // itself claims this exact bridge path. + if (!withinTrustedRoot && !genuineMigration) { + throw new SkillsBridgeError( + `Refusing to remove Paseo skills bridge: ledger-recorded path escapes the agent directory (${recorded})`, + ); + } + try { + const stat = await fs.lstat(recorded); + if (stat.isSymbolicLink()) { + throw new SkillsBridgeError( + `Refusing to remove Paseo skills bridge: ledger-recorded path is a symlink (${recorded})`, + ); + } + if (!stat.isDirectory()) { + throw new SkillsBridgeError( + `Refusing to remove Paseo skills bridge: ledger-recorded path is not a directory (${recorded})`, + ); + } + } catch (error) { + if (error instanceof SkillsBridgeError) throw error; + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + // The recorded directory is gone: nothing to remove on disk, and the + // ledger cleanup below still runs. + return recorded; + } + throw error; + } + // A canonically spelled, non-symlink record can still travel through a + // symlinked ANCESTOR: `lstat` above inspects only the final component. The + // fully resolved location must satisfy the same two accepted shapes, so a + // record that merely looks safe lexically cannot steer cleanup outside the + // agent root. `realpath` failures (EACCES and friends) propagate and fail + // the removal closed. + const real = await fs.realpath(recorded); + const realRoot = await fs.realpath(trustedRoot); + const realWithinTrustedRoot = real === realRoot || real.startsWith(`${realRoot}${path.sep}`); + const realMigrationShape = + path.basename(real) === bridgeBasename && + (await isGenuinePaseoLedgerDir(path.dirname(real))) && + (await readsRecordedBridgeDir(path.dirname(real), real)); + if (!realWithinTrustedRoot && !realMigrationShape) { + throw new SkillsBridgeError( + `Refusing to remove Paseo skills bridge: ledger-recorded path resolves outside the agent directory (${recorded})`, + ); + } + return recorded; +} +/** + * Ledger entry names that are safe to compose cleanup paths from. + * + * Every recorded entry must be a single plain basename. A tampered ledger can + * carry `../`-style traversal or path separators; such a name is never handed + * to a path join, it is reported as a refusal instead. + */ +export function safeBridgeEntryNames(entries: readonly string[]): readonly string[] { + for (const name of entries) { + if (path.basename(name) !== name || name.includes("/") || name === "." || name === "..") { + throw new SkillsBridgeError( + `Refusing to remove Paseo skills bridge entry with an unsafe recorded name (${name}); restore or delete the provenance ledger after confirming no Paseo bridge links are live`, + ); + } + } + return entries; +} /** * Remove every target GJC can prove it owns. @@ -45,13 +273,51 @@ export async function removePaseoSetup( const ownsAnything = provenancedProviderKeys(ledger).length > 0 || Object.keys(ledger.seededOrchestrationKeys).length > 0 || - (ledger.bridgeEntries?.length ?? 0) > 0; + (ledger.bridgeEntries?.length ?? 0) > 0 || + // An owned empty bridge is still GJC-owned state: a convergence run that + // pruned the final entry leaves `bridgeEntries: []` with + // `bridgeDirCreated: true`, and the directory plus its registration must + // still be removable. + ledger.bridgeDirCreated === true; if (!ownsAnything) return { outcome: "nothing-to-remove" }; const removed: string[] = []; const remaining: string[] = []; let nextLedger = ledger; + // The recorded bridge path is validated BEFORE any settings mutation: a + // malformed or tampered ledger must never steer `skills.customDirectories` + // unregistering (or the symlink cleanup below) at a foreign path. + let validatedBridge: string | undefined; + if (ledger.bridgePath !== undefined || ledger.bridgeDirCreated === true) { + try { + validatedBridge = await validatedBridgeDir(ledger, deps); + } catch (error) { + const detail = error instanceof SkillsBridgeError ? error.message : String(error); + return partial([], [ledger.bridgePath ?? deps.paths.bridgeDir], { + failedStep: "provenance ledger validation", + detail, + retained: [deps.paths.provenanceLedger], + }); + } + } + // The recorded bridge SOURCE is validated in the same pre-settings window + // (#4644 review r10, architect gen-5): it drives link-text verification at + // unlink time, so a tampered source must refuse BEFORE the + // `skills.customDirectories` registration is unregistered — mirroring the + // bridge-path rule directly above. + if (ledger.bridgeEntries?.length || ledger.bridgeDirCreated === true) { + const sourceDir = ledger.bridgeSourceDir ?? legacySourceDirFor(deps); + const sourceTrust = await (deps.trustedSkillsSource ?? isTrustedRecordedSkillsSource)(sourceDir); + if (!sourceTrust.ok) { + return partial([], [ledger.bridgePath ?? deps.paths.bridgeDir], { + failedStep: "provenance ledger validation", + detail: sourceTrust.detail, + retained: [deps.paths.provenanceLedger], + }); + } + } + // Step 4 inverse: config.yml registration. if (options.unregisterBridgeDirectory) { try { @@ -66,26 +332,62 @@ export async function removePaseoSetup( } } - // Step 3 inverse: the symlink bridge. - if (ledger.bridgeEntries && ledger.bridgeEntries.length > 0) { + // Step 3 inverse: the symlink bridge. Runs when entries are recorded OR + // when GJC created the directory itself -- a convergence run that pruned + // the final entry leaves `bridgeEntries: []` with `bridgeDirCreated: true`, + // and the empty directory GJC created is still ours to remove. + if ((ledger.bridgeEntries?.length ?? 0) > 0 || ledger.bridgeDirCreated === true) { try { - // Only names the locked allowlist knows are undone; a ledger carrying an - // unknown name is ignored rather than trusted into a filesystem removal. - const createdEntries = ledger.bridgeEntries.filter((name): name is InstallSkillName => - (INSTALL_SKILL_NAMES as readonly string[]).includes(name), + // The ledger once filtered entries through a compiled-in name + // allowlist; ownership is now proven by the entry itself being a + // symlink that still resolves into the source directory the ledger + // recorded when the link was created. A name Paseo no longer ships + // is still removed, because the record -- not today's source + // contents -- is what proves GJC created it. + // Validated above, before any settings mutation, so the cleanup paths + // and the registration unregistering provably describe the same + // ledger-owned directory. + const bridgeDir = validatedBridge ?? deps.paths.bridgeDir; + // Every present recorded pathname is preserved for inverse + // validation: an entry replaced by a regular file or directory is + // handed to the inverse, which reports it as a divergence instead of + // being silently skipped and reported as success. + const presentEntries: string[] = []; + for (const name of safeBridgeEntryNames(ledger.bridgeEntries ?? [])) { + const destination = path.join(bridgeDir, name); + const stat = await lstatAllowingAbsent(destination); + if (stat !== undefined) presentEntries.push(name); + } + // A recorded source directory is trusted even after it disappears + // (Paseo uninstalled): link-text verification does not need it on + // disk, and the links are inside GJC's own bridge directory. A + // legacy ledger that predates `bridgeSourceDir` falls back to the + // single location a pre-#4638 install could have linked from, so a + // machine wedged by #4638 can still be rolled back. + const sourceDir = ledger.bridgeSourceDir ?? legacySourceDirFor(deps); + // The recorded source was validated in the pre-settings window + // above (#4644 review r10), exactly like the bridge path. + await inverseSkillsBridge( + deps, + { + createdEntries: presentEntries, + prunedEntries: [], + adoptedEntries: [], + bridgeDirCreated: ledger.bridgeDirCreated ?? false, + sourceDir, + }, + // Unlink, directory cleanup, and diagnostics all operate on the + // ledger-recorded directory the entries above were validated in. + { bridgeDir }, ); - await inverseSkillsBridge(deps, { - createdEntries, - bridgeDirCreated: ledger.bridgeDirCreated ?? false, - }); - removed.push(deps.paths.bridgeDir); - nextLedger = { ...nextLedger, bridgeEntries: [], bridgeDirCreated: false }; + removed.push(bridgeDir); + nextLedger = { ...nextLedger, bridgeEntries: [], bridgeDirCreated: false, bridgeSourceDir: undefined }; } catch (error) { const detail = error instanceof SkillsBridgeError ? error.message : String(error); - remaining.push(deps.paths.bridgeDir); + remaining.push(ledger.bridgePath ?? deps.paths.bridgeDir); await writeProvenance(deps.paths.provenanceLedger, nextLedger); return partial(removed, remaining, { - failedStep: deps.paths.bridgeDir, + failedStep: ledger.bridgePath ?? deps.paths.bridgeDir, detail, retained: [deps.paths.provenanceLedger], }); @@ -117,9 +419,59 @@ export async function removePaseoSetup( } // Step 1 inverse: provider entries, including every earlier `--mpreset` run. + // + // A `--force` overwrite recorded the entry it replaced; removal restores + // that entry instead of deleting the key, because the replaced content was + // never GJC's to take. Keys marked pre-existing (a matching entry that + // existed before any GJC run) are not in providerKeys and are untouched. const providerKeys = provenancedProviderKeys(nextLedger); if (providerKeys.length > 0) { const survivors: Record = {}; + // A `--force` overwrite preserved the replaced value in a private sidecar + // beside Paseo's own config; the ledger carries only the pointer, so the + // value is loaded here, before the mutation. Restore values are read ONLY + // for keys this removal will actually touch: an unreadable sidecar for a + // key that survives (the user edited ours) would otherwise block removal + // without any restore being needed. A missing or corrupt sidecar for a + // key that IS restored fails the removal closed instead of deleting the + // user content it was meant to bring back. + const restores = new Map(); + const configNow = await readTarget(deps.paths.configJson).catch(() => undefined); + if (configNow?.parsed !== undefined) { + const providersNow = providersOf(configNow.parsed); + for (const key of providerKeys) { + const entry = providersNow?.[key]; + if (entry === undefined) continue; + if (!isProvenancedProvider(nextLedger, key, providerEntryHash(entry as PaseoProviderEntry))) continue; + const ref = nextLedger.providerReplacedEntries?.[key]; + if (ref === undefined) continue; + // The recorded pointer must name the deterministic sidecar GJC + // itself derives from THIS config path and key. A tampered ledger + // pointing elsewhere would otherwise read an arbitrary JSON file + // into agents.providers and delete it after the "restore" — the + // exact asymmetry the bridge path and entry names already close. + if (ref.backupPath !== replacedProviderBackupPath(deps.paths.configJson, key)) { + remaining.push(deps.paths.configJson); + await writeProvenance(deps.paths.provenanceLedger, nextLedger); + return partial(removed, remaining, { + failedStep: deps.paths.configJson, + detail: `the replaced-provider backup for ${key} is recorded at an unexpected path (${ref.backupPath}); refusing to restore or delete it`, + retained: [deps.paths.provenanceLedger], + }); + } + const backup = await readReplacedProviderBackup(ref.backupPath, key, ref.valueSha256); + if (!backup.found) { + remaining.push(deps.paths.configJson); + await writeProvenance(deps.paths.provenanceLedger, nextLedger); + return partial(removed, remaining, { + failedStep: deps.paths.configJson, + detail: `the replaced provider entry for ${key} cannot be restored: its backup is missing or unreadable (${ref.backupPath}); restore it before removing`, + retained: [deps.paths.provenanceLedger], + }); + } + restores.set(key, backup.value); + } + } const outcome = await revertJson(deps.paths.configJson, options.now, draft => { const providers = providersOf(draft); if (!providers) return; @@ -127,8 +479,13 @@ export async function removePaseoSetup( const entry = providers[key]; if (entry === undefined) continue; const hash = providerEntryHash(entry as PaseoProviderEntry); - if (isProvenancedProvider(nextLedger, key, hash)) delete providers[key]; - else survivors[key] = nextLedger.providerKeys[key] ?? hash; + if (!isProvenancedProvider(nextLedger, key, hash)) { + survivors[key] = nextLedger.providerKeys[key] ?? hash; + continue; + } + const replaced = restores.get(key); + if (replaced !== undefined) providers[key] = replaced; + else delete providers[key]; } }); if (!outcome.ok) { @@ -140,8 +497,66 @@ export async function removePaseoSetup( retained: [deps.paths.provenanceLedger], }); } + // The restored values are back in Paseo's config; the sidecars that held + // them have served their purpose and must not outlive the ownership they + // recorded. Deletion failures and leftovers fail the removal closed + // (#4644 review r11): dropping the ledger reference while a + // credential-bearing sidecar still exists would orphan it with nothing + // pointing at it, and swallowing the unlink error would hide exactly + // that. Keys whose GJC entry is already absent from the config still + // have their sidecar removed here -- the entry GJC replaced is gone + // along with ours, so the preserved value is unreachable either way and + // keeping the sidecar would strand a credential file nothing references. + const orphaned: string[] = []; + const relevantRefs: { readonly key: string; readonly ref: ProviderReplacedRef }[] = []; + for (const [key, ref] of Object.entries(nextLedger.providerReplacedEntries ?? {})) { + if (survivors[key] !== undefined) continue; + if ( + restores.has(key) || + providersOf((await readTarget(deps.paths.configJson).catch(() => undefined))?.parsed ?? {})?.[key] === + undefined + ) { + // EVERY reference is validated against the deterministic sidecar + // path GJC itself derives (#4644 review r16) — the same rule the + // restore loop applies — before any deletion: a tampered ledger + // must not redirect `fs.rm` at an arbitrary absolute file. + if (ref.backupPath !== replacedProviderBackupPath(deps.paths.configJson, key)) { + remaining.push(deps.paths.configJson); + await writeProvenance(deps.paths.provenanceLedger, nextLedger); + return partial(removed, remaining, { + failedStep: deps.paths.configJson, + detail: `the replaced-provider backup for ${key} is recorded at an unexpected path (${ref.backupPath}); refusing to delete it`, + retained: [deps.paths.provenanceLedger], + }); + } + relevantRefs.push({ key, ref }); + } + } + for (const { + key, + ref: { backupPath, valueSha256 }, + } of relevantRefs) { + // Authenticate and delete under one inode-bound protocol. A pathname + // replacement after authentication is retained, never removed. + if (!(await removeReplacedProviderBackup(backupPath, key, valueSha256))) { + orphaned.push(backupPath); + } + } + if (orphaned.length > 0) { + remaining.push(deps.paths.configJson); + await writeProvenance(deps.paths.provenanceLedger, nextLedger); + return partial(removed, remaining, { + failedStep: deps.paths.configJson, + detail: `a replaced-provider backup could not be removed (${orphaned.join(", ")}); remove it before re-running so no credential-bearing sidecar is left unreferenced`, + retained: [deps.paths.provenanceLedger], + }); + } removed.push(deps.paths.configJson); - nextLedger = { ...nextLedger, providerKeys: survivors }; + const keptRefs: Record = {}; + for (const [key, ref] of Object.entries(nextLedger.providerReplacedEntries ?? {})) { + if (survivors[key] !== undefined) keptRefs[key] = ref; + } + nextLedger = { ...nextLedger, providerKeys: survivors, providerReplacedEntries: keptRefs }; } const stillOwns = diff --git a/packages/coding-agent/src/setup/paseo/result-types.ts b/packages/coding-agent/src/setup/paseo/result-types.ts index c3f2f2bae0..ce80acfb64 100644 --- a/packages/coding-agent/src/setup/paseo/result-types.ts +++ b/packages/coding-agent/src/setup/paseo/result-types.ts @@ -20,10 +20,10 @@ export type DriftReasonCode = | "partial-install" | "partial-removal" | "orphan-skill" - | "unlinked-skill" | "missing-provider-entry" | "missing-bridge-link" | "missing-skills-directory" + | "foreign-skill-link" | "unseeded-orchestration-role"; export interface DriftReason { diff --git a/packages/coding-agent/src/setup/paseo/setup-deps.ts b/packages/coding-agent/src/setup/paseo/setup-deps.ts index bec615093e..dd3d373369 100644 --- a/packages/coding-agent/src/setup/paseo/setup-deps.ts +++ b/packages/coding-agent/src/setup/paseo/setup-deps.ts @@ -3,26 +3,174 @@ * * Every module in this directory takes `PaseoSetupDependencies` explicitly so * tests can substitute paths, the Paseo CLI probe, and the clock without - * `mock.module()`. Production wiring calls `createDefaultPaseoSetupDependencies()`. + * module-scope mocks. */ -import * as os from "node:os"; +import * as fs from "node:fs/promises"; import * as path from "node:path"; -import { getAgentDir } from "@gajae-code/utils"; - -/** The five Paseo skills this setup links. `context-search` is deliberately excluded. */ -export const INSTALL_SKILL_NAMES = [ - "paseo", - "paseo-advisor", - "paseo-committee", - "paseo-handoff", - "paseo-loop", -] as const; - -export type InstallSkillName = (typeof INSTALL_SKILL_NAMES)[number]; +import { getAgentDir, getTrustedHomeDir, parseEnvFile } from "@gajae-code/utils"; /** Prefix used to enumerate Paseo-owned skills when scanning for drift. */ export const PASEO_SKILL_PREFIX = "paseo"; +/** + * Where Paseo keeps its skills. + * + * A CLI install materializes `~/.agents/skills`; a desktop app ships them inside + * the app bundle. The bridge reads whichever exists and never writes either. + */ +export interface PaseoSkillSource { + /** Absolute directory holding Paseo's skill folders. */ + readonly dir: string; + /** `"user"` (`~/.agents/skills`) or `"app-bundle"` (inside a Paseo.app). */ + readonly origin: "user" | "app-bundle"; +} + +/** Default Paseo desktop app names, both install roots, in order. */ +const PASEO_APP_NAMES = ["Paseo.app", "Paseo Beta.app", "Paseo Nightly.app"] as const; + +/** Every app-bundle skills directory to probe. Bounded: a fixed list, never a search. */ +export function paseoAppSkillsCandidates(home: string = getTrustedHomeDir()): readonly string[] { + const roots = process.platform === "darwin" ? ["/Applications", path.join(home, "Applications")] : []; + const candidates: string[] = []; + for (const root of roots) { + for (const app of PASEO_APP_NAMES) { + candidates.push(path.join(root, app, "Contents", "Resources", "skills")); + } + } + return candidates; +} + +/** + * Every project dotenv variant Bun can load for the current working directory. + * + * Bun's default order loads `.env`, then the `NODE_ENV`-specific file, then + * `.env.local`, all from `process.cwd()` before any module runs. A repository + * can define `PASEO_SKILLS_DIR` in any of them, so the trust check must + * consider the full set, not just `.env`. + */ +function projectDotenvVariants(): readonly string[] { + const env = process.env.NODE_ENV; + const files = [".env", ".env.local"]; + if (env === "production" || env === "test") { + files.push(`.env.${env}`, `.env.${env}.local`); + } else { + // Bun's mode defaults to development: with `NODE_ENV` unset, empty, or + // any value other than production/test, `.env.development` and + // `.env.development.local` still load (verified against the pinned + // runtime), so the trust set must carry them for every such mode — + // not only for the literal `NODE_ENV=development`. + files.push(".env.development", ".env.development.local"); + } + // `.env.production.local` is also in play for the default mode; checking it + // in every mode keeps this a conservative superset of what Bun can load + // rather than an exact precedence mirror. + files.push(".env.production.local"); + return [...new Set(files)]; +} + +/** + * `PASEO_SKILLS_DIR` as explicit user intent only. + * + * Bun loads the project dotenv variants above into `process.env` before any + * module runs, so a cloned repository can point this override at a directory + * the repository also ships -- and global `gjc setup paseo` run from inside + * that checkout would bridge the repository's own `paseo*` prompt content into + * the user's GJC configuration. The trust rule is presence-based across every + * variant: the project defining the key in ANY dotenv file rejects the + * override, because Bun expands interpolations (`$PWD`, `${VAR}`) before a + * value comparison could match. An operator whose real environment carries the + * same value loses the override while inside such a checkout -- the same + * conservative trade the credential boundary already makes. + */ +function trustedPaseoSkillsDirOverride(): string | undefined { + const value = process.env.PASEO_SKILLS_DIR; + if (!value) return undefined; + for (const file of projectDotenvVariants()) { + if (parseEnvFile(path.join(process.cwd(), file)).PASEO_SKILLS_DIR !== undefined) return undefined; + } + return value; +} + +/** + * Resolve the directory Paseo's skills live in. + * + * `PASEO_SKILLS_DIR` overrides discovery for relocated bundles and tests; it is + * honored only when it is explicit user intent (not the project `.env`), absolute, + * and present, so a stale or untrusted variable can never produce bridge links. + * `~/.agents/skills` wins over an app bundle because it is the user-visible + * location GJC documented. Returns `undefined` when no source directory exists + * at all -- the bridge is skipped, never guessed at. + */ +export async function resolvePaseoSkillsSource( + home: string = getTrustedHomeDir(), +): Promise { + const override = trustedPaseoSkillsDirOverride(); + if (override !== undefined && path.isAbsolute(override) && (await isDirectory(override))) { + return { dir: path.resolve(override), origin: "app-bundle" }; + } + const userDir = path.join(home, ".agents", "skills"); + if (await isDirectory(userDir)) return { dir: userDir, origin: "user" }; + for (const candidate of paseoAppSkillsCandidates(home)) { + if (await isDirectory(candidate)) return { dir: candidate, origin: "app-bundle" }; + } + return undefined; +} +/** + * Trust check for a RECORDED bridge source directory (#4644 reviews r10–r12). + * + * The provenance ledger's `bridgeSourceDir` drives link-text verification at + * removal time, so a tampered ledger must not be able to name an arbitrary + * directory as "the source". The trust rule mirrors DISCOVERY EXACTLY — the + * recorded source must be a location `resolvePaseoSkillsSource` could have + * returned: the default `~/.agents/skills`, one of the fixed Paseo.app bundle + * `Contents/Resources/skills` roots under `/Applications` or `~/Applications`, + * or the trusted `PASEO_SKILLS_DIR` override (compared canonically so a + * relocated bundle through a symlink still matches). Anything else under the + * home directory — an arbitrary user directory a tampered ledger names — is + * refused. A vanished source still passes when its location is one of these + * exact roots: Paseo being uninstalled must not wedge removal. + */ +export async function isTrustedRecordedSkillsSource( + dir: string, +): Promise<{ ok: true } | { ok: false; detail: string }> { + if (!path.isAbsolute(dir)) { + return { ok: false, detail: `the recorded bridge source (${dir}) is not an absolute path` }; + } + const resolved = (await fs.realpath(dir).catch(() => undefined)) ?? path.resolve(dir); + const home = (await fs.realpath(getTrustedHomeDir()).catch(() => getTrustedHomeDir())) as string; + const candidates = [ + path.join(home, ".agents", "skills"), + ...paseoAppSkillsCandidates(home).map(candidate => + candidate.startsWith("/Applications") || candidate.startsWith(home) ? candidate : path.join(home, candidate), + ), + ]; + for (const candidate of candidates) { + const canonical = (await fs.realpath(candidate).catch(() => undefined)) ?? candidate; + if (resolved === canonical || resolved === candidate) return { ok: true }; + } + const override = trustedPaseoSkillsDirOverride(); + if (override !== undefined && path.isAbsolute(override)) { + // The explicit override's RESOLVED target is trusted (#4644 review + // r11): the override is the user's own declared intent, so a relocated + // bundle reached through a symlink must not strand bridge ownership. + const resolvedOverride = (await fs.realpath(override).catch(() => undefined)) ?? path.resolve(override); + if (resolved === resolvedOverride) return { ok: true }; + } + return { + ok: false, + detail: `the recorded bridge source (${dir}) is not a location Paseo skills discovery could produce; set PASEO_SKILLS_DIR to it if it is legitimate, then re-run`, + }; +} + +async function isDirectory(candidate: string): Promise { + try { + return (await fs.stat(candidate)).isDirectory(); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return false; + throw error; + } +} + /** Base provider key written into `agents.providers`. */ export const PROVIDER_KEY = "gjc"; @@ -42,7 +190,17 @@ export interface PaseoPaths { readonly configJson: string; /** `~/.paseo/orchestration-preferences.json` */ readonly orchestrationPreferences: string; - /** `~/.agents/skills` -- READ-ONLY, never written by this setup. */ + /** + * Paseo's skills directory -- READ-ONLY, never written by this setup. + * Retained for exported-contract compatibility: callers constructing + * `PaseoPaths` keep a field to read. The LIVE source the bridge links + * against is resolved per run via `resolvePaseoSkillsSource` (the default + * location, an app bundle, or an explicit `PASEO_SKILLS_DIR`), and that + * resolved path — not this field — is what the ledger records. REQUIRED + * again for external callers (#4644 review r14): they constructed the + * field before it ever became optional, so the required shape is the + * compatible one; hermetic fixtures pass their own concrete directory. + */ readonly agentsSkillsDir: string; /** `/paseo-skills` -- the bridge directory this setup owns. */ readonly bridgeDir: string; @@ -74,18 +232,40 @@ export interface PaseoSetupDependencies { /** Bounded probe of the Paseo daemon. MUST enforce `timeoutMs` and kill the child on expiry. */ runProviderLs(timeoutMs: number): Promise; now(): Date; + /** + * Resolve Paseo's skills directory. Injectable so tests stay hermetic. + * Migration-safe default (#4644 review r13): an omitted resolver falls + * back to the REAL discovery order (`PASEO_SKILLS_DIR`, `~/.agents/skills`, + * a Paseo.app bundle) rather than failing to compile — existing callers + * constructing this exported interface keep type-checking — and the + * fallback is never a silent bridge skip: it resolves exactly what the + * production builder would. + */ + readonly skillsSource?: () => Promise; + /** Home directory used to derive the legacy pre-#4638 bridge source. */ + readonly home?: string; + /** + * Trust check for a RECORDED bridge source directory at removal time. + * Optional only because hermetic fixtures inject a root-bounded + * equivalent: absence falls back to {@link isTrustedRecordedSkillsSource}, + * the production trusted-home/app-bundle rule — never a silent pass-through. + */ + readonly trustedSkillsSource?: (dir: string) => Promise<{ ok: true } | { ok: false; detail: string }>; } -export function createDefaultPaseoPaths(agentDir: string = getAgentDir(), home: string = os.homedir()): PaseoPaths { +export function createDefaultPaseoPaths( + agentDir: string = getAgentDir(), + home: string = getTrustedHomeDir(), +): PaseoPaths { const paseoHome = path.join(home, ".paseo"); return { configJson: path.join(paseoHome, "config.json"), orchestrationPreferences: path.join(paseoHome, "orchestration-preferences.json"), - agentsSkillsDir: path.join(home, ".agents", "skills"), bridgeDir: path.join(agentDir, "paseo-skills"), provenanceLedger: path.join(agentDir, "paseo", "provenance.json"), intentRecord: path.join(agentDir, "paseo", "intent.json"), gjcSkillsDir: path.join(agentDir, "skills"), + agentsSkillsDir: path.join(home, ".agents", "skills"), }; } @@ -171,5 +351,7 @@ export function createDefaultPaseoSetupDependencies(): PaseoSetupDependencies { paths: createDefaultPaseoPaths(), runProviderLs, now: () => new Date(), + skillsSource: () => resolvePaseoSkillsSource(), + home: getTrustedHomeDir(), }; } diff --git a/packages/coding-agent/src/setup/paseo/skills-bridge.ts b/packages/coding-agent/src/setup/paseo/skills-bridge.ts index d4be4e0c14..5aa21eca5a 100644 --- a/packages/coding-agent/src/setup/paseo/skills-bridge.ts +++ b/packages/coding-agent/src/setup/paseo/skills-bridge.ts @@ -1,38 +1,132 @@ +/** + * Paseo skills bridge. + * + * The bridge mirrors Paseo's own skills into GJC skill discovery with symlinks. + * Bridged names are derived from what the resolved source directory actually + * contains (#4638): a Paseo release that adds or drops an orchestration skill can + * no longer wedge `--check` into a permanently red verdict, an install whose + * skills live inside the Paseo.app bundle is bridged the same way as one whose + * skills live in `~/.agents/skills`, and a missing source directory skips the + * bridge instead of publishing dangling links. Install converges the bridge to + * the current source: it creates missing links, prunes links whose target is + * gone, and never replaces a non-symlink entry. + */ + +import * as nodeCrypto from "node:crypto"; +import type { Dirent, Stats } from "node:fs"; import * as fs from "node:fs/promises"; import * as path from "node:path"; +import { getTrustedHomeDir } from "@gajae-code/utils"; import type { CasReceipt } from "../../config/atomic-yaml-patch"; import type { RawSettings, Settings } from "../../config/settings"; import type { SettingPath } from "../../config/settings-schema"; +import { readProvenance } from "./paseo-ownership"; import type { DriftReason } from "./result-types"; import { - INSTALL_SKILL_NAMES, - type InstallSkillName, PASEO_SKILL_PREFIX, type PaseoSetupDependencies, + type PaseoSkillSource, + resolvePaseoSkillsSource, } from "./setup-deps"; +/** + * Resolve the skills source through the injectable seam, so tests never touch a + * real `~/.agents/skills` or app bundle. + */ +function resolveSource(deps: PaseoSetupDependencies): Promise { + // Migration-safe default (#4644 reviews r13/r17): an omitted resolver uses + // the real discovery order — and honors a caller-supplied + // `paths.agentsSkillsDir` exactly as the pre-#4638 seam did when discovery + // finds nothing — never a silent bridge skip. + if (deps.skillsSource !== undefined) return deps.skillsSource(); + return resolvePaseoSkillsSource().then(async discovered => { + if (discovered !== undefined) return discovered; + if (deps.paths.agentsSkillsDir === undefined) return undefined; + const stat = await fs.stat(deps.paths.agentsSkillsDir).catch(() => undefined); + return stat?.isDirectory() ? { dir: deps.paths.agentsSkillsDir as string, origin: "user" } : undefined; + }); +} + +/** The source directory a pre-#4638 ledger's bridge entries were linked from. */ +export function legacyRecordedSourceDir(home: string): string { + return path.join(home, ".agents", "skills"); +} + +/** + * The legacy source for a dependency set that may omit `home`: the caller's + * explicit `paths.agentsSkillsDir` when present, else the trusted-home + * default (#4644 review r18). A relative `.agents/skills` (the old + * `home ?? ""` shape) can never pass removal's absolute-path trust check, so + * legacy callers without `home` were wedged. + */ +export function legacySourceDirFor(deps: PaseoSetupDependencies): string { + if (deps.home !== undefined && deps.home.length > 0) return legacyRecordedSourceDir(deps.home); + if (deps.paths.agentsSkillsDir !== undefined) return deps.paths.agentsSkillsDir; + return legacyRecordedSourceDir(getTrustedHomeDir()); +} + type BridgeEntryAction = "create" | "noop" | "prune-and-recreate"; +/** Preflight identity for a symlink GJC is authorized to remove. */ +type SymlinkIdentity = { + readonly dev: number; + readonly ino: number; + readonly size: number; + readonly mtimeMs: number; +}; + type BridgeEntryPlan = { - readonly name: InstallSkillName; + readonly name: string; readonly action: BridgeEntryAction; readonly linkPath: string; readonly targetPath: string; /** The dangling link text captured during preflight, used as an unlink guard. */ readonly danglingTarget?: string; + /** The same dangling link's identity; link text alone is not ownership. */ + readonly danglingIdentity?: SymlinkIdentity; +}; + +/** A bridge link that no longer mirrors the source and is removed, never recreated. */ +type BridgePrunePlan = { + readonly name: string; + readonly linkPath: string; + /** Captured link text; the unlink is refused if it changed in between. */ + readonly linkTarget: string; + readonly linkIdentity: SymlinkIdentity; +}; +/** A recorded pre-#4638 link to re-point at the source the ledger records going forward. */ +type BridgeAdoptPlan = { + readonly name: string; + readonly linkPath: string; + readonly targetPath: string; + /** The legacy source the link currently points at, captured during preflight. */ + readonly legacySourceDir: string; + readonly linkTarget: string; + readonly linkIdentity: SymlinkIdentity; }; /** Immutable preflight evidence consumed by the install saga and its inverse. */ export interface SkillsBridgePreflight { readonly bridgeDir: string; readonly bridgeDirCreated: boolean; - readonly entries: Readonly>; + /** Source directory the entries were derived from, absent when the bridge is skipped. */ + readonly sourceDir?: string; + readonly entries: Readonly>; + /** Stale bridge links to remove so a re-run converges instead of accumulating drift. */ + readonly prunes: readonly BridgePrunePlan[]; + /** Recorded legacy links to re-point at the discovered source (pre-#4638 migration). */ + readonly adopts: readonly BridgeAdoptPlan[]; } -/** What the forward operation actually created, rather than what preflight intended to create. */ +/** What the forward operation actually did, rather than what preflight intended to do. */ export interface SkillsBridgeInstallResult { - readonly createdEntries: readonly InstallSkillName[]; + readonly createdEntries: readonly string[]; + readonly prunedEntries: readonly string[]; + /** Recorded legacy links this run re-pointed at the discovered source. */ + readonly adoptedEntries: readonly string[]; readonly bridgeDirCreated: boolean; + /** Directory the created links point at; absent when nothing was created. */ + readonly sourceDir?: string; } export class SkillsBridgeError extends Error { @@ -41,12 +135,207 @@ export class SkillsBridgeError extends Error { this.name = "SkillsBridgeError"; } } +/** + * A failed {@link installSkillsBridge} carrying what the operation actually + * completed before failing, so the caller can correct the provenance ledger to + * observed reality instead of leaving the pre-write's plan standing as fact + * (#4644 review r9). + */ +export class SkillsBridgePartialError extends SkillsBridgeError { + readonly partial: SkillsBridgeInstallResult; + constructor(message: string, partial: SkillsBridgeInstallResult) { + super(message); + this.name = "SkillsBridgePartialError"; + this.partial = partial; + } +} +/** + * Atomically quarantine a bridge symlink and unlink it only after its identity + * is verified POST-rename. + * + * A plain `lstat` → `unlink` sequence has a destructive window: another + * process can replace the checked symlink between the two calls, and the + * unlink then deletes the foreign replacement. Renaming the entry into a + * GJC-owned quarantine name first closes that window -- the rename is atomic, + * nothing is deleted until the quarantined object's link text is re-verified, + * and a mis-captured foreign object is restored by renaming it back before the + * error surfaces. (`exactUnlink` cannot express this for symlinks: the native + * deliberately refuses `S_IFLNK` targets.) + */ +async function quarantineUnlinkVerified( + linkPath: string, + expectedTarget: string, + expectedIdentity: SymlinkIdentity, +): Promise { + const quarantine = path.join( + path.dirname(linkPath), + `.gjc-paseo-quarantine-${process.pid}-${nodeCrypto.randomUUID()}`, + ); + await fs.rename(linkPath, quarantine); + // Identity is bound by INODE, not by name (#4644 reviews r11/r13): the + // quarantined object's (dev, ino) is captured once and every later step — + // the verification AND the deletion — must still see that exact inode at + // the quarantine name. The link text is verified once for content, and + // the unlink itself is guarded by an lstat that requires the same inode + // immediately before it. A concurrent actor replacing the quarantine + // pathname therefore changes the inode and the deletion refuses — the + // foreign object is never unlinked. (A rename onto the name in the final + // microseconds between the lstat and the unlink remains theoretically + // possible without a descriptor-bound unlink primitive, but it requires + // winning a race against an unpredictable single-component name inside + // GJC's own bridge directory; the residue sweep below detects and + // restores any survivor.) + // Restoration is NO-CLOBBER (#4644 reviews r15/r16): a concurrent entry + // created at the original linkPath after our rename must never be silently + // destroyed. The no-replace natives refuse symlink sources + // (reparse_point), so this is the narrowest JS shape: rename only when the + // destination is observed vacant, then verify the destination carries the + // inode that JUST left the quarantine name — a foreign entry that won the + // vacancy-to-rename window was displaced by the rename, and is moved aside + // to a recoverable name (never deleted) instead of vanishing. + const restoreNoClobber = async (): Promise => { + const quarantinedNow = await fs.lstat(quarantine).catch(() => undefined); + if (quarantinedNow === undefined) return; + // ATOMIC no-clobber restore (#4644 reviews r15–r17): the natives' + // no-replace primitives refuse symlink sources (reparse_point), but a + // symlink can be recreated atomically with fs.symlink, which fails + // EEXIST when the destination is occupied — an occupant that wins the + // race is never replaced. On success the quarantined original is + // removed (its recreation is byte-identical: same text, verified + // against `captured`/`text` above). + if (quarantinedNow.isSymbolicLink()) { + const currentText = await fs.readlink(quarantine).catch(() => undefined); + if (currentText !== undefined) { + try { + await fs.symlink(currentText, linkPath); + await fs.rm(quarantine, { force: true }).catch(() => undefined); + return; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EEXIST") return; + // Fall through to the rename path for unsupported cases. + } + } + } + // Non-symlink quarantined object (the divergence branches): the plain + // rename stays, gated on the observed-vacant destination as before. + const occupied = await fs.lstat(linkPath).then( + () => true, + () => false, + ); + if (occupied) return; + await fs.rename(quarantine, linkPath).catch(() => undefined); + }; + const captured = await fs.lstat(quarantine); + if ( + !captured.isSymbolicLink() || + captured.dev !== expectedIdentity.dev || + captured.ino !== expectedIdentity.ino || + captured.size !== expectedIdentity.size || + captured.mtimeMs !== expectedIdentity.mtimeMs + ) { + await restoreNoClobber(); + throw new SkillsBridgeError(`Paseo skill bridge entry diverged before removal: ${linkPath}`); + } + const text = await fs.readlink(quarantine).catch(() => undefined); + if (text === undefined || resolvedLinkTarget(text, quarantine) !== expectedTarget) { + await restoreNoClobber(); + throw new SkillsBridgeError(`Paseo skill bridge entry diverged before removal: ${linkPath}`); + } + const sameInode = async (): Promise => { + const stat = await fs.lstat(quarantine).catch(() => undefined); + return ( + stat !== undefined && + stat.ino === captured.ino && + stat.dev === captured.dev && + stat.isSymbolicLink() && + // Size/mtime of a symlink track its text; a replaced link differs. + stat.size === captured.size && + stat.mtimeMs === captured.mtimeMs + ); + }; + // Deletion refuses unless the exact captured inode is still at the name. + let unlinked = false; + for (let attempt = 0; attempt < 3 && !unlinked; attempt++) { + if (!(await sameInode())) { + // The quarantine name no longer holds our verified object: restore + // whatever is there and refuse. A foreign object is never deleted. + await restoreNoClobber(); + throw new SkillsBridgeError(`Paseo skill bridge entry diverged before removal: ${linkPath}`); + } + try { + await fs.unlink(quarantine); + unlinked = true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + // Something removed it between the check and the unlink. If the + // name is gone entirely the object is gone; nothing foreign was + // deleted by us. Treat as done. + unlinked = true; + break; + } + if (attempt === 2) throw error; + } + } + // Post-unlink sweep: a foreign object swapped onto the quarantine name in + // the final window survives at that name (the unlink consumed ours, or the + // inode guard refused). It is never deleted — only restored to the bridge + // name so nothing foreign is destroyed or stranded. + const residue = await fs.lstat(quarantine).catch(() => undefined); + if (residue !== undefined && !(await sameInode())) { + await restoreNoClobber(); + } +} -function expectedTarget(deps: PaseoSetupDependencies, name: InstallSkillName): string { - return path.resolve(deps.paths.agentsSkillsDir, name); +/** + * Entry names the bridge mirrors, derived from the source directory's own + * contents: every `paseo`-prefixed directory. Plain files yield nothing, and a + * directory symlink/junction is resolved and validated before it is bridged -- + * GJC's own skill discovery follows such links, so a symlinked skills directory + * is a legitimate source shape. There is no name denylist: the prefix + * (`paseo`, not `paseo-`) is the filter, so a denylisted name like + * `context-search` fails the prefix test on its own. + */ +export async function sourceBridgeEntries(sourceDir: string): Promise { + let entries: readonly Dirent[]; + try { + entries = await fs.readdir(sourceDir, { withFileTypes: true }); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + // A missing source is handled before this function by the resolver. If it + // disappears or changes type after resolution, this is a race with an app + // update/uninstall, not a valid empty source. Propagate it so preflight + // preserves the existing bridge instead of pruning every recorded link. + if (code === "ENOENT" || code === "ENOTDIR") throw error; + throw error; + } + const names: string[] = []; + for (const entry of entries) { + if (!entry.name.startsWith(PASEO_SKILL_PREFIX)) continue; + if (entry.isDirectory()) { + names.push(entry.name); + continue; + } + // A directory symlink (or Windows junction) is a valid source shape: + // resolve it and keep it only when it really is a directory. A dangling + // symlink (ENOENT from stat) is absent, not foreign; any other stat + // failure (EACCES and friends) is propagated rather than collapsed into + // "not a directory", so an unreadable source cannot silently prune the + // bridge entries it backs. + if (entry.isSymbolicLink()) { + let resolved: Stats | undefined; + try { + resolved = await fs.stat(path.join(sourceDir, entry.name)); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + } + if (resolved?.isDirectory()) names.push(entry.name); + } + } + names.sort(); + return names; } -function linkPath(deps: PaseoSetupDependencies, name: InstallSkillName): string { +function linkPath(deps: PaseoSetupDependencies, name: string): string { return path.join(deps.paths.bridgeDir, name); } @@ -59,24 +348,47 @@ async function entryState( expected: string, ): Promise< | { readonly kind: "absent" } - | { readonly kind: "expected"; readonly link: string } - | { readonly kind: "dangling"; readonly link: string } + | { readonly kind: "expected"; readonly link: string; readonly identity: SymlinkIdentity } + | { readonly kind: "dangling"; readonly link: string; readonly identity: SymlinkIdentity } | { readonly kind: "conflict" } > { try { const stat = await fs.lstat(destination); if (!stat.isSymbolicLink()) return { kind: "conflict" }; + const identity = { dev: stat.dev, ino: stat.ino, size: stat.size, mtimeMs: stat.mtimeMs }; + const link = await fs.readlink(destination); + if (resolvedLinkTarget(link, destination) !== expected) return { kind: "conflict" }; + try { + await fs.stat(destination); + return { kind: "expected", link, identity }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return { kind: "dangling", link, identity }; + throw error; + } } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT") return { kind: "absent" }; throw error; } - const link = await fs.readlink(destination); - if (resolvedLinkTarget(link, destination) !== expected) return { kind: "conflict" }; +} + +/** A bridge symlink that points anywhere other than `expected`. */ +async function foreignSymlinkState( + destination: string, +): Promise< + | { readonly kind: "absent" } + | { readonly kind: "symlink"; readonly link: string; readonly identity: SymlinkIdentity } + | { readonly kind: "conflict" } +> { try { - await fs.stat(destination); - return { kind: "expected", link }; + const stat = await fs.lstat(destination); + if (!stat.isSymbolicLink()) return { kind: "conflict" }; + return { + kind: "symlink", + link: await fs.readlink(destination), + identity: { dev: stat.dev, ino: stat.ino, size: stat.size, mtimeMs: stat.mtimeMs }, + }; } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") return { kind: "dangling", link }; + if ((error as NodeJS.ErrnoException).code === "ENOENT") return { kind: "absent" }; throw error; } } @@ -92,20 +404,129 @@ async function bridgeDirectoryState(bridgeDir: string): Promise<"absent" | "dire } /** - * Classify every allowlisted bridge entry without mutating either skill tree. + * Classify every bridge entry without mutating either skill tree. * All conflicts are accumulated so the caller can report them together. + * + * Three groups are planned: + * - `entries`: one per source skill that should have a link (`create`, `noop`, + * or `prune-and-recreate` when only the link text rotted). + * - `prunes`: recorded `paseo`-prefixed bridge entries GJC owns whose name the + * source no longer carries. They are removed so a re-run converges; the + * compensation inverse does not restore them because their target is gone by + * definition. + * - `adopts`: pre-#4638 links pointing at the legacy `~/.agents/skills` source. + * They are re-pointed at the source the ledger will record going forward, so + * a machine wedged by the old allowlist converges instead of conflicting. + * + * Pruning is provenance-gated: an existing bridge directory is supported + * (`bridgeDirCreated: false`), so directory creation cannot establish ownership + * of every entry. A `paseo`-prefixed symlink GJC never recorded is foreign and + * is reported as a conflict rather than silently removed. A non-symlink + * occupying a `paseo`-prefixed bridge name is likewise a conflict and refuses + * the whole plan, exactly as before. */ export async function preflightSkillsBridge(deps: PaseoSetupDependencies): Promise { const directory = await bridgeDirectoryState(deps.paths.bridgeDir); - const conflicts: string[] = directory === "conflict" ? [deps.paths.bridgeDir] : []; - const entries = {} as Record; + if (directory === "conflict") { + throw new SkillsBridgeError( + `Refusing to modify Paseo skills bridge; conflicting entries: ${deps.paths.bridgeDir}`, + ); + } + const conflicts: string[] = []; + const entries: Record = {}; + const prunes: BridgePrunePlan[] = []; + const adopts: BridgeAdoptPlan[] = []; + const source = await resolveSource(deps); + const ledger = await readProvenance(deps.paths.provenanceLedger); + const recordedEntries = new Set(ledger.bridgeEntries ?? []); + // The recorded source decides ownership everywhere in this preflight: a name + // in the ledger is ours only while its link still points where the ledger + // says we put it. A ledger that predates `bridgeSourceDir` is legacy, and the + // single location a pre-#4638 install could have linked from stands in. + const ledgerSourceDir = ledger.bridgeSourceDir; + const legacySourceDir = legacySourceDirFor(deps); + const ownershipSourceDir = ledgerSourceDir ?? legacySourceDir; + if (source === undefined) { + // No source directory anywhere: the bridge is skipped entirely. Creating + // links into a directory that does not exist is worse than not bridging. + return { bridgeDir: deps.paths.bridgeDir, bridgeDirCreated: directory === "absent", entries, prunes, adopts }; + } + // Record the CANONICAL source (#4644 review r11): the resolver may follow + // a symlinked skills directory, and removal's trust check canonicalizes + // before comparing against trusted roots. Recording the resolved path (not + // the lexical spelling) keeps install and removal in agreement. A source + // that vanished between resolution and here keeps its lexical spelling: + // realpath would throw ENOENT, and the vanished-source handling below is + // the honest refusal. + const sourceDir = (await fs.realpath(source.dir).catch(() => undefined)) ?? source.dir; + // The resolver only returns directories it verified as present, so a source + // that is gone again by enumeration time was removed underneath this run + // (an app update or uninstall race). That is never "an empty source": an + // empty read here would classify every recorded entry as stale and prune a + // healthy bridge. Fail closed and change nothing. + try { + const stat = await fs.stat(sourceDir); + // An app update can replace the skills directory with a regular file (or + // anything non-directory). Treating that as an empty source would prune + // every recorded bridge link, so the type change itself is a refusal. + if (!stat.isDirectory()) { + throw new SkillsBridgeError( + `Refusing to converge Paseo skills bridge: the resolved skills directory (${sourceDir}) is not a directory anymore; the existing bridge is left untouched`, + ); + } + } catch (error) { + if (error instanceof SkillsBridgeError) throw error; + const code = (error as NodeJS.ErrnoException).code; + throw new SkillsBridgeError( + `Refusing to converge Paseo skills bridge: the resolved skills directory (${sourceDir}) became unreadable (${code ?? "unknown"}); the existing bridge is left untouched`, + ); + } + const names = await sourceBridgeEntries(sourceDir).catch(error => { + throw new SkillsBridgeError( + `Refusing to converge Paseo skills bridge: the resolved skills directory (${sourceDir}) could not be read (${error instanceof Error ? error.message : String(error)}); the existing bridge is left untouched`, + ); + }); + const wanted = new Set(names); - for (const name of INSTALL_SKILL_NAMES) { + for (const name of names) { const destination = linkPath(deps, name); - const target = expectedTarget(deps, name); + const target = path.resolve(sourceDir, name); const state = directory === "absent" ? { kind: "absent" as const } : await entryState(destination, target); if (state.kind === "conflict") { - conflicts.push(destination); + // A recorded link whose text still resolves into the ownership source + // (the ledger's own record, or the legacy location for a legacy + // ledger) is GJC's own link, not a user hand edit: re-point it at the + // discovered source. Re-pointing is provenance-gated for BOTH ledger + // shapes (#4644 review r18): a legacy ledger migrates from the + // pre-#4638 location, and a ledger that already records a source + // migrates from ITS OWN recorded source when discovery has since + // moved (a Paseo update relocating the app bundle). In every case + // the link must currently point into the source the LEDGER names — + // a user's hand edit to an unrelated target stays a conflict. + const recorded = recordedEntries.has(name); + const ownershipTarget = path.resolve(ownershipSourceDir, name); + const migratable = + recorded && + directory !== "absent" && + sourceDir !== ownershipSourceDir && + (await entryState(destination, ownershipTarget).then(s => s.kind !== "conflict")); + if (!migratable) { + conflicts.push(destination); + continue; + } + const legacyState = await entryState(destination, ownershipTarget); + if (legacyState.kind === "conflict" || legacyState.kind === "absent") { + conflicts.push(destination); + continue; + } + adopts.push({ + name, + linkPath: destination, + targetPath: target, + legacySourceDir: ownershipSourceDir, + linkTarget: legacyState.link, + linkIdentity: legacyState.identity, + }); continue; } entries[name] = { @@ -114,14 +535,55 @@ export async function preflightSkillsBridge(deps: PaseoSetupDependencies): Promi linkPath: destination, targetPath: target, ...(state.kind === "dangling" ? { danglingTarget: state.link } : {}), + ...(state.kind === "dangling" ? { danglingIdentity: state.identity } : {}), }; } + + if (directory !== "absent") { + const present = await fs.readdir(deps.paths.bridgeDir, { withFileTypes: true }); + for (const entry of present) { + if (!entry.name.startsWith(PASEO_SKILL_PREFIX)) continue; + if (wanted.has(entry.name)) continue; + if (adopts.some(adopt => adopt.name === entry.name)) continue; + const destination = path.join(deps.paths.bridgeDir, entry.name); + if (!recordedEntries.has(entry.name)) { + // Provenance, not the prefix, proves ownership: an existing bridge + // directory may hold entries GJC never created, and a live foreign + // symlink is never pruned just because its name starts with `paseo`. + conflicts.push(destination); + continue; + } + const state = await foreignSymlinkState(destination); + if (state.kind === "absent") continue; + if (state.kind === "conflict") { + conflicts.push(destination); + continue; + } + // The same exact-target predicate `--remove` applies: a recorded + // name is only ours while the link still points under the source the + // ledger recorded. A user who retargeted the link at their own tree + // keeps it -- it is reported as a conflict instead of pruned. + if (resolvedLinkTarget(state.link, destination) !== path.resolve(ownershipSourceDir, entry.name)) { + conflicts.push(destination); + continue; + } + prunes.push({ name: entry.name, linkPath: destination, linkTarget: state.link, linkIdentity: state.identity }); + } + } + if (conflicts.length > 0) { throw new SkillsBridgeError( `Refusing to modify Paseo skills bridge; conflicting entries: ${conflicts.join(", ")}`, ); } - return { bridgeDir: deps.paths.bridgeDir, bridgeDirCreated: directory === "absent", entries }; + return { + bridgeDir: deps.paths.bridgeDir, + bridgeDirCreated: directory === "absent", + ...(source ? { sourceDir: (await fs.realpath(source.dir).catch(() => undefined)) ?? source.dir } : {}), + entries, + prunes, + adopts, + }; } async function createBridgeDirectory(preflight: SkillsBridgePreflight): Promise { @@ -136,11 +598,13 @@ async function createBridgeDirectory(preflight: SkillsBridgePreflight): Promise< async function pruneRecordedDangling(entry: BridgeEntryPlan): Promise { if (entry.danglingTarget === undefined) throw new SkillsBridgeError(`Missing dangling-link evidence for ${entry.linkPath}`); + if (entry.danglingIdentity === undefined) + throw new SkillsBridgeError(`Missing dangling-link identity for ${entry.linkPath}`); const state = await entryState(entry.linkPath, entry.targetPath); if (state.kind !== "dangling" || state.link !== entry.danglingTarget) { throw new SkillsBridgeError(`Paseo skill bridge entry diverged before pruning: ${entry.linkPath}`); } - await fs.unlink(entry.linkPath); + await quarantineUnlinkVerified(entry.linkPath, entry.targetPath, entry.danglingIdentity); } async function createNoReplace(entry: BridgeEntryPlan): Promise { @@ -155,45 +619,124 @@ async function createNoReplace(entry: BridgeEntryPlan): Promise { } } +async function pruneStale(plan: BridgePrunePlan): Promise { + await quarantineUnlinkVerified(plan.linkPath, resolvedLinkTarget(plan.linkTarget, plan.linkPath), plan.linkIdentity); +} + +export async function adoptLegacyLink(plan: BridgeAdoptPlan, legacySourceDir: string): Promise { + // The preflight recorded this exact symlink as GJC's own legacy link; the + // captured object is verified post-rename inside the quarantine, so a + // pathname swap can never delete a foreign link. The ORIGINAL link is + // restored if the replacement publish fails (#4644 review r18): the + // legacy link text is captured first and recreated atomically (EEXIST + // means a concurrent entry claimed the name — the legacy link then stays + // recoverable in the quarantine, never deleted by this path). + const legacyText = plan.linkTarget; + await quarantineUnlinkVerified(plan.linkPath, path.resolve(legacySourceDir, plan.name), plan.linkIdentity); + try { + await fs.symlink(plan.targetPath, plan.linkPath); + } catch (error) { + // Restore the original legacy link before surfacing the failure so a + // partial failure never leaves the bridge entry missing outright. + await fs.symlink(legacyText, plan.linkPath).catch(() => undefined); + throw error; + } +} + /** Create only preflight-approved bridge links; symlink publication never replaces an existing entry. */ export async function installSkillsBridge(preflight: SkillsBridgePreflight): Promise { + const hasWork = + Object.keys(preflight.entries).length > 0 || preflight.prunes.length > 0 || preflight.adopts.length > 0; let bridgeDirCreated = false; - if (preflight.bridgeDirCreated) { - await createBridgeDirectory(preflight); - bridgeDirCreated = true; - } - const createdEntries: InstallSkillName[] = []; - for (const name of INSTALL_SKILL_NAMES) { - const entry = preflight.entries[name]; - if (entry.action === "noop") continue; - if (entry.action === "prune-and-recreate") await pruneRecordedDangling(entry); - await createNoReplace(entry); - createdEntries.push(name); - } - return { createdEntries, bridgeDirCreated }; + const createdEntries: string[] = []; + const prunedEntries: string[] = []; + const adoptedEntries: string[] = []; + const partial = (): SkillsBridgeInstallResult => ({ + createdEntries: [...createdEntries], + prunedEntries: [...prunedEntries], + adoptedEntries: [...adoptedEntries], + bridgeDirCreated, + ...(preflight.sourceDir ? { sourceDir: preflight.sourceDir } : {}), + }); + const fail = (error: unknown): never => { + if (error instanceof SkillsBridgePartialError) throw error; + const message = + error instanceof SkillsBridgeError ? error.message : error instanceof Error ? error.message : String(error); + throw new SkillsBridgePartialError(message, partial()); + }; + try { + if (preflight.bridgeDirCreated && hasWork) { + await createBridgeDirectory(preflight); + bridgeDirCreated = true; + } + for (const plan of preflight.prunes) { + await pruneStale(plan); + prunedEntries.push(plan.name); + } + for (const entry of Object.values(preflight.entries)) { + if (entry.action === "noop") continue; + if (entry.action === "prune-and-recreate") await pruneRecordedDangling(entry); + await createNoReplace(entry); + createdEntries.push(entry.name); + } + for (const adopt of preflight.adopts) { + await adoptLegacyLink(adopt, adopt.legacySourceDir); + adoptedEntries.push(adopt.name); + } + } catch (error) { + fail(error); + } + return createdEntries.length > 0 || prunedEntries.length > 0 || adoptedEntries.length > 0 + ? { + createdEntries, + prunedEntries, + adoptedEntries, + bridgeDirCreated, + ...(preflight.sourceDir ? { sourceDir: preflight.sourceDir } : {}), + } + : { createdEntries, prunedEntries, adoptedEntries, bridgeDirCreated }; } /** * Undo exactly the links this run created. A changed link is reported as a * conflict rather than being deleted; this makes compensation safe after edits. + * Pruned stale links are deliberately not restored: their target is gone. */ export async function inverseSkillsBridge( deps: PaseoSetupDependencies, result: SkillsBridgeInstallResult, + options: { readonly bridgeDir?: string } = {}, ): Promise { + if (result.sourceDir === undefined) { + throw new SkillsBridgeError("Refusing to undo Paseo skill bridge entries without a recorded source directory"); + } + // Removal must operate on the SAME directory it validated. The ledger + // records the directory GJC actually created links in; a later profile or + // path migration must not make removal inspect the old directory while + // unlinking from the current one. + const bridgeDir = options.bridgeDir ?? deps.paths.bridgeDir; const diverged: string[] = []; - for (const name of result.createdEntries) { - const destination = linkPath(deps, name); - const state = await entryState(destination, expectedTarget(deps, name)); - if (state.kind !== "expected" || state.link !== expectedTarget(deps, name)) diverged.push(destination); + const removals: { readonly destination: string; readonly target: string; readonly identity: SymlinkIdentity }[] = []; + const ownedNames = [...result.createdEntries, ...result.adoptedEntries]; + for (const name of ownedNames) { + const destination = path.join(bridgeDir, name); + const target = path.resolve(result.sourceDir, name); + const state = await entryState(destination, target); + // `dangling` still carries link text pointing exactly where we wrote it; + // the source went away (Paseo uninstalled or updated), and a dead link in + // GJC's own bridge directory is safe -- and correct -- to remove. + if (state.kind !== "expected" && state.kind !== "dangling") diverged.push(destination); + else removals.push({ destination, target, identity: state.identity }); } if (diverged.length > 0) { throw new SkillsBridgeError(`Refusing to remove diverged Paseo skill bridge entries: ${diverged.join(", ")}`); } - for (const name of result.createdEntries) await fs.unlink(linkPath(deps, name)); + for (const removal of removals) { + await quarantineUnlinkVerified(removal.destination, removal.target, removal.identity); + } if (!result.bridgeDirCreated) return; try { - await fs.rmdir(deps.paths.bridgeDir); + await fs.rmdir(bridgeDir); } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOTEMPTY") return; if ((error as NodeJS.ErrnoException).code === "ENOENT") return; @@ -201,34 +744,99 @@ export async function inverseSkillsBridge( } } -/** Find Paseo-prefixed bridge or source entries that the locked install allowlist does not repair. */ -export async function scanSkillsBridgeDrift(deps: PaseoSetupDependencies): Promise { +/** + * Drift inside GJC's own installation. Two honest signals only: + * + * - `orphan-skill`: a `paseo`-prefixed bridge entry that is not a live symlink + * into the current source. Real drift; a re-run repairs it. + * - `missing-bridge-link`: a bridge entry the ledger says GJC created is gone + * from the bridge directory. Real drift in GJC's own mirror. + * + * A skill Paseo ships that GJC never bridged is NOT drift (#4638): the bridge + * mirrors what GJC chose to bridge at install time, and a Paseo release adding + * a skill must not turn `--check` red. Provenance decides what `--remove` + * touches, not this scan. + */ +export async function scanSkillsBridgeDrift( + deps: PaseoSetupDependencies, + recordedEntries?: readonly string[], +): Promise { const reasons: DriftReason[] = []; + // One deterministic reason per bridge entry (#4644 review r9): the + // structural pass below classifies a dangling link first, and the + // target-comparison pass must not re-report the same subject with a + // second code. + const reported = new Set(); + // Canonicalize EXACTLY like installation (#4644 review r12): install + // records and links to the realpath of a symlinked skills directory, so + // comparing expected targets against the resolver's lexical spelling would + // report a perfectly valid bridge as foreign drift. A source that cannot + // be resolved anymore keeps its lexical form for the structural checks. + const resolvedSource = await resolveSource(deps); + const source = + resolvedSource !== undefined + ? { + dir: (await fs.realpath(resolvedSource.dir).catch(() => undefined)) ?? resolvedSource.dir, + origin: resolvedSource.origin, + } + : undefined; const bridgeEntries = await fs.readdir(deps.paths.bridgeDir, { withFileTypes: true }).catch(error => { if ((error as NodeJS.ErrnoException).code === "ENOENT") return []; throw error; }); + const liveNames = new Set(bridgeEntries.map(entry => entry.name)); + for (const entry of bridgeEntries) { + if (!entry.name.startsWith(PASEO_SKILL_PREFIX)) continue; + const destination = path.join(deps.paths.bridgeDir, entry.name); + if (entry.isSymbolicLink()) { + try { + await fs.stat(destination); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + reasons.push({ + code: "orphan-skill", + subject: destination, + detail: "bridge symlink target no longer exists", + }); + reported.add(destination); + } + continue; + } + reasons.push({ + code: "orphan-skill", + subject: destination, + detail: "bridge entry is not a symlink; remove it or re-run gjc setup paseo", + }); + } + if (source === undefined) return reasons; + // A live bridge entry pointing outside the source is drift: the bridge + // mirrors Paseo's skills exactly, so a foreign target is a hand edit. for (const entry of bridgeEntries) { if (!entry.name.startsWith(PASEO_SKILL_PREFIX) || !entry.isSymbolicLink()) continue; const destination = path.join(deps.paths.bridgeDir, entry.name); - try { - await fs.stat(destination); - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; - reasons.push({ code: "orphan-skill", subject: destination, detail: "bridge symlink target no longer exists" }); + if (reported.has(destination)) continue; + const expected = path.resolve(source.dir, entry.name); + const state = await foreignSymlinkState(destination); + if (state.kind === "symlink" && resolvedLinkTarget(state.link, destination) !== expected) { + const recorded = (recordedEntries ?? []).includes(entry.name); + reasons.push({ + code: recorded ? "foreign-skill-link" : "orphan-skill", + subject: destination, + detail: recorded + ? "a recorded bridge entry does not point into the source the ledger records" + : `bridge symlink does not point into Paseo's skills directory (${source.dir})`, + }); } } - const sourceEntries = await fs.readdir(deps.paths.agentsSkillsDir, { withFileTypes: true }).catch(error => { - if ((error as NodeJS.ErrnoException).code === "ENOENT") return []; - throw error; - }); - for (const entry of sourceEntries) { - if (!entry.name.startsWith(PASEO_SKILL_PREFIX) || (INSTALL_SKILL_NAMES as readonly string[]).includes(entry.name)) - continue; + // Only entries GJC recorded creating can be "missing". A source skill GJC + // never bridged is Paseo's own surface, not a hole in GJC's installation. + for (const name of recordedEntries ?? []) { + if (liveNames.has(name)) continue; + if (!(await sourceBridgeEntries(source.dir)).includes(name)) continue; reasons.push({ - code: "unlinked-skill", - subject: path.join(deps.paths.agentsSkillsDir, entry.name), - detail: "Paseo skill is outside GJC's locked bridge allowlist", + code: "missing-bridge-link", + subject: path.join(deps.paths.bridgeDir, name), + detail: "bridge symlink is missing", }); } return reasons; @@ -250,11 +858,28 @@ function existingCustomDirectories(current: Readonly): string[] { return [...directories]; } -/** Append the bridge directory without discarding concurrent user config changes. */ -export async function registerSkillsBridgeDirectory(settings: Settings, bridgeDir: string): Promise { +/** + * Register the bridge directory without discarding concurrent user config + * changes. A path migration swaps the registration in ONE commit: the old + * recorded path leaves `skills.customDirectories` in the same atomic batch + * that adds the new one, so a crash can never leave both or neither, and the + * CAS receipt restores the exact prior array on undo. + */ +export async function registerSkillsBridgeDirectory( + settings: Settings, + bridgeDir: string, + options: { readonly replaces?: string } = {}, +): Promise { return settings.commitAtomicBatchWithCurrent(current => { const directories = existingCustomDirectories(current); - const next = directories.includes(bridgeDir) ? directories : [...directories, bridgeDir]; + let next = directories; + if (options.replaces !== undefined && options.replaces !== bridgeDir) { + next = next.filter(directory => directory !== options.replaces); + } + next = next.includes(bridgeDir) ? next : [...next, bridgeDir]; + if (next.length === directories.length && next.every((directory, index) => directories[index] === directory)) { + return []; + } return [{ path: "skills.customDirectories" as SettingPath, op: "set", value: next }]; }); } diff --git a/packages/coding-agent/test/setup-paseo.test.ts b/packages/coding-agent/test/setup-paseo.test.ts index 099b30e34e..98a34d4ccf 100644 --- a/packages/coding-agent/test/setup-paseo.test.ts +++ b/packages/coding-agent/test/setup-paseo.test.ts @@ -2,16 +2,26 @@ import { afterEach, describe, expect, test } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; +import { getTrustedHomeDir } from "@gajae-code/utils"; import { parseSetupArgs } from "../src/cli/setup-cli"; import { checkPaseoSetup, STALE_GUIDANCE } from "../src/setup/paseo/check"; -import { type CompletedStep, compensate, recoverIntent, SagaStepError } from "../src/setup/paseo/install-saga"; +import { + type CompletedStep, + compensate, + recoverIntent, + runJsonStep, + SagaStepError, +} from "../src/setup/paseo/install-saga"; import { currentIdentity, hashBytes, planPublish, publishPlan, + readReplacedProviderBackup, readTarget, + replacedProviderBackupPath, serializeJson, + writeReplacedProviderBackup, } from "../src/setup/paseo/json-publisher"; import { createOrchestrationSeed } from "../src/setup/paseo/orchestration-preferences"; import { @@ -20,13 +30,19 @@ import { INTENT_VERSION, type IntentRecord, isProvenancedProvider, + ProvenanceLedgerCorruptError, provenancedProviderKeys, readIntent, readProvenance, writeIntent, writeProvenance, } from "../src/setup/paseo/paseo-ownership"; -import { assertUsableFlags, PaseoSetupUsageError } from "../src/setup/paseo/paseo-setup"; +import { + assertUsableFlags, + correctBridgeOwnershipAfterFailure, + PaseoSetupUsageError, + runPaseoSetup, +} from "../src/setup/paseo/paseo-setup"; import { buildProviderEntry, hasProviderConflict, @@ -35,14 +51,29 @@ import { resolveGjcCommand, } from "../src/setup/paseo/provider-config"; import { removePaseoSetup } from "../src/setup/paseo/remove"; -import { checkExitCode, type SetupCheckStatus } from "../src/setup/paseo/result-types"; import { + checkExitCode, + type PaseoRemoveResult, + type SetupCheckResult, + type SetupCheckStatus, +} from "../src/setup/paseo/result-types"; +import { + createDefaultPaseoSetupDependencies, + isTrustedRecordedSkillsSource, type PaseoLsOutcome, type PaseoPaths, type PaseoSetupDependencies, parseProviderLs, + paseoAppSkillsCandidates, + resolvePaseoSkillsSource, } from "../src/setup/paseo/setup-deps"; -import { installSkillsBridge, preflightSkillsBridge, SkillsBridgeError } from "../src/setup/paseo/skills-bridge"; +import { + installSkillsBridge, + preflightSkillsBridge, + SkillsBridgeError, + SkillsBridgePartialError, + sourceBridgeEntries, +} from "../src/setup/paseo/skills-bridge"; const FIXTURE_PASSWORD = "$2b$10$FIXTUREFIXTUREFIXTUREFIXTUREFIXTUREFIXTUREFIXTUREFIXTUR"; const SKILL_NAMES = ["paseo", "paseo-advisor", "paseo-committee", "paseo-handoff", "paseo-loop"]; @@ -68,9 +99,12 @@ async function makeRoot(): Promise { return root; } +/** The fixture's path shape: the production contract (whose skills field is required) plus its own concrete source directory. */ +type FixturePaths = PaseoPaths; + interface Fixture { readonly root: string; - readonly paths: PaseoPaths; + readonly paths: FixturePaths; readonly deps: PaseoSetupDependencies; readonly probes: number[]; readonly spawned: string[][]; @@ -93,7 +127,7 @@ async function makeFixture(outcome: PaseoLsOutcome = { kind: "timeout", timeoutM await fs.mkdir(agentsSkills, { recursive: true }); await fs.mkdir(path.join(agentDir, "skills"), { recursive: true }); - const paths: PaseoPaths = { + const paths: FixturePaths = { configJson: path.join(paseoHome, "config.json"), orchestrationPreferences: path.join(paseoHome, "orchestration-preferences.json"), agentsSkillsDir: agentsSkills, @@ -105,6 +139,8 @@ async function makeFixture(outcome: PaseoLsOutcome = { kind: "timeout", timeoutM const probes: number[] = []; const spawned: string[][] = []; + // The skills source is injected, never discovered: a developer machine that + // happens to carry ~/.agents/skills or a Paseo.app must not leak into a test. const deps: PaseoSetupDependencies = { paths, runProviderLs: async timeoutMs => { @@ -112,11 +148,21 @@ async function makeFixture(outcome: PaseoLsOutcome = { kind: "timeout", timeoutM return outcome; }, now: () => new Date("2026-01-01T00:00:00.000Z"), + skillsSource: async () => ({ dir: agentsSkills, origin: "user" }), + home, + // Hermetic stand-in for the production trusted-home rule: only the + // fixture's own root is a trusted source root. + trustedSkillsSource: async dir => { + const rel = path.relative(root, path.resolve(dir)); + return rel === "" || rel.startsWith("..") || path.isAbsolute(rel) + ? { ok: false, detail: `the recorded bridge source (${dir}) is outside every trusted Paseo source root` } + : { ok: true }; + }, }; return { root, paths, deps, probes, spawned }; } -async function seedConfig(paths: PaseoPaths, providers: Record = {}): Promise { +async function seedConfig(paths: FixturePaths, providers: Record = {}): Promise { const config = { daemon: { auth: { password: FIXTURE_PASSWORD }, port: 4317 }, agents: { providers: { claude: { enabled: true }, ...providers } }, @@ -124,10 +170,12 @@ async function seedConfig(paths: PaseoPaths, providers: Record await fs.writeFile(paths.configJson, serializeJson(config), { mode: 0o600 }); } -async function seedSkills(paths: PaseoPaths, extra: string[] = []): Promise { +async function seedSkills(paths: FixturePaths, extra: string[] = []): Promise { + const sourceDir = paths.agentsSkillsDir; + if (sourceDir === undefined) throw new Error("fixture carries no skills source"); for (const name of [...SKILL_NAMES, ...extra]) { - await fs.mkdir(path.join(paths.agentsSkillsDir, name), { recursive: true }); - await fs.writeFile(path.join(paths.agentsSkillsDir, name, "SKILL.md"), `# ${name}\n`); + await fs.mkdir(path.join(sourceDir, name), { recursive: true }); + await fs.writeFile(path.join(sourceDir, name, "SKILL.md"), `# ${name}\n`); } } @@ -404,7 +452,10 @@ describe("four-state check (AC-16, AC-17, AC-18)", () => { await seedSkills(fixture.paths); await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); for (const name of SKILL_NAMES) { - await fs.symlink(path.join(fixture.paths.agentsSkillsDir, name), path.join(fixture.paths.bridgeDir, name)); + await fs.symlink( + path.join(fixture.paths.agentsSkillsDir as string, name), + path.join(fixture.paths.bridgeDir, name), + ); } const resolution = resolveGjcCommand(); const command = resolution.ok ? resolution.command : [process.execPath, "acp"]; @@ -431,6 +482,14 @@ describe("four-state check (AC-16, AC-17, AC-18)", () => { expect(checkExitCode(result)).toBe(0); }); + test("an injected missing source is authoritative in check mode (#4644 review r20)", async () => { + const fixture = await cleanL1(lsOk("gjc")); + const result = await checkPaseoSetup({ ...fixture.deps, skillsSource: async () => undefined }); + expect(result.status).toBe("drift"); + if (result.status !== "drift") throw new Error("expected drift"); + expect(result.reasons).toContainEqual(expect.objectContaining({ code: "missing-skills-directory" })); + }); + test("clean L1 plus a daemon omitting the provider is stale with guidance", async () => { const fixture = await cleanL1(lsOk("claude")); const result = await checkPaseoSetup(fixture.deps); @@ -495,18 +554,19 @@ describe("four-state check (AC-16, AC-17, AC-18)", () => { }); describe("skills bridge", () => { - test("links exactly the five allowlisted skills and excludes context-search (AC-6)", async () => { + test("links every paseo-prefixed source skill except the denylist (AC-6, #4638)", async () => { const fixture = await makeFixture(); - await seedSkills(fixture.paths, ["context-search"]); + await seedSkills(fixture.paths, ["context-search", "paseo-help", "unrelated-skill"]); const preflight = await preflightSkillsBridge(fixture.deps); await installSkillsBridge(preflight); const linked = (await fs.readdir(fixture.paths.bridgeDir)).sort(); - expect(linked).toEqual([...SKILL_NAMES].sort()); + expect(linked).toEqual([...SKILL_NAMES, "paseo-help"].sort()); expect(linked).not.toContain("context-search"); + expect(linked).not.toContain("unrelated-skill"); }); - test("a foreign file at an allowlisted name refuses before any mutation", async () => { + test("a foreign file at a bridged name refuses before any mutation", async () => { const fixture = await makeFixture(); await seedSkills(fixture.paths); await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); @@ -517,6 +577,17 @@ describe("skills bridge", () => { expect(await snapshotTree(fixture.paths.bridgeDir)).toBe(before); }); + test("a foreign file squatting on a name the source no longer carries refuses too", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + await fs.writeFile(path.join(fixture.paths.bridgeDir, "paseo-retired"), "user file\n"); + const before = await snapshotTree(fixture.paths.bridgeDir); + + await expect(preflightSkillsBridge(fixture.deps)).rejects.toBeInstanceOf(SkillsBridgeError); + expect(await snapshotTree(fixture.paths.bridgeDir)).toBe(before); + }); + test("a symlink pointing elsewhere refuses before any mutation", async () => { const fixture = await makeFixture(); await seedSkills(fixture.paths); @@ -532,7 +603,10 @@ describe("skills bridge", () => { const fixture = await makeFixture(); await seedSkills(fixture.paths); await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); - await fs.symlink(path.join(fixture.paths.agentsSkillsDir, "paseo"), path.join(fixture.paths.bridgeDir, "paseo")); + await fs.symlink( + path.join(fixture.paths.agentsSkillsDir as string, "paseo"), + path.join(fixture.paths.bridgeDir, "paseo"), + ); const preflight = await preflightSkillsBridge(fixture.deps); const result = await installSkillsBridge(preflight); @@ -540,19 +614,2161 @@ describe("skills bridge", () => { expect(result.createdEntries.length).toBe(SKILL_NAMES.length - 1); }); - test("both protected skill trees are byte-identical across install and check (AC-8, AC-19)", async () => { + test("a source entry that is a file, not a directory, is never linked", async () => { const fixture = await makeFixture(); - await seedSkills(fixture.paths, ["context-search"]); - await fs.writeFile(path.join(fixture.paths.gjcSkillsDir, "mine.md"), "# mine\n"); + await seedSkills(fixture.paths); + await fs.writeFile(path.join(fixture.paths.agentsSkillsDir as string, "paseo-file"), "not a skill\n"); + const preflight = await preflightSkillsBridge(fixture.deps); + await installSkillsBridge(preflight); + expect(await fs.readdir(fixture.paths.bridgeDir)).not.toContain("paseo-file"); + }); - const agentsBefore = await snapshotTree(fixture.paths.agentsSkillsDir); - const gjcBefore = await snapshotTree(fixture.paths.gjcSkillsDir); + /** Install without the full saga but with a realistic ledger, so preflight's provenance gate can run. */ + async function installWithLedger(deps: PaseoSetupDependencies): Promise { + const preflight = await preflightSkillsBridge(deps); + await installSkillsBridge(preflight); + await writeProvenance(deps.paths.provenanceLedger, { + version: 1, + providerKeys: {}, + seededOrchestrationKeys: {}, + bridgePath: deps.paths.bridgeDir, + bridgeEntries: [...Object.keys(preflight.entries), ...preflight.adopts.map(adopt => adopt.name)], + bridgeDirCreated: false, + ...(preflight.sourceDir ? { bridgeSourceDir: preflight.sourceDir } : {}), + }); + } - await installSkillsBridge(await preflightSkillsBridge(fixture.deps)); - await checkPaseoSetup(fixture.deps); + test("install converges the bridge after a Paseo release adds and drops skills (#4638)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); - expect(await snapshotTree(fixture.paths.agentsSkillsDir)).toBe(agentsBefore); - expect(await snapshotTree(fixture.paths.gjcSkillsDir)).toBe(gjcBefore); + // Paseo 0.4.0: paseo-loop is gone, paseo-help is new. + await fs.rm(path.join(fixture.paths.agentsSkillsDir as string, "paseo-loop"), { recursive: true }); + await fs.mkdir(path.join(fixture.paths.agentsSkillsDir as string, "paseo-help"), { recursive: true }); + await fs.writeFile( + path.join(fixture.paths.agentsSkillsDir as string, "paseo-help", "SKILL.md"), + "# paseo-help\n", + ); + + const second = await installSkillsBridge(await preflightSkillsBridge(fixture.deps)); + expect(second.prunedEntries).toEqual(["paseo-loop"]); + expect(second.createdEntries).toEqual(["paseo-help"]); + const linked = (await fs.readdir(fixture.paths.bridgeDir)).sort(); + expect(linked).toEqual([...SKILL_NAMES.slice(0, 4), "paseo-help"].sort()); + // No dangling links survive the release change. + for (const name of linked) { + const stat = await fs.stat(path.join(fixture.paths.bridgeDir, name)); + expect(stat.isDirectory()).toBe(true); + } + }); + + test("a re-run after a source skill is deleted prunes the dead link instead of leaving it (#4638)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + await fs.rm(path.join(fixture.paths.agentsSkillsDir as string, "paseo-committee"), { recursive: true }); + + const result = await installSkillsBridge(await preflightSkillsBridge(fixture.deps)); + expect(result.prunedEntries).toEqual(["paseo-committee"]); + await expect(fs.lstat(path.join(fixture.paths.bridgeDir, "paseo-committee"))).rejects.toMatchObject({ + code: "ENOENT", + }); + }); + test("a foreign paseo-prefixed symlink is never pruned, with or without provenance (#4644 review)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + + // A live user symlink at a name the source does not carry and no ledger + // ever recorded: directory creation cannot prove ownership, so install + // must refuse instead of silently deleting it. + await fs.symlink( + path.join(fixture.paths.agentsSkillsDir as string, "paseo"), + path.join(fixture.paths.bridgeDir, "paseo-mine"), + ); + const before = await snapshotTree(fixture.paths.bridgeDir); + + await expect(preflightSkillsBridge(fixture.deps)).rejects.toBeInstanceOf(SkillsBridgeError); + expect(await snapshotTree(fixture.paths.bridgeDir)).toBe(before); + }); + test("a retargeted recorded link is a conflict, never pruned (#4644 review r2)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + + // The user retargets a ledger-recorded name at their own tree, then a + // Paseo release drops that name from the source. Setup must apply the + // same exact-target predicate remove does: the recorded NAME is not + // ownership once the link no longer points where the ledger recorded. + await fs.rm(path.join(fixture.paths.bridgeDir, "paseo-loop")); + await fs.symlink(path.join(fixture.root, "user-own-tree"), path.join(fixture.paths.bridgeDir, "paseo-loop")); + await fs.rm(path.join(fixture.paths.agentsSkillsDir as string, "paseo-loop"), { recursive: true }); + const before = await snapshotTree(fixture.paths.bridgeDir); + + await expect(preflightSkillsBridge(fixture.deps)).rejects.toBeInstanceOf(SkillsBridgeError); + expect(await snapshotTree(fixture.paths.bridgeDir)).toBe(before); + }); + + test("adoption is refused when the ledger already records a source (#4644 review r2)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + // The ledger records ~/.agents/skills as its source, but the discovered + // source is the app bundle and a recorded link points somewhere else + // entirely: adoption exists only for legacy ledgers, so this conflicts. + const bundle = path.join(fixture.root, "Applications", "Paseo.app", "Contents", "Resources", "skills"); + for (const name of ["paseo"]) { + await fs.mkdir(path.join(bundle, name), { recursive: true }); + await fs.writeFile(path.join(bundle, name, "SKILL.md"), `# ${name}\n`); + } + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + await fs.symlink(path.join(fixture.root, "elsewhere"), path.join(fixture.paths.bridgeDir, "paseo")); + await writeProvenance(fixture.paths.provenanceLedger, { + version: 1, + providerKeys: {}, + seededOrchestrationKeys: {}, + bridgePath: fixture.paths.bridgeDir, + bridgeEntries: ["paseo"], + bridgeDirCreated: false, + bridgeSourceDir: fixture.paths.agentsSkillsDir, + }); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => ({ dir: bundle, origin: "app-bundle" }), + }; + + await expect(preflightSkillsBridge(deps)).rejects.toBeInstanceOf(SkillsBridgeError); + }); + + test("PASEO_SKILLS_DIR from a project .env is not honored (#4644 review r2)", async () => { + const root = await makeRoot(); + const home = path.join(root, "home"); + const userDir = path.join(home, ".agents", "skills"); + await fs.mkdir(userDir, { recursive: true }); + // A cloned repository ships this .env and the directory it points at. + const repoDir = path.join(root, "repo"); + const repoSkills = path.join(repoDir, "skills"); + await fs.mkdir(repoSkills, { recursive: true }); + await fs.mkdir(path.join(repoSkills, "paseo-evil"), { recursive: true }); + await Bun.write(path.join(repoDir, ".env"), `PASEO_SKILLS_DIR=${repoSkills}\n`); + const priorCwd = process.cwd(); + process.chdir(repoDir); + const prior = process.env.PASEO_SKILLS_DIR; + process.env.PASEO_SKILLS_DIR = repoSkills; + try { + await expect(resolvePaseoSkillsSource(home)).resolves.toEqual({ dir: userDir, origin: "user" }); + } finally { + process.chdir(priorCwd); + if (prior === undefined) delete process.env.PASEO_SKILLS_DIR; + else process.env.PASEO_SKILLS_DIR = prior; + } + }); + + test("a bridge-step provenance write failure leaves the ledger intact and no links behind (#4644 review r3)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + // Complete one install so provider/orchestration steps and their ledger + // records exist, then reset ONLY the bridge and make the provenance + // directory unwritable. The next run must fail while committing the + // bridge provenance -- AFTER the earlier steps, AT the bridge step -- + // proving the record-before-mutation ordering: no link appears and the + // ledger is unchanged (the failed write rolls back to the old record). + await runPaseoSetup({}, fixture.deps); + const before = await fs.readFile(fixture.paths.provenanceLedger, "utf8"); + await fs.rm(fixture.paths.bridgeDir, { recursive: true }); + const ledgerParent = path.dirname(fixture.paths.provenanceLedger); + await fs.chmod(ledgerParent, 0o555); + let outcome: Awaited> | undefined; + try { + outcome = await runPaseoSetup({}, fixture.deps); + } catch { + // A thrown error is also acceptable; both must leave no unrecorded + // links. + } finally { + await fs.chmod(ledgerParent, 0o755); + } + + // The bridge directory must not exist: no link was created before the + // record committed. + await expect(fs.stat(fixture.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + // The failed atomic write left the previous ledger byte-identical and no + // temporary litter behind. + expect(await fs.readFile(fixture.paths.provenanceLedger, "utf8")).toBe(before); + expect((await fs.readdir(ledgerParent)).filter(name => name.endsWith(".tmp"))).toEqual([]); + if (outcome?.kind === "install") { + expect(outcome.result.outcome).not.toBe("installed"); + } + }); + + test("a user-created exact-target link is never recorded or removed (#4644 review r3)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + // The user pre-creates a link whose target matches what GJC would write, + // in an existing bridge directory no ledger ever recorded. + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + await fs.symlink( + path.join(fixture.paths.agentsSkillsDir as string, "paseo"), + path.join(fixture.paths.bridgeDir, "paseo"), + ); + + const install = await runPaseoSetup({}, fixture.deps); + expect(install.kind).toBe("install"); + // The user link is a noop to install but is NOT adopted into ownership. + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeEntries).not.toContain("paseo"); + expect(ledger.bridgeEntries).toContain("paseo-advisor"); + + // Remove cleans only what GJC recorded; the user's link survives. + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("removed"); + expect((await fs.lstat(path.join(fixture.paths.bridgeDir, "paseo"))).isSymbolicLink()).toBe(true); + }); + + test("a corrupt provenance ledger is an explicit error, never an empty one (#4644 review r3)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await fs.mkdir(path.dirname(fixture.paths.provenanceLedger), { recursive: true }); + await Bun.write(fixture.paths.provenanceLedger, "{ not json at all"); + await expect(readProvenance(fixture.paths.provenanceLedger)).rejects.toThrow(/corrupt/); + // Check surfaces the corruption instead of silently owning nothing. + await expect(checkPaseoSetup(fixture.deps)).rejects.toThrow(/corrupt/); + }); + + test("prune ownership stays recorded until the unlink completes (#4644 review r3)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + // A Paseo release drops a skill; the next install prunes it. + await fs.rm(path.join(fixture.paths.agentsSkillsDir as string, "paseo-loop"), { recursive: true }); + const rerun = await runPaseoSetup({}, fixture.deps); + expect(rerun.kind).toBe("install"); + // After the prune completes the name is no longer owned... + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeEntries).not.toContain("paseo-loop"); + // ...and the link is gone. + await expect(fs.lstat(path.join(fixture.paths.bridgeDir, "paseo-loop"))).rejects.toMatchObject({ + code: "ENOENT", + }); + // The pre-prune record (written before mutation) included the name, so a + // crash between record and unlink could not strand an unowned link; that + // window is exercised by the ownership-superset invariant below. + expect([...(ledger.bridgeEntries ?? [])].sort()).toEqual([ + "paseo", + "paseo-advisor", + "paseo-committee", + "paseo-handoff", + ]); + }); + + test("a failed adoption restores the original legacy link (#4644 review r18)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + // A recorded legacy link pointing at the legacy source. The adoption + // plan is driven through installSkillsBridge with a preflight whose + // replacement target COLLIDES with an occupant planted on the bridge + // name after the preflight: the quarantine rename removes our link, + // the replacement publish then fails EEXIST, and the ORIGINAL legacy + // link must be restored. + const name = "paseo"; + const legacyTarget = path.join(fixture.paths.agentsSkillsDir as string, name); + const bridgeName = path.join(fixture.paths.bridgeDir, name); + await fs.symlink(legacyTarget, bridgeName); + const legacyStat = await fs.lstat(bridgeName); + const plan = { + name, + linkPath: bridgeName, + targetPath: path.join(fixture.paths.agentsSkillsDir as string, "paseo-loop"), + legacySourceDir: fixture.paths.agentsSkillsDir as string, + linkTarget: legacyTarget, + linkIdentity: { + dev: legacyStat.dev, + ino: legacyStat.ino, + size: legacyStat.size, + mtimeMs: legacyStat.mtimeMs, + }, + }; + // Occupy the bridge name so the replacement publish hits EEXIST. + await fs.rm(bridgeName); + await Bun.write(bridgeName, "occupant\n"); + const { adoptLegacyLink } = await import("../src/setup/paseo/skills-bridge"); + await expect(adoptLegacyLink(plan, plan.legacySourceDir)).rejects.toThrow(); + // The occupant was never destroyed and the bridge name still holds it + // (nothing foreign is deleted); the legacy link is recoverable in the + // quarantine. With a VACANT name the original link comes back exactly. + expect(await fs.readFile(bridgeName, "utf8")).toBe("occupant\n"); + }); + + test("quarantine restoration never clobbers an entry claiming the bridge name (#4644 review r15)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + // GJC's recorded link and a foreign entry racing for the same name: the + // divergence restore must leave a foreign occupant of the bridge name + // untouched rather than rename the quarantined object over it. + const name = "paseo-loop"; + const bridgeName = path.join(fixture.paths.bridgeDir, name); + await fs.symlink(path.join(fixture.paths.agentsSkillsDir as string, "paseo"), bridgeName); + const foreignStat = await fs.lstat(bridgeName); + + await expect( + installSkillsBridge({ + bridgeDir: fixture.paths.bridgeDir, + bridgeDirCreated: false, + entries: {}, + prunes: [ + { + name, + linkPath: bridgeName, + linkTarget: path.join(fixture.paths.agentsSkillsDir as string, name), + linkIdentity: { + dev: foreignStat.dev, + ino: foreignStat.ino, + size: foreignStat.size, + mtimeMs: foreignStat.mtimeMs, + }, + }, + ], + adopts: [], + }), + ).rejects.toBeInstanceOf(SkillsBridgeError); + // The foreign link survives at the bridge name. + expect((await fs.lstat(bridgeName)).isSymbolicLink()).toBe(true); + // No destructive rename left the bridge dir with litter beyond the + // quarantined object (recoverable, never deleted). + const names = (await fs.readdir(fixture.paths.bridgeDir)).filter(entry => entry !== name); + for (const leftover of names) { + expect(leftover.startsWith(".gjc-paseo-quarantine-")).toBe(true); + } + }); + + test("a swapped pathname is detected post-rename and the foreign link restored (#4644 review r3)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + // A foreign link occupies a name the (hand-built) preflight believes is + // GJC's own with a different recorded target. + await fs.symlink( + path.join(fixture.paths.agentsSkillsDir as string, "paseo"), + path.join(fixture.paths.bridgeDir, "paseo-loop"), + ); + const foreignStat = await fs.lstat(path.join(fixture.paths.bridgeDir, "paseo-loop")); + const before = await snapshotTree(fixture.paths.bridgeDir); + + await expect( + installSkillsBridge({ + bridgeDir: fixture.paths.bridgeDir, + bridgeDirCreated: false, + entries: {}, + prunes: [ + { + name: "paseo-loop", + linkPath: path.join(fixture.paths.bridgeDir, "paseo-loop"), + linkTarget: path.join(fixture.paths.agentsSkillsDir as string, "paseo-loop"), + linkIdentity: { + dev: foreignStat.dev, + ino: foreignStat.ino, + size: foreignStat.size, + mtimeMs: foreignStat.mtimeMs, + }, + }, + ], + adopts: [], + }), + ).rejects.toBeInstanceOf(SkillsBridgeError); + // The foreign link is never destroyed (#4644 reviews r3/r15): the + // original path is VACANT after the quarantine rename, so the + // no-clobber restore puts it back exactly as it was; when a concurrent + // entry had claimed the path meanwhile, the quarantined foreign object + // stays AT the quarantine name instead of being renamed over it. + expect(await snapshotTree(fixture.paths.bridgeDir)).toBe(before); + }); + + test("a same-target replacement after preflight is preserved (#4644 review r20)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + const name = "paseo-loop"; + const bridgeName = path.join(fixture.paths.bridgeDir, name); + const target = path.join(fixture.paths.agentsSkillsDir as string, name); + await fs.symlink(target, bridgeName); + const original = await fs.lstat(bridgeName); + const plan = { + name, + linkPath: bridgeName, + linkTarget: target, + linkIdentity: { + dev: original.dev, + ino: original.ino, + size: original.size, + mtimeMs: original.mtimeMs, + }, + }; + // The replacement has the exact expected link text, so content-only + // verification would delete it. Its inode is not GJC's preflight inode. + const replacementPath = path.join(fixture.paths.bridgeDir, "replacement"); + await fs.symlink(target, replacementPath); + await fs.rename(replacementPath, bridgeName); + await expect( + installSkillsBridge({ + bridgeDir: fixture.paths.bridgeDir, + bridgeDirCreated: false, + entries: {}, + prunes: [plan], + adopts: [], + }), + ).rejects.toBeInstanceOf(SkillsBridgeError); + expect((await fs.lstat(bridgeName)).isSymbolicLink()).toBe(true); + expect(await fs.readlink(bridgeName)).toBe(target); + }); + + test("an interpolated project .env override is rejected by presence (#4644 review r3)", async () => { + const root = await makeRoot(); + const home = path.join(root, "home"); + const userDir = path.join(home, ".agents", "skills"); + await fs.mkdir(userDir, { recursive: true }); + const repoDir = path.join(root, "repo"); + const repoSkills = path.join(repoDir, "skills"); + await fs.mkdir(path.join(repoSkills, "paseo-evil"), { recursive: true }); + // Bun expands $PWD before process.env sees the value, so a literal + // comparison would match nothing; presence must be the rule. + await Bun.write(path.join(repoDir, ".env"), "PASEO_SKILLS_DIR=$PWD/skills\n"); + const priorCwd = process.cwd(); + process.chdir(repoDir); + process.env.PASEO_SKILLS_DIR = repoSkills; + try { + await expect(resolvePaseoSkillsSource(home)).resolves.toEqual({ dir: userDir, origin: "user" }); + } finally { + process.chdir(priorCwd); + delete process.env.PASEO_SKILLS_DIR; + } + }); + + test("removal uses the ledger-recorded bridge directory throughout (#4644 review r3)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + // Simulate a path migration: the ledger still records the original + // directory (which holds the links), while deps.paths points elsewhere. + const moved = path.join(fixture.root, "moved-paseo-skills"); + await fs.mkdir(moved, { recursive: true }); + const migratedDeps: PaseoSetupDependencies = { + ...fixture.deps, + paths: { ...fixture.deps.paths, bridgeDir: moved }, + }; + + const remove = await removePaseoSetup(migratedDeps, { now: new Date() }); + // #4644 review r14: a recorded path OUTSIDE the agent directory must be + // confined by both the basename rule and the genuine-migration rule. + // `moved-paseo-skills` carries the bridge family basename, so the + // basename rule alone accepts it — the migration rule (no genuine GJC + // ledger beside the victim) still refuses it. Whichever rule fires, + // the failure path reports the recorded directory and cleans nothing. + expect(["removed", "partial-removal"]).toContain(remove.outcome); + if (remove.outcome === "partial-removal") { + expect(remove.evidence.detail).toMatch( + /does not carry the bridge directory name|escapes the agent directory|not a location Paseo skills discovery|neither the configured bridge .* nor an authenticated migration record/, + ); + } + }); + + test("a valid but empty source bridges nothing and records no phantom bridge (#4644 review r5)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedConfig(fixture.paths); + // The source directory exists but ships no `paseo*` skills: an + // intentional no-bridge state. No directory is created, nothing is + // registered, and the ledger must not claim a bridge GJC never built + // (a phantom record would make --remove straddle a missing path and + // --check look healthy while loading nothing). + await runPaseoSetup({}, fixture.deps); + await runPaseoSetup({}, fixture.deps); + + await expect(fs.stat(fixture.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeDirCreated).toBeFalsy(); + expect(ledger.bridgePath).toBeUndefined(); + expect(ledger.bridgeEntries).toEqual([]); + + // Removal is still a clean no-op-with-provenance: the daemon credential + // and the foreign provider survive, and no bridge path is touched. + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => undefined, + }; + const remove = await runPaseoSetup({ remove: true }, deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("removed"); + const config = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + const providers = (config.agents as { providers: Record }).providers; + expect(Object.keys(providers).sort()).toEqual(["claude"]); + }); + test("install and remove serialize on one per-agent mutation lock (#4644 review r5)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + + // The install pauses INSIDE its bridge step and stays there; the remove + // starts while the install still holds the lock, so the two genuinely + // overlap (#4644 review r7: releasing the install before starting the + // remove proves nothing about serialization). + const installEnteredBridge = Promise.withResolvers(); + const releaseInstall = Promise.withResolvers(); + const gatedSource = fixture.deps.skillsSource; + const installDeps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => { + const source = await gatedSource?.(); + installEnteredBridge.resolve(); + await releaseInstall.promise; + return source; + }, + }; + + const installPromise = runPaseoSetup({}, installDeps); + // Wait until the install is provably inside its bridge step (it holds + // the mutation lock and is about to create links). + await installEnteredBridge.promise; + const removePromise = runPaseoSetup({ remove: true }, fixture.deps); + + // While the install is blocked mid-mutation, the remove must NOT be able + // to finish: un-serialized, it would clear the ledger while the install + // is still creating links and registering the bridge. The settle + // observer is attached BEFORE the wait so a remove that finished early + // cannot escape the assertion. + let removeSettled = false; + const removeObserved = removePromise.then( + () => { + removeSettled = true; + }, + () => { + removeSettled = true; + }, + ); + await Bun.sleep(200); + expect(removeSettled).toBe(false); + void removeObserved; + + // Release the install; both now run to completion in serialized order. + releaseInstall.resolve(); + const install = await installPromise; + expect(install.kind).toBe("install"); + const remove = await removePromise; + expect(remove.kind).toBe("remove"); + + // The terminal state is consistent whichever side won the serialized + // order: every live bridge link is covered by the ledger record, and a + // completed remove leaves no owned link or entry behind. + const ledger = await readProvenance(fixture.paths.provenanceLedger); + const liveLinks = (await fs.readdir(fixture.paths.bridgeDir).catch(() => [])).filter(name => + name.startsWith("paseo"), + ); + const recorded = new Set(ledger.bridgeEntries ?? []); + for (const name of liveLinks) { + expect(recorded.has(name)).toBe(true); + } + if (remove.kind === "remove" && remove.result.outcome === "removed") { + expect(ledger.bridgeEntries ?? []).toEqual([]); + expect(liveLinks).toEqual([]); + } + }); + + test("a pre-existing identical provider entry is never claimed as GJC-owned (#4644 review r5)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + // Run 1: GJC installs and records its entry. + await runPaseoSetup({}, fixture.deps); + // The ledger is lost (or the machine is rebuilt): the config still + // carries an entry byte-identical to what GJC writes, but GJC no + // longer has any record of creating it. + await writeProvenance(fixture.paths.provenanceLedger, { + version: 1, + providerKeys: {}, + seededOrchestrationKeys: {}, + }); + + const install = await runPaseoSetup({}, fixture.deps); + expect(install.kind).toBe("install"); + + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.providerKeys.gjc).toBeUndefined(); + expect(ledger.providerPreexistingKeys?.gjc).toBe(true); + + // A later remove leaves the entry in place: it was never GJC's. + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + const providers = (after.agents as { providers: Record }).providers; + expect(providers.gjc).toBeDefined(); + }); + + test("a --force overwrite restores the replaced provider entry on remove (#4644 review r5)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const userEntry = { ...buildProviderEntry([process.execPath, "acp"]), label: "USER EDIT", enabled: false }; + await seedConfig(fixture.paths, { gjc: userEntry }); + + const install = await runPaseoSetup({ force: true }, fixture.deps); + expect(install.kind).toBe("install"); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.providerKeys.gjc).toBeDefined(); + const ref = ledger.providerReplacedEntries?.gjc; + expect(typeof ref?.backupPath).toBe("string"); + expect(path.isAbsolute(ref?.backupPath ?? "")).toBe(true); + // The pointer targets a private sidecar holding the replaced value; the + // ledger itself never serializes the entry (#4644 review r7: a provider + // value can carry credential-bearing env/argument content, and GJC-side + // durable state is credential-free by contract). + const sidecar = JSON.parse(await fs.readFile(ref?.backupPath ?? "", "utf8")) as { + key: string; + value: unknown; + }; + expect(sidecar.key).toBe("gjc"); + expect(sidecar.value).toEqual(userEntry); + expect((await fs.stat(ref?.backupPath ?? "")).mode & 0o777).toBe(0o600); + expect(await fs.readFile(fixture.paths.provenanceLedger, "utf8")).not.toContain("USER EDIT"); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + const providers = (after.agents as { providers: Record }).providers; + expect(providers.gjc).toEqual(userEntry); + // The sidecar served its restore and is cleaned up with the ownership. + await expect(fs.stat(ref?.backupPath ?? "")).rejects.toMatchObject({ code: "ENOENT" }); + }); + + test("a migration with a tampered ledger name or path refuses instead of cleaning (#4644 review r5)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + + // Tamper: a traversal entry name inside the recorded set. + const victimDir = path.join(fixture.root, "outside"); + await fs.mkdir(victimDir, { recursive: true }); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { + ...ledger, + bridgeEntries: ["paseo", "../../outside/paseo-skills"], + }); + + const newBridge = path.join(fixture.root, "agentdir-new", "paseo-skills"); + const migratedDeps: PaseoSetupDependencies = { + ...fixture.deps, + paths: { ...fixture.deps.paths, bridgeDir: newBridge }, + }; + + const install = await runPaseoSetup({}, migratedDeps); + expect(install.kind).toBe("install"); + if (install.kind !== "install") throw new Error("expected an install outcome"); + expect(install.result.outcome).toBe("partial-install"); + // The old directory's real links are untouched by the refusal. + for (const name of ["paseo", "paseo-advisor"]) { + await expect(fs.lstat(path.join(fixture.paths.bridgeDir, name))).resolves.toBeDefined(); + } + await expect(fs.lstat(victimDir)).resolves.toBeDefined(); + }); + + test("an unreadable source directory fails closed and preserves the bridge (#4644 review r5)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + const before = await snapshotTree(fixture.paths.bridgeDir); + + // The source directory exists but cannot be read: an app update or a + // permission change mid-run. Every recorded entry must survive. + await fs.chmod(fixture.paths.agentsSkillsDir as string, 0o000); + try { + // Preflight refuses before any mutation; nothing is pruned. + await expect(runPaseoSetup({}, fixture.deps)).rejects.toBeInstanceOf(SkillsBridgeError); + } finally { + await fs.chmod(fixture.paths.agentsSkillsDir as string, 0o755); + } + expect(await snapshotTree(fixture.paths.bridgeDir)).toBe(before); + }); + + test("a source that vanishes mid-run fails closed and preserves the bridge (#4644 review r5)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + const before = await snapshotTree(fixture.paths.bridgeDir); + + // The resolver verifies the directory, then it disappears before + // enumeration: the preflight must refuse rather than prune everything. + const vanishable = path.join(fixture.root, "vanishing-skills"); + await fs.cp(fixture.paths.agentsSkillsDir as string, vanishable, { recursive: true }); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => { + await fs.rm(vanishable, { recursive: true, force: true }); + return { dir: vanishable, origin: "user" }; + }, + }; + + // Preflight refuses rather than pruning; the bridge is untouched. + await expect(runPaseoSetup({}, deps)).rejects.toBeInstanceOf(SkillsBridgeError); + expect(await snapshotTree(fixture.paths.bridgeDir)).toBe(before); + }); + + test("a resolved source that becomes empty prunes to a recorded-created empty bridge, and remove cleans it (#4644 review r5)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + + // Every skill disappears but the directory itself remains valid. + for (const name of SKILL_NAMES) { + await fs.rm(path.join(fixture.paths.agentsSkillsDir as string, name), { recursive: true }); + } + const converged = await runPaseoSetup({}, fixture.deps); + expect(converged.kind).toBe("install"); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeEntries).toEqual([]); + expect(ledger.bridgeDirCreated).toBe(true); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("removed"); + await expect(fs.stat(fixture.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + }); + test("a --force overwrite of a scalar provider value is restored on remove (#4644 review r6)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths, { gjc: "user scalar value" }); + + const install = await runPaseoSetup({ force: true }, fixture.deps); + expect(install.kind).toBe("install"); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + // A scalar prior is preserved through the same sidecar pointer, never + // serialized into the ledger itself. + const sidecar = JSON.parse(await fs.readFile(ledger.providerReplacedEntries?.gjc?.backupPath ?? "", "utf8")) as { + key: string; + value: unknown; + }; + expect(sidecar.value).toBe("user scalar value"); + expect(await fs.readFile(fixture.paths.provenanceLedger, "utf8")).not.toContain("user scalar value"); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + const providers = (after.agents as { providers: Record }).providers; + expect(providers.gjc).toBe("user scalar value"); + }); + + test("a --force overwrite of a null provider value is restored on remove (#4644 review r6)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const config = { + daemon: { auth: { password: FIXTURE_PASSWORD }, port: 4317 }, + agents: { providers: { claude: { enabled: true }, gjc: null } }, + }; + await fs.writeFile(fixture.paths.configJson, serializeJson(config), { mode: 0o600 }); + + const install = await runPaseoSetup({ force: true }, fixture.deps); + expect(install.kind).toBe("install"); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + const providers = (after.agents as { providers: Record }).providers; + expect(providers.gjc).toBeNull(); + }); + + test("a missing replaced-provider sidecar fails removal closed instead of deleting the key (#4644 review r7)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const userEntry = { ...buildProviderEntry([process.execPath, "acp"]), label: "USER EDIT", enabled: false }; + await seedConfig(fixture.paths, { gjc: userEntry }); + await runPaseoSetup({ force: true }, fixture.deps); + + // The private sidecar is lost (deleted, restored from a partial backup): + // removal must not fall back to deleting content it cannot restore. + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await fs.rm(ledger.providerReplacedEntries?.gjc?.backupPath ?? "", { force: true }); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("partial-removal"); + if (remove.result.outcome === "partial-removal") { + expect(remove.result.evidence.detail).toContain("cannot be restored"); + } + // GJC's entry survives: the restore never happened, so neither may the delete. + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + expect(providersOf(after).gjc).toBeDefined(); + }); + + test("replaced-provider sidecar names are injective across colliding sanitized keys (#4644 review r8)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + // `a/b` and `a_b` sanitize identically; before r8 both mapped to ONE + // sidecar path, so the second `--force` renamed over the first key's + // only preserved copy of the user's value. + const slashRef = await writeReplacedProviderBackup(fixture.paths.configJson, "a/b", "slash value"); + const underscoreRef = await writeReplacedProviderBackup(fixture.paths.configJson, "a_b", "underscore value"); + expect(slashRef.backupPath).not.toBe(underscoreRef.backupPath); + // Both preserved values survive side by side and read back exactly. + const slash = await readReplacedProviderBackup(slashRef.backupPath, "a/b", slashRef.valueSha256); + expect(slash).toEqual({ found: true, value: "slash value" }); + const underscore = await readReplacedProviderBackup(underscoreRef.backupPath, "a_b", underscoreRef.valueSha256); + expect(underscore).toEqual({ found: true, value: "underscore value" }); + }); + + test("an existing replaced-provider sidecar is never clobbered (#4644 review r8)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + const first = await writeReplacedProviderBackup(fixture.paths.configJson, "gjc", "user value"); + // A later write of the SAME key with DIFFERENT bytes must fail closed + // instead of replacing the user's preserved value. + await expect(writeReplacedProviderBackup(fixture.paths.configJson, "gjc", "attacker value")).rejects.toThrow( + /already exists at this path with different content/, + ); + // The first value is intact. + const reread = await readReplacedProviderBackup(first.backupPath, "gjc", first.valueSha256); + expect(reread).toEqual({ found: true, value: "user value" }); + // Re-writing the exact same bytes is idempotent and keeps the ref. + const again = await writeReplacedProviderBackup(fixture.paths.configJson, "gjc", "user value"); + expect(again.backupPath).toBe(first.backupPath); + expect(again.valueSha256).toBe(first.valueSha256); + }); + test("a sidecar conflict during the provider step rolls the published entry back instead of stranding an unowned overwrite (#4644 review r8)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const userEntry = { ...buildProviderEntry([process.execPath, "acp"]), label: "USER EDIT" }; + await seedConfig(fixture.paths, { gjc: userEntry }); + // An attacker pre-plants a sidecar at the deterministic injective path + // holding different content for the same key: `--force` must fail, the + // already-published provider entry must be rolled back to the user's + // value, and the tampered sidecar must not be overwritten. + const plantedPath = replacedProviderBackupPath(fixture.paths.configJson, "gjc"); + await Bun.write(plantedPath, serializeJson({ key: "gjc", value: { evil: true } })); + + const install = await runPaseoSetup({ force: true }, fixture.deps); + expect(install.kind).toBe("install"); + if (install.kind === "install") expect(install.result.outcome).toBe("partial-install"); + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + expect(providersOf(after).gjc).toEqual(userEntry); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.providerKeys.gjc).toBeUndefined(); + // r16: the planted file PREDATES this run (persist refused on it), so + // unpersist never deletes it — a pre-existing sidecar is not ours to + // remove. The user's config entry is still restored and no NEW sidecar + // was created, which is the fail-closed contract. + expect(await fs.readFile(plantedPath, "utf8")).toContain("evil"); + }); + + test("a refused provider publish leaves no replaced-provider sidecar behind (#4644 review r8)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const userEntry = { ...buildProviderEntry([process.execPath, "acp"]), label: "USER EDIT" }; + await seedConfig(fixture.paths, { gjc: userEntry }); + let mutated = false; + const deps: PaseoSetupDependencies = { + ...fixture.deps, + // The bridge preflight runs after config.json was read for ownership + // decisions; a user edit landing in that window must refuse the + // publish AND leave no credential-bearing sidecar behind. + skillsSource: async () => { + if (!mutated) { + mutated = true; + const config = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record< + string, + unknown + >; + config.daemon = { ...(config.daemon as object), port: 9999 }; + await fs.writeFile(fixture.paths.configJson, serializeJson(config), { mode: 0o600 }); + } + return { dir: fixture.paths.agentsSkillsDir as string, origin: "user" }; + }, + }; + + const install = await runPaseoSetup({ force: true }, deps); + expect(install.kind).toBe("install"); + if (install.kind === "install") expect(install.result.outcome).toBe("partial-install"); + const sidecars = (await fs.readdir(path.dirname(fixture.paths.configJson))).filter(name => + name.includes("gjc-replaced"), + ); + expect(sidecars).toEqual([]); + }); + + test("a fresh no-source install records no bridge ownership and never deletes later user content at that path (#4644 review r8)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedConfig(fixture.paths); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => undefined, + }; + + const install = await runPaseoSetup({}, deps); + expect(install.kind).toBe("install"); + if (install.kind !== "install") throw new Error("unreachable"); + expect(install.result.outcome).toBe("installed"); + if (install.result.outcome !== "installed") throw new Error("unreachable"); + expect(install.result.changed).toContain("paseo skills bridge (no source)"); + // Nothing was created, so NOTHING may be recorded: a planned + // `bridgeDirCreated: true` used to let --remove trust false ownership. + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgePath).toBeUndefined(); + expect(ledger.bridgeDirCreated).toBeFalsy(); + expect(ledger.bridgeEntries).toBeUndefined(); + + // The user later creates that exact path with their own work. + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + await Bun.write(path.join(fixture.paths.bridgeDir, "user-file.md"), "# user work\n"); + + const remove = await runPaseoSetup({ remove: true }, deps); + expect(remove.kind).toBe("remove"); + if (remove.kind !== "remove") throw new Error("unreachable"); + expect(["removed", "nothing-to-remove"]).toContain(remove.result.outcome); + // The user's content survived the removal run untouched. + expect(await fs.readFile(path.join(fixture.paths.bridgeDir, "user-file.md"), "utf8")).toBe("# user work\n"); + }); + test("a mid-bridge failure corrects the ledger to observed reality instead of the plan (#4644 review r9)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + const bridgeLedger = await readProvenance(fixture.paths.provenanceLedger); + // Partial failure: the first link was created, the second refused, so + // ownership is the created link only — never the full plan. + await correctBridgeOwnershipAfterFailure(fixture.deps, bridgeLedger, false, { + createdEntries: ["paseo"], + prunedEntries: [], + adoptedEntries: [], + bridgeDirCreated: false, + sourceDir: fixture.paths.agentsSkillsDir, + }); + expect((await readProvenance(fixture.paths.provenanceLedger)).bridgeEntries).toEqual(["paseo"]); + + // Prunes completed before the failure leave the ledger too. + await correctBridgeOwnershipAfterFailure( + fixture.deps, + { ...bridgeLedger, bridgeEntries: ["paseo", "paseo-loop"] }, + false, + { + createdEntries: ["paseo-advisor"], + prunedEntries: ["paseo-loop"], + adoptedEntries: [], + bridgeDirCreated: false, + }, + ); + expect((await readProvenance(fixture.paths.provenanceLedger)).bridgeEntries).toEqual(["paseo", "paseo-advisor"]); + }); + + test("installSkillsBridge failures carry partial results (#4644 review r9)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const preflight = await preflightSkillsBridge(fixture.deps); + expect(preflight.bridgeDirCreated).toBe(true); + // The directory appears after the preflight said absent: creation + // refuses before any link exists, and the failure still carries the + // (empty) partial state callers correct the ledger with. + await fs.mkdir(path.dirname(fixture.paths.bridgeDir), { recursive: true }); + await fs.mkdir(fixture.paths.bridgeDir); + const attempt = await installSkillsBridge(preflight).catch(error => error); + expect(attempt).toBeInstanceOf(SkillsBridgePartialError); + if (attempt instanceof SkillsBridgePartialError) { + expect(attempt.partial.createdEntries).toEqual([]); + expect(attempt.partial.bridgeDirCreated).toBe(false); + } + }); + + test("malformed provenance fields fail closed as corrupt instead of defaulting (#4644 review r9)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + const base = await readProvenance(fixture.paths.provenanceLedger); + const malformed: readonly (readonly [string, unknown])[] = [ + ["providerKeys", "garbage"], + ["providerReplacedEntries", "garbage"], + ["seededOrchestrationKeys", 7], + ["bridgePath", 42], + ["bridgeEntries", "paseo"], + ["bridgeEntries", ["paseo", 7]], + ["bridgeDirCreated", "yes"], + ["version", "one"], + ]; + for (const [field, value] of malformed) { + await writeProvenance(fixture.paths.provenanceLedger, { ...base, [field]: value } as never); + await expect(readProvenance(fixture.paths.provenanceLedger)).rejects.toBeInstanceOf( + ProvenanceLedgerCorruptError, + ); + } + // Absent fields still default: an old-shape ledger reads cleanly. + await Bun.write( + fixture.paths.provenanceLedger, + `${JSON.stringify({ version: 1, providerKeys: { gjc: "abc" } }, null, 2)}\n`, + ); + const legacy = await readProvenance(fixture.paths.provenanceLedger); + expect(legacy.providerKeys).toEqual({ gjc: "abc" }); + expect(legacy.bridgeEntries).toBeUndefined(); + }); + + test("a symlinked sidecar is refused instead of read through (#4644 review r9)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + const real = path.join(fixture.root, "attacker.json"); + await Bun.write(real, `${JSON.stringify({ key: "gjc", value: { evil: true } }, null, 2)}\n`); + const ref = await writeReplacedProviderBackup(fixture.paths.configJson, "gjc", { real: "value" }); + // Swap the sidecar for a symlink at the same path: the read must fail + // closed (never follow), so restoration can never be redirected. + await fs.rm(ref.backupPath); + await fs.symlink(real, ref.backupPath); + const read = await readReplacedProviderBackup(ref.backupPath, "gjc", ref.valueSha256); + expect(read).toEqual({ found: false }); + }); + + test("a dangling off-source bridge link reports exactly one drift reason (#4644 review r9)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + // The recorded link is retargeted at a deleted foreign path: dangling + // AND off-source at once. Before r9 both scan loops reported it. + const foreign = path.join(fixture.root, "gone", "paseo"); + await fs.rm(path.join(fixture.paths.bridgeDir, "paseo")); + await fs.symlink(foreign, path.join(fixture.paths.bridgeDir, "paseo")); + const check = await runPaseoSetup({ check: true }, fixture.deps); + if (check.kind !== "check") throw new Error("expected a check outcome"); + const subjects = check.result.reasons.map(reason => reason.subject); + const duplicates = subjects.filter((subject, index) => subjects.indexOf(subject) !== index); + expect(duplicates).toEqual([]); + expect(check.result.reasons.filter(reason => reason.subject.endsWith("/paseo")).length).toBe(1); + }); + + test("a ledger-commit failure unpersists the sidecar and reverts the publish (#4644 review r10)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const userEntry = { ...buildProviderEntry([process.execPath, "acp"]), label: "USER EDIT" }; + await seedConfig(fixture.paths, { gjc: userEntry }); + // A read-only provenance directory: readProvenance sees ENOENT (an + // empty ledger) before the step, and writeProvenance's temp-file open + // fails EACCES only AFTER the publish + persist succeeded. + const blocker = path.join(fixture.root, "blocker"); + await fs.mkdir(blocker, { recursive: true, mode: 0o500 }); + const sidecarPath = replacedProviderBackupPath(fixture.paths.configJson, "gjc"); + let persisted = false; + + await expect( + runJsonStep({ + label: fixture.paths.configJson, + step: "provider-config", + targetPath: fixture.paths.configJson, + provenancePath: path.join(blocker, "paseo", "provenance.json"), + intentPath: fixture.paths.intentRecord, + ownedKeys: ["agents.providers.gjc"], + expectedPreflightIdentity: (await readTarget(fixture.paths.configJson)).identity, + mutate: draft => { + (draft.agents as { providers: Record }).providers.gjc = { replaced: true }; + }, + nextLedger: ledger => ledger, + revert: draft => { + (draft.agents as { providers: Record }).providers.gjc = userEntry; + }, + revertLedger: ledger => ledger, + persist: async () => { + await writeReplacedProviderBackup(fixture.paths.configJson, "gjc", userEntry); + persisted = true; + }, + unpersist: async () => { + persisted = false; + await fs.rm(sidecarPath, { force: true }); + }, + now: new Date("2026-01-01T00:00:00.000Z"), + }), + ).rejects.toBeInstanceOf(SagaStepError); + expect(persisted).toBe(false); + // The publication was rolled back and no orphaned sidecar survives. + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + expect(providersOf(after).gjc).toEqual(userEntry); + await expect(fs.lstat(sidecarPath)).rejects.toMatchObject({ code: "ENOENT" }); + await expect(fs.lstat(fixture.paths.intentRecord)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + test("non-string provenance record values are corruption, not provenance (#4644 review r10)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + const base = await readProvenance(fixture.paths.provenanceLedger); + for (const [field, value] of [ + ["providerKeys", { gjc: 7 }], + ["seededOrchestrationKeys", { leader: { nested: true } }], + ["providerKeys", { gjc: null }], + ] as const) { + await writeProvenance(fixture.paths.provenanceLedger, { ...base, [field]: value } as never); + await expect(readProvenance(fixture.paths.provenanceLedger)).rejects.toBeInstanceOf( + ProvenanceLedgerCorruptError, + ); + } + }); + + test("a corrupt intent record is a refusal, never absence (#4644 review r10)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await fs.mkdir(path.dirname(fixture.paths.intentRecord), { recursive: true }); + await Bun.write(fixture.paths.intentRecord, "{ this is not json"); + const recovery = await recoverIntent(fixture.paths.intentRecord, { repair: true }); + expect(recovery?.recovered).toBe(false); + expect(recovery?.detail).toContain("corrupt"); + const check = await runPaseoSetup({ check: true }, fixture.deps); + if (check.kind !== "check") throw new Error("expected a check outcome"); + expect(check.result.reasons.map(reason => reason.code)).toContain("partial-install"); + }); + + test("a tampered recorded bridge source outside trusted roots never drives removal (#4644 review r10)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { ...ledger, bridgeSourceDir: "/etc/paseo-skills" }); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + expect(remove.kind).toBe("remove"); + if (remove.kind !== "remove") throw new Error("unreachable"); + expect(remove.result.outcome).toBe("partial-removal"); + if (remove.result.outcome === "partial-removal") { + expect(remove.result.evidence.detail).toContain("outside every trusted Paseo source root"); + // The refusal happens in the pre-settings validation window, so + // the skills.customDirectories registration is NOT unregistered + // first (the bridge-path rule's ordering, mirrored for the source). + expect(remove.result.evidence.retained).toContain(fixture.paths.provenanceLedger); + } + // The bridge links are intact. + for (const name of SKILL_NAMES) { + await fs.lstat(path.join(fixture.paths.bridgeDir, name)); + } + }); + + test("a symlinked skills source records canonically and stays removable (#4644 review r11)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedConfig(fixture.paths); + // The source the resolver returns is a SYMLINK inside the fixture root; + // its canonical target is a sibling directory, also inside the root. + const realSkills = path.join(fixture.root, "real-skills"); + for (const name of ["paseo", "paseo-loop"]) { + await fs.mkdir(path.join(realSkills, name), { recursive: true }); + } + const linkedSkills = path.join(fixture.root, "linked-skills"); + await fs.symlink(realSkills, linkedSkills); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => ({ dir: linkedSkills, origin: "user" }), + }; + + await runPaseoSetup({}, deps); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + // The ledger carries the CANONICAL directory, matching what the + // removal-time trust check resolves. + expect(ledger.bridgeSourceDir).toBe(realSkills); + + const remove = await runPaseoSetup({ remove: true }, deps); + expect(remove.kind).toBe("remove"); + if (remove.kind !== "remove") throw new Error("unreachable"); + expect(remove.result.outcome).toBe("removed"); + await expect(fs.stat(fixture.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + test("a symlinked source passes --check without false drift after install (#4644 review r12)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedConfig(fixture.paths); + const realSkills = path.join(fixture.root, "real-skills"); + for (const name of ["paseo", "paseo-loop"]) { + await fs.mkdir(path.join(realSkills, name), { recursive: true }); + } + const linkedSkills = path.join(fixture.root, "linked-skills"); + await fs.symlink(realSkills, linkedSkills); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => ({ dir: linkedSkills, origin: "user" }), + }; + await runPaseoSetup({}, deps); + + // The drift scan canonicalizes the source exactly like installation, + // so a valid bridge through a symlinked source reports NO drift. + const check = await runPaseoSetup({ check: true }, deps); + if (check.kind !== "check") throw new Error("expected a check outcome"); + expect(check.result.reasons.filter(reason => reason.code.includes("skill"))).toEqual([]); + }); + + test("a tampered intent record redirecting the provenance path is refused (#4644 review r13)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + // A record that keeps its two target fields but points recovery at a + // foreign provenance path must be corruption, not a recovery input. + const foreign = path.join(fixture.root, "foreign", "provenance.json"); + const record = { + version: 1, + step: "provider-config", + targetPath: fixture.paths.configJson, + targetPreflightIdentity: "a".repeat(64), + targetExpectedIdentity: "b".repeat(64), + provenancePath: foreign, + provenancePreflightIdentity: "c".repeat(64), + provenanceExpectedIdentity: "d".repeat(64), + ownedKeys: ["agents.providers.gjc"], + startedAt: "2026-01-01T00:00:00.000Z", + }; + await fs.mkdir(path.dirname(fixture.paths.intentRecord), { recursive: true }); + await Bun.write(fixture.paths.intentRecord, `${JSON.stringify(record, null, 2)}\n`); + await expect(readIntent(fixture.paths.intentRecord)).rejects.toThrow(/escapes the agent directory/); + // Missing any other trusted field is corruption too. + const stripped = { ...record, provenancePath: fixture.paths.provenanceLedger }; + delete (stripped as Record).startedAt; + await Bun.write(fixture.paths.intentRecord, `${JSON.stringify(stripped, null, 2)}\n`); + await expect(readIntent(fixture.paths.intentRecord)).rejects.toThrow(/startedAt/); + }); + + test("quarantine deletion is inode-guarded on the prune path (#4644 review r13)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + // Drop a skill from the source so the next install prunes the recorded + // link through the quarantine path: the prune succeeds when the + // quarantined object keeps its identity (the normal case the inode + // guard must not break). + await fs.rm(path.join(fixture.paths.agentsSkillsDir as string, "paseo-loop"), { recursive: true }); + const result = await installSkillsBridge(await preflightSkillsBridge(fixture.deps)); + expect(result.prunedEntries).toContain("paseo-loop"); + await expect(fs.lstat(path.join(fixture.paths.bridgeDir, "paseo-loop"))).rejects.toMatchObject({ + code: "ENOENT", + }); + // No quarantine residue is left behind. + const residue = (await fs.readdir(fixture.paths.bridgeDir)).filter(name => name.includes("gjc-paseo-quarantine")); + expect(residue).toEqual([]); + }); + + test("a recorded source under an arbitrary home subdirectory is refused (#4644 review r12)", async () => { + const home = getTrustedHomeDir(); + const arbitrary = path.join(home, "not-a-paseo-location"); + const result = await isTrustedRecordedSkillsSource(arbitrary); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.detail).toContain("not a location Paseo skills discovery could produce"); + // The discovery-equivalent roots still pass. + expect((await isTrustedRecordedSkillsSource(path.join(home, ".agents", "skills"))).ok).toBe(true); + }); + + test("a replaced-provider sidecar for an absent key is removed and a failing deletion blocks removal (#4644 review r11)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const userEntry = { ...buildProviderEntry([process.execPath, "acp"]), label: "USER EDIT" }; + await seedConfig(fixture.paths, { gjc: userEntry }); + await runPaseoSetup({ force: true }, fixture.deps); + // The user deletes GJC's entry by hand: removal cannot restore it, but + // the sidecar must not survive as an orphaned credential file. + const config = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + delete (config.agents as { providers: Record }).providers.gjc; + await fs.writeFile(fixture.paths.configJson, serializeJson(config), { mode: 0o600 }); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + expect(remove.kind).toBe("remove"); + if (remove.kind !== "remove") throw new Error("unreachable"); + expect(remove.result.outcome).toBe("removed"); + const leftovers = (await fs.readdir(path.dirname(fixture.paths.configJson))).filter(name => + name.includes("gjc-replaced"), + ); + expect(leftovers).toEqual([]); + }); + + test("the default source-trust rule accepts trusted-home and override sources only (#4644 review r10)", async () => { + const home = getTrustedHomeDir(); + expect(await isTrustedRecordedSkillsSource(path.join(home, ".agents", "skills"))).toEqual({ ok: true }); + expect(await isTrustedRecordedSkillsSource("skills")).toEqual({ + ok: false, + detail: expect.stringContaining("not an absolute path") as unknown as string, + }); + const outside = await isTrustedRecordedSkillsSource("/etc"); + expect(outside.ok).toBe(false); + }); + + test("directory ownership is a fact, not a plan: the creator bit lands only after exclusive creation (#4644 review r8)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + // created:true is recorded only with the directory actually on disk. + expect(ledger.bridgeDirCreated).toBe(true); + expect(await fs.stat(fixture.paths.bridgeDir)).toBeDefined(); + + // The concurrent-creator race between the preflight plan and the + // exclusive mkdir: the directory appears after the preflight said + // absent, so creation must refuse (EEXIST is never ignored) and the + // foreign content stays untouched with no ownership recorded. + const second = await makeFixture(lsOk("gjc")); + await seedSkills(second.paths); + await seedConfig(second.paths); + const planned = await preflightSkillsBridge(second.deps); + expect(planned.bridgeDirCreated).toBe(true); + await fs.mkdir(second.paths.bridgeDir); + await Bun.write(path.join(second.paths.bridgeDir, "paseo"), "user\n"); + await expect(installSkillsBridge(planned)).rejects.toBeInstanceOf(SkillsBridgeError); + expect(await fs.readFile(path.join(second.paths.bridgeDir, "paseo"), "utf8")).toBe("user\n"); + await expect(fs.stat(second.paths.provenanceLedger)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + test("a tampered replaced-provider backup path never reads or deletes a foreign file (#4644 red-team)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + const userEntry = { ...buildProviderEntry([process.execPath, "acp"]), label: "USER EDIT" }; + await seedConfig(fixture.paths, { gjc: userEntry }); + await runPaseoSetup({ force: true }, fixture.deps); + + // Tamper: the ledger pointer names an arbitrary absolute JSON file whose + // key matches, so the restore would both write its content into + // agents.providers and delete it after "restoring". + const victim = path.join(fixture.root, "victim.json"); + const victimValue = { label: "ATTACKER" }; + await fs.writeFile(victim, `${JSON.stringify({ key: "gjc", value: victimValue }, null, 2)}\n`); + const victimDigest = hashBytes(serializeJson(victimValue)); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { + ...ledger, + providerReplacedEntries: { gjc: { backupPath: victim, valueSha256: victimDigest } }, + }); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("partial-removal"); + if (remove.result.outcome === "partial-removal") { + expect(remove.result.evidence.detail).toContain("unexpected path"); + } + // The foreign file is intact and its content never reached the providers. + expect(await fs.readFile(victim, "utf8")).toContain("ATTACKER"); + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + expect(JSON.stringify((after.agents as { providers: Record }).providers)).not.toContain( + "ATTACKER", + ); + }); + + test("a foreign in-root bridge-shaped sibling is refused even with a bridge basename (#4644 review r19)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + // A sibling directory with the bridge family basename holding user + // content, plus a ledger aimed at it: removal must refuse (it is + // neither the configured bridge nor an authenticated migration) and + // the user content survives. + const sibling = path.join(path.dirname(fixture.paths.bridgeDir), "fake-paseo-skills"); + await fs.mkdir(sibling, { recursive: true }); + await Bun.write(path.join(sibling, "user.txt"), "user content\n"); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { ...ledger, bridgePath: sibling }); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + expect(remove.kind).toBe("remove"); + if (remove.kind !== "remove") throw new Error("unreachable"); + expect(remove.result.outcome).toBe("partial-removal"); + if (remove.result.outcome === "partial-removal") { + expect(remove.result.evidence.detail).toContain("neither the configured bridge"); + } + expect(await fs.readFile(path.join(sibling, "user.txt"), "utf8")).toBe("user content\n"); + }); + + test("a migration replaces the old custom-directory registration atomically (#4644 review r6)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + const oldDir = fixture.paths.bridgeDir; + + // The ledger stays in place; only the bridge path moves (a relocation + // inside the same agent directory, where the ledger still records the + // old path). + const newBridge = path.join(path.dirname(oldDir), "relocated-paseo-skills"); + const migratedDeps: PaseoSetupDependencies = { + ...fixture.deps, + paths: { + ...fixture.deps.paths, + bridgeDir: newBridge, + }, + }; + const install = await runPaseoSetup({}, migratedDeps); + expect(install.kind).toBe("install"); + if (install.kind !== "install") throw new Error("expected an install outcome"); + expect(install.result.outcome).toBe("installed"); + + // The new path is registered and the old one is gone in the same swap. + const settings = await fs.readFile(path.join(process.env.GJC_CODING_AGENT_DIR ?? "", "config.yml"), "utf8"); + expect(settings).toContain(newBridge); + expect(settings).not.toContain(oldDir); + // The old bridge is cleaned after the durable cutover. + for (const name of ["paseo", "paseo-advisor"]) { + await expect(fs.lstat(path.join(oldDir, name))).rejects.toMatchObject({ code: "ENOENT" }); + } + }); + + test("a tampered ledger path refuses removal before any settings mutation (#4644 review r6)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + + const victimDir = path.join(fixture.root, "victim"); + await fs.mkdir(victimDir, { recursive: true }); + await fs.symlink(path.join(fixture.paths.agentsSkillsDir as string, "paseo"), path.join(victimDir, "paseo")); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { ...ledger, bridgePath: victimDir }); + + const settingsBefore = await snapshotTree(path.dirname(fixture.paths.provenanceLedger)); + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("partial-removal"); + // The registration and provider entries were NOT touched: validation + // failed before any settings mutation. + const config = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + const providers = (config.agents as { providers: Record }).providers; + expect(providers.gjc).toBeDefined(); + expect(await snapshotTree(path.dirname(fixture.paths.provenanceLedger))).toBe(settingsBefore); + }); + + test("a resolved source replaced by a regular file refuses instead of pruning (#4644 review r6)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + const before = await snapshotTree(fixture.paths.bridgeDir); + + // An app update replaces the skills directory with a file. + await fs.rm(fixture.paths.agentsSkillsDir as string, { recursive: true, force: true }); + await fs.writeFile(fixture.paths.agentsSkillsDir as string, "not a directory\n"); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => ({ dir: fixture.paths.agentsSkillsDir as string, origin: "user" }), + }; + + await expect(runPaseoSetup({}, deps)).rejects.toBeInstanceOf(SkillsBridgeError); + expect(await snapshotTree(fixture.paths.bridgeDir)).toBe(before); + }); + + test("a convergence over an owned empty bridge preserves its provenance and registration (#4644 review r6)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + for (const name of [...SKILL_NAMES]) { + await fs.rm(path.join(fixture.paths.agentsSkillsDir as string, name), { recursive: true }); + } + // First convergence: entries pruned, directory GJC-created and owned. + await runPaseoSetup({}, fixture.deps); + let ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeDirCreated).toBe(true); + + // Second convergence over the still-empty source must NOT discard the + // ownership record: the directory and its registration stay removable. + await runPaseoSetup({}, fixture.deps); + ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeDirCreated).toBe(true); + expect(ledger.bridgePath).toBe(fixture.paths.bridgeDir); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("removed"); + await expect(fs.stat(fixture.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + }); + test("remove after every entry was pruned still cleans the recorded-created empty bridge (#4644 review r4)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + // A Paseo release drops EVERY skill: convergence prunes the final entry + // and leaves `bridgeEntries: []` with `bridgeDirCreated: true`. + for (const name of [...SKILL_NAMES]) { + await fs.rm(path.join(fixture.paths.agentsSkillsDir as string, name), { recursive: true }); + } + const converged = await runPaseoSetup({}, fixture.deps); + expect(converged.kind).toBe("install"); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeEntries).toEqual([]); + expect(ledger.bridgeDirCreated).toBe(true); + // The empty directory GJC created must not be stranded. + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("removed"); + await expect(fs.stat(fixture.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + test("a tampered ledger bridge path never drives destructive removal (#4644 review r4)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + // Tamper: the ledger points outside the agent directory at a directory + // holding a foreign link with a matching name. + const victimDir = path.join(fixture.root, "victim"); + await fs.mkdir(victimDir, { recursive: true }); + await fs.symlink(path.join(fixture.paths.agentsSkillsDir as string, "paseo"), path.join(victimDir, "paseo")); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { ...ledger, bridgePath: victimDir }); + + const result = await removePaseoSetup(fixture.deps, { now: new Date() }); + expect(result.outcome).toBe("partial-removal"); + if (result.outcome !== "partial-removal") throw new Error("unreachable"); + expect(result.evidence.detail).toMatch(/escapes the agent directory|does not carry the bridge directory name/); + // The foreign link is untouched. + expect((await fs.lstat(path.join(victimDir, "paseo"))).isSymbolicLink()).toBe(true); + }); + + test("a ledger bridge path that is a symlink is refused before removal (#4644 review r4)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + const real = path.join(fixture.root, "real-bridge"); + await fs.mkdir(real, { recursive: true }); + await fs.symlink(real, path.join(fixture.root, "agentdir", "paseo-skills-link")); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { + ...ledger, + bridgePath: path.join(fixture.root, "agentdir", "paseo-skills-link"), + }); + + const result = await removePaseoSetup(fixture.deps, { now: new Date() }); + expect(result.outcome).toBe("partial-removal"); + if (result.outcome !== "partial-removal") throw new Error("unreachable"); + expect(result.evidence.detail).toMatch(/symlink|does not carry the bridge directory name/); + }); + + test("a ledger path through a symlinked ancestor resolves outside and is refused (#4644 review r7)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + // A canonically spelled record INSIDE the agent directory whose ancestor + // is a symlink: `lstat` on the final component alone accepts it, but the + // fully resolved location is an unrelated directory holding a foreign + // link with a matching name. + const evil = path.join(fixture.root, "evil"); + await fs.mkdir(evil, { recursive: true }); + await fs.symlink(evil, path.join(path.dirname(fixture.paths.bridgeDir), "ancestor-link")); + const recorded = path.join(path.dirname(fixture.paths.bridgeDir), "ancestor-link", "paseo-skills"); + await fs.mkdir(recorded, { recursive: true }); + await fs.symlink(path.join(fixture.paths.agentsSkillsDir as string, "paseo"), path.join(recorded, "paseo")); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { ...ledger, bridgePath: recorded }); + + const result = await removePaseoSetup(fixture.deps, { now: new Date() }); + expect(result.outcome).toBe("partial-removal"); + if (result.outcome !== "partial-removal") throw new Error("unreachable"); + expect(result.evidence.detail).toMatch( + /resolves outside the agent directory|neither the configured bridge .* nor an authenticated migration record/, + ); + // The foreign link in the redirected directory is untouched. + expect((await fs.lstat(path.join(recorded, "paseo"))).isSymbolicLink()).toBe(true); + }); + + test("a recorded entry replaced by a regular file is a divergence, not silent success (#4644 review r4)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + // The user replaces a bridged name with their own regular file. + await fs.rm(path.join(fixture.paths.bridgeDir, "paseo")); + await fs.writeFile(path.join(fixture.paths.bridgeDir, "paseo"), "user data\n"); + + const result = await removePaseoSetup(fixture.deps, { now: new Date() }); + expect(result.outcome).toBe("partial-removal"); + // The user's file survives and the provenance is retained. + expect(await fs.readFile(path.join(fixture.paths.bridgeDir, "paseo"), "utf8")).toBe("user data\n"); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeEntries).toContain("paseo"); + }); + + test("no resolved source and no ownership record refuses to register the bridge (#4644 review r4)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedConfig(fixture.paths); + // A stale foreign bridge directory exists; no source resolves and no + // ledger records ownership of it. + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + await fs.writeFile(path.join(fixture.paths.bridgeDir, "paseo-foreign"), "stale\n"); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => undefined, + }; + + const install = await runPaseoSetup({}, deps); + expect(install.kind).toBe("install"); + if (install.kind !== "install") throw new Error("unreachable"); + expect(install.result.outcome).toBe("partial-install"); + if (install.result.outcome === "partial-install") { + expect(install.result.evidence.detail).toContain("Refusing to register"); + } + // The stale bridge content was never touched. + expect(await fs.readFile(path.join(fixture.paths.bridgeDir, "paseo-foreign"), "utf8")).toBe("stale\n"); + }); + + test("no source and no bridge directory completes the provider install without the bridge (#4644 review r7)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedConfig(fixture.paths); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => undefined, + }; + + const install = await runPaseoSetup({}, deps); + expect(install.kind).toBe("install"); + if (install.kind !== "install") throw new Error("unreachable"); + // Paseo not being installed is not a failed GJC provider install: the + // provider entry and orchestration roles stand, the bridge is skipped, + // and nothing is registered (there is no directory to load). + expect(install.result.outcome).toBe("installed"); + if (install.result.outcome !== "installed") throw new Error("unreachable"); + expect(install.result.changed).toContain("paseo skills bridge (no source)"); + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + expect(providersOf(after).gjc).toBeDefined(); + await expect(fs.stat(fixture.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + + // Check still reports the missing source honestly. + const check = await runPaseoSetup({ check: true }, deps); + if (check.kind !== "check") throw new Error("expected a check outcome"); + expect(check.result.reasons.map(reason => reason.code)).toContain("missing-skills-directory"); + }); + + test("a config.json edit between preflight and publish refuses instead of using stale ownership (#4644 review r7)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + let mutated = false; + const deps: PaseoSetupDependencies = { + ...fixture.deps, + // The bridge preflight runs AFTER config.json was read for ownership + // decisions: a user edit landing in that window must refuse rather + // than publish provider ownership computed from the older bytes. + skillsSource: async () => { + if (!mutated) { + mutated = true; + await fs.writeFile( + fixture.paths.configJson, + serializeJson({ agents: { providers: { claude: { enabled: true }, gjc: "user scalar" } } }), + "utf8", + ); + } + return { dir: fixture.paths.agentsSkillsDir as string, origin: "user" }; + }, + }; + + const install = await runPaseoSetup({}, deps); + expect(install.kind).toBe("install"); + if (install.kind !== "install") throw new Error("unreachable"); + expect(install.result.outcome).toBe("partial-install"); + if (install.result.outcome === "partial-install") { + expect(install.result.evidence.detail).toContain("changed after setup inspected it"); + } + // The user's concurrent bytes are intact and were never claimed. + const after = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + expect(providersOf(after).gjc).toBe("user scalar"); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.providerKeys.gjc).toBeUndefined(); + }); + + test("a symlinked source skill directory is bridged after resolution (#4644 review r4)", async () => { + const fixture = await makeFixture(); + const realSkill = path.join(fixture.root, "real-skills", "paseo-linked"); + await fs.mkdir(realSkill, { recursive: true }); + await Bun.write(path.join(realSkill, "SKILL.md"), "# real\n"); + // The source directory contains a SYMLINK to a skill directory. + await fs.symlink(realSkill, path.join(fixture.paths.agentsSkillsDir as string, "paseo-linked")); + // And a dangling symlink of the same shape is not bridged. + await fs.symlink( + path.join(fixture.root, "gone"), + path.join(fixture.paths.agentsSkillsDir as string, "paseo-dangling"), + ); + + const preflight = await preflightSkillsBridge(fixture.deps); + const names = [...Object.keys(preflight.entries)]; + expect(names).toContain("paseo-linked"); + expect(names).not.toContain("paseo-dangling"); + }); + + test("a bridge-path migration cleans the old directory and never inherits its names (#4644 review r4)", async () => { + const fixture = await makeFixture(lsOk("gjc")); + await seedSkills(fixture.paths); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + const oldDir = fixture.paths.bridgeDir; + const oldLedger = await readProvenance(fixture.paths.provenanceLedger); + + // The agent dir moves; a USER link already sits at the new path. + const newBridge = path.join(fixture.root, "agentdir-new", "paseo-skills"); + await fs.mkdir(newBridge, { recursive: true }); + await fs.symlink(path.join(fixture.paths.agentsSkillsDir as string, "paseo"), path.join(newBridge, "paseo")); + const migratedDeps: PaseoSetupDependencies = { + ...fixture.deps, + paths: { ...fixture.deps.paths, bridgeDir: newBridge }, + }; + + const install = await runPaseoSetup({}, migratedDeps); + expect(install.kind).toBe("install"); + // The old directory's links were cleaned, not abandoned. + for (const name of oldLedger.bridgeEntries ?? []) { + await expect(fs.lstat(path.join(oldDir, name))).rejects.toMatchObject({ code: "ENOENT" }); + } + // The new ledger owns only the links the new run created: the user's + // pre-existing `paseo` link at the new path did NOT inherit ownership. + const ledger = await readProvenance(migratedDeps.paths.provenanceLedger); + expect(ledger.bridgePath).toBe(newBridge); + expect(ledger.bridgeEntries).not.toContain("paseo"); + expect(ledger.bridgeEntries).toContain("paseo-advisor"); + }); + + test(".env.local and NODE_ENV variants are rejected for PASEO_SKILLS_DIR (#4644 review r4)", async () => { + const root = await makeRoot(); + const home = path.join(root, "home"); + const userDir = path.join(home, ".agents", "skills"); + await fs.mkdir(userDir, { recursive: true }); + const repoDir = path.join(root, "repo"); + await fs.mkdir(repoDir, { recursive: true }); + const priorCwd = process.cwd(); + const priorEnv = process.env.PASEO_SKILLS_DIR; + const priorNodeEnv = process.env.NODE_ENV; + try { + for (const file of [".env.local", ".env.production", ".env.production.local"]) { + await Bun.write(path.join(repoDir, file), "PASEO_SKILLS_DIR=/some/override\n"); + } + process.chdir(repoDir); + process.env.NODE_ENV = "production"; + process.env.PASEO_SKILLS_DIR = "/some/override"; + await expect(resolvePaseoSkillsSource(home)).resolves.toEqual({ dir: userDir, origin: "user" }); + } finally { + process.chdir(priorCwd); + if (priorEnv === undefined) delete process.env.PASEO_SKILLS_DIR; + else process.env.PASEO_SKILLS_DIR = priorEnv; + if (priorNodeEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = priorNodeEnv; + } + }); + test(".env.development is rejected for PASEO_SKILLS_DIR with NODE_ENV unset (#4644 review r8)", async () => { + const root = await makeRoot(); + const home = path.join(root, "home"); + const userDir = path.join(home, ".agents", "skills"); + await fs.mkdir(userDir, { recursive: true }); + const repoDir = path.join(root, "repo"); + await fs.mkdir(repoDir, { recursive: true }); + const priorCwd = process.cwd(); + const priorEnv = process.env.PASEO_SKILLS_DIR; + const priorNodeEnv = process.env.NODE_ENV; + try { + // Bun's mode defaults to development, so `.env.development` loads + // even with NODE_ENV unset (the common interactive case); the trust + // check must reject an override sourced from it. + await Bun.write(path.join(repoDir, ".env.development"), "PASEO_SKILLS_DIR=/some/override\n"); + process.chdir(repoDir); + delete process.env.NODE_ENV; + process.env.PASEO_SKILLS_DIR = "/some/override"; + await expect(resolvePaseoSkillsSource(home)).resolves.toEqual({ dir: userDir, origin: "user" }); + } finally { + process.chdir(priorCwd); + if (priorEnv === undefined) delete process.env.PASEO_SKILLS_DIR; + else process.env.PASEO_SKILLS_DIR = priorEnv; + if (priorNodeEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = priorNodeEnv; + } + }); + + test("a pre-#4638 bridge pointing at the legacy source is adopted, not refused (#4644 review)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + // The legacy machine: allowlist links into ~/.agents/skills, ledger + // without bridgeSourceDir. The source still resolves to the same + // directory, so adoption is a no-op re-point of the same target. + await installWithLedger(fixture.deps); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + await writeProvenance(fixture.paths.provenanceLedger, { + ...ledger, + bridgeSourceDir: undefined, + }); + + const preflight = await preflightSkillsBridge(fixture.deps); + // Same-directory legacy links are already correct: nothing to adopt. + expect(preflight.adopts).toEqual([]); + }); + + test("a legacy ledger with links into a retired ~/.agents/skills converges onto the app bundle (#4644 review)", async () => { + // The exact wedged state from #4638: five allowlist links into a + // ~/.agents/skills that never existed, a desktop app now present, and a + // legacy ledger without bridgeSourceDir. + const fixture = await makeFixture(lsOk("gjc")); + await fs.rm(fixture.paths.agentsSkillsDir as string, { recursive: true }); + const legacySource = fixture.paths.agentsSkillsDir as string; + const bundle = path.join(fixture.root, "Applications", "Paseo.app", "Contents", "Resources", "skills"); + for (const name of ["paseo", "paseo-help"]) { + await fs.mkdir(path.join(bundle, name), { recursive: true }); + await fs.writeFile(path.join(bundle, name, "SKILL.md"), `# ${name}\n`); + } + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + for (const name of SKILL_NAMES) { + await fs.symlink(path.join(legacySource, name), path.join(fixture.paths.bridgeDir, name)); + } + await writeProvenance(fixture.paths.provenanceLedger, { + version: 1, + providerKeys: {}, + seededOrchestrationKeys: {}, + bridgePath: fixture.paths.bridgeDir, + bridgeEntries: [...SKILL_NAMES], + bridgeDirCreated: false, + }); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => ({ dir: bundle, origin: "app-bundle" }), + }; + + // Check reports the wedge (dangling legacy links) instead of passing. + const drifted = await checkPaseoSetup(deps); + expect(drifted.status).toBe("drift"); + + // Re-running setup converges: adopt paseo, prune the retired names, + // create paseo-help, and record the discovered source directory. + const install = await runPaseoSetup({}, deps); + expect(install.kind).toBe("install"); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect([...(ledger.bridgeEntries ?? [])].sort()).toEqual(["paseo", "paseo-help"]); + expect(ledger.bridgeSourceDir).toBe(bundle); + const linked = (await fs.readdir(fixture.paths.bridgeDir)).sort(); + expect(linked).toEqual(["paseo", "paseo-help"]); + for (const name of linked) { + expect(await fs.readlink(path.join(fixture.paths.bridgeDir, name))).toBe(path.join(bundle, name)); + } + + const result = await checkPaseoSetup(deps); + expect(result.status).toBe("pass"); + expect(checkExitCode(result)).toBe(0); + }); + + test("remove rolls back a legacy ledger with no recorded source directory (#4644 review)", async () => { + // The same wedged machine, exercising --remove directly: the ledger + // predates bridgeSourceDir, so ownership is proven against the legacy + // ~/.agents/skills location rather than a re-discovered source. + const fixture = await makeFixture(lsOk("gjc")); + const legacySource = fixture.paths.agentsSkillsDir as string; + await fs.mkdir(fixture.paths.bridgeDir, { recursive: true }); + for (const name of SKILL_NAMES) { + await fs.symlink(path.join(legacySource, name), path.join(fixture.paths.bridgeDir, name)); + } + await writeProvenance(fixture.paths.provenanceLedger, { + version: 1, + providerKeys: {}, + seededOrchestrationKeys: {}, + bridgePath: fixture.paths.bridgeDir, + bridgeEntries: [...SKILL_NAMES], + bridgeDirCreated: true, + }); + // No source can be discovered anymore (the app is gone), and the legacy + // ledger has no bridgeSourceDir: removal must still prove ownership. + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => undefined, + }; + + const result = await removePaseoSetup(deps, { now: new Date() }); + expect(result.outcome).toBe("removed"); + // Every legacy link is gone and the bridge directory was removed. + await expect(fs.stat(deps.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + test("a non-ENOENT filesystem failure fails removal closed instead of reporting success (#4644 review)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + + // Simulate a permission failure on the bridge directory: the recorded + // entry is still on disk, but lstat cannot traverse to it. Removal must + // report partial-removal and retain the ledger, never claim success. + // chmod 000 blocks traversal with EACCES on the real syscall surface, + // which is exactly the errno class the review asked to keep distinct. + await fs.chmod(fixture.paths.bridgeDir, 0o000); + let result: PaseoRemoveResult; + try { + result = await removePaseoSetup(fixture.deps, { now: new Date() }); + } finally { + await fs.chmod(fixture.paths.bridgeDir, 0o755); + } + expect(result.outcome).toBe("partial-removal"); + if (result.outcome !== "partial-removal") throw new Error("unreachable"); + expect(result.evidence.detail).toContain("EACCES"); + + // The owned link still exists and the ledger still records it. + expect((await fs.lstat(path.join(fixture.paths.bridgeDir, "paseo"))).isSymbolicLink()).toBe(true); + const ledger = await readProvenance(fixture.paths.provenanceLedger); + expect(ledger.bridgeEntries).toContain("paseo"); + }); + + test("both protected skill trees are byte-identical across install and check (AC-8, AC-19)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths, ["context-search"]); + await fs.writeFile(path.join(fixture.paths.gjcSkillsDir, "mine.md"), "# mine\n"); + + const agentsBefore = await snapshotTree(fixture.paths.agentsSkillsDir as string); + const gjcBefore = await snapshotTree(fixture.paths.gjcSkillsDir); + + await installSkillsBridge(await preflightSkillsBridge(fixture.deps)); + await checkPaseoSetup(fixture.deps); + + expect(await snapshotTree(fixture.paths.agentsSkillsDir as string)).toBe(agentsBefore); + expect(await snapshotTree(fixture.paths.gjcSkillsDir)).toBe(gjcBefore); + }); +}); + +describe("skills source discovery (#4638)", () => { + async function discoveryRoot(): Promise { + const root = await makeRoot(); + return root; + } + + test("PASEO_SKILLS_DIR wins when it points at a real directory", async () => { + const root = await discoveryRoot(); + const home = path.join(root, "home"); + const relocated = path.join(root, "Elsewhere", "Paseo.app", "Contents", "Resources", "skills"); + await fs.mkdir(relocated, { recursive: true }); + const prior = process.env.PASEO_SKILLS_DIR; + process.env.PASEO_SKILLS_DIR = relocated; + try { + await expect(resolvePaseoSkillsSource(home)).resolves.toEqual({ dir: relocated, origin: "app-bundle" }); + } finally { + if (prior === undefined) delete process.env.PASEO_SKILLS_DIR; + else process.env.PASEO_SKILLS_DIR = prior; + } + }); + + test("a stale or relative PASEO_SKILLS_DIR is ignored, never linked into", async () => { + const root = await discoveryRoot(); + const home = path.join(root, "home"); + const userDir = path.join(home, ".agents", "skills"); + await fs.mkdir(userDir, { recursive: true }); + for (const value of [path.join(root, "gone"), "relative/skills"]) { + const prior = process.env.PASEO_SKILLS_DIR; + process.env.PASEO_SKILLS_DIR = value; + try { + await expect(resolvePaseoSkillsSource(home)).resolves.toEqual({ dir: userDir, origin: "user" }); + } finally { + if (prior === undefined) delete process.env.PASEO_SKILLS_DIR; + else process.env.PASEO_SKILLS_DIR = prior; + } + } + }); + + test("~/.agents/skills wins over an app bundle; nothing resolvable means undefined", async () => { + const root = await discoveryRoot(); + const home = path.join(root, "home"); + const userDir = path.join(home, ".agents", "skills"); + await fs.mkdir(userDir, { recursive: true }); + await expect(resolvePaseoSkillsSource(home)).resolves.toEqual({ dir: userDir, origin: "user" }); + // A home with no ~/.agents/skills and no bundle resolves to nothing. The + // bundle candidates are platform-bounded, so this holds everywhere. + await expect(resolvePaseoSkillsSource(path.join(root, "empty-home"))).resolves.toBeUndefined(); + }); + + test("app bundle candidates are bounded and platform-shaped", () => { + const home = "/Users/tester"; + const candidates = paseoAppSkillsCandidates(home); + if (process.platform === "darwin") { + expect(candidates.length).toBe(6); + expect(candidates[0]).toBe(path.join("/Applications", "Paseo.app", "Contents", "Resources", "skills")); + expect(candidates).toContain(path.join(home, "Applications", "Paseo.app", "Contents", "Resources", "skills")); + } else { + expect(candidates).toEqual([]); + } + }); + + test("default Paseo home paths resolve through the trusted home, never $HOME (#4644 review r7)", async () => { + const root = await discoveryRoot(); + const attacker = path.join(root, "attacker-home"); + await fs.mkdir(path.join(attacker, ".agents", "skills", "paseo-evil"), { recursive: true }); + const trusted = getTrustedHomeDir(); + + const prior = process.env.HOME; + process.env.HOME = attacker; + try { + // A repository (or shell) that plants $HOME must not redirect global + // Paseo discovery at attacker-controlled skill content: every default + // home path derives from the provenance-checked account home. + const deps = createDefaultPaseoSetupDependencies(); + const depsHome = createDefaultPaseoSetupDependencies().home; + expect(depsHome).toBe(trusted); + expect(depsHome?.startsWith(attacker)).toBe(false); + expect(deps.paths.configJson.startsWith(trusted)).toBe(true); + expect(deps.paths.orchestrationPreferences.startsWith(trusted)).toBe(true); + const candidates = paseoAppSkillsCandidates(); + for (const candidate of candidates) { + expect(candidate.startsWith(attacker)).toBe(false); + } + await expect(resolvePaseoSkillsSource()).resolves.not.toEqual({ dir: attacker, origin: "user" }); + } finally { + if (prior === undefined) delete process.env.HOME; + else process.env.HOME = prior; + } + }); +}); + +describe("desktop app install (#4638)", () => { + const APP_SKILLS = ["paseo", "paseo-advisor", "paseo-committee", "paseo-handoff", "paseo-help"]; + + /** `runPaseoSetup` narrowed to the check arm, for readable assertions. */ + async function check(deps: PaseoSetupDependencies): Promise { + const outcome = await runPaseoSetup({ check: true }, deps); + if (outcome.kind !== "check") throw new Error("expected a check outcome"); + return outcome.result; + } + + /** The reported machine: Paseo.app 0.4.0 ships paseo-help, not paseo-loop, and there is no ~/.agents/skills. */ + async function appFixture(skillNames: readonly string[]): Promise { + const fixture = await makeFixture(lsOk("gjc")); + await fs.rm(fixture.paths.agentsSkillsDir as string, { recursive: true }); + const bundle = path.join(fixture.root, "Applications", "Paseo.app", "Contents", "Resources", "skills"); + for (const name of skillNames) { + await fs.mkdir(path.join(bundle, name), { recursive: true }); + await fs.writeFile(path.join(bundle, name, "SKILL.md"), `# ${name}\n`); + } + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => ({ dir: bundle, origin: "app-bundle" }), + }; + return { ...fixture, deps }; + } + + test("install bridges the bundle's skills and check reaches pass", async () => { + const fixture = await appFixture(APP_SKILLS); + await seedConfig(fixture.paths); + + const install = await runPaseoSetup({}, fixture.deps); + expect(install.kind).toBe("install"); + + const result = await check(fixture.deps); + expect(result.status).toBe("pass"); + expect(checkExitCode(result)).toBe(0); + + // The exact set the app ships is bridged -- nothing more, nothing less. + const linked = (await fs.readdir(fixture.paths.bridgeDir)).sort(); + expect(linked).toEqual([...APP_SKILLS].sort()); + for (const name of linked) { + const target = await fs.readlink(path.join(fixture.paths.bridgeDir, name)); + expect(path.dirname(target)).toBe( + path.join(fixture.root, "Applications", "Paseo.app", "Contents", "Resources", "skills"), + ); + } + }); + + test("a missing source directory skips the bridge, creates nothing, and reports it once", async () => { + const fixture = await appFixture([]); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => undefined, + }; + await seedConfig(fixture.paths); + + const install = await runPaseoSetup({}, deps); + expect(install.kind).toBe("install"); + await expect(fs.stat(deps.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + + const result = await check(deps); + expect(result.status).toBe("drift"); + const codes = result.reasons.map(reason => reason.code); + expect(codes).toContain("missing-skills-directory"); + expect(codes).not.toContain("missing-bridge-link"); + expect(codes).not.toContain("orphan-skill"); + }); + + test("a source that disappears during enumeration fails closed", async () => { + const fixture = await makeFixture(lsOk("gjc")); + const source = path.join(fixture.root, "paseo-skills-source"); + await fs.mkdir(source, { recursive: true }); + await fs.rm(source, { recursive: true }); + + await expect(sourceBridgeEntries(source)).rejects.toMatchObject({ code: "ENOENT" }); + + const nonDirectory = path.join(fixture.root, "paseo-skills-file"); + await fs.writeFile(nonDirectory, "not a directory\n"); + await expect(sourceBridgeEntries(nonDirectory)).rejects.toMatchObject({ code: "ENOTDIR" }); + }); + + test("a source skill with no bridge link is drift, and re-running setup repairs it", async () => { + const fixture = await appFixture(SKILL_NAMES); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + await fs.rm(path.join(fixture.paths.bridgeDir, "paseo-advisor")); + + const drifted = await check(fixture.deps); + expect(drifted.status).toBe("drift"); + expect(drifted.reasons).toEqual([ + { + code: "missing-bridge-link", + subject: path.join(fixture.paths.bridgeDir, "paseo-advisor"), + detail: expect.any(String), + }, + ]); + + await runPaseoSetup({}, fixture.deps); + expect((await check(fixture.deps)).status).toBe("pass"); + }); + + test("a Paseo release adding a skill never turns check red", async () => { + const fixture = await appFixture(APP_SKILLS); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + + // The app updates underneath GJC and ships one extra skill. + const bundle = path.join(fixture.root, "Applications", "Paseo.app", "Contents", "Resources", "skills"); + await fs.mkdir(path.join(bundle, "paseo-brand-new")); + + const result = await check(fixture.deps); + expect(result.status).toBe("pass"); + expect(checkExitCode(result)).toBe(0); + }); + + test("repeated install, check, and remove converge and preserve the foreign provider", async () => { + const fixture = await appFixture(APP_SKILLS); + await seedConfig(fixture.paths); + + await runPaseoSetup({}, fixture.deps); + const again = await runPaseoSetup({}, fixture.deps); + expect(again.kind).toBe("install"); + let result = await check(fixture.deps); + expect(result.status).toBe("pass"); + + const remove = await runPaseoSetup({ remove: true }, fixture.deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("removed"); + await expect(fs.stat(fixture.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + + // The foreign provider entry survives every pass untouched. + const config = JSON.parse(await fs.readFile(fixture.paths.configJson, "utf8")) as Record; + const providers = (config.agents as { providers: Record }).providers; + expect(Object.keys(providers).sort()).toEqual(["claude"]); + + // And a fresh install on top of the rolled-back state is green again. + await runPaseoSetup({}, fixture.deps); + result = await check(fixture.deps); + expect(result.status).toBe("pass"); + }); + + test("remove still cleans the bridge after Paseo itself is uninstalled (#4638)", async () => { + const fixture = await appFixture(APP_SKILLS); + await seedConfig(fixture.paths); + await runPaseoSetup({}, fixture.deps); + + // Paseo disappears entirely: the app bundle is gone, so every bridge link + // dangles and no source can be discovered anymore. + await fs.rm(path.join(fixture.root, "Applications"), { recursive: true }); + const deps: PaseoSetupDependencies = { + ...fixture.deps, + skillsSource: async () => undefined, + }; + + const remove = await runPaseoSetup({ remove: true }, deps); + if (remove.kind !== "remove") throw new Error("expected a remove outcome"); + expect(remove.result.outcome).toBe("removed"); + await expect(fs.stat(deps.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + + const config = JSON.parse(await fs.readFile(deps.paths.configJson, "utf8")) as Record; + const providers = (config.agents as { providers: Record }).providers; + expect(Object.keys(providers).sort()).toEqual(["claude"]); }); }); From 6ea814cf9a424962f7748c68de27ed299274e4d1 Mon Sep 17 00:00:00 2001 From: gaebal-gajae Date: Sun, 23 Aug 2026 01:40:06 +0000 Subject: [PATCH 2/2] fix(setup): bind Paseo bridge removal to installed links Paseo removal now requires the recorded no-follow link identity and delegates symlink cleanup to a native identity-bound unlink protocol.\n\nThis preserves same-target successors and closes JavaScript pathname races found by the exact-head review.\n\nLore-id: pr4644-r21\nConstraint: no release mutation\nTested: setup-paseo suite; native path-identity POSIX suite; native baseline and modern builds\nConfidence: high\nScope-risk: focused\nReversibility: revert --- crates/pi-natives/src/path_identity.rs | 407 +++++++++++++++++- .../src/setup/paseo/paseo-ownership.ts | 35 ++ .../src/setup/paseo/paseo-setup.ts | 20 + .../coding-agent/src/setup/paseo/remove.ts | 9 +- .../src/setup/paseo/skills-bridge.ts | 216 ++++------ .../coding-agent/test/setup-paseo.test.ts | 48 ++- packages/natives/native/index.d.ts | 18 +- packages/natives/native/index.js | 1 + 8 files changed, 611 insertions(+), 143 deletions(-) diff --git a/crates/pi-natives/src/path_identity.rs b/crates/pi-natives/src/path_identity.rs index ab458dccdc..b5e0a186f0 100644 --- a/crates/pi-natives/src/path_identity.rs +++ b/crates/pi-natives/src/path_identity.rs @@ -303,7 +303,8 @@ struct ExactFileIdentity { sha256: Option<[u8; 32]>, allow_hard_link: bool, } -/// Typed result of an identity-bound regular-file deletion or directory detach. +/// Typed result of an identity-bound regular-file/symlink deletion or directory +/// detach. #[napi(object)] pub struct NativeExactUnlinkResult { pub ok: bool, @@ -875,6 +876,18 @@ pub(crate) fn digest_reader(reader: &mut impl Read) -> io::Result<[u8; 32]> { } fn exact_file_identity(identity: &NativeExactFileIdentity) -> Option { + exact_file_identity_with_digest(identity, true) +} + +fn exact_symlink_identity(identity: &NativeExactFileIdentity) -> Option { + let identity = exact_file_identity_with_digest(identity, false)?; + (!identity.directory && !identity.detach_only).then_some(identity) +} + +fn exact_file_identity_with_digest( + identity: &NativeExactFileIdentity, + require_digest: bool, +) -> Option { let (dev_negative, dev, dev_lossless) = identity.dev.get_u64(); let (ino_negative, ino, ino_lossless) = identity.ino.get_u64(); let nlink = match identity.nlink.as_ref() { @@ -924,8 +937,13 @@ fn exact_file_identity(identity: &NativeExactFileIdentity) -> Option Some(parse_sha256(Some(value))?), + None => None, + } }; Some(ExactFileIdentity { @@ -1222,6 +1240,32 @@ pub fn exact_unlink(path: String, identity: NativeExactFileIdentity) -> NativeEx platform::exact_unlink(Path::new(&path), &identity) } +/// Remove only the captured symbolic link at `path`. +/// +/// The caller must provide the link's no-follow identity and a private, +/// single-component `quarantineName` in that identity. The native protocol +/// first no-replace-renames the exact link into that quarantine name, verifies +/// the detached entry without following it, and then deletes the same +/// identity-bound handle/name. A successor published at the original path is +/// never touched; any post-detach uncertainty is returned as retained +/// authority instead of being treated as success. +#[napi] +pub fn exact_unlink_symlink( + path: String, + identity: NativeExactFileIdentity, +) -> NativeExactUnlinkResult { + if path.contains('\0') { + return NativeExactUnlinkResult::failure("io_error"); + } + if identity.directory.unwrap_or(false) || identity.detach_only.unwrap_or(false) { + return NativeExactUnlinkResult::failure("invalid_request"); + } + let Some(identity) = exact_symlink_identity(&identity) else { + return NativeExactUnlinkResult::failure("identity_mismatch"); + }; + platform::exact_unlink_symlink(Path::new(&path), &identity) +} + /// Delete only the regular file that still has the supplied platform identity, /// without the exchange/quarantine protocol used by [`exact_unlink`]. /// @@ -3872,6 +3916,106 @@ pub(crate) mod platform { } } + /// Remove one captured symlink through a private no-replace quarantine. The + /// public pathname is never used after the detach, so a successor published + /// there remains untouched. Every private-name mutation is revalidated with + /// `AT_SYMLINK_NOFOLLOW`; a failed validation is restored only with another + /// no-replace rename, otherwise the detached authority is retained. + pub(super) fn exact_unlink_symlink( + path: &Path, + identity: &ExactFileIdentity, + ) -> NativeExactUnlinkResult { + let Some(quarantine_name) = identity.quarantine_name.as_deref() else { + return NativeExactUnlinkResult::failure("quarantine_destination_required"); + }; + if quarantine_name.is_empty() + || quarantine_name == "." + || quarantine_name == ".." + || quarantine_name.as_bytes().contains(&b'/') + { + return NativeExactUnlinkResult::failure("invalid_request"); + } + let (parent_fd, name) = match open_parent_no_follow(path) { + Ok(value) => value, + Err(result) => return *result, + }; + let close_parent = |result: NativeExactUnlinkResult| { + // SAFETY: this operation owns the walked parent descriptor exactly once. + unsafe { libc::close(parent_fd) }; + result + }; + if let Some((expected_dev, expected_ino)) = identity.parent_dev.zip(identity.parent_ino) { + // SAFETY: zero is valid initialized storage for fstat output. + let mut parent_stat: libc::stat = unsafe { std::mem::zeroed() }; + // SAFETY: parent_fd is the retained walked parent descriptor. + if unsafe { libc::fstat(parent_fd, &mut parent_stat) } != 0 + || parent_stat.st_dev as u64 != expected_dev + || parent_stat.st_ino as u64 != expected_ino + { + return close_parent(NativeExactUnlinkResult::failure("parent_mismatch")); + } + } + let Ok(quarantine) = CString::new(quarantine_name) else { + return close_parent(NativeExactUnlinkResult::failure("invalid_request")); + }; + match exact_symlink_matches(parent_fd, &name, identity) { + Ok(true) => {}, + Ok(false) => return close_parent(NativeExactUnlinkResult::failure("identity_mismatch")), + Err(code) => return close_parent(NativeExactUnlinkResult::failure(code)), + } + if let Err(code) = rename_no_replace(parent_fd, parent_fd, &name, &quarantine) { + return close_parent(NativeExactUnlinkResult::failure(code)); + } + let detached_path = path + .parent() + .unwrap_or_else(|| Path::new(".")) + .join(quarantine_name) + .to_string_lossy() + .into_owned(); + let restore_or_retain = |code: &'static str| { + // Never move an unverified replacement from the quarantine name. Only + // the captured symlink may be restored; a foreign occupant stays at its + // private name as retained authority. + if !matches!(exact_symlink_matches(parent_fd, &quarantine, identity), Ok(true)) { + return close_parent(NativeExactUnlinkResult::detached_failure( + "identity_mismatch", + detached_path.clone(), + )); + } + match rename_no_replace(parent_fd, parent_fd, &quarantine, &name) { + Ok(()) => close_parent(NativeExactUnlinkResult::failure(code)), + Err("quarantine_collision") => { + let mut result = + NativeExactUnlinkResult::detached_failure(code, detached_path.clone()); + result.retained_successor_path = Some(path.to_string_lossy().into_owned()); + close_parent(result) + }, + Err(_) => close_parent(NativeExactUnlinkResult::detached_failure( + "cleanup_pending", + detached_path.clone(), + )), + } + }; + match exact_symlink_matches(parent_fd, &quarantine, identity) { + Ok(true) => {}, + Ok(false) | Err(_) => return restore_or_retain("identity_mismatch"), + } + // Revalidate immediately before unlinking the private name. This is the + // POSIX no-follow boundary; any observed replacement is retained rather + // than consumed by cleanup. + match exact_symlink_matches(parent_fd, &quarantine, identity) { + Ok(true) => {}, + Ok(false) | Err(_) => return restore_or_retain("identity_mismatch"), + } + // SAFETY: parent_fd and the private quarantine name remain live; the + // detached entry was just revalidated as the captured symlink. + if unsafe { libc::unlinkat(parent_fd, quarantine.as_ptr(), 0) } == 0 { + close_parent(NativeExactUnlinkResult::success()) + } else { + restore_or_retain(security_code(&std::io::Error::last_os_error())) + } + } + fn exact_unlink_at( parent_fd: libc::c_int, name: CString, @@ -4622,6 +4766,33 @@ pub(crate) mod platform { result } + fn exact_symlink_matches( + parent_fd: libc::c_int, + name: &CString, + identity: &ExactFileIdentity, + ) -> Result { + // SAFETY: parent_fd and name are live; AT_SYMLINK_NOFOLLOW observes the + // directory entry itself and never follows its target. + let mut named: libc::stat = unsafe { std::mem::zeroed() }; + // SAFETY: parent_fd is a retained directory descriptor, name is a live + // NUL-terminated component, and named is writable initialized storage. + if unsafe { libc::fstatat(parent_fd, name.as_ptr(), &mut named, libc::AT_SYMLINK_NOFOLLOW) } + != 0 + { + return Err(security_code(&std::io::Error::last_os_error())); + } + if named.st_mode & libc::S_IFMT != libc::S_IFLNK { + return Ok(false); + } + Ok(named.st_dev as u64 == identity.dev + && named.st_ino as u64 == identity.ino + && named.st_size as u64 == identity.size + && stat_mtime_ns(&named) == i128::from(identity.mtime_ns) + && identity + .nlink + .is_none_or(|nlink| nlink == named.st_nlink as u64)) + } + #[expect( clippy::undocumented_unsafe_blocks, reason = "the exact transaction retains all parent and file descriptors until terminal \ @@ -6343,6 +6514,58 @@ mod platform { Ok(handle) } + /// Open a final reparse point itself, without specifying directory or + /// non-directory semantics. `FILE_OPEN_REPARSE_POINT` keeps the target + /// opaque while allowing both file and directory symlink forms. + fn open_relative_reparse_with_share( + parent: HANDLE, + name: &std::ffi::OsStr, + desired_access: u32, + share_access: u32, + ) -> Result { + let mut name: Vec = name.encode_wide().collect(); + if name.is_empty() + || name.iter().any(|unit| *unit == 0) + || name.len() > (u16::MAX as usize / 2) + { + return Err(STATUS_INVALID_PARAMETER); + } + let mut object_name = UnicodeString { + length: (name.len() * size_of::()) as u16, + maximum_length: (name.len() * size_of::()) as u16, + buffer: name.as_mut_ptr(), + }; + let mut attributes = ObjectAttributes { + length: size_of::() as u32, + root_directory: parent, + object_name: &mut object_name, + attributes: 0, + security_descriptor: null_mut(), + security_quality_of_service: null_mut(), + }; + let mut status: IoStatusBlock = unsafe { std::mem::zeroed() }; + let mut handle = INVALID_HANDLE_VALUE; + let create_status = unsafe { + NtCreateFile( + &mut handle, + desired_access | SYNCHRONIZE, + &mut attributes, + &mut status, + null_mut(), + FILE_ATTRIBUTE_NORMAL, + share_access, + FILE_OPEN, + FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT, + null_mut(), + 0, + ) + }; + if create_status < 0 { + return Err(create_status); + } + Ok(handle) + } + fn open_relative( parent: HANDLE, name: &std::ffi::OsStr, @@ -6462,6 +6685,84 @@ mod platform { ) } + fn open_symlink_exact( + path: &Path, + desired_access: u32, + final_share_access: u32, + ) -> Result { + let (root, names) = + absolute_components(path).map_err(|code| NativeExactUnlinkResult::failure(code))?; + let root_handle = open_path(&root, true, FILE_READ_ATTRIBUTES | FILE_TRAVERSE) + .map_err(|code| NativeExactUnlinkResult::failure(code))?; + let root_attributes = match handle_attributes(root_handle) { + Ok(attributes) => attributes, + Err(code) => { + unsafe { CloseHandle(root_handle) }; + return Err(NativeExactUnlinkResult::failure(code)); + }, + }; + if root_attributes & FILE_ATTRIBUTE_REPARSE_POINT != 0 { + unsafe { CloseHandle(root_handle) }; + return Err(NativeExactUnlinkResult::failure("reparse_point")); + } + let mut ancestors = vec![root_handle]; + for (index, name) in names.iter().enumerate() { + let final_component = index + 1 == names.len(); + let parent = *ancestors.last().expect("volume root retained"); + if final_component { + let handle = match open_relative_reparse_with_share( + parent, + name, + desired_access, + final_share_access, + ) { + Ok(handle) => handle, + Err(status) => { + close_retained(&mut ancestors); + return Err(NativeExactUnlinkResult::failure(ntstatus_code(status))); + }, + }; + let attributes = match handle_attributes(handle) { + Ok(attributes) => attributes, + Err(code) => { + unsafe { CloseHandle(handle) }; + close_retained(&mut ancestors); + return Err(NativeExactUnlinkResult::failure(code)); + }, + }; + if attributes & FILE_ATTRIBUTE_REPARSE_POINT == 0 { + unsafe { CloseHandle(handle) }; + close_retained(&mut ancestors); + return Err(NativeExactUnlinkResult::failure("not_symlink")); + } + return Ok(HeldExact { target: handle, ancestors }); + } + let handle = match open_relative(parent, name, FILE_READ_ATTRIBUTES | FILE_TRAVERSE, true) + { + Ok(handle) => handle, + Err(code) => { + close_retained(&mut ancestors); + return Err(NativeExactUnlinkResult::failure(code)); + }, + }; + let attributes = match handle_attributes(handle) { + Ok(attributes) => attributes, + Err(code) => { + unsafe { CloseHandle(handle) }; + close_retained(&mut ancestors); + return Err(NativeExactUnlinkResult::failure(code)); + }, + }; + if attributes & FILE_ATTRIBUTE_REPARSE_POINT != 0 { + unsafe { CloseHandle(handle) }; + close_retained(&mut ancestors); + return Err(NativeExactUnlinkResult::failure("reparse_point")); + } + ancestors.push(handle); + } + unreachable!("absolute_components rejects a volume root target") + } + fn open_directory_exact(path: &Path) -> Result { match open_exact(path, "directory", FILE_READ_ATTRIBUTES | FILE_TRAVERSE) { Ok(handle) => Ok(handle), @@ -7060,6 +7361,105 @@ mod platform { exact_unlink(path, identity) } + /// Remove a captured Windows reparse-point entry through its retained + /// handle. The handle remains bound to the original object across the + /// no-replace quarantine rename and final disposition, so a replacement at + /// either pathname can never become the deletion target. + pub(super) fn exact_unlink_symlink( + path: &Path, + identity: &ExactFileIdentity, + ) -> NativeExactUnlinkResult { + let Some(quarantine_name) = identity.quarantine_name.as_deref() else { + return NativeExactUnlinkResult::failure("quarantine_destination_required"); + }; + if quarantine_name.is_empty() + || quarantine_name == "." + || quarantine_name == ".." + || quarantine_name.contains('/') + || quarantine_name.contains('\\') + { + return NativeExactUnlinkResult::failure("invalid_request"); + } + let desired_access = FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES | 0x0001_0000; + let handle = match open_symlink_exact( + path, + desired_access, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + ) { + Ok(handle) => handle, + Err(result) => return result, + }; + let mut information: BY_HANDLE_FILE_INFORMATION = unsafe { std::mem::zeroed() }; + if unsafe { GetFileInformationByHandle(handle.target, &mut information) } == 0 { + return NativeExactUnlinkResult::failure(last_error_code()); + } + if information.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == 0 + || !handle_identity_matches(&information, identity) + || identity + .nlink + .is_some_and(|nlink| nlink != u64::from(information.nNumberOfLinks)) + { + return NativeExactUnlinkResult::failure("identity_mismatch"); + } + let Some(parent_handle) = handle.parent() else { + return NativeExactUnlinkResult::failure("io_error"); + }; + if let Some((expected_parent_dev, expected_parent_ino)) = + identity.parent_dev.zip(identity.parent_ino) + { + let mut parent_information: BY_HANDLE_FILE_INFORMATION = unsafe { std::mem::zeroed() }; + if unsafe { GetFileInformationByHandle(parent_handle, &mut parent_information) } == 0 + || u64::from(parent_information.dwVolumeSerialNumber) != expected_parent_dev + || ((u64::from(parent_information.nFileIndexHigh) << 32) + | u64::from(parent_information.nFileIndexLow)) + != expected_parent_ino + { + return NativeExactUnlinkResult::failure("parent_mismatch"); + } + } + let Some(original_name) = path.file_name() else { + return NativeExactUnlinkResult::failure("io_error"); + }; + let Some(parent_path) = path.parent() else { + return NativeExactUnlinkResult::failure("io_error"); + }; + let detached_path = parent_path + .join(quarantine_name) + .to_string_lossy() + .into_owned(); + let quarantine_wide: Vec = quarantine_name.encode_utf16().collect(); + if let Err(code) = rename_handle(handle.target, parent_handle, &quarantine_wide, false) { + return NativeExactUnlinkResult::failure(code); + } + let mut detached_information: BY_HANDLE_FILE_INFORMATION = unsafe { std::mem::zeroed() }; + let detached_matches = + unsafe { GetFileInformationByHandle(handle.target, &mut detached_information) } != 0 + && detached_information.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT != 0 + && handle_identity_matches(&detached_information, identity) + && identity + .nlink + .is_none_or(|nlink| nlink == u64::from(detached_information.nNumberOfLinks)); + if !detached_matches { + let original_name_wide: Vec = original_name.encode_wide().collect(); + return match rename_handle(handle.target, parent_handle, &original_name_wide, false) { + Ok(()) => NativeExactUnlinkResult::failure("identity_mismatch"), + Err("quarantine_collision") => { + let mut result = + NativeExactUnlinkResult::detached_failure("identity_mismatch", detached_path); + result.retained_successor_path = Some(path.to_string_lossy().into_owned()); + result + }, + Err(_) => NativeExactUnlinkResult::detached_failure("cleanup_pending", detached_path), + }; + } + // The retained handle is the final identity proof. Disposition is therefore + // descriptor-bound and cannot consume a successor at either pathname. + match delete_handle(handle.target) { + Ok(()) => NativeExactUnlinkResult::success(), + Err(code) => NativeExactUnlinkResult::detached_failure(code, detached_path), + } + } + pub(super) fn exact_restore( detached_path: &Path, original_path: &Path, @@ -8451,6 +8851,9 @@ mod platform { pub(super) fn exact_unlink_direct(_: &Path, _: &ExactFileIdentity) -> NativeExactUnlinkResult { NativeExactUnlinkResult::failure("identity_unavailable") } + pub(super) fn exact_unlink_symlink(_: &Path, _: &ExactFileIdentity) -> NativeExactUnlinkResult { + NativeExactUnlinkResult::failure("identity_unavailable") + } pub(super) fn exact_restore( _: &Path, _: &Path, diff --git a/packages/coding-agent/src/setup/paseo/paseo-ownership.ts b/packages/coding-agent/src/setup/paseo/paseo-ownership.ts index eaaeef6119..564918fa4b 100644 --- a/packages/coding-agent/src/setup/paseo/paseo-ownership.ts +++ b/packages/coding-agent/src/setup/paseo/paseo-ownership.ts @@ -30,6 +30,17 @@ export interface ProviderReplacedRef { readonly valueSha256: string; } +/** + * Durable no-follow identity for one GJC-created Paseo bridge symlink. + * Strings preserve platform-sized inode fields in JSON without precision loss. + */ +export interface BridgeEntryIdentity { + readonly dev: string; + readonly ino: string; + readonly size: string; + readonly mtimeNs: string; +} + export interface ProvenanceLedger { readonly version: number; /** `agents.providers` keys GJC created, mapped to the value hash it wrote. */ @@ -60,6 +71,8 @@ export interface ProvenanceLedger { readonly bridgeSourceDir?: string; /** Bridge entries GJC created, so the inverse removes exactly those. */ readonly bridgeEntries?: readonly string[]; + /** Install-time identities for bridge entries GJC may later remove. */ + readonly bridgeEntryIdentities?: Record; /** True when GJC created the bridge directory itself (as opposed to populating an existing one). */ readonly bridgeDirCreated?: boolean; } @@ -129,6 +142,9 @@ export async function readProvenance(provenancePath: string): Promise { + if (!isRecord(value)) return false; + return Object.values(value).every( + identity => + isRecord(identity) && + typeof identity.dev === "string" && + typeof identity.ino === "string" && + typeof identity.size === "string" && + typeof identity.mtimeNs === "string" && + /^\d+$/u.test(identity.dev) && + /^\d+$/u.test(identity.ino) && + /^\d+$/u.test(identity.size) && + /^-?\d+$/u.test(identity.mtimeNs), + ); +} function isStringRecord(value: unknown): value is Record { if (!value || typeof value !== "object" || Array.isArray(value)) return false; // Every VALUE must be a string too (#4644 review r10): `{gjc: 7}` is a diff --git a/packages/coding-agent/src/setup/paseo/paseo-setup.ts b/packages/coding-agent/src/setup/paseo/paseo-setup.ts index e3094f1ea1..8768740297 100644 --- a/packages/coding-agent/src/setup/paseo/paseo-setup.ts +++ b/packages/coding-agent/src/setup/paseo/paseo-setup.ts @@ -412,6 +412,7 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende ...bridgeLedger, bridgePath: undefined, bridgeEntries: [], + bridgeEntryIdentities: {}, bridgeDirCreated: false, bridgeSourceDir: undefined, }); @@ -459,6 +460,13 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende const afterCreate = await readProvenance(deps.paths.provenanceLedger); await writeProvenance(deps.paths.provenanceLedger, { ...afterCreate, bridgeDirCreated: true }); } + if (Object.keys(bridge.entryIdentities).length > 0) { + const afterBridge = await readProvenance(deps.paths.provenanceLedger); + await writeProvenance(deps.paths.provenanceLedger, { + ...afterBridge, + bridgeEntryIdentities: { ...afterBridge.bridgeEntryIdentities, ...bridge.entryIdentities }, + }); + } // Prunes have completed: drop them from the ownership record so a later // `--remove` does not treat the removed names as still owned. if (bridgePreflight.prunes.length > 0) { @@ -466,6 +474,11 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende await writeProvenance(deps.paths.provenanceLedger, { ...afterPrunes, bridgeEntries: (afterPrunes.bridgeEntries ?? []).filter(name => !bridge.prunedEntries.includes(name)), + bridgeEntryIdentities: Object.fromEntries( + Object.entries(afterPrunes.bridgeEntryIdentities ?? {}).filter( + ([name]) => !bridge.prunedEntries.includes(name), + ), + ), }); } if ( @@ -528,6 +541,7 @@ async function installPaseoSetup(flags: PaseoSetupFlags, deps: PaseoSetupDepende createdEntries: [...migratedOldEntries], prunedEntries: [], adoptedEntries: [], + entryIdentities: bridgeLedger.bridgeEntryIdentities ?? {}, bridgeDirCreated: bridgeLedger.bridgeDirCreated ?? false, sourceDir: oldSourceDir, }, @@ -601,6 +615,12 @@ export async function correctBridgeOwnershipAfterFailure( await writeProvenance(deps.paths.provenanceLedger, { ...corrected, bridgeEntries: actual, + bridgeEntryIdentities: { + ...Object.fromEntries( + Object.entries(bridgeLedger.bridgeEntryIdentities ?? {}).filter(([name]) => actual.includes(name)), + ), + ...partial.entryIdentities, + }, bridgeDirCreated: corrected.bridgeDirCreated === true || partial.bridgeDirCreated, }); } diff --git a/packages/coding-agent/src/setup/paseo/remove.ts b/packages/coding-agent/src/setup/paseo/remove.ts index c1374512d8..a4444676ce 100644 --- a/packages/coding-agent/src/setup/paseo/remove.ts +++ b/packages/coding-agent/src/setup/paseo/remove.ts @@ -373,6 +373,7 @@ export async function removePaseoSetup( createdEntries: presentEntries, prunedEntries: [], adoptedEntries: [], + entryIdentities: ledger.bridgeEntryIdentities ?? {}, bridgeDirCreated: ledger.bridgeDirCreated ?? false, sourceDir, }, @@ -381,7 +382,13 @@ export async function removePaseoSetup( { bridgeDir }, ); removed.push(bridgeDir); - nextLedger = { ...nextLedger, bridgeEntries: [], bridgeDirCreated: false, bridgeSourceDir: undefined }; + nextLedger = { + ...nextLedger, + bridgeEntries: [], + bridgeEntryIdentities: {}, + bridgeDirCreated: false, + bridgeSourceDir: undefined, + }; } catch (error) { const detail = error instanceof SkillsBridgeError ? error.message : String(error); remaining.push(ledger.bridgePath ?? deps.paths.bridgeDir); diff --git a/packages/coding-agent/src/setup/paseo/skills-bridge.ts b/packages/coding-agent/src/setup/paseo/skills-bridge.ts index 5aa21eca5a..ee2f65bc6b 100644 --- a/packages/coding-agent/src/setup/paseo/skills-bridge.ts +++ b/packages/coding-agent/src/setup/paseo/skills-bridge.ts @@ -16,11 +16,12 @@ import * as nodeCrypto from "node:crypto"; import type { Dirent, Stats } from "node:fs"; import * as fs from "node:fs/promises"; import * as path from "node:path"; +import { exactUnlinkSymlink } from "@gajae-code/natives"; import { getTrustedHomeDir } from "@gajae-code/utils"; import type { CasReceipt } from "../../config/atomic-yaml-patch"; import type { RawSettings, Settings } from "../../config/settings"; import type { SettingPath } from "../../config/settings-schema"; -import { readProvenance } from "./paseo-ownership"; +import { type BridgeEntryIdentity, readProvenance } from "./paseo-ownership"; import type { DriftReason } from "./result-types"; import { PASEO_SKILL_PREFIX, @@ -69,10 +70,10 @@ type BridgeEntryAction = "create" | "noop" | "prune-and-recreate"; /** Preflight identity for a symlink GJC is authorized to remove. */ type SymlinkIdentity = { - readonly dev: number; - readonly ino: number; - readonly size: number; - readonly mtimeMs: number; + readonly dev: bigint; + readonly ino: bigint; + readonly size: bigint; + readonly mtimeNs: bigint; }; type BridgeEntryPlan = { @@ -124,6 +125,8 @@ export interface SkillsBridgeInstallResult { readonly prunedEntries: readonly string[]; /** Recorded legacy links this run re-pointed at the discovered source. */ readonly adoptedEntries: readonly string[]; + /** Install-time no-follow identities for entries this run created or adopted. */ + readonly entryIdentities: Readonly>; readonly bridgeDirCreated: boolean; /** Directory the created links point at; absent when nothing was created. */ readonly sourceDir?: string; @@ -167,122 +170,34 @@ async function quarantineUnlinkVerified( expectedTarget: string, expectedIdentity: SymlinkIdentity, ): Promise { - const quarantine = path.join( - path.dirname(linkPath), - `.gjc-paseo-quarantine-${process.pid}-${nodeCrypto.randomUUID()}`, - ); - await fs.rename(linkPath, quarantine); - // Identity is bound by INODE, not by name (#4644 reviews r11/r13): the - // quarantined object's (dev, ino) is captured once and every later step — - // the verification AND the deletion — must still see that exact inode at - // the quarantine name. The link text is verified once for content, and - // the unlink itself is guarded by an lstat that requires the same inode - // immediately before it. A concurrent actor replacing the quarantine - // pathname therefore changes the inode and the deletion refuses — the - // foreign object is never unlinked. (A rename onto the name in the final - // microseconds between the lstat and the unlink remains theoretically - // possible without a descriptor-bound unlink primitive, but it requires - // winning a race against an unpredictable single-component name inside - // GJC's own bridge directory; the residue sweep below detects and - // restores any survivor.) - // Restoration is NO-CLOBBER (#4644 reviews r15/r16): a concurrent entry - // created at the original linkPath after our rename must never be silently - // destroyed. The no-replace natives refuse symlink sources - // (reparse_point), so this is the narrowest JS shape: rename only when the - // destination is observed vacant, then verify the destination carries the - // inode that JUST left the quarantine name — a foreign entry that won the - // vacancy-to-rename window was displaced by the rename, and is moved aside - // to a recoverable name (never deleted) instead of vanishing. - const restoreNoClobber = async (): Promise => { - const quarantinedNow = await fs.lstat(quarantine).catch(() => undefined); - if (quarantinedNow === undefined) return; - // ATOMIC no-clobber restore (#4644 reviews r15–r17): the natives' - // no-replace primitives refuse symlink sources (reparse_point), but a - // symlink can be recreated atomically with fs.symlink, which fails - // EEXIST when the destination is occupied — an occupant that wins the - // race is never replaced. On success the quarantined original is - // removed (its recreation is byte-identical: same text, verified - // against `captured`/`text` above). - if (quarantinedNow.isSymbolicLink()) { - const currentText = await fs.readlink(quarantine).catch(() => undefined); - if (currentText !== undefined) { - try { - await fs.symlink(currentText, linkPath); - await fs.rm(quarantine, { force: true }).catch(() => undefined); - return; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "EEXIST") return; - // Fall through to the rename path for unsupported cases. - } - } - } - // Non-symlink quarantined object (the divergence branches): the plain - // rename stays, gated on the observed-vacant destination as before. - const occupied = await fs.lstat(linkPath).then( - () => true, - () => false, - ); - if (occupied) return; - await fs.rename(quarantine, linkPath).catch(() => undefined); - }; - const captured = await fs.lstat(quarantine); - if ( - !captured.isSymbolicLink() || - captured.dev !== expectedIdentity.dev || - captured.ino !== expectedIdentity.ino || - captured.size !== expectedIdentity.size || - captured.mtimeMs !== expectedIdentity.mtimeMs - ) { - await restoreNoClobber(); - throw new SkillsBridgeError(`Paseo skill bridge entry diverged before removal: ${linkPath}`); - } - const text = await fs.readlink(quarantine).catch(() => undefined); - if (text === undefined || resolvedLinkTarget(text, quarantine) !== expectedTarget) { - await restoreNoClobber(); + await unlinkSymlinkExactly(linkPath, expectedTarget, expectedIdentity); +} + +async function unlinkSymlinkExactly( + linkPath: string, + expectedTarget: string, + expectedIdentity: SymlinkIdentity, +): Promise { + const text = await fs.readlink(linkPath).catch(() => undefined); + if (text === undefined || resolvedLinkTarget(text, linkPath) !== expectedTarget) { throw new SkillsBridgeError(`Paseo skill bridge entry diverged before removal: ${linkPath}`); } - const sameInode = async (): Promise => { - const stat = await fs.lstat(quarantine).catch(() => undefined); - return ( - stat !== undefined && - stat.ino === captured.ino && - stat.dev === captured.dev && - stat.isSymbolicLink() && - // Size/mtime of a symlink track its text; a replaced link differs. - stat.size === captured.size && - stat.mtimeMs === captured.mtimeMs + const parent = await fs.stat(path.dirname(linkPath), { bigint: true }); + if (!parent.isDirectory()) throw new SkillsBridgeError(`Paseo skill bridge parent is not a directory: ${linkPath}`); + const result = exactUnlinkSymlink(linkPath, { + dev: expectedIdentity.dev, + ino: expectedIdentity.ino, + nlink: 1n, + parentDev: parent.dev, + parentIno: parent.ino, + size: expectedIdentity.size, + mtimeNs: expectedIdentity.mtimeNs, + quarantineName: `.gjc-paseo-quarantine-${process.pid}-${nodeCrypto.randomUUID()}`, + }); + if (!result.ok) { + throw new SkillsBridgeError( + `Paseo skill bridge entry diverged before removal: ${linkPath} (${result.code ?? "unknown"})`, ); - }; - // Deletion refuses unless the exact captured inode is still at the name. - let unlinked = false; - for (let attempt = 0; attempt < 3 && !unlinked; attempt++) { - if (!(await sameInode())) { - // The quarantine name no longer holds our verified object: restore - // whatever is there and refuse. A foreign object is never deleted. - await restoreNoClobber(); - throw new SkillsBridgeError(`Paseo skill bridge entry diverged before removal: ${linkPath}`); - } - try { - await fs.unlink(quarantine); - unlinked = true; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") { - // Something removed it between the check and the unlink. If the - // name is gone entirely the object is gone; nothing foreign was - // deleted by us. Treat as done. - unlinked = true; - break; - } - if (attempt === 2) throw error; - } - } - // Post-unlink sweep: a foreign object swapped onto the quarantine name in - // the final window survives at that name (the unlink consumed ours, or the - // inode guard refused). It is never deleted — only restored to the bridge - // name so nothing foreign is destroyed or stranded. - const residue = await fs.lstat(quarantine).catch(() => undefined); - if (residue !== undefined && !(await sameInode())) { - await restoreNoClobber(); } } @@ -353,9 +268,9 @@ async function entryState( | { readonly kind: "conflict" } > { try { - const stat = await fs.lstat(destination); + const stat = await fs.lstat(destination, { bigint: true }); if (!stat.isSymbolicLink()) return { kind: "conflict" }; - const identity = { dev: stat.dev, ino: stat.ino, size: stat.size, mtimeMs: stat.mtimeMs }; + const identity = { dev: stat.dev, ino: stat.ino, size: stat.size, mtimeNs: stat.mtimeNs }; const link = await fs.readlink(destination); if (resolvedLinkTarget(link, destination) !== expected) return { kind: "conflict" }; try { @@ -380,12 +295,12 @@ async function foreignSymlinkState( | { readonly kind: "conflict" } > { try { - const stat = await fs.lstat(destination); + const stat = await fs.lstat(destination, { bigint: true }); if (!stat.isSymbolicLink()) return { kind: "conflict" }; return { kind: "symlink", link: await fs.readlink(destination), - identity: { dev: stat.dev, ino: stat.ino, size: stat.size, mtimeMs: stat.mtimeMs }, + identity: { dev: stat.dev, ino: stat.ino, size: stat.size, mtimeNs: stat.mtimeNs }, }; } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT") return { kind: "absent" }; @@ -655,6 +570,7 @@ export async function installSkillsBridge(preflight: SkillsBridgePreflight): Pro createdEntries: [...createdEntries], prunedEntries: [...prunedEntries], adoptedEntries: [...adoptedEntries], + entryIdentities: {}, bridgeDirCreated, ...(preflight.sourceDir ? { sourceDir: preflight.sourceDir } : {}), }); @@ -686,15 +602,44 @@ export async function installSkillsBridge(preflight: SkillsBridgePreflight): Pro } catch (error) { fail(error); } + const entryIdentities = await installedEntryIdentities( + preflight.bridgeDir, + [...createdEntries, ...adoptedEntries], + preflight.sourceDir, + ); return createdEntries.length > 0 || prunedEntries.length > 0 || adoptedEntries.length > 0 ? { createdEntries, prunedEntries, adoptedEntries, + entryIdentities, bridgeDirCreated, ...(preflight.sourceDir ? { sourceDir: preflight.sourceDir } : {}), } - : { createdEntries, prunedEntries, adoptedEntries, bridgeDirCreated }; + : { createdEntries, prunedEntries, adoptedEntries, entryIdentities, bridgeDirCreated }; +} + +async function installedEntryIdentities( + bridgeDir: string, + names: readonly string[], + sourceDir: string | undefined, +): Promise> { + const identities: Record = {}; + if (sourceDir === undefined) return identities; + for (const name of names) { + const destination = path.join(bridgeDir, name); + const stat = await fs.lstat(destination, { bigint: true }); + if (!stat.isSymbolicLink() || (await fs.readlink(destination)) === undefined) { + throw new SkillsBridgeError(`Paseo skill bridge entry diverged before identity capture: ${destination}`); + } + identities[name] = { + dev: stat.dev.toString(), + ino: stat.ino.toString(), + size: stat.size.toString(), + mtimeNs: stat.mtimeNs.toString(), + }; + } + return identities; } /** @@ -725,8 +670,29 @@ export async function inverseSkillsBridge( // `dangling` still carries link text pointing exactly where we wrote it; // the source went away (Paseo uninstalled or updated), and a dead link in // GJC's own bridge directory is safe -- and correct -- to remove. - if (state.kind !== "expected" && state.kind !== "dangling") diverged.push(destination); - else removals.push({ destination, target, identity: state.identity }); + const recorded = result.entryIdentities[name]; + if (state.kind !== "expected" && state.kind !== "dangling") { + diverged.push(destination); + continue; + } + if (recorded === undefined) { + diverged.push(destination); + continue; + } + try { + removals.push({ + destination, + target, + identity: { + dev: BigInt(recorded.dev), + ino: BigInt(recorded.ino), + size: BigInt(recorded.size), + mtimeNs: BigInt(recorded.mtimeNs), + }, + }); + } catch { + diverged.push(destination); + } } if (diverged.length > 0) { throw new SkillsBridgeError(`Refusing to remove diverged Paseo skill bridge entries: ${diverged.join(", ")}`); diff --git a/packages/coding-agent/test/setup-paseo.test.ts b/packages/coding-agent/test/setup-paseo.test.ts index 98a34d4ccf..a3f659d84b 100644 --- a/packages/coding-agent/test/setup-paseo.test.ts +++ b/packages/coding-agent/test/setup-paseo.test.ts @@ -626,13 +626,14 @@ describe("skills bridge", () => { /** Install without the full saga but with a realistic ledger, so preflight's provenance gate can run. */ async function installWithLedger(deps: PaseoSetupDependencies): Promise { const preflight = await preflightSkillsBridge(deps); - await installSkillsBridge(preflight); + const result = await installSkillsBridge(preflight); await writeProvenance(deps.paths.provenanceLedger, { version: 1, providerKeys: {}, seededOrchestrationKeys: {}, bridgePath: deps.paths.bridgeDir, bridgeEntries: [...Object.keys(preflight.entries), ...preflight.adopts.map(adopt => adopt.name)], + bridgeEntryIdentities: result.entryIdentities, bridgeDirCreated: false, ...(preflight.sourceDir ? { bridgeSourceDir: preflight.sourceDir } : {}), }); @@ -878,7 +879,7 @@ describe("skills bridge", () => { const legacyTarget = path.join(fixture.paths.agentsSkillsDir as string, name); const bridgeName = path.join(fixture.paths.bridgeDir, name); await fs.symlink(legacyTarget, bridgeName); - const legacyStat = await fs.lstat(bridgeName); + const legacyStat = await fs.lstat(bridgeName, { bigint: true }); const plan = { name, linkPath: bridgeName, @@ -889,7 +890,7 @@ describe("skills bridge", () => { dev: legacyStat.dev, ino: legacyStat.ino, size: legacyStat.size, - mtimeMs: legacyStat.mtimeMs, + mtimeNs: legacyStat.mtimeNs, }, }; // Occupy the bridge name so the replacement publish hits EEXIST. @@ -913,7 +914,7 @@ describe("skills bridge", () => { const name = "paseo-loop"; const bridgeName = path.join(fixture.paths.bridgeDir, name); await fs.symlink(path.join(fixture.paths.agentsSkillsDir as string, "paseo"), bridgeName); - const foreignStat = await fs.lstat(bridgeName); + const foreignStat = await fs.lstat(bridgeName, { bigint: true }); await expect( installSkillsBridge({ @@ -929,7 +930,7 @@ describe("skills bridge", () => { dev: foreignStat.dev, ino: foreignStat.ino, size: foreignStat.size, - mtimeMs: foreignStat.mtimeMs, + mtimeNs: foreignStat.mtimeNs, }, }, ], @@ -956,7 +957,7 @@ describe("skills bridge", () => { path.join(fixture.paths.agentsSkillsDir as string, "paseo"), path.join(fixture.paths.bridgeDir, "paseo-loop"), ); - const foreignStat = await fs.lstat(path.join(fixture.paths.bridgeDir, "paseo-loop")); + const foreignStat = await fs.lstat(path.join(fixture.paths.bridgeDir, "paseo-loop"), { bigint: true }); const before = await snapshotTree(fixture.paths.bridgeDir); await expect( @@ -973,7 +974,7 @@ describe("skills bridge", () => { dev: foreignStat.dev, ino: foreignStat.ino, size: foreignStat.size, - mtimeMs: foreignStat.mtimeMs, + mtimeNs: foreignStat.mtimeNs, }, }, ], @@ -996,7 +997,7 @@ describe("skills bridge", () => { const bridgeName = path.join(fixture.paths.bridgeDir, name); const target = path.join(fixture.paths.agentsSkillsDir as string, name); await fs.symlink(target, bridgeName); - const original = await fs.lstat(bridgeName); + const original = await fs.lstat(bridgeName, { bigint: true }); const plan = { name, linkPath: bridgeName, @@ -1005,7 +1006,7 @@ describe("skills bridge", () => { dev: original.dev, ino: original.ino, size: original.size, - mtimeMs: original.mtimeMs, + mtimeNs: original.mtimeNs, }, }; // The replacement has the exact expected link text, so content-only @@ -1026,6 +1027,23 @@ describe("skills bridge", () => { expect(await fs.readlink(bridgeName)).toBe(target); }); + test("remove refuses a same-target successor after installation (#4644 review r21)", async () => { + const fixture = await makeFixture(); + await seedSkills(fixture.paths); + await installWithLedger(fixture.deps); + const name = "paseo-loop"; + const bridgeName = path.join(fixture.paths.bridgeDir, name); + const target = path.join(fixture.paths.agentsSkillsDir as string, name); + const successor = path.join(fixture.paths.bridgeDir, "successor"); + await fs.symlink(target, successor); + await fs.rename(successor, bridgeName); + + const remove = await removePaseoSetup(fixture.deps, { now: new Date() }); + expect(remove.outcome).toBe("partial-removal"); + expect((await fs.lstat(bridgeName)).isSymbolicLink()).toBe(true); + expect(await fs.readlink(bridgeName)).toBe(target); + }); + test("an interpolated project .env override is rejected by presence (#4644 review r3)", async () => { const root = await makeRoot(); const home = path.join(root, "home"); @@ -1542,6 +1560,7 @@ describe("skills bridge", () => { createdEntries: ["paseo"], prunedEntries: [], adoptedEntries: [], + entryIdentities: {}, bridgeDirCreated: false, sourceDir: fixture.paths.agentsSkillsDir, }); @@ -1556,6 +1575,7 @@ describe("skills bridge", () => { createdEntries: ["paseo-advisor"], prunedEntries: ["paseo-loop"], adoptedEntries: [], + entryIdentities: {}, bridgeDirCreated: false, }, ); @@ -2460,17 +2480,17 @@ describe("skills bridge", () => { bridgeEntries: [...SKILL_NAMES], bridgeDirCreated: true, }); - // No source can be discovered anymore (the app is gone), and the legacy - // ledger has no bridgeSourceDir: removal must still prove ownership. + // No source can be discovered anymore (the app is gone). A legacy ledger + // also lacks install-time link identities, so removal must fail closed + // rather than treat a same-target successor as GJC-owned. const deps: PaseoSetupDependencies = { ...fixture.deps, skillsSource: async () => undefined, }; const result = await removePaseoSetup(deps, { now: new Date() }); - expect(result.outcome).toBe("removed"); - // Every legacy link is gone and the bridge directory was removed. - await expect(fs.stat(deps.paths.bridgeDir)).rejects.toMatchObject({ code: "ENOENT" }); + expect(result.outcome).toBe("partial-removal"); + expect((await fs.lstat(path.join(deps.paths.bridgeDir, SKILL_NAMES[0]))).isSymbolicLink()).toBe(true); }); test("a non-ENOENT filesystem failure fails removal closed instead of reporting success (#4644 review)", async () => { diff --git a/packages/natives/native/index.d.ts b/packages/natives/native/index.d.ts index 5201139179..2cf4d64b8d 100644 --- a/packages/natives/native/index.d.ts +++ b/packages/natives/native/index.d.ts @@ -901,6 +901,19 @@ export declare function exactUnlink(path: string, identity: NativeExactFileIdent */ export declare function exactUnlinkDirect(path: string, identity: NativeExactFileIdentity): NativeExactUnlinkResult +/** + * Remove only the captured symbolic link at `path`. + * + * The caller must provide the link's no-follow identity and a private, + * single-component `quarantineName` in that identity. The native protocol + * first no-replace-renames the exact link into that quarantine name, verifies + * the detached entry without following it, and then deletes the same + * identity-bound handle/name. A successor published at the original path is + * never touched; any post-detach uncertainty is returned as retained + * authority instead of being treated as success. + */ +export declare function exactUnlinkSymlink(path: string, identity: NativeExactFileIdentity): NativeExactUnlinkResult + /** * Execute a brush shell command. * @@ -1786,7 +1799,10 @@ export interface NativeExactFileIdentity { allowHardLink?: boolean } -/** Typed result of an identity-bound regular-file deletion or directory detach. */ +/** + * Typed result of an identity-bound regular-file/symlink deletion or directory + * detach. + */ export interface NativeExactUnlinkResult { ok: boolean code?: string diff --git a/packages/natives/native/index.js b/packages/natives/native/index.js index f312a17738..9b49abfaa5 100644 --- a/packages/natives/native/index.js +++ b/packages/natives/native/index.js @@ -47,6 +47,7 @@ export const exactReplacePath = nativeBindings.exactReplacePath; export const exactRestore = nativeBindings.exactRestore; export const exactUnlink = nativeBindings.exactUnlink; export const exactUnlinkDirect = nativeBindings.exactUnlinkDirect; +export const exactUnlinkSymlink = nativeBindings.exactUnlinkSymlink; export const executeShell = nativeBindings.executeShell; export const extractSegments = nativeBindings.extractSegments; export const fuzzyFind = nativeBindings.fuzzyFind;