Skip to content

refactor(db): drop unreachable aggressiveEnabled fallback in engines-map derivation - #15601

Merged
diegosouzapw merged 3 commits into
diegosouzapw:release/v3.8.52from
woodsonl:fix/remove-aggressive-enabled-fallback
Oct 6, 2026
Merged

diegosouzapw merged 3 commits into
diegosouzapw:release/v3.8.52from
woodsonl:fix/remove-aggressive-enabled-fallback

Conversation

@woodsonl

@woodsonl woodsonl commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

⚠️ base-red inherited: #15306

Summary

Removes the unreachable aggressiveEnabled fallback from the engines-map derivation in src/lib/db/compression.ts and pins the derived behavior with a unit test.

deriveEnginesMap() called aggressiveEnabled(config.aggressive), which read toRecord(value).enabled === true. Nothing can put an enabled key there: config.aggressive is assigned only from normalizeAggressiveConfig(), an explicit-pick normalizer that never emits the key; AggressiveConfig and DEFAULT_AGGRESSIVE_CONFIG have no enabled field; the strict aggressiveConfigSchema has none. The helper always returned false, so the derivation behaved identically with it removed. The only scenario it nominally covered (a stored aggressive.enabled=true, reachable only through an outside write) keeps its existing behavior: Aggressive shows as off unless defaultMode is "aggressive".

The change keeps case "aggressive": break; with a comment so aggressive derives to off instead of falling into the default-combo branch, and leaves the defaultMode fallback below it untouched. A legacy install with defaultMode: "aggressive" still derives aggressive as on.

Commits:

  • 4d4b3c793c refactor(db): drop unreachable aggressiveEnabled fallback in engines-map derivation
  • 200ef74584 docs(db): align deriveEnginesMap comments with the aggressive derivation

Test Coverage

src/lib/db/compression.ts — getCompressionSettings() read path
├── stored `engines` row present ── parseStoredEnginesMap() (unchanged)
│   ├── valid row → enginesExplicit=true, derive SKIPPED
│   │     ★★★ tests/unit/compression/compression-engines-map-migration.test.ts (round-trip)
│   └── unusable row → warn + derive fallback (unchanged; compression-settings-warn.test.ts)
└── no stored row → deriveEnginesMap(config)                ← CHANGED FUNCTION
    ├── case "caveman"/"rtk"/"ultra" (unchanged)
    │     ★★★ migration test (caveman via defaultMode "standard")
    ├── case "aggressive"                                    ← CHANGED LINE
    │   ├── default: enabled=false (dead helper removed)
    │   │     ★★ NEW test 1 "keeps aggressive off by default"
    │   └── rogue stored aggressive.enabled=true ignored
    │         ★★★ NEW test 3 "stored enabled=true outside the schema is ignored"
    ├── default: structural engines from default combo (unchanged, out of diff scope)
    └── defaultMode fallback → SINGLE_MODE_ENGINE
          ├── defaultMode="aggressive" → on (only on-signal)
          │     ★★ NEW test 2 "defaultMode 'aggressive' is the only signal"
          └── defaultMode="standard" → caveman ★★★ migration test

User flows: legacy pre-102 install → derived map, enginesExplicit=false (display-only,
dispatch stays legacy) — pinned by tests 1-3 · panel save → stored row → trusted
(enginesExplicit=true) — pinned by migration test.

Legend: ★ smoke · ★★ happy-path value pin · ★★★ edge+error pin
Verdict: 5/5 changed+adjacent paths guarded · 0 gaps · fast path (no generation needed)

Tests: 6782 → 6783 (+1 new)
Coverage: 100% value-weighted (100% including 0 weakly covered paths)
Test value: 1 test written, 0 rejected by the authoring gate, 0 existing tests extended, 0 paths weakly covered.

The new test, tests/unit/compression/compression-engines-map-derived-defaults.test.ts, pins the derived engines map through the real getCompressionSettings() + SQLite boundary (temp DATA_DIR, no test-only seam):

  1. No engines row: aggressive off.
  2. defaultMode: "aggressive": aggressive on (the fallback this PR keeps).
  3. Stored aggressive.enabled=true, reachable only through an outside write: ignored.

Base control: the test also passes against the base version of compression.ts, which proves the removal is behavior-preserving and the test pins the derived value, not the implementation.

Pre-Landing Review

No issues found. Ship pass: 3 specialists (testing, maintainability, performance), 0 defects; one maintainability note at confidence 3 (pre-existing comment enumeration outside the diff) held to the appendix as low confidence.

Outside review: Codex adversarial unavailable (local preflight reports model_unusable; the model-resolver script is missing). The native Claude adversarial pass ran instead and found no defects in the diff.

Follow-ups from the adversarial pass, all pre-existing and outside this diff, recorded here rather than fixed in this PR:

  1. A partial engines write flips dispatch authority. PUT /api/settings/compression accepts a subset engine map; sanitizeEnginesForWrite() persists only the present keys, and the next read treats the partial map as explicit (enginesExplicit=true), defaulting every unlisted engine to off. A legacy install running defaultMode: "aggressive" could lose aggressive compression on one partial panel save, with no warning. Suggested fix: merge partial writes onto the stored row, or derive unlisted engines from the legacy fields before marking the map explicit.
  2. The pinning test covers the read path only. A bake-path test (panel save after a legacy defaultMode install keeps aggressive on) would guard the migration story end to end.
  3. Lower-priority notes: aggressive.preserveSystemPrompt and ultra.preserveSystemPrompt pass the Zod schemas but their normalizers silently drop them; planFromHeader()'s engine:<id> branch and resolveStackSteps() read config.engines without the enginesExplicit gate (the main dispatch path is gated); the PUT schema accepts and persists a junk enginesExplicit row.

