From 8a565957006366ed5e5e231effb7641409a3c654 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 16 Aug 2026 14:19:56 -0400 Subject: [PATCH] docs(#3574): record ADR-3574 install materialization primitives (#3575) * docs(#3574): record ADR-3574 install materialization primitives Epic #2866 phase 6 was scoped on the premise that materializing a layout is implemented three times and should become one module. Measured against the tree, the premise does not hold: the three sites overlap in shape and diverge in mechanism. applySurface prunes by allow-list precisely so it structurally cannot delete a user's files. installRuntimeArtifacts wipes a prefix-scoped set and restores a snapshot. A single writer has to pick one, and picking either trades a working guarantee for a different one. So the ADR declines phase 6's first acceptance criterion and says why, because the next reader who notices three similar loops should find this file rather than rediscover the conflict. What is extracted instead is the genuinely shared part: durable user-artifact staging for #1874-F19, reusing the migration primitive that copies strictly before delete and never dereferences a symlink, plus the retired-kind prune both callers already share. The agents bypass closes on its own terms. Two of the issue's premises were also stale: ten runtimes have already migrated off the inline agent dispatch, and the duplication comment's deliberate-until condition is partly met. Closes #3574 * chore(#3574): backfill changeset pr number --------- Co-authored-by: sim --- .changeset/vivid-jaguars-purr.md | 5 + ...3574-install-materialization-primitives.md | 167 ++++++++++++++++++ docs/adr/README.md | 1 + 3 files changed, 173 insertions(+) create mode 100644 .changeset/vivid-jaguars-purr.md create mode 100644 docs/adr/3574-install-materialization-primitives.md diff --git a/.changeset/vivid-jaguars-purr.md b/.changeset/vivid-jaguars-purr.md new file mode 100644 index 000000000..4c08ca7f2 --- /dev/null +++ b/.changeset/vivid-jaguars-purr.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3575 +--- +**Recorded why install materialization stays three loops, not one** — an architecture decision for epic #2866 phase 6. Measuring the three sites showed they diverge in mechanism rather than duplicate each other, so unifying them would have broken a prune that structurally cannot delete user files. (#3574) diff --git a/docs/adr/3574-install-materialization-primitives.md b/docs/adr/3574-install-materialization-primitives.md new file mode 100644 index 000000000..24c39c150 --- /dev/null +++ b/docs/adr/3574-install-materialization-primitives.md @@ -0,0 +1,167 @@ +# ADR-3574: Install materialization shares primitives, not one writer + +- **Status:** Accepted +- **Date:** 2026-08-16 +- **Issue:** [#3574](https://github.com/open-gsd/gsd-core/issues/3574) +- **Epic:** [#2866](https://github.com/open-gsd/gsd-core/issues/2866) — Phase 6 ([#2875](https://github.com/open-gsd/gsd-core/issues/2875)) +- **Amends:** none. Constrained by [ADR-58](58-runtime-install-policy-module.md), [ADR-3660](3660-runtime-artifact-layout-module.md), [ADR-1508](1508-runtime-artifact-conversion-module.md). + +## Context + +Epic #2866 Phase 6 was scoped on the premise that *"materialize this layout"* is implemented three +times and skipped once, and that the remedy is to extract the +preserve → prune → stage → copy → restore choreography into **one** module with the three sites +becoming callers. + +That premise was measured against the tree on 2026-08-16, after Phase 5 ([#2874](https://github.com/open-gsd/gsd-core/issues/2874)) +landed. **It does not hold.** The three sites overlap in *shape* and diverge in *mechanism*: + +| step | `installRuntimeArtifacts`
`src/install-engine.cts:770-958` | `applySurface`
`src/surface.cts:359-452` | agent loop
`bin/install.js:11120+` | +|---|---|---|---| +| preserve | snapshot-based, **`skills` kind only** (`_snapshotDir`) | **none** | none | +| prune | `_removeGsdEntries`, prefix-scoped wipe | `pruneSkillDirs` — **allow-list, never wipes** | stale `gsd-*` unlink | +| stage | copies straight into `dest` | **temp dir first**, then syncs | inline transform, no staging dir | +| restore | `_restoreDir` the snapshot | none | none | + +The divergence is deliberate on at least one side. `applySurface`'s prune is allow-list precisely +so that it *structurally cannot* delete a user's files — its own doc comment ties that to the +#2973/#3664 user-directory-preserving fix. `installRuntimeArtifacts` instead wipes a prefix-scoped +set and restores a snapshot of the one user-owned directory it knows about. + +A single writer must pick one of these. Forcing `applySurface` onto snapshot-restore would replace +a design that *cannot* lose user files with one that deletes them and puts them back — trading a +structural guarantee for a procedural one. Forcing `installRuntimeArtifacts` onto temp-staging adds +a staging hop it does not need. + +Two further premises of the original scoping are stale: + +- **The `agents` bypass is shrinking, not static.** `_DESCRIPTOR_AGENTS_RUNTIMES` + (`bin/install.js:11152`) already routes ten runtimes — cursor, windsurf, augment, trae, codebuddy, + copilot, antigravity, qwen, kimi, zcode — through the descriptor. The inline `_hostBehaviors()` + dispatch survives only for codex, cline, hermes and generic runtimes. +- **The duplication comment is stale in the opposite direction.** It lives at + `src/runtime-artifact-layout.cts:277-297` (not the range #2875 cites) and reads: *"That + duplication is deliberate until the second `layout.kinds` consumer — `applySurface` … — mirrors + the legacy agent pipeline."* That condition has **partly** been met, via `agentCtx` / + `stageAgentsForRuntimeWithConverter` in `applySurface`. + +Separately and independently, **#1874-F19 is confirmed real**: `preserveUserArtifacts` +(`src/install-engine.cts:168-179`) builds an in-memory `Map` via `readFileSync`, and +`restoreUserArtifacts` (`:187-195`) writes it back. Nothing touches disk in between. Any process +death between the intervening wipe and the restore loses the content outright, at four call sites +(`install-engine.cts:633`, `:705`; `bin/install.js:8658`, `:11056`). + +## Decision + +### 1. There will be no single materializer module + +The three choreographies stay distinct. This ADR explicitly declines Phase 6's first acceptance +criterion as written — *"One module writes a `Layout`; the three former call sites delegate to it"* — +because satisfying it requires breaking one of two mechanisms that are each correct for their own +caller. + +Recording the refusal is the point: the next reader who notices three similar-looking loops should +find this file rather than re-derive the extraction and rediscover the conflict. + +### 2. What IS extracted: durable user-artifact staging (F19) + +`preserveUserArtifacts` / `restoreUserArtifacts` move to a shared module and stage to a **durable +on-disk path before any wipe**, reusing `copyPreservingSymlink` +(`src/installer-migrations.cts:166-177`) — a pure two-argument function with no migration-specific +state, already used by the `backup-and-remove` migration action in exactly this +copy-strictly-before-delete order. + +`copyPreservingSymlink` is the correct primitive for a second reason beyond durability: it never +dereferences a symlink target. Its own doc comment records why — dereferencing could copy the bytes +behind a link like `~/.ssh/id_rsa` into the backup tree. A hand-rolled `copyFileSync` here would +reintroduce that. + +The journal / `backupRoot` / `runId` scaffolding around it is migration-specific and is **not** +extracted. Only the primitive is shared. + +### 3. What IS extracted: the retired-kind prune + +`pruneRetiredRuntimeArtifacts` is already called by both `installRuntimeArtifacts` and +`applySurface` with the same intent. That is genuine shared behavior rather than parallel +evolution, and it is the one step where a single owner costs nothing. + +### 4. The `agents` bypass is closed on its own terms + +Removing the inline `_hostBehaviors()` agent dispatch so the descriptor is authoritative for every +runtime is **independent of the prune question** and proceeds regardless. It is the part of Phase 6 +whose evidence survived scrutiny intact, and ten runtimes have already made the trip. + +### 5. Placement and content ownership are unchanged + +Per [ADR-3660](3660-runtime-artifact-layout-module.md), placement knowledge outside +`runtime-artifact-layout` is drift; the extracted primitives consume `Layout`, never re-derive it. +Per [ADR-1508](1508-runtime-artifact-conversion-module.md), *"Layout owns placement; this module +owns content"*, and the conversion module imports nothing upward. The primitives sit **downstream** +of both and introduce no upward dependency. + +Per [ADR-58](58-runtime-install-policy-module.md), these primitives are on the **adapter** side of +the pure-policy/thin-adapter split — they execute IO. Phase 5 routed that IO through an injectable +seam and established that the write-confinement *decisions* (`hasExistingSymlinkBetween`, +`assertDestWithinConfigHome`) stay outside the adapter, so a fake cannot certify an install the real +filesystem would refuse. **The extraction must not relocate those decisions.** + +## What this ADR does not decide + +- **Whether the three choreographies ever unify.** The `applySurface` descriptor-agents migration is + partly landed; when it completes, the shapes may converge enough that the question is worth + reopening on evidence. Revisit then, not before — and not by re-deriving the extraction this file + declines. +- **The prune model itself.** Whether allow-list or prefix-scoped-wipe is the better default across + the installer is a real question and a separate one. Nothing here endorses either as canonical. +- **`USER_OWNED_ARTIFACTS`' membership.** #2875 names `USER-PROFILE.md` as at risk; that could not be + confirmed in this codebase state — `dev-preferences.md` is confirmed at three of four call sites. + The implementing phase must enumerate the list rather than inherit the claim. + +## Consequences + +- **Phase 6's acceptance criterion 1 is not met as written, deliberately.** #2875 needs a scope + update to match this decision before implementation. That is the cost of having measured the + premise instead of executing it. +- Three loops that look duplicated remain, now with a recorded reason. Future reviews should treat + *this file*, not the loops, as the answer. +- The `agents` bypass closes; the `runtime-artifact-layout.cts:277-297` comment is rewritten rather + than deleted, because its "deliberate until X" framing is stale in a way a plain deletion would + not capture. +- **F19's durability fix lands here rather than in #1874**, per maintainer direction on #2875. + #1874's F5, F6 and F18 are untouched. +- The regression test for F19 must inject the crash window by monkeypatching the `fs` method and + restoring in a `finally` — **never** via `chmod`/permission tricks, which root bypasses, yielding a + test that passes with zero coverage in root Docker and CI. + +## Alternatives considered + +1. **One materializer, three callers delegate — Phase 6 as originally scoped.** Rejected on the + measured evidence above: it forces either `applySurface` off its non-wiping prune or + `installRuntimeArtifacts` into unnecessary temp-staging. +2. **One materializer with a preservation-strategy parameter.** Rejected. It preserves both + behaviors but makes the module own two concepts and defer the choice to its callers — the exact + widening [ADR-2866](2866-install-surface-resolution.md) warns future reviews to resist, and a + strategy flag is how a shared module becomes two modules wearing one name. +3. **Do nothing; leave F19 to #1874.** Rejected. This phase rewrites that precise choreography, so + landing the durability fix elsewhere means two conflicting passes over the same code. +4. **Delete the duplication comment as no longer true.** Rejected. It is not simply false — it is + stale in a specific, informative way, and its "deliberate until X" condition is now partly met. + A rewrite carries that; a deletion loses it. + +## A note on the evidence + +Blast-radius figures for this seam are **not** reliable and were not used to justify anything above. +`get_impact` on `installRuntimeArtifacts` resolved to a same-named test helper +(`tests/adapter-declarative-equivalence.test.cjs:52`) and reported zero affected — the same +name-collision failure mode that produced a misleading clean radius during +[#3544](https://github.com/open-gsd/gsd-core/issues/3544). `applySurface` returned **CRITICAL / +184+** from one tool and **LOW / 0** from another, disambiguating to two different in-file matches of +the same name. The decision above rests on read code, not on those numbers. + +## References + +- Epic: [#2866](https://github.com/open-gsd/gsd-core/issues/2866); this phase: [#2875](https://github.com/open-gsd/gsd-core/issues/2875); this ADR: [#3574](https://github.com/open-gsd/gsd-core/issues/3574) +- Durability finding: [#1874](https://github.com/open-gsd/gsd-core/issues/1874)-F19 (and its closed child #1878 — do not re-file) +- Placement seam: [ADR-3660](3660-runtime-artifact-layout-module.md) · content seam: [ADR-1508](1508-runtime-artifact-conversion-module.md) · policy/adapter split: [ADR-58](58-runtime-install-policy-module.md) +- The epic's own frame: [ADR-2866](2866-install-surface-resolution.md), which mandated that this module owe its own ADR +- User-directory preservation this ADR protects: #2973, #3664 diff --git a/docs/adr/README.md b/docs/adr/README.md index a43e6c2c6..9e91b0b1f 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -254,6 +254,7 @@ These govern the system as it stands. Cite these. | [ADR-3212](3212-lexical-seam-consolidation.md) | The Lexical Seam — Safe Pattern Construction, Line-Terminator Normalization, and Tokenizer-First Stateful Grammars | Accepted | — | | [ADR-3408](3408-state-write-path-preservation.md) | STATE.md Write Path — One Declared Policy, One Write Seam | Accepted | — | | [ADR-3409](3409-unreachable-shell-guard-arms.md) | Shell Guards Must Observe Their Own Failure Arm | Accepted | — | +| [ADR-3574](3574-install-materialization-primitives.md) | Install materialization shares primitives, not one writer | Accepted | — | | [ADR-3660](3660-runtime-artifact-layout-module.md) | Runtime Artifact Layout Module owns per-runtime artifact placement | Accepted | [ADR-1239](1239-gsd-embeddable-orchestration-engine.md) | ### Proposed