* test(#3760): failing-first regression for non-object config section Locks the contract from the issue's Expected section before any fix exists: a legacy-key section holding a string, number, boolean or array must be preserved verbatim, reported, and never persisted in an expanded form. Covers both blocks the issue names (branching_strategy -> git.*, sub_repos -> planning.*), the migrateOnDisk multiRepo branch that shares the shape, and the loader write paths that are what actually reach the user's config.json. Includes the negative-space cases that must keep hoisting ({} , null, absent section, canonical-nested-wins) and two fast-check properties. Refs #3760 * fix(#3760): refuse a legacy-key migration into a non-object config section normalizeLegacyKeys hoisted a legacy top-level key into its canonical nested section by spreading `result[section] ?? {}`. `??` guards only null and undefined, so a section holding a string was enumerated by index — `{...'main'}` is `{0:'m',1:'a',2:'i',3:'n'}` — while a number or boolean spread to `{}` and the value vanished. Because a fired block always pushed a Normalization, and every caller treats a non-empty normalizations array as 'config is dirty', that shape was written back to .planning/config.json and the original value became unrecoverable. Both blocks the issue names are fixed via one shared hoistLegacyKey helper, plus the two further sites that share the shape and are reachable from the same input: migrateOnDisk's multiRepo branch, and the loader's two `if (!planning) planning = {}` guards, where a non-empty string is truthy and the following assignment threw a strict-mode TypeError that the enclosing catch swallowed — discarding the user's entire config. A present non-object section now blocks its own migration. The section, the legacy key, and the file are left byte-identical; no Normalization is pushed, so nothing marks the config dirty; the refusal is reported in-band as `skipped[]` and out-of-band through the ADR-1411 warnUnusableInput seam (new frozen reason config_section_not_object). null and undefined keep their long-standing 'absent' meaning and still create the section. This is the nested-section analog of the ADR-227 shape check _readConfigFile already performs on the top-level document: valid JSON is not a config object. isConfigSection is exported and shared by both modules rather than copied. Fixes #3760 * fix(#3760): keep the multiRepo marker when planning cannot receive it Follow-up from the isolated adversarial review, and the same defect class as the two blocks the issue names — in the block it did not name. normalizeLegacyKeys block 3 deleted `multiRepo` and pushed a Normalization before anything consulted the planning section, deferring 'can this section receive sub_repos?' to the caller that runs filesystem detection. By then the marker was already gone and the config was already dirty, so with {"multiRepo":true,"planning":"docs"} the loader wrote the file back with multiRepo removed, the sub_repos injection silently no-opped against the string, and no diagnostic was emitted at all. migrateOnDisk warned for the same input; the ~30-caller loadConfig path did not. Section validity is knowable from the parsed config alone — detection is only needed for the VALUE, not for whether the destination can hold it. The refusal moves into block 3: the marker is kept, no Normalization is pushed, and a skipped entry is recorded, so all three callers inherit the preservation and the diagnostic together. The caller-side guards drop to pure narrowing. Also from review: skipped[] now reports sectionType ('string' | 'number' | 'boolean' | 'array') instead of sectionValue. migrateOnDisk's report is printed verbatim by `migrate-config`, and this module already masks config values on the set/unset output path; the type is the whole diagnostic and the value is still in the file. And `migrate-config --raw` no longer answers a refused migration with 'No legacy keys found — config is already canonical.' Legacy keys WERE found and declined, and the decline is the one thing only the user can fix by hand. Refs #3760 * fix(#3760): keep configuration.cjs dependency-free; emit from its callers The remote matrix caught a regression my own change introduced: adding `require('./unusable-input.cjs')` to configuration.cts broke the #3571 install-layout contract. `configuration.cjs` must load from a layout holding only itself plus bin/shared/*.manifest.json — the installer does not co-locate arbitrary siblings — so the new require failed at load time: Cannot find module './unusable-input.cjs' Require stack: - /tmp/gsd-3571-.../.codex/gsd-core/bin/lib/configuration.cjs pinned by 'co-located bin/shared manifests let configuration.cjs load without sdk/shared' in tests/install.test.cjs (3 failures). The contract is deliberate and the test is right, so the module goes back to zero sibling requires and the out-of-band diagnostic moves to the callers that already carry a dependency budget and hold the resolved path: cmdMigrateConfig (config.cts) and loadConfigResolved (config-loader.cts). normalizeLegacyKeys keeps reporting refusals in-band via skipped[], which is what lets it be pure and dependency-free at the same time. The emission-count and dedup assertions move to tests/config-loader.test.cjs, where the diagnostic now originates. A new assertion pins the inverse for the module itself — migrateOnDisk must emit ZERO diagnostics while still reporting skipped[] — so regrowing a sibling require fails a unit test instead of only the install suite. CONTEXT.md records why the emitter is the caller. Refs #3760 * chore(#3760): backfill changeset pr number to 3767 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -101,7 +101,7 @@ Module owning dispatch-event creation, redaction, and logger behavior for the Co
|
||||
Module policy that defines query-time behavior when `.planning/config.json` is absent: use built-in defaults for parity-sensitive query Interfaces, and emit parity-aligned empty model ids for pre-project model resolution surfaces.
|
||||
|
||||
### Configuration Module
|
||||
Module owning legacy-key normalization, defaults merge, and explicit on-disk migration for `.planning/config.json`. Interface: `normalizeLegacyKeys(parsed) → { parsed, normalizations[] }` (idempotent, pure, returns the list of normalizations applied), `mergeDefaults(parsed) → MergedConfig` (deep-merge of parsed config over canonical defaults), `migrateOnDisk(cwd) → MigrationReport` (explicit, opt-in, called by the installer and by `gsd-tools migrate-config`). Invariants: legacy top-level keys (`branching_strategy`, `sub_repos`, `multiRepo`, `depth`) are normalized into their canonical nested locations in the returned value; defaults come from the shared `gsd-core/bin/shared/config-defaults.manifest.json`; schema (`VALID_CONFIG_KEYS`, `RUNTIME_STATE_KEYS`, `DYNAMIC_KEY_PATTERNS`) comes from `gsd-core/bin/shared/config-schema.manifest.json`. Note: `loadConfig` (project config read + merge) was extracted to the Config Loader Module (`config-loader.cjs`) per ADR-857 phase 2e (#885); `configuration.cjs` now provides only the pure normalization and defaults primitives that `config-loader.cjs` depends on. Source of truth: `gsd-core/bin/lib/configuration.cjs`, consumed via `bin/lib/config-loader.cjs` and `bin/lib/config-schema.cjs`. Eliminates the recurring #3523-class drift bug structurally.
|
||||
Module owning legacy-key normalization, defaults merge, and explicit on-disk migration for `.planning/config.json`. Interface: `normalizeLegacyKeys(parsed) → { parsed, normalizations[], skipped[] }` (idempotent, pure, returns the list of normalizations applied plus the list of migrations declined), `isConfigSection(value) → boolean` (is a value usable as a nested config section — a non-null, non-array object), `mergeDefaults(parsed) → MergedConfig` (deep-merge of parsed config over canonical defaults), `migrateOnDisk(cwd) → MigrationReport` (explicit, opt-in, called by the installer and by `gsd-tools migrate-config`). Invariants: legacy top-level keys (`branching_strategy`, `sub_repos`, `multiRepo`, `depth`) are normalized into their canonical nested locations in the returned value; **a destination section that is present but is NOT an object blocks its own migration rather than being overwritten (#3760)** — spreading such a value expands a string into character keys (`{...'main'}` is `{0:'m',1:'a',2:'i',3:'n'}`) and collapses a number or boolean to `{}`, and because a reported normalization is what marks a config dirty, that shape was then written to the user's `config.json`; the refusal leaves the section, the legacy key, and the file byte-identical and records a `skipped` entry in-band — the nested-section analog of the top-level ADR-227 shape check `_readConfigFile` already performs. **The ADR-1411 out-of-band `UNUSABLE_REASON.CONFIG_SECTION_NOT_OBJECT` diagnostic is emitted by this module's CALLERS — `cmdMigrateConfig` (Config CRUD) and `loadConfigResolved` (Config Loader) — never by this module**, because `configuration.cjs` must load from an install layout containing only itself plus `bin/shared/*.manifest.json` (the #3571 contract, pinned by `tests/install.test.cjs` "co-located bin/shared manifests let configuration.cjs load without sdk/shared"); a sibling `require` the installer does not co-locate fails at load time with `MODULE_NOT_FOUND`. Reporting in-band via `skipped[]` is what keeps the module both pure AND dependency-free; defaults come from the shared `gsd-core/bin/shared/config-defaults.manifest.json`; schema (`VALID_CONFIG_KEYS`, `RUNTIME_STATE_KEYS`, `DYNAMIC_KEY_PATTERNS`) comes from `gsd-core/bin/shared/config-schema.manifest.json`. Note: `loadConfig` (project config read + merge) was extracted to the Config Loader Module (`config-loader.cjs`) per ADR-857 phase 2e (#885); `configuration.cjs` now provides only the pure normalization and defaults primitives that `config-loader.cjs` depends on. Source of truth: `gsd-core/bin/lib/configuration.cjs`, consumed via `bin/lib/config-loader.cjs` and `bin/lib/config-schema.cjs`. Eliminates the recurring #3523-class drift bug structurally.
|
||||
|
||||
### Planning Scope Module
|
||||
Leaf module owning the frozen `SCOPE` discriminator (`COMPLETE` / `TRUNCATED` / `UNSCOPED` / `UNREADABLE`) that every consolidated `.planning/` semantic derivation returns alongside its payload, per ADR-3180 Decision 2. It exists to make one distinction representable: `COMPLETE` with zero items is a REAL answer (a phase genuinely has no plans; a milestone genuinely has no phases yet), while the other three with zero items are NON-answers — the derivation could not see all of its input. Before it, those two cases were output-identical, which is the failure class epic #3180 removes: a truncated milestone window returned `phase_count: 0` with no error, indistinguishable from a freshly-declared milestone. It is a frozen enum rather than a message string because `CONTRIBUTING.md` bans raw-text matching on outputs and requires a typed IR, so callers branch on `result.scope === SCOPE.TRUNCATED`. Pure and import-free — the bottom of the dependency graph, so any consumer can depend on it without a cycle (mirrors the Phase Id Module's leaf position). Source of truth: `gsd-core/bin/lib/planning-scope.cjs` (generated from `src/planning-scope.cts`). The contract is PROVISIONAL: #3183 is its first real implementation, and ADR-3180 requires the ADR be amended before Phase 2 rather than the contract worked around, if it does not fit.
|
||||
@@ -168,7 +168,7 @@ Cross-seam principle (ADR-1411, epic #1411): context resolution — config loadi
|
||||
Diagnostic-output convention for the Resolution Provenance principle (ADR-1411 P3, #1416). Config-interpreting read verbs expose `Resolution<T> { value, configured, reason, warnings }` (`src/resolution.cts`); agent-skills is the first adopter, where `value = { block, skills_count }` and `source`/`degraded` remain config-provenance extras outside the envelope. Other read verbs expose at least `warnings[]` (e.g. capability-state `{ runtimeConfigDir, capabilities, warnings? }`) without `configured`/`reason`, which are meaningful only for config-interpreting verbs. Mutation verbs expose `warnings[]` (advisory) PLUS `errors[]` (operation-not-applied), e.g. capability-writer `{ capabilities, warnings, errors }`. The shared seam across all shapes is `warnings: string[]`; a single generic `Resolution<T>` across read+write verbs was rejected by the deletion test (`configured`/`reason` are meaningless for capability verbs; `errors[]` cannot fold into `warnings[]`) — ADR-1411 P3 amendment. Recurrence prevention is delivered by P4's CI guard (a configured input resolving empty must carry a `reason`), not by a shared envelope. A CI guard (`scripts/lint-resolution-provenance.cjs`, wired into `lint:ci`) enforces that every registered config-interpreting read verb keeps a `configured_empty`/`not_configured` contract test; the registry in that script is the registration point for future verbs (ADR-1411 P4 / #1417).
|
||||
|
||||
### Unusable Input Diagnostic Module
|
||||
Leaf module owning the **out-of-band** half of ADR-1411's "corrupt is not absent" amendment (epic #1879). Where a read already returns a provenance envelope the cause is named in-band (`ConfigResolution.reason`, #1880); where a read returns a bare sentinel or a plausible default it cannot extend, the return value is preserved exactly and the cause is surfaced here instead. Interface: `UNUSABLE_REASON` (frozen reason enum — one entry per condition that has an emitting call site; adding a reason is three coordinated changes: enum + call site + the test locking `Object.keys(...).sort()`), `warnUnusableInput({reason, source?, content?}) → boolean` (returns whether this call actually wrote, so tests assert emission *counts* on a typed surface rather than scraping stderr), plus the `_resetUnusableInputWarningsForTests` / `_unusableInputWarningCountForTests` seams. Dedup key is `<normalized source>\0<reason>` — **both halves are load-bearing**: keying on the path alone would let a second, different fault on the same file go unreported, and keying on message prose would couple the guard to wording (ADR-1411 dedup clause). Path separators are deliberately **not** normalized: an earlier revision folded backslashes to `/` so two spellings of one Windows path would not double-report, but a backslash is a legal filename character on Linux and macOS, so that folding collapsed two genuinely distinct POSIX files onto one key and swallowed the second file's diagnostic. The trade is now one-directional — two spellings of one Windows path may report twice (noise), but two distinct files can never silence each other (lost signal), and ADR-1411 ranks the swallow the worse failure; ASCII control characters are stripped from the source before it is keyed or written, because the key separator is NUL (a crafted path could otherwise forge a collision) and because a path carrying ANSI escapes would replay into the operator's terminal. Callers with no path (in-memory content) fall back to a short content digest so *different* bad inputs still key differently. The diagnostic is **unconditional** — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since "an opt-in nobody sets is indistinguishable from the silence #1879 is about" — and **never throws**: a failed stderr write is swallowed so a degraded read is never escalated into a crash. Adopted by `extractFrontmatter` (#1882, `frontmatter_unterminated`) and by `getRoadmapPhaseInternal`/`getMilestoneInfo` (#1881, `roadmap_unreadable`); `planning-workspace`/`verify` (#1883) follow. #1881 detects on the errno alone: `platformReadSync` returns `null` for ENOENT and its callers convert that to an errno-less Error, so reporting unconditionally in those catches would flag every project without a ROADMAP.md as corrupt. Exists as a shared seam rather than a per-site copy because four sites need identical behavior and four hand-rolled copies is `RULESET.GENERATIVE-FIX` by construction. Source of truth: `gsd-core/bin/lib/unusable-input.cjs` (generated from `src/unusable-input.cts`). Test anchor: `tests/unusable-input.test.cjs`. See Resolution Provenance, Config Loader Module.
|
||||
Leaf module owning the **out-of-band** half of ADR-1411's "corrupt is not absent" amendment (epic #1879). Where a read already returns a provenance envelope the cause is named in-band (`ConfigResolution.reason`, #1880); where a read returns a bare sentinel or a plausible default it cannot extend, the return value is preserved exactly and the cause is surfaced here instead. Interface: `UNUSABLE_REASON` (frozen reason enum — one entry per condition that has an emitting call site; adding a reason is three coordinated changes: enum + call site + the test locking `Object.keys(...).sort()`), `warnUnusableInput({reason, source?, content?}) → boolean` (returns whether this call actually wrote, so tests assert emission *counts* on a typed surface rather than scraping stderr), plus the `_resetUnusableInputWarningsForTests` / `_unusableInputWarningCountForTests` seams. Dedup key is `<normalized source>\0<reason>` — **both halves are load-bearing**: keying on the path alone would let a second, different fault on the same file go unreported, and keying on message prose would couple the guard to wording (ADR-1411 dedup clause). Path separators are deliberately **not** normalized: an earlier revision folded backslashes to `/` so two spellings of one Windows path would not double-report, but a backslash is a legal filename character on Linux and macOS, so that folding collapsed two genuinely distinct POSIX files onto one key and swallowed the second file's diagnostic. The trade is now one-directional — two spellings of one Windows path may report twice (noise), but two distinct files can never silence each other (lost signal), and ADR-1411 ranks the swallow the worse failure; ASCII control characters are stripped from the source before it is keyed or written, because the key separator is NUL (a crafted path could otherwise forge a collision) and because a path carrying ANSI escapes would replay into the operator's terminal. Callers with no path (in-memory content) fall back to a short content digest so *different* bad inputs still key differently. The diagnostic is **unconditional** — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since "an opt-in nobody sets is indistinguishable from the silence #1879 is about" — and **never throws**: a failed stderr write is swallowed so a degraded read is never escalated into a crash. Adopted by `extractFrontmatter` (#1882, `frontmatter_unterminated`) and by `getRoadmapPhaseInternal`/`getMilestoneInfo` (#1881, `roadmap_unreadable`); `planning-workspace`/`verify` (#1883) follow. Tenth site: `cmdMigrateConfig` (Config CRUD Module) and `loadConfigResolved` (Config Loader Module) emit `config_section_not_object` when a legacy-key migration's destination section holds a non-object (#3760). The detecting module, `configuration.cjs`, deliberately does NOT emit: it reports in-band as `skipped[]` and its callers — which hold both the resolved path and an unconstrained dependency budget — do the emitting, because `configuration.cjs` must stay loadable with no sibling requires under the #3571 install-layout contract. #1881 detects on the errno alone: `platformReadSync` returns `null` for ENOENT and its callers convert that to an errno-less Error, so reporting unconditionally in those catches would flag every project without a ROADMAP.md as corrupt. Exists as a shared seam rather than a per-site copy because four sites need identical behavior and four hand-rolled copies is `RULESET.GENERATIVE-FIX` by construction. Source of truth: `gsd-core/bin/lib/unusable-input.cjs` (generated from `src/unusable-input.cts`). Test anchor: `tests/unusable-input.test.cjs`. See Resolution Provenance, Config Loader Module.
|
||||
|
||||
### Worktree Safety Policy Module
|
||||
CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult` (per-entry gauntlet: branch → base → deletions → **advisory scope conformance (#2596)** → SUMMARY-rescue → clean-worktree → merge → remove; the scope check compares the branch's committed diff against the entry's declared `files_modified` and appends `WAVE_CLEANUP_WARNING`-coded entries to a `warnings` channel WITHOUT touching `ok` — an advisory, not a gate, and skipped entirely with no git call when no scope was declared), `planWaveScopeConformance(changedPaths, declaredFiles, branch) → WaveCleanupWarning[]` (pure; literal-prefix path coverage deliberately mirroring the submodule-intersection gate's glob-prefix rule rather than introducing a second matcher; over-accepts by design because a false alarm costs an advisory more than a miss), `isSummaryArtifactRelPath(relPath) → boolean` (the single definition of "executor-written SUMMARY artifact", shared with `defaultFindSummaryFiles` so the rescue walker and the scope exemption cannot drift), `WAVE_CLEANUP_WARNING` (frozen advisory-code enum: `scope_out_of_declared`, `scope_check_unavailable`), `planWorktreeRecordAgent(manifestRaw, fields) → RecordAgentPlan` (write-strict per-agent manifest append; validates each field at write time via the same `normalizeCleanupManifestEntry` rules the reader enforces; fail-closed on a missing/garbled field or a duplicate `(worktree_path, branch)` the reader would dedup away), `cmdWorktreeRecordAgent(cwd, args, deps) → RecordAgentCmdResult` (thin deps-injectable IO wrapper for the `worktree record-agent` verb), `planWorktreeCreate(fields) → WorktreeCreatePlan` (write-strict `worktree create` planner — same missing-field-hint and `normalizeCleanupManifestEntry` validation as `planWorktreeRecordAgent`, pure/no-git), `executeWorktreeCreatePlan(plan, repoRoot, deps) → WorktreeCreateResult` (bounded `git rev-parse --verify` base check THEN `git worktree add -b <branch> <path> <base>`; fail-closed `base_unresolved`/`git_timeout`/`worktree_add_failed`; returns `cwd` — the working directory an executor spawn would use), `cmdWorktreeCreate(cwd, args, deps) → WorktreeCreateCmdResult` (CLI verb: requires `--root` — confinement is mandatory, not opt-in; omitting it fails closed with `reason:'root_required'` before any git side effect, rather than silently creating an unconfined worktree (#3050); plans, creates the worktree, then appends the manifest entry so it is immediately manageable by cleanup-wave/reap-orphans; dedupes by `(worktree_path, branch)`). #2584 ADR-1239 Codex-binding amendment, Phase 2: `worktree create` is the git-worktree-creation primitive for `dispatch.isolation: orchestrator-worktree` hosts — consumed since #2584 Phase 3 — `executor-isolation-dispatch.md` calls it to create the worktree an `orchestrator-worktree` host is then process-spawned into. `worktree record-agent` / `worktree create` accept an optional `--files` recording the plan's declared scope, consumed by the advisory scope-conformance check above; a blank or omitted value leaves the 4-field on-disk entry shape unchanged. Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps) → {root, reason}` (a projection over `resolveWorktreeContext`; returns the `reason` alongside `root` — a `git_timed_out` reason means `root` is a best-effort cwd fallback, not a confirmed resolution, and callers must surface that risk rather than trust it silently, #3050) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module.
|
||||
|
||||
Reference in New Issue
Block a user