Exploratory QA

Functional smoke on the changed read path (node:test, isolated temp DATA_DIR, no server, no external mutation):

  • Derived engines map (off by default / on via defaultMode=aggressive / rogue stored enabled ignored): 3 pass, 0 fail.
  • Engines-map migration and round-trip: 2 pass, 0 fail.
  • Settings TTL cache lane: 5 pass, 0 fail. Chatcore consumer lane: 3 pass, 0 fail.

Evidence ledger label tests-unit-compression: exit 0, 13 tests total, receipt FRESH at ship time. Full unit shards, Vitest, the coverage gate and the build run in CI per CONTRIBUTING.md.

Design Review

No frontend files changed; design review skipped.

Eval Results

No prompt-related files changed; evals skipped.

Scope Drift

Scope Check: CLEAN
Intent: remove the dead aggressiveEnabled fallback, keep the derived engines map identical, pin it with a test.
Delivered: exactly that, plus comment corrections the review pass requested.

Plan Completion

Plan completion audit: not run (no plan is bound to this branch and no docs/designs/ file matches). Fix: add "Plan: " to the PR body, or run /autoplan.

Verification Results

No plan-specific verification obligations (no plan bound). Gate results on the shipped tree: focused unit lanes 13/13 PASS; lint (suppression-aware) PASS; typecheck:core PASS; Prettier PASS; coverage gate PASS (100% value-weighted); QA verdict PASS (materialized, no open items).

Documentation

Status: current - no documentation changes required; docs already match what shipped.

Audited scope: branch fix/remove-aggressive-enabled-fallback vs origin/release/v3.8.52 (2 commits, 2 files). The diff removes the module-private dead helper aggressiveEnabled() from deriveEnginesMap() in src/lib/db/compression.ts (the aggressive case now leaves the derived legacy toggle off, with explanatory comments) and adds tests/unit/compression/compression-engines-map-derived-defaults.test.ts (3 tests locking the derived-map behavior). No exported API, endpoint, flag, or user-facing behavior changed. Working tree clean (no staged, unstaged, or untracked content). The repo has no authored .tmpl doc templates.

Documentation health:
README.md - Current (Aggressive engine row, pipeline alt-text and profile mentions unchanged by the diff; still accurate)
CLAUDE.md - Current (no derived-engines-map or aggressive enabled-flag claims)
docs/compression/COMPRESSION_ENGINES.md - Current (read in full; modes table still accurate; validation globs tests/unit/compression/*.test.ts already cover the new test)
docs/compression/COMPRESSION_GUIDE.md - Current (mode selection, per-combo and per-request override sections match post-diff code)
docs/compression/EXTENDING_COMPRESSION.md - Current (mode table and defaultMode selection match; the new test locks exactly the documented defaultMode semantics)
CHANGELOG.md - Not applicable (untouched by branch; metadata owned by the parent)

Verification of the stale-doc question: repo-wide search found no doc naming aggressiveEnabled or deriveEnginesMap; targeted sweeps (aggressive, engines map, defaultMode, backfill, migration 102) across docs/, README.md, CLAUDE.md, AGENTS.md, llm.txt, CONTRIBUTING.md, docs/reference/API_REFERENCE.md and docs/openapi.yaml found no claim the diff makes stale. The aggressive mentions in docs/i18n/*/CHANGELOG.md mirrors are historical release records, not current-behavior docs.

Coverage debt: none attributable to this diff. The removed helper was module-private; no public surface changed. The aggressive engine retains reference coverage (README pipeline table, COMPRESSION_GUIDE.md, COMPRESSION_ENGINES.md modes table).

Diagram drift: none - docs/diagrams/compression-pipeline.svg lists the Aggressive engine, which still exists.

Test plan

  • node --import tsx/esm --test tests/unit/compression/compression-engines-map-derived-defaults.test.ts: 3 pass / 0 fail (also green against the base source)
  • node --import tsx/esm --test tests/unit/compression/compression-engines-map-migration.test.ts: 2 pass / 0 fail
  • node --import tsx/esm --test tests/unit/compression-settings-cache.test.ts: 5 pass / 0 fail
  • node --import tsx/esm --test tests/unit/chatcore-compression-settings.test.ts: 3 pass / 0 fail
  • npm run lint and npm run typecheck:core: clean

…map derivation

normalizeAggressiveConfig never emits an enabled key and the strict
aggressive config schema has none either, so the helper always returned
false. Keep the case branch as an explicit no-op so aggressive derives
to off and still picks up the defaultMode fallback below.
The header listed aggressive among engines read from dedicated config
blocks and the case comment did not note the deliberate split from the
default-combo structural path; both now state the surviving invariant.
@woodsonl
woodsonl requested a review from diegosouzapw as a code owner October 5, 2026 23:01
@diegosouzapw
diegosouzapw merged commit 38a2660 into diegosouzapw:release/v3.8.52 Oct 6, 2026
42 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants