diff --git a/.changeset/kind-lynx-wake.md b/.changeset/kind-lynx-wake.md new file mode 100644 index 000000000..5e0c7a316 --- /dev/null +++ b/.changeset/kind-lynx-wake.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3767 +--- +**A malformed config section no longer destroys the value it holds — or gets written back to disk.** If `.planning/config.json` had a `git` or `planning` key holding a string instead of an object, migrating a legacy top-level key into it expanded that string into numbered character keys (`"main"` became `{"0":"m","1":"a","2":"i","3":"n"}`), and the result was saved over the original file — so the value could not be recovered. Numbers and booleans were dropped outright. The migration is now declined instead: the section, the legacy key, and the file are left exactly as written, and a warning names the file so it can be fixed by hand. (#3760) diff --git a/CONTEXT.md b/CONTEXT.md index ef3432b0c..808cf1812 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -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 { 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` 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 `\0` — **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 `\0` — **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 `; 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. diff --git a/src/config-loader.cts b/src/config-loader.cts index 83c2daae7..21657a349 100644 --- a/src/config-loader.cts +++ b/src/config-loader.cts @@ -12,7 +12,8 @@ * * Dependencies (leaf modules only): * - node:fs / node:os / node:path (stdlib) - * - ./configuration.cjs (normalizeLegacyKeys, CONFIG_DEFAULTS as CANONICAL_CONFIG_DEFAULTS) + * - ./configuration.cjs (normalizeLegacyKeys, isConfigSection, CONFIG_DEFAULTS as CANONICAL_CONFIG_DEFAULTS) + * - ./unusable-input.cjs (warnUnusableInput, UNUSABLE_REASON — #3760) * - ./config-schema.cjs (VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS) * - ./planning-workspace.cjs (planningDir, planningRoot) * - ./shell-command-projection.cjs (execGit, platformWriteSync, platformReadSync) @@ -31,11 +32,17 @@ const { planningDir, planningRoot } = planningWorkspace; import coreUtilsModule = require('./core-utils.cjs'); const { detectSubRepos } = coreUtilsModule; // ─── Configuration Module (generated CJS mirror) ──────────────────────────── -import { CONFIG_DEFAULTS as CANONICAL_CONFIG_DEFAULTS, normalizeLegacyKeys } from './configuration.cjs'; +import { CONFIG_DEFAULTS as CANONICAL_CONFIG_DEFAULTS, normalizeLegacyKeys, isConfigSection } from './configuration.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import configSchema = require('./config-schema.cjs'); const { VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS, isCentralConfigKey: _isCentralConfigKeyFn } = configSchema; import { KNOWN_RUNTIMES, KNOWN_PROVIDERS, ADAPTIVE_TIER_VALUES } from './model-catalog.cjs'; +// #3760: the ADR-1411 out-of-band diagnostic seam. loadConfig returns `.config` +// alone, so an in-band `skipped` record would be unreachable to nearly every +// caller — "a reason no caller reads is an unreachable field" (ADR-1411). +// eslint-disable-next-line @typescript-eslint/no-require-imports +import unusableInputModule = require('./unusable-input.cjs'); +const { UNUSABLE_REASON: _UNUSABLE_REASON, warnUnusableInput: _warnUnusableInput } = unusableInputModule; // ─── Federated Config (ADR-857 phase 3b) ───────────────────────────────────── // eslint-disable-next-line @typescript-eslint/no-require-imports import federatedConfigModule = require('./federated-config.cjs'); @@ -680,13 +687,25 @@ function loadConfigResolved(cwd: string, options: Record = {}): } if (rootRead.kind !== 'ok') throw new Error('root config absent or unusable'); rootParsed = rootRead.data; - const { parsed: rootNormalized, normalizations: rootNorms } = normalizeLegacyKeys(rootParsed); + const { parsed: rootNormalized, normalizations: rootNorms, skipped: rootSkipped } = normalizeLegacyKeys(rootParsed); + if (rootSkipped.length > 0) { + _warnUnusableInput({ reason: _UNUSABLE_REASON.CONFIG_SECTION_NOT_OBJECT, source: rootConfigPath }); + } if (rootNorms.length > 0) { for (const norm of rootNorms as unknown as NormalizationEntry[]) { if (norm.requiresFilesystem && !(rootNormalized as ParsedConfig).planning?.['sub_repos']) { const detected = getDetectedSubRepos(); if (detected.length > 0) { - if (!(rootNormalized as ParsedConfig).planning) (rootNormalized as ParsedConfig).planning = {}; + // #3760: `if (!planning) planning = {}` treated a non-empty STRING as an + // already-present section, and the next line then assigned onto a + // primitive — a strict-mode TypeError the enclosing catch swallowed, + // discarding the user's whole config. `requiresFilesystem` now only + // reaches here when the section is absent or an object (configuration.cts + // block 3 refuses otherwise and reports it via `skipped`), so this + // narrowing chooses between merge and create and never discards. + if (!isConfigSection((rootNormalized as ParsedConfig).planning)) { + (rootNormalized as ParsedConfig).planning = {}; + } (rootNormalized as ParsedConfig).planning!['sub_repos'] = detected; (rootNormalized as ParsedConfig).planning!['commit_docs'] = false; } @@ -720,7 +739,10 @@ function loadConfigResolved(cwd: string, options: Record = {}): let configDirty = false; { - const { parsed: normalized, normalizations } = normalizeLegacyKeys(fileData); + const { parsed: normalized, normalizations, skipped } = normalizeLegacyKeys(fileData); + if (skipped.length > 0) { + _warnUnusableInput({ reason: _UNUSABLE_REASON.CONFIG_SECTION_NOT_OBJECT, source: configPath }); + } if (normalizations.length > 0) { Object.keys(fileData).forEach(k => delete (fileData as Record)[k]); Object.assign(fileData, normalized); @@ -729,7 +751,8 @@ function loadConfigResolved(cwd: string, options: Record = {}): if (norm.requiresFilesystem && !fileData.planning?.['sub_repos']) { const detected = getDetectedSubRepos(); if (detected.length > 0) { - if (!fileData.planning) fileData.planning = {}; + // #3760 — see the identical guard on the root-config path above. + if (!isConfigSection(fileData.planning)) fileData.planning = {}; fileData.planning['sub_repos'] = detected; fileData.planning['commit_docs'] = false; } @@ -744,7 +767,10 @@ function loadConfigResolved(cwd: string, options: Record = {}): if (detected.length > 0) { const sorted = [...currentSubRepos].sort(); if (JSON.stringify(sorted) !== JSON.stringify(detected)) { - if (!fileData.planning) fileData.planning = {}; + // #3760 — reachable only when `planning` already yielded a non-empty + // sub_repos array, so it is an object here; the narrowing keeps the + // assignment total rather than relying on that from three frames away. + if (!isConfigSection(fileData.planning)) fileData.planning = {}; fileData.planning['sub_repos'] = detected; configDirty = true; } diff --git a/src/config.cts b/src/config.cts index 639c66f05..a757ebf6b 100644 --- a/src/config.cts +++ b/src/config.cts @@ -28,6 +28,13 @@ const { VALID_CONFIG_KEYS, isValidConfigKey, getCapabilityConfigSchema } = confi import { isSecretKey, maskSecret } from './secrets.cjs'; import { normalizeConfiguredDefaultReviewers, INSTANCE_NAME_PATTERN, KNOWN_REVIEWER_SLUGS } from './review-reviewer-selection.cjs'; import { migrateOnDisk } from './configuration.cjs'; +// #3760: the ADR-1411 out-of-band diagnostic. It lives here rather than inside +// `migrateOnDisk` because `configuration.cjs` must stay loadable from an install +// layout holding only itself plus its manifests (#3571) — see the note at the top +// of configuration.cts. +// eslint-disable-next-line @typescript-eslint/no-require-imports +import unusableInputModule = require('./unusable-input.cjs'); +const { UNUSABLE_REASON, warnUnusableInput } = unusableInputModule; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -1199,14 +1206,38 @@ function cmdMigrateConfig(cwd: string, raw: boolean): void { const ws = process.env['GSD_WORKSTREAM'] || null; const report = migrateOnDisk(cwd, ws || undefined); + // #3760: deduplicated on (path, reason), so a repeated invocation stays quiet. + if (report.skipped.length > 0) { + warnUnusableInput({ + reason: UNUSABLE_REASON.CONFIG_SECTION_NOT_OBJECT, + source: path.join(planningDir(cwd, ws || undefined), 'config.json'), + }); + } + if (raw) { - if (!report.migrated) { + // #3760: a refused migration is NOT an already-canonical config. Reporting + // "no legacy keys found" when a legacy key was found and declined would send + // the user away believing there is nothing to fix — and the thing to fix is + // the one thing only they can fix, by hand. + const declined = (report.skipped as Array<{ from: string; to: string; section: string; sectionType: string }>); + const declinedLines = declined.map( + s => ` ${s.from} → ${s.to} SKIPPED: '${s.section}' holds a ${s.sectionType}, not an object`, + ); + if (!report.migrated && declined.length === 0) { const msg = 'No legacy keys found — config is already canonical.'; output(msg, true, msg); + } else if (!report.migrated) { + const lines = [ + 'Not migrated — every legacy key found was left in place:', + ...declinedLines, + 'Fix the section by hand, then re-run. Nothing was written.', + ].join('\n'); + output(lines, true, lines); } else { const lines = [ `Migrated: ${String(report.wrote)}`, ...(report.normalizations as Array<{ from: string; to: string }>).map(n => ` ${n.from} → ${n.to}`), + ...declinedLines, ].join('\n'); output(lines, true, lines); } diff --git a/src/configuration.cts b/src/configuration.cts index 36e5c4268..815ed1be3 100644 --- a/src/configuration.cts +++ b/src/configuration.cts @@ -12,6 +12,16 @@ import { readFileSync, writeFileSync, existsSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; +// ⚠️ DO NOT add a sibling `.cjs` import to this module. `configuration.cjs` must load +// from an install layout containing ONLY itself plus `bin/shared/*.manifest.json` — +// that is the #3571 contract, pinned by "co-located bin/shared manifests let +// configuration.cjs load without sdk/shared" in tests/install.test.cjs. A `require` +// for a sibling that the installer does not co-locate fails at load time with +// MODULE_NOT_FOUND. This is why #3760's out-of-band diagnostic is emitted by this +// module's CALLERS (`cmdMigrateConfig` in config.cts, `loadConfigResolved` in +// config-loader.cts) rather than here: `normalizeLegacyKeys` reports refusals +// in-band via `skipped[]`, which keeps it both pure AND dependency-free. + // In .cts (CommonJS output) files, `require` is available as a global. const _require: NodeRequire = require; @@ -129,53 +139,167 @@ interface Normalization { requiresFilesystem?: boolean; } +/** + * A legacy-key migration that could not run because its destination section is + * present but is not an object. + * + * This is deliberately NOT a `Normalization`. Every caller treats a non-empty + * `normalizations` array as "the config changed, write it back" — reporting a + * refusal there would persist a migration that did not happen. Reporting it + * here keeps the two claims separate: `normalizations` is what changed, + * `skipped` is what was declined and why. (#3760) + */ +interface SkippedNormalization { + /** The legacy top-level key that was left in place. */ + from: string; + /** Where it would have gone, had the section been usable. */ + to: string; + /** The section key that blocked the migration (`git`, `planning`). */ + section: string; + reason: 'non_object_section'; + /** The legacy value, preserved. */ + value: unknown; + /** + * What the section holds, as a type name — `'string' | 'number' | 'boolean' | + * 'array'` — NOT the value itself. + * + * The type is the whole diagnostic ("this is a string where an object belongs"), + * and the value is still sitting untouched in the file, so echoing it buys + * nothing. It would cost something: `cmdMigrateConfig` prints this report + * verbatim on `gsd-tools migrate-config --json`, and config values are treated + * as potentially secret-bearing elsewhere in this module (`maskSecret` on the + * `config set/unset` output path). + */ + sectionType: string; +} + +/** Type name for a report — `'array'` for arrays, otherwise `typeof`. */ +function describeSectionType(value: unknown): string { + return Array.isArray(value) ? 'array' : typeof value; +} + interface NormalizeLegacyKeysResult { parsed: Record; normalizations: Normalization[]; + /** Migrations declined because the destination section is a non-object (#3760). */ + skipped: SkippedNormalization[]; } interface MigrateOnDiskResult { migrated: boolean; normalizations: Normalization[]; wrote: string | null; + /** Migrations declined because the destination section is a non-object (#3760). */ + skipped: SkippedNormalization[]; +} + +/** + * Is `value` usable as a config SECTION — something a nested key can be written into? + * + * Three cases, and the distinction between the second and third is the whole of #3760: + * + * - `null` / `undefined` — ABSENT. Not a section yet, but nothing is lost by creating one. + * Callers substitute `{}`; this predicate reports `false` and callers check for absence + * separately, so the two are never conflated. + * - a non-null, non-array `object` — a SECTION. Spreading it is safe and correct. + * - anything else (string, number, boolean, array) — a PRESENT NON-OBJECT, i.e. user data + * that a spread would destroy. `{...'main'}` is `{0:'m',1:'a',2:'i',3:'n'}`; `{...7}` is + * `{}`. An array is included here because `typeof [] === 'object'` and `{...['a']}` is + * `{0:'a'}` — the identical expansion a bare `typeof` guard would wave through. + * + * This is the nested-section analog of the top-level shape check ADR-227 already requires + * in `_readConfigFile` (`config-loader.cts`): valid JSON is not the same as a config object. + * It lives here, exported, rather than being copied into `config-loader.cts`, because two + * hand-rolled copies of one predicate is `DEFECT.GENERATIVE-FIX` by construction — the same + * reasoning that made `unusable-input.cts` a shared seam. + */ +function isConfigSection(value: unknown): value is Record { + return value !== null && typeof value === 'object' && !Array.isArray(value); } // ─── Exported functions ─────────────────────────────────────────────────────── + +/** + * Hoist a legacy top-level key into its canonical nested section. + * + * Refuses — and reports the refusal — when the destination section is present but is not an + * object. Refusing means leaving BOTH the section and the legacy key exactly as they were and + * pushing no `Normalization`, so this block never marks the config dirty and nothing it + * touched can be written back. That is the #3760 contract: never expand, never drop, never + * persist a shape the user did not write. + */ +function hoistLegacyKey( + result: Record, + normalizations: Normalization[], + skipped: SkippedNormalization[], + legacyKey: string, + section: string, + field: string, +): void { + const raw = result[section]; + // `null`/`undefined` mean "no section yet" and have always been treated as absent here. + // They carry no user data, so creating the section loses nothing. + const absent = raw === null || raw === undefined; + if (!absent && !isConfigSection(raw)) { + skipped.push({ + from: legacyKey, + to: `${section}.${field}`, + section, + reason: 'non_object_section', + value: result[legacyKey], + sectionType: describeSectionType(raw), + }); + return; + } + // `raw` is already narrowed by the isConfigSection guard above. + const existing: Record = absent ? {} : raw; + const value = result[legacyKey]; + // Canonical nested wins when it is already set; otherwise the legacy value is hoisted. + result[section] = existing[field] === undefined + ? { ...existing, [field]: value } + : { ...existing }; + delete result[legacyKey]; + normalizations.push({ from: legacyKey, to: `${section}.${field}`, value }); +} + function normalizeLegacyKeys(parsed: Record): NormalizeLegacyKeysResult { const result: Record = { ...parsed }; const normalizations: Normalization[] = []; + const skipped: SkippedNormalization[] = []; // 1. branching_strategy → git.branching_strategy if (Object.prototype.hasOwnProperty.call(result, 'branching_strategy')) { - const value = result['branching_strategy']; - const git = (result['git'] ?? {}) as Record; - if (git['branching_strategy'] === undefined) { - result['git'] = { ...git, branching_strategy: value }; - } - else { - // canonical nested wins — just delete the stale top-level - result['git'] = { ...git }; - } - delete result['branching_strategy']; - normalizations.push({ from: 'branching_strategy', to: 'git.branching_strategy', value }); + hoistLegacyKey(result, normalizations, skipped, 'branching_strategy', 'git', 'branching_strategy'); } // 2. top-level sub_repos → planning.sub_repos if (Object.prototype.hasOwnProperty.call(result, 'sub_repos')) { - const value = result['sub_repos']; - const planning = (result['planning'] ?? {}) as Record; - if (planning['sub_repos'] === undefined) { - result['planning'] = { ...planning, sub_repos: value }; - } - else { - // canonical nested wins — just drop the stale top-level - result['planning'] = { ...planning }; - } - delete result['sub_repos']; - normalizations.push({ from: 'sub_repos', to: 'planning.sub_repos', value }); + hoistLegacyKey(result, normalizations, skipped, 'sub_repos', 'planning', 'sub_repos'); } // 3. multiRepo: true → marker (filesystem detection deferred to migrateOnDisk / caller) if (result['multiRepo'] === true) { - delete result['multiRepo']; - normalizations.push({ from: 'multiRepo', to: 'planning.sub_repos', value: true, requiresFilesystem: true }); + // #3760: refuse here too, for the same reason as blocks 1 and 2 — and it must be + // decided HERE, not in the caller. The caller is the one that runs filesystem + // detection, but whether `planning` can receive the result is knowable from the + // parsed config alone. Deciding it later meant `multiRepo` had already been + // deleted and a Normalization already pushed: the config was written, the + // sub_repos injection silently no-opped against the non-object section, and the + // user's `multiRepo: true` was consumed with nothing to show for it and no + // diagnostic. Refusing here keeps the marker, keeps the config clean of a + // migration that did not happen, and gives all three callers the same report. + const planning = result['planning']; + if (planning !== null && planning !== undefined && !isConfigSection(planning)) { + skipped.push({ + from: 'multiRepo', + to: 'planning.sub_repos', + section: 'planning', + reason: 'non_object_section', + value: true, + sectionType: describeSectionType(planning), + }); + } + else { + delete result['multiRepo']; + normalizations.push({ from: 'multiRepo', to: 'planning.sub_repos', value: true, requiresFilesystem: true }); + } } // 4. top-level depth → granularity if (Object.prototype.hasOwnProperty.call(result, 'depth') && !Object.prototype.hasOwnProperty.call(result, 'granularity')) { @@ -185,7 +309,7 @@ function normalizeLegacyKeys(parsed: Record): NormalizeLegacyKe delete result['depth']; normalizations.push({ from: 'depth', to: 'granularity', value: mapped }); } - return { parsed: result, normalizations }; + return { parsed: result, normalizations, skipped }; } function mergeDefaults(parsed: Record): Record { @@ -202,11 +326,11 @@ function migrateOnDisk(cwd: string, workstream?: string): MigrateOnDiskResult { } catch { // File missing — nothing to migrate - return { migrated: false, normalizations: [], wrote: null }; + return { migrated: false, normalizations: [], wrote: null, skipped: [] }; } const trimmed = raw.trim(); if (trimmed === '') { - return { migrated: false, normalizations: [], wrote: null }; + return { migrated: false, normalizations: [], wrote: null, skipped: [] }; } let parsed: unknown; try { @@ -214,23 +338,30 @@ function migrateOnDisk(cwd: string, workstream?: string): MigrateOnDiskResult { } catch { // Malformed — can't migrate - return { migrated: false, normalizations: [], wrote: null }; - } - const { parsed: normalized, normalizations } = normalizeLegacyKeys(parsed as Record); - if (normalizations.length === 0) { - return { migrated: false, normalizations: [], wrote: null }; + return { migrated: false, normalizations: [], wrote: null, skipped: [] }; } + const { parsed: normalized, normalizations, skipped } = normalizeLegacyKeys(parsed as Record); // Resolve multiRepo filesystem detection const result: Record = { ...normalized }; for (const norm of normalizations) { if (norm.requiresFilesystem) { const detected = detectSubRepos(cwd); if (detected.length > 0) { - const planning = (result['planning'] ?? {}) as Record; - result['planning'] = { ...planning, sub_repos: detected, commit_docs: false }; + // #3760: `requiresFilesystem` is pushed by block 3 only when `planning` was + // absent or an object, and nothing between there and here changes it — so + // `isConfigSection` picks between merge and create, and never has to discard. + const planning = result['planning']; + const existing = isConfigSection(planning) ? planning : {}; + result['planning'] = { ...existing, sub_repos: detected, commit_docs: false }; } } } + if (normalizations.length === 0) { + // Nothing changed — and that now includes the case where every legacy key was + // REFUSED. Returning early here is what keeps the corrupted-section input from + // reaching writeFileSync at all. + return { migrated: false, normalizations: [], wrote: null, skipped }; + } try { writeFileSync(configPath, JSON.stringify(result, null, 2)); } @@ -238,13 +369,14 @@ function migrateOnDisk(cwd: string, workstream?: string): MigrateOnDiskResult { const msg = err instanceof Error ? err.message : String(err); throw new Error(`Failed to write migrated config at ${configPath}: ${msg}`); } - return { migrated: true, normalizations, wrote: configPath }; + return { migrated: true, normalizations, wrote: configPath, skipped }; } export { normalizeLegacyKeys, mergeDefaults, migrateOnDisk, + isConfigSection, CONFIG_DEFAULTS, VALID_CONFIG_KEYS, RUNTIME_STATE_KEYS, diff --git a/src/unusable-input.cts b/src/unusable-input.cts index 268542aa6..c1418a273 100644 --- a/src/unusable-input.cts +++ b/src/unusable-input.cts @@ -78,6 +78,18 @@ const UNUSABLE_REASON = Object.freeze({ * planning-snapshot's projectSections field) */ PROJECT_UNREADABLE: 'project_unreadable', + /** + * A config.json parsed cleanly, but a section that a legacy-key migration needed to write + * into (`git`, `planning`) holds something that is not an object — a string, number, boolean + * or array. Distinct from the section being absent: absence is the ordinary path and the + * migration simply creates the section. Only a PRESENT non-object is corruption, and it is + * exactly the ADR-227 "shape, not just parseability" class one level down from the top-level + * document check `_readConfigFile` already performs. The migration is refused rather than + * applied — spreading the value would enumerate a string into character keys, and a number + * or boolean into nothing at all — so the section, the legacy key, and the file on disk are + * all left exactly as the user wrote them. (#3760, tenth #1879 site) + */ + CONFIG_SECTION_NOT_OBJECT: 'config_section_not_object', } as const); type UnusableReason = (typeof UNUSABLE_REASON)[keyof typeof UNUSABLE_REASON]; @@ -96,6 +108,8 @@ const REASON_PROSE: Readonly> = Object.freeze({ 'config.json exists but could not be read or parsed; the config field fell back to unavailable', [UNUSABLE_REASON.PROJECT_UNREADABLE]: 'PROJECT.md exists but could not be read; the projectSections field fell back to unavailable', + [UNUSABLE_REASON.CONFIG_SECTION_NOT_OBJECT]: + 'a config section that a legacy-key migration targets holds a non-object value; the migration was SKIPPED and both the section and the legacy key were left unchanged — fix the section by hand, or the legacy key will never migrate', }); // ─── Dedup state ────────────────────────────────────────────────────────────── diff --git a/tests/config-loader.test.cjs b/tests/config-loader.test.cjs index edb1c13ec..72a5fdb22 100644 --- a/tests/config-loader.test.cjs +++ b/tests/config-loader.test.cjs @@ -1455,3 +1455,149 @@ describe('#3532 shadowed global-defaults warning', () => { assert.equal(configLoader._warnedShadowedGlobalKeys.size, 0); }); }); + +// ─── Regressions — #3760: a non-object section must not be expanded or written ── +// +// The loader is the surface that actually WRITES: a non-empty `normalizations` +// array marks the config dirty and `platformWriteSync` persists it. Before the +// fix a config holding `{"git":"main","branching_strategy":"none"}` came back +// from `normalizeLegacyKeys` as `{"git":{"0":"m","1":"a","2":"i","3":"n",...}}` +// and that object was written to the user's file — the original `"main"` was +// unrecoverable afterwards. +// +// The loader's own multiRepo branches carried a second, quieter form of the +// same defect: `if (!fileData.planning) fileData.planning = {}` treats a +// non-empty STRING as an already-present section, so the next line +// (`fileData.planning['sub_repos'] = detected`) threw a strict-mode TypeError. +// That throw was swallowed by the enclosing catch, which discarded the entire +// configuration and fell back to defaults — the ADR-1411 silent-fallback +// failure, reached from an input the user can trivially write by hand. + +describe('regressions — #3760 loader never expands or persists a non-object section', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = makeTempProject('gsd-3760-loader-'); + _resetRuntimeWarningCacheForTests(); + }); + + afterEach(() => { + if (tmpDir) cleanup(tmpDir); + tmpDir = null; + }); + + function readRawConfig() { + return fs.readFileSync(path.join(tmpDir, '.planning', 'config.json'), 'utf-8'); + } + + test('loadConfig leaves a string git section untouched on disk', () => { + writeConfig(tmpDir, { git: 'main', branching_strategy: 'none' }); + const before = readRawConfig(); + + let config; + assert.doesNotThrow(() => { config = loadConfig(tmpDir); }); + + assert.equal(readRawConfig(), before, 'the config file must not be rewritten with an expanded section'); + // The loader projects `git.branching_strategy` to a flat top-level key, so the + // legacy value the migration declined to move is still what resolution sees. + assert.equal(config.branching_strategy, 'none', 'the legacy value must still resolve'); + }); + + test('loadConfig leaves a string planning section untouched on disk', () => { + writeConfig(tmpDir, { planning: 'docs', sub_repos: ['a'] }); + const before = readRawConfig(); + + assert.doesNotThrow(() => { loadConfig(tmpDir); }); + + assert.equal(readRawConfig(), before); + assert.equal(JSON.parse(readRawConfig()).planning, 'docs'); + }); + + test('loadConfig does not discard the config when multiRepo meets a string planning section', () => { + // Pre-fix the loader threw a TypeError assigning sub_repos onto the string, + // and the enclosing catch discarded the user's entire config. It also + // consumed `multiRepo` and wrote the file back with no diagnostic at all. + fs.mkdirSync(path.join(tmpDir, 'sub', '.git'), { recursive: true }); + writeConfig(tmpDir, { planning: 'docs', multiRepo: true, model_profile: 'balanced' }); + const before = readRawConfig(); + + let config; + assert.doesNotThrow(() => { config = loadConfig(tmpDir); }); + + assert.equal( + config.model_profile, 'balanced', + 'an unrelated user setting must survive — a swallowed TypeError would have reverted it to the default', + ); + assert.equal( + readRawConfig(), before, + 'nothing migrated, so the file must be byte-identical — planning intact AND multiRepo still present', + ); + }); + + test('multiRepo still migrates normally when the planning section is usable', () => { + // Negative space: refusing on a bad section must not break the good path. + fs.mkdirSync(path.join(tmpDir, 'sub', '.git'), { recursive: true }); + writeConfig(tmpDir, { multiRepo: true }); + + assert.doesNotThrow(() => { loadConfig(tmpDir); }); + + assert.deepEqual(JSON.parse(readRawConfig()).planning.sub_repos, ['sub']); + assert.equal(JSON.parse(readRawConfig()).multiRepo, undefined, 'the marker is consumed once honored'); + }); + + test('loadConfigResolved reports a usable resolution and writes no expanded section', () => { + writeConfig(tmpDir, { git: 'main', branching_strategy: 'none' }); + const before = readRawConfig(); + + let resolution; + assert.doesNotThrow(() => { resolution = loadConfigResolved(tmpDir); }); + + assert.equal(readRawConfig(), before); + assert.equal(resolution.source, 'root', 'a present, parseable config still resolves from the project'); + assert.equal(resolution.degraded, false); + }); + + test('a refused section emits exactly one deduplicated diagnostic, and a repeat emits none', () => { + // The loader is where the out-of-band diagnostic has to live: loadConfig + // returns `.config` alone, so an in-band `skipped` record would be unreachable + // to nearly every caller (ADR-1411: "a reason no caller reads is an + // unreachable field"). Asserted on the typed emission counter, never by + // scraping stderr prose. + const { + _resetUnusableInputWarningsForTests, + _unusableInputEmissionCountForTests, + } = require('../gsd-core/bin/lib/unusable-input.cjs'); + + function emissionsDuring(fn) { + const before = _unusableInputEmissionCountForTests(); + const original = process.stderr.write; + process.stderr.write = () => true; + try { fn(); } finally { process.stderr.write = original; } + return _unusableInputEmissionCountForTests() - before; + } + + _resetUnusableInputWarningsForTests(); + writeConfig(tmpDir, { git: 'main', branching_strategy: 'none' }); + + const first = emissionsDuring(() => { loadConfig(tmpDir); }); + assert.equal(first, 1, 'the operator must be told once'); + + const second = emissionsDuring(() => { loadConfig(tmpDir); }); + assert.equal(second, 0, 'the ADR-1411 dedup guard must suppress the repeat'); + }); + + test('an already-canonical config is still migrated normally', () => { + // Negative space: the guard must not suppress a legitimate hoist. + writeConfig(tmpDir, { git: { remote: 'origin' }, branching_strategy: 'none' }); + + const config = loadConfig(tmpDir); + // Assert on the FILE for the section shape (the loader flattens `git.*` into + // top-level keys, so the resolved object has no `git` to inspect) and on the + // resolved value for the projection. + const onDisk = JSON.parse(readRawConfig()); + assert.equal(onDisk.git.branching_strategy, 'none', 'a well-formed section must still receive the hoisted key'); + assert.equal(onDisk.git.remote, 'origin', 'existing section keys must be preserved'); + assert.equal(onDisk.branching_strategy, undefined, 'the stale top-level key is consumed'); + assert.equal(config.branching_strategy, 'none', 'the hoisted value still resolves'); + }); +}); diff --git a/tests/configuration-migrate-config.test.cjs b/tests/configuration-migrate-config.test.cjs index 964d92234..4430f1565 100644 --- a/tests/configuration-migrate-config.test.cjs +++ b/tests/configuration-migrate-config.test.cjs @@ -193,3 +193,420 @@ test('mergeDefaults clones defaults without JSON serialization fragility (#321)' }); }); } + +// ──────────────────────────────────────────────────────────────────────── +// Regressions — #3760: a non-object legacy-key SECTION must never be spread +// into character keys, never be silently dropped, and never be persisted. +// +// `normalizeLegacyKeys` hoisted a legacy top-level key into its canonical +// nested section by spreading whatever `result[section] ?? {}` produced. `??` +// guards only null/undefined, so a section holding a STRING was enumerated by +// index — `{...'main'}` is `{0:'m',1:'a',2:'i',3:'n'}` — and a number or +// boolean spread to `{}`, dropping the value outright. Because a fired block +// always pushes a Normalization, and every caller treats a non-empty +// `normalizations` array as "config is dirty", the mangled object was WRITTEN +// BACK to .planning/config.json and the original value became unrecoverable. +// +// The contract these cases lock (issue #3760, "Expected"): preserve the value, +// report it, never expand, never drop, never persist. +// ──────────────────────────────────────────────────────────────────────── +{ + const { describe: __d3760, test: __t3760, beforeEach: __be3760 } = require('node:test'); + const __assert3760 = require('node:assert/strict'); + const __fs3760 = require('node:fs'); + const __os3760 = require('node:os'); + const __path3760 = require('node:path'); + const __fc3760 = require('./helpers/fast-check-setup.cjs'); + const { cleanup: __cleanup3760 } = require('./helpers.cjs'); + const __configuration3760 = require('../gsd-core/bin/lib/configuration.cjs'); + const { + UNUSABLE_REASON: __REASON3760, + _resetUnusableInputWarningsForTests: __resetWarn3760, + _unusableInputEmissionCountForTests: __emissions3760, + } = require('../gsd-core/bin/lib/unusable-input.cjs'); + + const { normalizeLegacyKeys: __normalize3760, migrateOnDisk: __migrate3760 } = __configuration3760; + + /** + * Run `fn` with stderr swallowed and report how many NEW diagnostics it wrote. + * The count comes from the typed seam, never from parsing stderr prose + * (CONTRIBUTING.md — Prohibited: Raw Text Matching on Test Outputs). Restored + * in a `finally` inside this standalone helper, which is the one place + * CONTRIBUTING.md permits try/finally. + */ + function __emissionsDuring3760(fn) { + const before = __emissions3760(); + const original = process.stderr.write; + process.stderr.write = () => true; + try { + fn(); + } finally { + process.stderr.write = original; + } + return __emissions3760() - before; + } + + /** A temp project carrying `.planning/config.json` with exactly `text`. */ + function __project3760(text) { + const dir = __fs3760.mkdtempSync(__path3760.join(__os3760.tmpdir(), 'gsd-3760-')); + __fs3760.mkdirSync(__path3760.join(dir, '.planning'), { recursive: true }); + __fs3760.writeFileSync(__path3760.join(dir, '.planning', 'config.json'), text, 'utf-8'); + return dir; + } + + function __readConfig3760(dir) { + return __fs3760.readFileSync(__path3760.join(dir, '.planning', 'config.json'), 'utf-8'); + } + + // ── normalizeLegacyKeys — the two blocks named in #3760 ────────────────── + + __d3760('regressions — #3760 normalizeLegacyKeys non-object section', () => { + __t3760('block 1 — a string git section is preserved, never expanded into character keys', () => { + const { parsed, normalizations, skipped } = __normalize3760({ git: 'main', branching_strategy: 'none' }); + + __assert3760.strictEqual( + parsed.git, 'main', + 'the string section must survive verbatim, not become {0:"m",1:"a",...}', + ); + __assert3760.strictEqual( + parsed.branching_strategy, 'none', + 'the legacy top-level key must be preserved, not deleted into a section that could not accept it', + ); + __assert3760.deepStrictEqual( + normalizations, [], + 'a block that could not run must not report a normalization — a reported normalization is what marks the config dirty and gets it written', + ); + __assert3760.strictEqual(skipped.length, 1, 'the refusal must be reported, not silent'); + __assert3760.deepStrictEqual(skipped[0], { + from: 'branching_strategy', + to: 'git.branching_strategy', + section: 'git', + reason: 'non_object_section', + value: 'none', + sectionType: 'string', + }); + // The offending section VALUE is deliberately absent from the report: it is + // still in the file untouched, and this report is printed verbatim by + // `migrate-config --json`. + __assert3760.ok( + !Object.prototype.hasOwnProperty.call(skipped[0], 'sectionValue'), + 'the report must name the section TYPE, never echo its value', + ); + }); + + __t3760('block 2 — a string planning section is preserved, never expanded into character keys', () => { + const { parsed, normalizations, skipped } = __normalize3760({ planning: 'docs', sub_repos: ['a'] }); + + __assert3760.strictEqual(parsed.planning, 'docs'); + __assert3760.deepStrictEqual(parsed.sub_repos, ['a']); + __assert3760.deepStrictEqual(normalizations, []); + __assert3760.strictEqual(skipped.length, 1); + __assert3760.strictEqual(skipped[0].section, 'planning'); + __assert3760.strictEqual(skipped[0].to, 'planning.sub_repos'); + }); + + __t3760('a number section is preserved, not silently dropped', () => { + // `{...7}` is `{}` — the pre-fix code dropped the 7 without a trace, which + // is the SAME data loss as the character-key expansion, only quieter. + const { parsed, normalizations, skipped } = __normalize3760({ git: 7, branching_strategy: 'none' }); + __assert3760.strictEqual(parsed.git, 7); + __assert3760.strictEqual(parsed.branching_strategy, 'none'); + __assert3760.deepStrictEqual(normalizations, []); + __assert3760.strictEqual(skipped.length, 1); + }); + + __t3760('an array section is preserved verbatim', () => { + // typeof [] === 'object', so a plain typeof guard would let an array + // through and `{...['a','b']}` is `{0:'a',1:'b'}` — the same expansion. + const { parsed, normalizations, skipped } = __normalize3760({ git: ['a', 'b'], branching_strategy: 'none' }); + __assert3760.deepStrictEqual(parsed.git, ['a', 'b']); + __assert3760.strictEqual(parsed.branching_strategy, 'none'); + __assert3760.deepStrictEqual(normalizations, []); + __assert3760.strictEqual(skipped.length, 1); + }); + + __t3760('a boolean section is preserved', () => { + const { parsed, normalizations, skipped } = __normalize3760({ git: true, branching_strategy: 'none' }); + __assert3760.strictEqual(parsed.git, true); + __assert3760.deepStrictEqual(normalizations, []); + __assert3760.strictEqual(skipped.length, 1); + }); + + __t3760('an empty-string section is preserved (falsy but still a non-object)', () => { + // Boundary: '' is falsy, so a truthiness-based guard would treat it as + // absent and overwrite it. It is a value the user wrote; it survives. + const { parsed, normalizations, skipped } = __normalize3760({ git: '', branching_strategy: 'none' }); + __assert3760.strictEqual(parsed.git, ''); + __assert3760.strictEqual(parsed.branching_strategy, 'none'); + __assert3760.deepStrictEqual(normalizations, []); + __assert3760.strictEqual(skipped.length, 1); + }); + + // ── negative space: these must keep working exactly as before ────────── + + __t3760('an empty-object section still hoists (limit: {} IS a valid section)', () => { + const { parsed, normalizations, skipped } = __normalize3760({ git: {}, branching_strategy: 'none' }); + __assert3760.deepStrictEqual(parsed.git, { branching_strategy: 'none' }); + __assert3760.strictEqual(parsed.branching_strategy, undefined); + __assert3760.strictEqual(normalizations.length, 1); + __assert3760.deepStrictEqual(skipped, []); + }); + + __t3760('a null section is treated as absent and still hoists (limit-1)', () => { + const { parsed, normalizations, skipped } = __normalize3760({ git: null, branching_strategy: 'none' }); + __assert3760.deepStrictEqual(parsed.git, { branching_strategy: 'none' }); + __assert3760.strictEqual(normalizations.length, 1); + __assert3760.deepStrictEqual(skipped, []); + }); + + __t3760('an absent section is created and the legacy key hoisted', () => { + const { parsed, normalizations, skipped } = __normalize3760({ branching_strategy: 'none' }); + __assert3760.deepStrictEqual(parsed.git, { branching_strategy: 'none' }); + __assert3760.strictEqual(normalizations.length, 1); + __assert3760.deepStrictEqual(skipped, []); + }); + + __t3760('a canonical nested value still wins over the stale top-level', () => { + const { parsed, normalizations, skipped } = __normalize3760({ + git: { branching_strategy: 'x' }, + branching_strategy: 'y', + }); + __assert3760.deepStrictEqual(parsed.git, { branching_strategy: 'x' }); + __assert3760.strictEqual(parsed.branching_strategy, undefined); + __assert3760.strictEqual(normalizations.length, 1); + __assert3760.deepStrictEqual(skipped, []); + }); + }); + + // ── property: no JSON value ever becomes character keys ────────────────── + + __d3760('regressions — #3760 properties', () => { + __t3760('property — a section is never expanded into character keys, for any JSON value', () => { + const sectionArb = __fc3760.oneof( + __fc3760.string(), + __fc3760.integer(), + __fc3760.boolean(), + __fc3760.constant(null), + __fc3760.array(__fc3760.string()), + __fc3760.dictionary(__fc3760.string(), __fc3760.string()), + ); + __fc3760.assert( + __fc3760.property(sectionArb, __fc3760.string(), (section, legacy) => { + const { parsed } = __normalize3760({ git: section, branching_strategy: legacy }); + const out = parsed.git; + if (section === null || (typeof section === 'object' && !Array.isArray(section))) { + // Absent-or-object: the hoist is legitimate and must still happen. + __assert3760.ok(out !== null && typeof out === 'object' && !Array.isArray(out)); + return true; + } + // Every non-object (string, number, boolean, array) survives verbatim. + // For a string this is the exact defect: no index keys derived from + // the string's characters may appear anywhere in the output. + __assert3760.deepStrictEqual(out, section); + return true; + }), + { numRuns: 200 }, + ); + }); + + __t3760('property — normalization stays idempotent across the skip path', () => { + // CONTEXT.md:104 states normalizeLegacyKeys is idempotent. The skip path + // must not break that: re-running over its own output must be a no-op. + const sectionArb = __fc3760.oneof( + __fc3760.string(), + __fc3760.integer(), + __fc3760.constant(null), + __fc3760.array(__fc3760.string()), + __fc3760.dictionary(__fc3760.string(), __fc3760.string()), + ); + __fc3760.assert( + __fc3760.property(sectionArb, (section) => { + const once = __normalize3760({ planning: section, sub_repos: ['a'] }).parsed; + const twice = __normalize3760({ ...once }).parsed; + __assert3760.deepStrictEqual(twice, once); + return true; + }), + { numRuns: 200 }, + ); + }); + }); + + // ── migrateOnDisk — the corruption must never reach the file ───────────── + + __d3760('regressions — #3760 migrateOnDisk never persists a corrupted section', () => { + let dirs; + __be3760(() => { dirs = []; __resetWarn3760(); }); + + function track(dir) { dirs.push(dir); return dir; } + + __t3760('a non-object git section leaves the file byte-identical', () => { + const text = JSON.stringify({ git: 'main', branching_strategy: 'none' }, null, 2); + const dir = track(__project3760(text)); + try { + let result; + __emissionsDuring3760(() => { result = __migrate3760(dir); }); + __assert3760.strictEqual(result.migrated, false, 'nothing was migrated, so nothing may be written'); + __assert3760.strictEqual(result.wrote, null); + __assert3760.strictEqual( + __readConfig3760(dir), text, + 'the config file must be byte-identical — this is the data loss #3760 is about', + ); + __assert3760.strictEqual(result.skipped.length, 1); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + + __t3760('a CRLF-formatted config with a non-object section is equally untouched', () => { + const text = JSON.stringify({ git: 'main', branching_strategy: 'none' }, null, 2).replace(/\n/g, '\r\n'); + const dir = track(__project3760(text)); + try { + let result; + __emissionsDuring3760(() => { result = __migrate3760(dir); }); + __assert3760.strictEqual(result.migrated, false); + __assert3760.strictEqual(__readConfig3760(dir), text); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + + __t3760('the multiRepo marker is KEPT, not consumed, when planning cannot receive it', () => { + // multiRepo's entire meaning is "detect sub-repos and write them into + // planning.sub_repos". If planning cannot receive them, consuming the marker + // destroys the user's intent and leaves nothing to show for it. The refusal + // is decided in normalizeLegacyKeys block 3, where it is still knowable — + // not in the caller, by which point the marker is already gone. + const text = JSON.stringify({ planning: 'docs', multiRepo: true }, null, 2); + const dir = track(__project3760(text)); + // A real sub-repo, so detectSubRepos() returns a non-empty list. + __fs3760.mkdirSync(__path3760.join(dir, 'sub', '.git'), { recursive: true }); + try { + let result; + __emissionsDuring3760(() => { result = __migrate3760(dir); }); + __assert3760.strictEqual(result.migrated, false, 'nothing migrated, so nothing written'); + __assert3760.strictEqual( + __readConfig3760(dir), text, + 'the file must be byte-identical: planning intact AND multiRepo still present', + ); + __assert3760.strictEqual(result.skipped.length, 1, 'the refusal must be reported'); + __assert3760.strictEqual(result.skipped[0].from, 'multiRepo'); + __assert3760.strictEqual(result.skipped[0].sectionType, 'string'); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + + __t3760('multiRepo still migrates normally when planning is a usable section', () => { + // Negative space: the block-3 guard must not break the ordinary path. + const dir = track(__project3760(JSON.stringify({ planning: { commit_docs: true }, multiRepo: true }, null, 2))); + __fs3760.mkdirSync(__path3760.join(dir, 'sub', '.git'), { recursive: true }); + try { + let result; + __emissionsDuring3760(() => { result = __migrate3760(dir); }); + const after = JSON.parse(__readConfig3760(dir)); + __assert3760.strictEqual(result.migrated, true); + __assert3760.deepStrictEqual(after.planning.sub_repos, ['sub']); + __assert3760.strictEqual(after.multiRepo, undefined, 'the marker is consumed once it has been honored'); + __assert3760.deepStrictEqual(result.skipped, []); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + + __t3760('multiRepo still migrates normally when planning is absent', () => { + const dir = track(__project3760(JSON.stringify({ multiRepo: true }, null, 2))); + __fs3760.mkdirSync(__path3760.join(dir, 'sub', '.git'), { recursive: true }); + try { + let result; + __emissionsDuring3760(() => { result = __migrate3760(dir); }); + const after = JSON.parse(__readConfig3760(dir)); + __assert3760.strictEqual(result.migrated, true); + __assert3760.deepStrictEqual(after.planning.sub_repos, ['sub']); + __assert3760.deepStrictEqual(result.skipped, []); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + + __t3760('migrateOnDisk itself emits nothing — configuration.cjs must stay dependency-free', () => { + // #3571 pins that `configuration.cjs` loads from an install layout holding + // only itself plus its manifests, so it cannot require the diagnostic seam. + // The refusal is reported in-band here; the CALLERS do the emitting. If this + // ever starts emitting, configuration.cjs has grown a sibling require and the + // install layout will fail at load time with MODULE_NOT_FOUND. + const dir = track(__project3760(JSON.stringify({ git: 'main', branching_strategy: 'none' }, null, 2))); + try { + let result; + const emitted = __emissionsDuring3760(() => { result = __migrate3760(dir); }); + __assert3760.strictEqual(emitted, 0, 'the pure module must not reach the diagnostic seam'); + __assert3760.strictEqual(result.skipped.length, 1, 'but it must still report the refusal in-band'); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + + __t3760('the reason is the typed enum entry, not ad-hoc prose', () => { + __assert3760.strictEqual(__REASON3760.CONFIG_SECTION_NOT_OBJECT, 'config_section_not_object'); + }); + }); + + // ── the CLI must not call a refused migration "already canonical" ──────── + + __d3760('regressions — #3760 migrate-config reports a refusal', () => { + let dirs; + __be3760(() => { dirs = []; }); + + __t3760('--json carries the skipped entry and no section value', () => { + const dir = __project3760(JSON.stringify({ git: 'main', branching_strategy: 'none' }, null, 2)); + dirs.push(dir); + try { + const res = runMigrateConfig(dir); + __assert3760.strictEqual(res.status, 0, 'a refusal is a reported result, not a fault'); + const parsed = JSON.parse(res.stdout); + __assert3760.strictEqual(parsed.migrated, false); + __assert3760.strictEqual(parsed.skipped.length, 1); + __assert3760.strictEqual(parsed.skipped[0].sectionType, 'string'); + __assert3760.ok( + !Object.prototype.hasOwnProperty.call(parsed.skipped[0], 'sectionValue'), + 'the JSON surface must not echo the section value', + ); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + + __t3760('--raw does NOT claim the config is already canonical', () => { + // The refusal is the one thing only the user can fix. Telling them there + // is nothing to fix is worse than saying nothing. + const dir = __project3760(JSON.stringify({ git: 'main', branching_strategy: 'none' }, null, 2)); + dirs.push(dir); + try { + const res = runMigrateConfig(dir, ['--raw']); + __assert3760.strictEqual(res.status, 0); + __assert3760.ok( + !res.stdout.includes('already canonical'), + `a declined migration must not be reported as already-canonical; got: ${res.stdout}`, + ); + __assert3760.ok( + res.stdout.includes('branching_strategy'), + 'the declined key must be named so the user knows what to fix', + ); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + + __t3760('--raw still reports an already-canonical config as canonical', () => { + // Negative space: the new branch must not swallow the ordinary no-op message. + const dir = __project3760(JSON.stringify({ git: { branching_strategy: 'none' } }, null, 2)); + dirs.push(dir); + try { + const res = runMigrateConfig(dir, ['--raw']); + __assert3760.strictEqual(res.status, 0); + __assert3760.ok(res.stdout.includes('already canonical'), `got: ${res.stdout}`); + } finally { + for (const d of dirs) __cleanup3760(d); + } + }); + }); +} diff --git a/tests/unusable-input.test.cjs b/tests/unusable-input.test.cjs index 84b9a3328..0c8575977 100644 --- a/tests/unusable-input.test.cjs +++ b/tests/unusable-input.test.cjs @@ -72,7 +72,7 @@ describe('UNUSABLE_REASON', () => { // (enum + call site + this assertion) instead of a silent widening. assert.deepStrictEqual( Object.keys(UNUSABLE_REASON).sort(), - ['CONFIG_UNREADABLE', 'FRONTMATTER_UNTERMINATED', 'LAST_ACTIVITY_UNPARSEABLE', 'PROJECT_UNREADABLE', 'ROADMAP_UNREADABLE', 'STATE_UNREADABLE'], + ['CONFIG_SECTION_NOT_OBJECT', 'CONFIG_UNREADABLE', 'FRONTMATTER_UNTERMINATED', 'LAST_ACTIVITY_UNPARSEABLE', 'PROJECT_UNREADABLE', 'ROADMAP_UNREADABLE', 'STATE_UNREADABLE'], ); assert.strictEqual(UNUSABLE_REASON.FRONTMATTER_UNTERMINATED, 'frontmatter_unterminated'); });