diff --git a/.changeset/eager-tigers-greet.md b/.changeset/eager-tigers-greet.md new file mode 100644 index 000000000..85bb7d28a --- /dev/null +++ b/.changeset/eager-tigers-greet.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2678 +--- +**Editing `src/` no longer trips an undocumented changeset-lint failure** — CONTRIBUTING.md listed the Changeset Required triggers without `src/`, the path that compiles into every `gsd-core/bin/lib/*.cjs`, so contributors touching it hit a CI failure the docs said could not happen — and a local run of the lint reported success regardless, because it silently requires `GITHUB_BASE_REF` to see the branch at all. Both are now documented, and the config-loader test-helper that reset only one of its two warning-dedup sets now resets both. (#2674) diff --git a/CONTEXT.md b/CONTEXT.md index 5c431a91a..fef92faf0 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -110,7 +110,7 @@ Module owning project-root resolution from any starting directory. Walks the anc Module owning projection from project/workstream context to concrete `.planning` paths. Policy precedence is `explicit workstream > env workstream > env project > root`. Invalid workspace context is a validation error at this seam rather than a silent fallback. ### Resolution Provenance -Cross-seam principle (ADR-1411, epic #1411): context resolution — config loading, project-root anchoring, workstream resolution — must report its provenance, not fall open silently to defaults. A resolver anchors deterministically to the project root (one walk-up module, no dependence on an arbitrary descendant cwd), returns *what* it resolved **and** *where it came from* (`source`/`degraded`), and surfaces a diagnostic when a *configured* input resolves empty (`not configured` and `configured-but-empty` are distinguishable). The resolution-side analog of ADR-227 (input-validation shape). Target seams: Config Loader Module (`loadConfig` → `ConfigResolution { config, source, degraded }`), Project-Root Resolution Module (single nearest-`.planning/` walk-up, retiring ad-hoc resolvers like `resolvePlanningCwd`), I/O Module (`Resolution { value, configured, reason, warnings }` output envelope). A configured input resolving empty without a reason is a CI-guarded regression. **P1 (nearest-.planning/ heuristic) shipped in #1413; P2 (loadConfigResolved + agent-skills diagnostic) shipped in #1415 / closes #1366**: `loadConfigResolved` now implements the Config Loader seam target; `cmdAgentSkills` uses `findProjectRoot` + `loadConfigResolved` and emits `configured`/`reason`/`source`/`degraded` in its `--json` IR. +Cross-seam principle (ADR-1411, epic #1411): context resolution — config loading, project-root anchoring, workstream resolution — must report its provenance, not fall open silently to defaults. A resolver anchors deterministically to the project root (one walk-up module, no dependence on an arbitrary descendant cwd), returns *what* it resolved **and** *where it came from* (`source`/`degraded`), and surfaces a diagnostic when a *configured* input resolves empty (`not configured` and `configured-but-empty` are distinguishable). The resolution-side analog of ADR-227 (input-validation shape). Target seams: Config Loader Module (`loadConfig` → `ConfigResolution { config, source, degraded }`), Project-Root Resolution Module (single nearest-`.planning/` walk-up, retiring ad-hoc resolvers like `resolvePlanningCwd`), I/O Module (`Resolution { value, configured, reason, warnings }` output envelope). A configured input resolving empty without a reason is a CI-guarded regression. **P1 (nearest-.planning/ heuristic) shipped in #1413; P2 (loadConfigResolved + agent-skills diagnostic) shipped in #1415 / closes #1366**: `loadConfigResolved` now implements the Config Loader seam target; `cmdAgentSkills` uses `findProjectRoot` + `loadConfigResolved` and emits `configured`/`reason`/`source`/`degraded` in its `--json` IR. **Corrupt is not absent (ADR-1411 amendment 2026-07-26, epic #1879 Phase 0 / #2674):** the principle above governs a resolution *miss*; input that is present but *not usable* (a `SyntaxError`, an errno such as `EACCES`/`EIO`, or a malformed structure with no exception at all) is a distinct class that must stay distinguishable from genuine absence. The defect in that class is not the fallback — ADR-227 requires malformed input to be coerced rather than propagated, and this ADR already permits a fallback — it is that the fallback is **invisible**. So every current return value is preserved and the cause is made visible by one of two mechanisms: **in-band**, where the result already carries a provenance envelope, name the cause in it (`ConfigResolution` gains a `reason`; `Resolution`'s four documented values all describe a miss, so new unusable-input values are introduced with the first adopter) and also expose it on the surface callers actually use, since a `reason` no caller reads is an unreachable field; **out-of-band**, where the read returns a bare sentinel or a plausible default it cannot extend, keep that value and emit a deduplicated `stderr` diagnostic keyed on resolved-path + errno, reusing the `_warnedUnknownConfigKeys` guard pattern. The diagnostic is unconditional — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since an opt-in nobody sets is the same silence. Throwing is **not** the cluster's answer — it stays confined to ADR-227's genuinely-fatal carve-out, decided per call, never inferred from the return shape. ### Resolution Convention 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). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6b3bc4446..243dc13be 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -199,7 +199,13 @@ This writes `.changeset/--.md`. Three random words → co Fragments are consolidated into `CHANGELOG.md` at release time by the release workflow. See [`.changeset/README.md`](.changeset/README.md) for the format spec and [#2975](https://github.com/open-gsd/gsd-core/issues/2975) for the rationale. -**CI enforcement:** the `Changeset Required` workflow (`scripts/changeset/lint.cjs`) fails any PR that touches `bin/`, `gsd-core/`, `agents/`, `commands/`, `hooks/`, or `sdk/src/` without a `.changeset/*.md` fragment. The gate also **validates the content** of every changed fragment: a fragment whose frontmatter does not parse (e.g. a `pr: 0` placeholder that was never backfilled to the real PR number) fails the gate with `fail_invalid_fragment`, naming the offending file. This stops a malformed fragment from merging to `next` and only detonating later in the release job's CHANGELOG render. +**CI enforcement:** the `Changeset Required` workflow (`scripts/changeset/lint.cjs`) fails any PR that touches `bin/`, `gsd-core/`, `src/`, `agents/`, `commands/`, `hooks/`, or `sdk/src/` without a `.changeset/*.md` fragment. (`src/` is the TypeScript source of truth compiled into `gsd-core/bin/lib/*.cjs`, so editing it is a user-facing change even though the generated `.cjs` is gitignored and never appears in the diff.) + +> **Running it locally.** The lint derives its changed-file set from `GITHUB_BASE_REF`, which only CI sets. `node scripts/changeset/lint.cjs` on a developer machine therefore does **not** evaluate your branch and can report success on a PR that CI will fail. Pass the base explicitly to reproduce the CI result: +> +> ```bash +> GITHUB_BASE_REF=next node scripts/changeset/lint.cjs +> ``` The gate also **validates the content** of every changed fragment: a fragment whose frontmatter does not parse (e.g. a `pr: 0` placeholder that was never backfilled to the real PR number) fails the gate with `fail_invalid_fragment`, naming the offending file. This stops a malformed fragment from merging to `next` and only detonating later in the release job's CHANGELOG render. **Opt-out:** PRs with no user-facing impact (test refactors, lint config changes, CI tweaks, formatting-only changes) can add the `no-changelog` label. The lint honors it. When unsure whether a change is user-facing, **add the fragment**. diff --git a/docs/adr/1411-resolution-provenance.md b/docs/adr/1411-resolution-provenance.md index 7f68320fa..b48458910 100644 --- a/docs/adr/1411-resolution-provenance.md +++ b/docs/adr/1411-resolution-provenance.md @@ -87,3 +87,51 @@ P3 is therefore narrowed to an honest convention rather than a forced generic: - The shared contract is documented, not forced: read verbs expose `warnings[]`; mutation verbs expose `warnings[]` + `errors[]`; `configured`/`reason` appear only on config-interpreting read verbs. Recurrence prevention does not depend on a shared envelope — it is delivered by P4's CI guard (a configured input resolving empty must carry a `reason`). (#1416) + +## Amendment — 2026-07-26: corrupt is not absent + +This ADR reasons exclusively about a **resolution miss** — ambient context is "off", the lookup finds nothing, resolution falls open to defaults. It is silent on the adjacent case: input that is *present but not usable*. That silence is why five engine read paths (#1879) could fold an unusable input into the very value that means "genuinely absent" without contradicting an Accepted ADR. + +The failure detection differs per site and is not one mechanism — naming them precisely, because the fix differs with them: + +| Site | How "not usable" is detected | +|---|---| +| `config-loader.cts` (#1880) | `SyntaxError` from `JSON.parse`, or an errno (`EACCES`) re-thrown by `platformReadSync` | +| `roadmap-parser.cts` (#1881) | errno only — the parse is regex over text and cannot throw | +| `frontmatter.cts` (#1882) | **neither** — no I/O and no throw site; an opening `---` with no closing fence is a *structural* check the function must make for itself | +| `planning-workspace.cts` / `verify.cts` (#1883) | errno from `readdirSync` (`EACCES`/`EIO`) | +| `planning-workspace.cts` (#1884) | an errno that was swallowed, then misclassified as a different condition | + +### What the two governing ADRs actually permit + +Read together rather than selectively, ADR-1411 and ADR-227 converge, and they do **not** license throwing as a cluster-wide answer: + +- **ADR-227's Decision** requires malformed input to be *"silently coerced to the contract's safe default … It MUST NOT be propagated. Throw only if the surrounding codebase treats throws as a normal-flow signal (it usually does not …)"*, and its rejection of throwing carves out only fields where a value is *"genuinely fatal (not just malformed) … a per-field decision, not the general rule."* Malformed is explicitly on the coerce side of that line. +- **This ADR's own Decision** says: *"A resolver may fall back, but the fallback **must be a visible value, not a silent substitution**."* + +The gap in the five sites is therefore **not** that they fall back. It is that they fall back **invisibly**. Continuity is correct and stays; the silence is the defect. + +### The pattern — keep the fallback, make it visible + +Both mechanisms below preserve every current return value. Neither changes a return type, so no caller that treats "absent" and "unusable" identically breaks. + +- **In-band, where the result already carries provenance.** A read whose result is a provenance envelope names the cause in that envelope. `loadConfigResolved`'s `ConfigResolution { config, source, degraded }` is the first adopter (#1880): genuine absence keeps `degraded:false`; an unusable config sets `degraded:true` and adds a `reason`. `Resolution` (`src/resolution.cts`) has **no** value for this case today — its documented vocabulary is `resolved` / `not_configured` / `configured_empty` / `configured_unresolved`, all of which describe a miss. #1880 introduces the unusable-input values and is responsible for documenting them alongside the existing four. +- **Out-of-band, where the return is a bare value that cannot carry provenance.** A read that returns a bare sentinel or a plausible default keeps returning exactly that, and emits a **deduplicated `stderr` diagnostic** naming the file and the errno. This covers `getRoadmapPhaseInternal` (#1881), `findContextMdIn` / `listMilestoneArchiveDirs` (#1883), `getMilestoneInfo` (#1881) — whose fallback is a populated `{ version: 'v1.0', name: 'milestone' }` rather than an empty sentinel, and a plausible-looking default is *more* in need of a diagnostic than an empty one, not less — and `extractFrontmatter` (#1882), which returns `{}`. The repo's existing seam is `config-loader.cts`'s `_warnedUnknownConfigKeys` guard around `process.stderr.write`. + + **This is unconditional, and that is a deliberate divergence from ADR-227.** ADR-227's Tradeoff proposes mitigating silent coercion with *"an opt-in debug log (`process.env.GSD_DEBUG`)"*; that env var has never been implemented, and an opt-in nobody sets is indistinguishable from the silence #1879 is about. This ADR's own Decision is the stronger rule and the one that governs here — degradation must be **visible**, not discoverable-on-request. The `_warnedUnknownConfigKeys` precedent is likewise unconditional. Appliers follow this ADR, not ADR-227's tradeoff, on that point. + + **Dedup key.** Key the guard on the *resolved absolute path plus the errno*, not on the message text or the bare errno. Keying too coarsely suppresses a genuine second failure in a different file; keying on prose couples the guard to wording. + +**Throwing is not the cluster's answer.** It remains available only under ADR-227's genuinely-fatal carve-out, decided per call and justified in that PR — never inferred from the return shape. `withPlanningLock` (#1884) is the one site that qualifies, and it already throws; its defect is that it throws the *wrong* error after swallowing the real one. + +**Detection, not propagation, where there is no exception.** #1882 takes the out-of-band mechanism above like its siblings — the difference is only in how the condition is *found*. `extractFrontmatter` takes a `string`, does no I/O, and has no throw site, so there is nothing to catch: an opening `---` with no closing fence is a structural check the function must make for itself, and having made it, it distinguishes malformed-truncated from well-formed-and-empty and emits the same deduplicated diagnostic. The check has to be written; the signal shape is not a new one. + +**Wiring clause.** A `reason` that exists only inside an envelope no caller reads is not a delivered signal — it is an unreachable field. An in-band adopter MUST also expose the cause on the surface its callers actually use. `loadConfig`, the thin wrapper over `loadConfigResolved`, returns `.config` alone to roughly thirty call sites; adding `reason` to the envelope without a diagnostic on that path leaves every one of them exactly as blind as before. + +**Caller audit is mandatory per applier.** Implemented as specified, neither mechanism can break a caller — no return type changes. The audit exists to prove the applier *did* implement it as specified, which is a different claim. The concrete hazard: `src/state.cts` carries a comment recording that a defensive `try/catch` around `getMilestoneInfo` was **deliberately removed** under the #2245 audit because that function "never throws". An applier who reaches for a throw here — the intuitive fix, and the one this amendment rules out — silently breaks that invariant. Each applying PR records its caller audit for that reason. + +**CI ratchet.** `scripts/lint-resolution-provenance.cjs`'s `REGISTRY` currently holds one verb (`agent-skills`). The config-loader seam is not registered, so nothing today would catch a regression of #1880's contract. #1880 registers it. + +**Test methodology.** Assert the typed surface, not the diagnostic prose — `CONTRIBUTING.md`'s *Prohibited: Raw Text Matching on Test Outputs* applies to `stderr` as much as to `stdout`, and `tests/roadmap-parser.test.cjs` already states the local convention for this call surface. Where the mechanism's only observable is a diagnostic, the applier exposes the typed surface (a frozen reason enum, or the dedup set) and asserts on that. + +First appliers: #1880 (in-band), #1881 / #1882 / #1883 (out-of-band), #1884 (genuinely-fatal carve-out, already throwing) — epic #1879, Phase 0 = #2674. diff --git a/src/config-loader.cts b/src/config-loader.cts index 0017e725a..bce884b14 100644 --- a/src/config-loader.cts +++ b/src/config-loader.cts @@ -308,8 +308,14 @@ function _warnUnknownProfileOverrides(parsed: Record, configLab // Internal helper exposed for tests so per-process warning state can be reset // between cases that intentionally exercise the warning path repeatedly. +// Clears BOTH dedup sets: _warnedConfigKeys (runtime/model-policy/tier warnings) +// and _warnedUnknownConfigKeys (unknown top-level keys). Omitting the latter made +// this a silent no-op for the suite that exists to test it — the leaked state +// suppressed any later case reusing a key, and the existing cases only passed +// because each picked a key name no other case reused (#2674). function _resetRuntimeWarningCacheForTests(): void { _warnedConfigKeys.clear(); + _warnedUnknownConfigKeys.clear(); } // ─── FIX 2: Federated overlay helpers ──────────────────────────────────────── diff --git a/tests/config-loader.test.cjs b/tests/config-loader.test.cjs index 8fe33753d..e1335c499 100644 --- a/tests/config-loader.test.cjs +++ b/tests/config-loader.test.cjs @@ -231,6 +231,32 @@ describe('loadConfig — unknown-key warning dedup', () => { // Should appear at most once assert.ok(warnings.length <= 1, `warning emitted more than once: ${warnings.length} times`); }); + + // #2674: the two cases above only pass because each picks a key name no other + // case reuses — so neither can observe whether the documented reset actually + // runs. _resetRuntimeWarningCacheForTests is documented as resetting + // "per-process warning state", and this suite's beforeEach calls it expecting + // exactly that, but it cleared only _warnedConfigKeys and left + // _warnedUnknownConfigKeys populated. Any later case that reused a key would + // have its warning silently suppressed by the previous case's leaked state. + // Asserts on the exported Set rather than stderr prose (CONTRIBUTING.md — + // Prohibited: Raw Text Matching on Test Outputs). + test('_resetRuntimeWarningCacheForTests clears the unknown-key dedup set', () => { + writeConfig(tmpDir, { __gsd_reset_probe__: true }); + loadConfig(tmpDir); + assert.ok( + configLoader._warnedUnknownConfigKeys.size > 0, + 'precondition: loading an unknown key must populate the unknown-key dedup set', + ); + + configLoader._resetRuntimeWarningCacheForTests(); + + assert.equal( + configLoader._warnedUnknownConfigKeys.size, + 0, + 'the documented per-process warning-state reset must clear the unknown-key dedup set too', + ); + }); }); // ─── malformed JSON handling ──────────────────────────────────────────────────