feat(config): governed v8 defaults-flip frame — conservative preset, migration, rollback, inventory (#2504) - #2797
Conversation
…migration, rollback, inventory (#2504) Ships the governance frame required by #2504 around the armed v8 defaults flips (auto_review release-gated burn-in flip; parallel-first new-plan default from v7.132.0). No new default flip, no version changes. - preset key ('default'|'conservative'): conservative restores the pre-flip v7 defaults (auto_review disabled, serial new plans) as the lowest-precedence resolution layer; explicit keys always win - migratePresetsConfig: exempt the new preset enum values from the v6.12 dormant-legacy-key strip so the conservative preset survives configs with an agents block (the installer's own shape) — final-critic round-1 fix, with agent-block and legacy-value regression coverage - config doctor: DEFAULT_FLIPS info findings (kill switch + preset per flip, gated config_format_version < 3), 'Defaults changes (v8)' report section, and an idempotent --fix acknowledgment stamp (closes the version-never- written-back gap); availableMigrations rename surface unchanged - save_plan: preset-aware new-plan parallelization default via new _internals.loadPluginConfigWithMeta seam (fail-open to v8 default) - docs: docs/defaults-governance.md inventory (evidence citations, K3 UX-3/6/7 dispositions, non-goals, kill switches, rollback, cost delta); trust-posture amendments in architecture/installation/design-rationale; Compatibility Matrix in installation.md; regenerated schema artifacts - tests: conservative-preset (incl. agents-block survival), defaults-flip- upgrade, defaults-flip-rollback (frozen #2504 acceptance contracts); parallelization-default hardened with XDG isolation; config-doctor.test.ts fixture pinned to the current format version (line-neutral per FR-006) - catalog.ts producer citations repointed for the index.ts line shift
Drift check reportFound 2 drift finding(s): 0 error, 0 warning, 2 notice. required-check-contract (2)
|
There was a problem hiding this comment.
🔵 Needs a closer look
It reshapes behavior-changing configuration defaults across resolution, doctor, and save_plan and contains a confirmed summary-count discrepancy, so a human should verify the governance behavior before approval.
Pull request overview
This PR adds the governance frame around opencode-swarm's already-armed v8 default flips (the release-gated auto_review.enabled flip and the parallel-first new-plan default). It introduces no new default flip and no version changes; instead it supplies the escape hatch, migration surface, inventory, and doc corrections that #2504 requires. It fits into the config-resolution and /swarm config doctor subsystems, plus the save_plan new-plan default.
Changes:
- New top-level
preset("default" | "conservative") key;CONSERVATIVE_PRESET_BASEapplied as the lowest-precedence deep-merge layer so explicit user keys always win, with a defense-in-depthpresetguard in the auto-review release gate, plumbed at plugin init, the final-review gate, andsave_plan. - Config doctor surfaces governed
defaults-flipinfo findings whileconfig_format_version < 3, adds an idempotent--fixacknowledgment stamp to version 3, and a "Defaults changes (v8)" report section. - New
docs/defaults-governance.mdinventory plus amended trust-posture/compatibility docs, regenerated schema JSON + config table, and a release fragment; three new frozen-contract test suites.
File summaries
| File | Description |
|---|---|
src/config/schema.ts |
Adds preset schema field, CONSERVATIVE_PRESET_BASE, and preset guard in autoReviewEnabledByRelease |
src/config/loader.ts |
Exempts preset enum from the v6.12 dormant-key strip; applies conservative base layer (step 3b) |
src/services/config-doctor.ts |
Adds DEFAULT_FLIPS, defaults-flip findings, preset validation, and the --fix version stamp — summary info count not updated for the new findings |
src/tools/save-plan.ts |
Adds _internals config-load seam and preset-aware new-plan parallelization default — a relocated comment now mislabels the config import |
src/tools/phase-complete/gates/final-review-gate.ts |
Passes preset into the release context (defense-in-depth) |
src/index.ts |
Plumbs config.preset into init-time auto-review resolution |
src/commands/doctor.ts |
Renders the "Defaults changes (v8)" markdown section |
src/observability/catalog.ts |
Repoints two src/index.ts producer citations (+8 line shift) — verified correct |
src/services/config-doctor.test.ts |
Stamps the shared test config at v3; hardens the version-default test |
tests/unit/config/*.test.ts |
New conservative-preset / upgrade / rollback suites; XDG isolation for parallelization-default |
docs/* (governance, installation, configuration, architecture, design-rationale, schema, release fragment) |
Inventory, compatibility matrix, and amended trust-posture documentation |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for (const flip of DEFAULT_FLIPS) { | ||
| if (configVersion < flip.sinceVersion) { | ||
| findings.push({ | ||
| id: 'defaults-flip', | ||
| title: `v8 default change pending for "${flip.field}"`, | ||
| description: | ||
| `Default flips from ${flip.v7Default} to ${flip.v8Default}. ` + | ||
| `Kill switch: set ${flip.killSwitch}. ` + | ||
| `Restore all v7 defaults with preset: "${flip.preset}". ` + | ||
| `Evidence and rollback: ${flip.evidence} (#2504).`, | ||
| severity: 'info', | ||
| path: flip.field, | ||
| currentValue: 'v7 default', | ||
| autoFixable: false, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Confirmed and fixed in ea1c3a8: the severity counts are now recomputed after the defaults-flip emission loop, so summary.info (and the .swarm/config-doctor.json artifact summary) include the new findings. Verified by probe: a v7-era config now reports Info: 2 matching the 2 defaults-flip rows, and tests/unit/commands/doctor-defaults-flip-markdown-2504.test.ts + the upgrade suite pin the behavior.
…er-guard base SHA
Fixes from the PR #2797 swarm-pr-review round (10 lanes, 2 reviewer shards, critic challenge): - PRR-004 (MEDIUM): the user-config-alone recovery fallback now mirrors steps 3/3b (migratePresetsConfig + conservative base layer), so a broken project config no longer defeats the conservative preset; the schema preprocess could otherwise fill a partial auto_review section with the release-gated v8 default before any preset-aware consumer saw it - PRR-001/006 (MEDIUM): runConfigDoctor recomputes severity counts after the defaults-flip emission so the report header and the .swarm/config-doctor.json artifact include the new findings - PRR-026 (MEDIUM): the --fix acknowledgment stamp strips a UTF-8 BOM before JSON.parse (matching the loader) instead of silently skipping - PRR-005 (LOW): stamp failures now log (fail-open, but signaled) - PRR-002 (LOW): the 'QA gate check' comment moved back above the qa-gate-profile import it describes - PRR-021 (MEDIUM): design-rationale automation example now shows the actual schema defaults (read-only capabilities on), matching the amended bullet - Coverage: user-config stamp path, formatDoctorMarkdown 'Defaults changes (v8)' section (formatDoctorMarkdown now exported), user-config-alone fallback with preset, explicit-'default' and unknown-preset fail-open pins (PRR-011/012/013/014/015/017/018) Also syncs the branch with main (release 7.183.1) refreshing the stale release-owner-guard base SHA; the PR body was restructured to the repo's commit-pr contract (## Summary / ## Invariant audit / ## Test plan).
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) + MiniMax-M2.7-highspeed (explorer B) (parallel explore, distinct lenses) → GLM-5-turbo (critique) ↔ GLM-5-turbo (critique) (cross-critique) → MiniMax-M2.7-highspeed (fallback arbiter) (arbiter: blind-spot + synthesize) Looking at this PR carefully — it's the governance frame for v8 defaults flips (#2504): a 🔍 PR IntentReconstructed from issue #2504, PR description, and commit messages:
📦 Implementation SummaryThe PR adds ✅ /
|
| Obligation | Status | Evidence |
|---|---|---|
| O-001 | SUPPORTED |
src/config/schema.ts:3667 — preset enum added; loader.ts:707 — base-layer injection |
| O-002 | SUPPORTED |
config-doctor.ts:2234 — DEFAULT_FLIPS loop; config-doctor.ts:2762 — stamp logic |
| O-003 | SUPPORTED |
save-plan.ts:942 — conservativePresetActive flag; save-plan.ts:1011 — default injected |
| O-004 | SUPPORTED |
docs/defaults-governance.md — full inventory with evidence and kill switches |
| O-005 | SUPPORTED |
architecture.md:1925, installation.md:809, design-rationale.md:303 — all amended |
| O-006 | SUPPORTED |
opencode-swarm.schema.json regenerated; docs/configuration.md table updated |
🚨 Confirmed Findings
Only findings with concrete PR-introduced defects. Everything below is grounded in exact file:line evidence.
[MEDIUM] xdgDir temp directory leaks when test throws before cleanup
- Location:
tests/unit/config/defaults-flip-upgrade.test.ts:35(and line 161) - Why it matters:
xdgDircreated viacanonicalMkdtempinbeforeEachhas noafterEachcleanup. If a test body throws before its ownfs.rmSynccall (line 161'sdir),xdgDirleaks to disk permanently for the test process lifetime. The same pattern exists for thedircreated inside the--fix stamps USER configtest at line 161 —fs.rmSync(dir)is inline, not inafterEach. - Evidence:
beforeEach(line 35) createsxdgDirbutafterEach(line 35–43) only restoresXDG_CONFIG_HOMEand cleansxdgDiritself — it does not clean thedirvariables created inside individual tests at lines 58–65 and 161. The parallelization-default test (line 20–53) has the same pattern fixed with an explicitfs.rmSync(xdgDir, ...)inafterEach; this file does not. - Fix direction: Add
fs.rmSync(xdgDir, { recursive: true, force: true })toafterEach(mirroringparallelization-default.test.ts:50–54), and move the--fix stamps USER configtest'sfs.rmSync(dir, ...)inline call intoafterEach.
[MEDIUM] stripUnrecognizedKeys receives wrong input in user-config fallback path
- Location:
src/config/loader.ts:845 - Why it matters: In the user-config-alone fallback path,
stripUnrecognizedKeysnow receivesuserPresetReady(post-migration + post-conservative-base) instead ofuserSanitized. The conservative base layer injectsauto_review: { enabled: false }beforesafeParseis called. If the user's partialauto_reviewsection (e.g.{ mode: "gate" }) then triggers a partial-success parse with unexpected Zod issues,stripUnrecognizedKeyswill strip keys based onuserPresetReady's error shape — which includes issues from the pre-filled section — potentially stripping different keys than it would have fromuserSanitized. - Evidence:
loader.ts:815–822—userPresetReadyis constructed fromuserPresetMigrated.preset === 'conservative' ? deepMergeFn(CONSERVATIVE_PRESET_BASE, userPresetMigrated) : userPresetMigrated, then passed tosafeParseat line 824. The resultinguserParseResult.erroris the error from parsinguserPresetReady, whose shape includes issues from the pre-filled base. Line 845 then passesuserPresetReady(notuserSanitized) tostripUnrecognizedKeys. - Fix direction: Use
userPresetMigrated(pre-base-layer) as the strip input, matching what would have happened on the normal merge path where the fallback ladder callsstripUnrecognizedKeys(userSanitized, ...). The conservative base is already applied to the parsed result via the deep-merge before Zod — it does not need to be in the strip input.
[LOW] Infinity bypasses stamp idempotency guard silently
- Location:
src/services/config-doctor.ts:2795 - Why it matters:
shouldStampistypeof currentVersion !== 'number' || !Number.isInteger(currentVersion) || currentVersion < 0 || currentVersion < CURRENT_CONFIG_FORMAT_VERSION.Infinity < 3isfalse, soshouldStamp = false— the stamp is silently skipped, leavingInfinityas the config version. A future bump ofCURRENT_CONFIG_FORMAT_VERSIONpastInfinity(impossible today but not structurally prevented) would still not restamp. The schema caps at9007199254740991but the guard doesn't explicitly rejectInfinity—Infinitypassestypeof === 'number'andNumber.isInteger(Infinity)isfalse. - Evidence:
config-doctor.ts:2795— the guard chain;Infinitytriggers!Number.isInteger(Infinity)→false, thenInfinity < 0→false, thenInfinity < 3→false, so the overall expression isfalse. - Fix direction: Add an explicit
!Number.isFinite(currentVersion)guard (or useNumber.isSafeInteger) to rejectInfinityand-Infinityexplicitly.
🔬 Unverified but Plausible Risks
These look suspicious but cannot be structurally proven without runtime validation.
-
Risk:
deepMergeFnmutatesCONSERVATIVE_PRESET_BASEin place.- Why suspicious:
src/config/loader.ts:711—deepMergeFn(CONSERVATIVE_PRESET_BASE, mergedRaw)is called withCONSERVATIVE_PRESET_BASEas first argument. IfdeepMergeFnmutates its first argument, the constant is corrupted for the process lifetime. - What would verify it: Inspect
deepMergeFnbody for in-place mutation, or addObject.freeze+ runtimedeepEqualassertion in a stress test with multiple loader invocations.
- Why suspicious:
-
Risk: The conservative preset fallback in step 3b could have XDG/read-order issues.
- Why suspicious: The fallback in
loader.ts:815is a separate code path (fires when project config is unparseable). The user-config fallback is a less-tested execution path than the normal merge ladder. - What would verify it: Integration test that writes a broken project config + a conservative user config and asserts the user fallback correctly materializes
auto_review.enabled: false.
- Why suspicious: The fallback in
🧪 Test / Coverage Gaps
- Gap:
defaults-flip-rollback.test.tstestsresolveAutoReviewConfigat a simulated v8 release but does not test the full loader path (loadPluginConfig) with the conservative preset — only the direct unit function. A regression inbuildConfigWithMeta's step 3b deep-merge would not be caught.- Evidence:
defaults-flip-rollback.test.ts:24— directresolveAutoReviewConfigcall; noloadPluginConfigorloadPluginConfigWithMetain the file.
- Evidence:
- Gap: The conservative preset rollback test does not verify that a v8-era config without a preset key still resolves to
enabled: trueat v8. Without this negative assertion, a future regression that makes the preset become the default (instead of the absent/omitted case) would go undetected.- Evidence:
defaults-flip-rollback.test.ts— all three tests either set an explicitenabledor setpreset: 'conservative'; no test for{}(no preset, no explicit key) at v8.
- Evidence:
📋 Shipped-vs-Claimed Gaps
None found. The docs/defaults-governance.md inventory covers both flip entries with evidence citations; the kill switches are documented and tested; the conservative preset behavior is described accurately in all three amended docs files; the Compatibility Matrix correctly shows all four config scenarios.
📝 Merge Recommendation
[APPROVE_WITH_FIXES]
Two medium findings (temp-dir leak, wrong strip input) and one low (Infinity bypass) are real, fixable defects introduced by this PR. No CRITICAL findings survive the challenge.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ |
🔁 Validation provenance
Findings KILLED (refuted with evidence):
loader.ts:199null guard —rawis a requiredRecord<string, unknown>from callers; contract guarantees non-nullloader.ts:822type safety — ZodsafeParsefailure always produces definederror; TypeScript's type narrowing is correctloader.ts:845input diverge — intentional; fallback must mirror steps 3/3b for conservative preset survival per PRR-004 review contractschema.ts:622plain string type — enum values validated bysafeParsebefore reachingautoReviewEnabledByRelease; any invalid string is rejected earlierschema.ts:639null guard —contexthas default{};presetis typed optional/string from Zoddoctor.ts:186null guard —findingsis a requiredConfigFinding[]field onConfigDoctorResultconfig-doctor.test.ts:2926test correctness — both{}and{ config_format_version: undefined }produce absent key in parsed object; equivalent test pathconfig-doctor.ts:105data mismatch —execution_profile.parallelization_enabledis the correct plan-file path; the kill switch description says "(per plan)" which is accuratesave-plan.ts:954error handling — intentional fail-open design; documented in PR description and code commentdefaults-flip-upgrade.test.ts:78stale data — file read + parse is synchronous; no async flush gap existsdefaults-flip-upgrade.test.ts:102malformed version — schema rejects non-integer versions (minimum: 0, maximum: 9007199254740991)parallelization-default.test.ts:53silent catch — intentional; ENOENT is expected; Bun test runner isolates each fileparallelization-default.test.ts:43state leakage —afterEachrestoresXDG_CONFIG_HOMEin a try/catch before deletion; safedoctor.ts:189markdown injection —DEFAULT_FLIPSdescriptions are static strings from internal constants; not user-derivedloader.ts:711deep-merge mutation — CONSERVATIVE_PRESET_BASE is Readonly and documented as a contract; the code is written assuming pure semanticsloader.ts:825inconsistent input —userPresetReadyis the correct input because the conservative base must be in the error-provenance pathschema.ts:672runtime mutability — documented as a design constraint; not a runtime bugindex.ts:1680kill-switch bypass —autoReviewEnabledByReleasecheckscontext.preset === 'conservative'first; release gate never runs under conservativesave-plan.ts:944logic null guard — empty string is intentional fallback to CWD; documented behaviorconfig-doctor.ts:2796silent noop — no file to stamp exists; no-op is correctconfig-doctor.ts:2817fail-open stamp — intentionally non-fatal; documentedconservative-preset.test.ts:269isolation gap — catch in afterEach still runs the restore; file-scoped runnerconservative-preset.test.ts:261assertion weakness — intentional DI seam test; the seam itself is tested separatelydefaults-flip-rollback.test.ts:25XDG isolation — confirmed existing gap; not introduced by this PR (the PR's own test fileconservative-preset.test.tscorrectly adds XDG isolation)defaults-flip-upgrade.test.ts:35resource leak — KILLED as null, REINTRODUCED above as confirmed MEDIUMdefaults-flip-upgrade.test.ts:161resource leak — KILLED as null, REINTRODUCED above as confirmed MEDIUMparallelization-default.test.ts:20concurrency — KILLED; Bun runs tests in the same file serially by default
Findings SURVIVING (with new evidence):
defaults-flip-upgrade.test.tstemp-dir leaks → MEDIUM (confirmed above)loader.ts:845wrong strip input → MEDIUM (confirmed above)config-doctor.ts:2795Infinity bypass → LOW (confirmed above)
Blind-spot findings added: None — the PR touches a well-tested surface with good review coverage; no new structural defects found beyond the confirmed items above.
Documentation findings (docs only, not PR-introduced code defects): All five documentation items (compatibility matrix rows, migration steps inconsistency, v7 release qualification, behavioral documentation changes) describe the PR's intentional documented behavior. None are code defects.
🔒 Reviewed by a 3-model cross-family adversarial debate (architect → dual-lens parallel explorers → cross-critique → arbiter) for high recall with low false-positive noise. Findings are advisory — verify before acting.
Closes #2504
PR head: see the commit history (synced with main; release-please owns versions).
Summary
Ships the governance frame required by issue #2504 (Workstream F PR 12 of 21, consolidating EPIC #1677) around the already-armed v8 defaults flips — the release-gated
auto_review.enabledflip (approved burn-in pin + major>=8 gate) and the parallel-first new-plan default (v7.132.0). No new default flip, no version changes.presetconfig key ("default" | "conservative", optional): the conservative preset restores the pre-flip (v7) defaults —auto_review.enabled: falseand serial new plans — applied as the lowest-precedence layer in config resolution so explicit user values always win (including partial sections likeauto_review: { mode: "gate" }and configs with anagentsblock, the installer's own shape).defaults-flipinfo findings (naming the kill switch and the preset) whileconfig_format_version < 3, with a "Defaults changes (v8)" report section;/swarm config doctor --fixacknowledges them by stampingconfig_format_version: 3— idempotently; passive runs never write. This also fixes the gap where applied migrations re-advertised forever because the version was never written back.save_planconsults the resolved preset — underconservative, new plans default to serial; otherwise the v8 parallel-first default applies. Explicit profile keys always win; config-load failures fail open to the v8 default.docs/defaults-governance.md(per-flip production-evidence citations — [Workstream H] PR 08 of 15: Prove default-path PR-review completion with findings, recovery and restoration #2585 live host proof, [Workstream H] PR 09 of 15: Qualify PR-review completion across supported runtimes, hosts and enabled features #2586 partial Windows-only cells honestly scoped, [Workstream E] PR 03 of 09: Prove hybrid retrieval and graph-memory quality across languages and hosts #2490/[Workstream E] PR 04 of 09: Enforce review routing and synthesize high-confidence findings #2491/[Workstream F] PR 11 of 21: Ship the governed HarnessOpt capstone with held-out validation #2503 quality decisions, pinned cost baseline; K3 UX-3/UX-6/UX-7 dispositions; five non-goal families; kill switches; rollback; cost-delta statement); amended overgeneralized "all default false" claims inarchitecture.md/installation.md/design-rationale.md; Compatibility Matrix ininstallation.md; regeneratedopencode-swarm.schema.json+docs/configuration.mdtable.defaultsemantics.Invariant audit
server()-resolution path changes; the preset layer lives in the pre-existing synchronousbuildConfigWithMeta;bun run buildexit 0 + Node-ESMawait import('./dist/index.js')OK; no new init-path I/O.bun:imports introduced; plugin shape{ id, server }unchanged.atomicWriteFileSyncalready used byapplySafeAutoFixes..swarm/artifacts.save_plan's change only selects the new-planparallelization_enableddefault; ledger/projection paths untouched; plan-schema default staysfalse.bun testonly._internalsDI seam (no newmock.module);canonicalMkdtempeverywhere (check:test-tmpdirexit 0);check:mock-cleanupexit 0.validateConfigKeycase added per the schema-introspection coverage test;check:eventspass;check:registry-citationsexit 0 (no new anchor drift).docs/releases/pending/2504-v8-defaults-governance.mdshipped; no version/CHANGELOG/.release-please-manifest.jsonhand edits.Test plan
bun test tests/unit/config/conservative-preset.test.ts tests/unit/config/defaults-flip-upgrade.test.ts tests/unit/config/defaults-flip-rollback.test.ts tests/unit/commands/doctor-defaults-flip-markdown-2504.test.ts-> 45 pass / 0 fail (preset schema/loader/release-seam contracts incl. agents-block + user-config fallback + explicit-default+ fail-open unknown value; upgrade/stamp idempotency incl. user-config stamp path; per-flip kill switches; markdown section).bun test src/services/config-doctor.test.ts tests/unit/services/config-doctor-migration-fix.test.ts tests/unit/config-doctor-startup-isolation.test.ts tests/unit/config/parallelization-default.test.ts tests/unit/config/loader-unknown-top-level-warn.test.ts tests/unit/config/adversarial-schema.test.ts tests/unit/scripts/generate-config-schema.test.ts tests/unit/agents/reviewer-auto-review-opt-in.test.ts tests/unit/review/cost-baseline.test.ts tests/unit/utils/atomic-write-ratchet.test.ts tests/unit/scripts/retention-registry-rows.test.ts tests/unit/hooks/delegation-gate-parallel-guidance-2532.test.ts tests/unit/config/auto-select-architect.test.ts-> all pass (the single doctor-suite local failure is the documented pre-existing XDG-override artifact ingetConfigPaths, clean on CI).src/services/config-doctor.test.ts(changed file): the shared fixture now pinsconfig_format_version: 3and the SC-006 absent-version test passesundefinedexplicitly, so the version-gating suite keeps testing the v1 path with explicit overrides.bun run typecheck-> exit 0;bunx @biomejs/biome ci <changed files>-> exit 0 (repo-wide 94 pre-existing findings identical at base — pristine-archive parity verified);bun run check:events,check:mock-cleanup,check:invariants,check:test-file-cap,check:registry-citations,scripts/check-test-tmpdir.sh,scripts/check-test-clock.sh-> all exit 0;bun run build-> exit 0; Node-ESM import of./dist/index.js-> OK;scan-deferred.sh-> clean;bun run drift:check --enforce-> one LOCAL-ONLY capture-hash finding (pr-standards.ymlvs this machine's captured evidence; the workflow file is untouched by this diff and a pristinegit archive HEADrun exits 0).