* docs(#2674): amend adr-1411 with the corrupt-is-not-absent house pattern ADR-1411 reasons only about a resolution miss. It is silent on input that is present but not usable, which is how five engine read paths (#1879) could fold an unusable input into the value meaning 'genuinely absent' without contradicting an Accepted ADR. Read together, ADR-1411 and ADR-227 converge and do not license throwing as the cluster's answer: ADR-227 requires malformed input to be coerced rather than propagated and carves out only genuinely-fatal fields, while ADR-1411 already permits a fallback provided it is 'a visible value, not a silent substitution'. The defect in these five sites is therefore not that they fall back but that they fall back invisibly. Records the pattern that follows: every current return value is preserved, and the cause is made visible in-band where the result already carries a provenance envelope, or out-of-band via a deduplicated stderr diagnostic where it returns a bare value it cannot extend. Throwing stays confined to ADR-227's genuinely-fatal carve-out, decided per call. Also names the per-applier caller audit and the lint-resolution-provenance registry gap. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2674): prove the warning-state reset misses the unknown-key dedup set The two existing cases in this suite only pass because each picks a key name no other case reuses, so neither can observe whether the reset the beforeEach calls actually runs. Failing-first: asserts the exported _warnedUnknownConfigKeys is empty after _resetRuntimeWarningCacheForTests(). It is not - the helper clears only _warnedConfigKeys despite documenting itself as resetting per-process warning state. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2674): reset the unknown-key dedup set with the runtime warning cache _resetRuntimeWarningCacheForTests documents itself as resetting per-process warning state but cleared only _warnedConfigKeys, leaving _warnedUnknownConfigKeys populated across cases. The suite that exists to test that set - 'loadConfig - unknown-key warning dedup' - calls the helper in beforeEach expecting exactly this, so the reset was a silent no-op for it; both cases passed only because each picked a key name the other never reused. Any later case reusing a key would have had its warning suppressed by leaked state. Found while amending ADR-1411, which names this dedup guard as the pattern five downstream PRs (#1880-#1884) will adopt - shipping the ADR without the fix would have propagated the footgun to each of them. Folded in here per CLAUDE.md's no-defer rule rather than filed. RED verified on 3c4895841 (test only, no fix): linux-node22 reported 'FAIL tests/config-loader.test.cjs - the documented per-process warning-state reset must clear the unknown-key dedup set too'. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2674): document src/ in the changeset-lint trigger list CONTRIBUTING.md presented the Changeset Required trigger list as bin/, gsd-core/, agents/, commands/, hooks/, sdk/src/ - omitting src/, which scripts/changeset/lint.cjs has in USER_FACING_PREFIXES. src/ is the TypeScript source of truth compiled into gsd-core/bin/lib/*.cjs, so it is the most-edited user-facing path in the repo and the omission sends any contributor who touches it into a CI failure the doc says cannot happen. Also documents that the lint reads GITHUB_BASE_REF, which only CI sets, so running it bare locally reports success without evaluating the branch. This PR hit exactly that: a local run said ok_fragment_present and CI failed fail_missing_fragment on the same diff. Found while opening this PR; folded in per the no-defer rule. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2674): add Fixed changeset for the src/ trigger-list and reset fixes Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2674): restore the round-2 review corrections to the amendment These edits were made in response to the second isolated review pass but never staged: later commits used targeted `git add <file>` for the test and the source fix, so the two markdown files stayed dirty and shipped nothing. The branch carried the round-1 text, including the ADR-227 misquote the reviewer raised as a blocker. Restores: the unconditional-diagnostic clause (ADR-227's GSD_DEBUG opt-in was never implemented, so citing it as the precedent was wrong), the dedup key, #1882 folded into the out-of-band mechanism instead of a fourth mechanism-less category, the narrowed caller-audit rationale, and the test-methodology clause. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
5
.changeset/eager-tigers-greet.md
Normal file
5
.changeset/eager-tigers-greet.md
Normal file
@@ -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)
|
||||
@@ -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<T> { 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<T> { 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<T>`'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<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).
|
||||
|
||||
@@ -199,7 +199,13 @@ This writes `.changeset/<adjective>-<noun>-<noun>.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**.
|
||||
|
||||
|
||||
@@ -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<T>` (`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.
|
||||
|
||||
@@ -308,8 +308,14 @@ function _warnUnknownProfileOverrides(parsed: Record<string, unknown>, 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 ────────────────────────────────────────
|
||||
|
||||
@@ -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 ──────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user