fix(eq): resolve base-speaker preset live instead of pinning a stale copy
_onDispositivoCambiado() bootstrapped a device-level preset entry for every never-seen device id, including the built-in speaker. That persistent level-3 entry masked later global-preset edits (level 3 beats level 4 on every resolution), so disconnecting a BT device or cold-starting without one could leave the EQ stuck on an outdated copy instead of the current global preset. The base speaker is now excluded from the first-seen bootstrap: disconnect and cold start always resolve through the live hierarchy. BT/wired/USB devices keep their bootstrap behavior unchanged.
This commit is contained in:
@@ -0,0 +1,193 @@
|
||||
# Archive Report: EQ Device Disconnect / Cold-Start Revert
|
||||
|
||||
**Change**: eq-device-disconnect-revert
|
||||
**Archive Date**: 2026-07-10
|
||||
**Artifact Store Mode**: hybrid
|
||||
**Status**: ARCHIVED
|
||||
|
||||
---
|
||||
|
||||
## Verification Verdict
|
||||
|
||||
**PASS WITH WARNINGS**
|
||||
|
||||
- **Critical Issues**: 0
|
||||
- **Warnings**: 1
|
||||
- **Suggestions**: 3
|
||||
- **Tests Passed**: 43/43 (target file), 68/68 (target + regression suite)
|
||||
- **Tasks Complete**: 15/16 (Phase 5.1 intentionally pending as non-blocking manual gate)
|
||||
|
||||
---
|
||||
|
||||
## Change Summary
|
||||
|
||||
**Scope**: EQ device disconnect handling and cold-start resolution guarantee
|
||||
|
||||
**What was changed**:
|
||||
- Added first-seen bootstrap guard in `_onDispositivoCambiado()` to exclude `altavozInterno` (base speaker) from device-preset caching, ensuring the base device always resolves live through the 4-level hierarchy (matrix → station → device → global)
|
||||
- Added 5 new test cases (D.1–D.5) covering disconnect revert, cold-start guarantees, connect-disconnect-reconnect cycles, station/matrix override interaction, and toggle-off invariance
|
||||
- All changes isolated to `lib/estado/estado_ecualizador.dart` (~9 lines modified) and `test/estado/estado_ecualizador_test.dart` (~214 lines added)
|
||||
|
||||
**Why it matters**:
|
||||
- Fixes a correctness gap: disconnect and cold-start events now explicitly re-resolve and apply the EQ preset, not coincidentally
|
||||
- Closes a L3-masks-L4 bug: base speaker can no longer be pinned by a stale device-level entry from the old bootstrap
|
||||
- Maintains full byte-for-byte compatibility when multi-device EQ is disabled
|
||||
|
||||
---
|
||||
|
||||
## Engram Artifacts (Observation IDs for Traceability)
|
||||
|
||||
All SDD artifacts persisted to Engram project `pluriwave` for cross-session recovery and audit trail:
|
||||
|
||||
| Artifact | Topic Key | Observation ID | Created | Details |
|
||||
|----------|-----------|----------------|---------|---------|
|
||||
| **Proposal** | `sdd/eq-device-disconnect-revert/proposal` | #2301 | 2026-07-10 17:53:45 | Intent, scope, approach, risks, rollback plan |
|
||||
| **Spec (Delta)** | `sdd/eq-device-disconnect-revert/spec` | #2306 | 2026-07-10 17:59:15 | 5 formal requirements, 6 scenarios; no prior main spec existed |
|
||||
| **Design** | `sdd/eq-device-disconnect-revert/design` | #2303 | 2026-07-10 17:58:01 | 6 architecture decisions, bootstrap guard fix, testing strategy |
|
||||
| **Tasks** | `sdd/eq-device-disconnect-revert/tasks` | #2315 | 2026-07-10 18:11:25 | Phases 1–5, review workload forecast (Low risk, single PR) |
|
||||
| **Apply Progress** | `sdd/eq-device-disconnect-revert/apply-progress` | #2323 | 2026-07-10 19:02:32 | Phases 1–4 complete (15 tasks), TDD cycle evidence, deviations log |
|
||||
| **Verify Report** | `sdd/eq-device-disconnect-revert/verify-report` | #2325 | 2026-07-10 23:38:59 | Completeness, spec compliance, coherence, TDD compliance, assertions, issues found |
|
||||
|
||||
**This Archive Report** will be persisted to Engram as:
|
||||
- **Topic Key**: `sdd/eq-device-disconnect-revert/archive-report`
|
||||
- **Type**: architecture
|
||||
- **Project**: pluriwave
|
||||
|
||||
---
|
||||
|
||||
## Spec Merge Status
|
||||
|
||||
### Delta Spec → Main Spec
|
||||
|
||||
| Action | File | Result |
|
||||
|--------|------|--------|
|
||||
| **Create** | `openspec/specs/multi-device-eq/spec.md` | ✅ Created (no prior spec existed; delta copied as full spec) |
|
||||
|
||||
**Content merged**: 5 ADDED Requirements with 6 scenarios (no MODIFIED or REMOVED entries). All requirements cover disconnect/cold-start revert behavior within the 4-level EQ hierarchy. On-disk delta spec at `openspec/changes/eq-device-disconnect-revert/specs/multi-device-eq/spec.md` was used as authoritative source (post-verify refinement applied by orchestrator to resolve WARNING-1 wording drift).
|
||||
|
||||
---
|
||||
|
||||
## Filesystem Archive Structure
|
||||
|
||||
**Move completed**: `openspec/changes/eq-device-disconnect-revert/` → `openspec/changes/archive/2026-07-10-eq-device-disconnect-revert/`
|
||||
|
||||
**Contents preserved**:
|
||||
- ✅ `proposal.md` — Intent and approach
|
||||
- ✅ `design.md` — Architecture decisions and bootstrap guard fix
|
||||
- ✅ `tasks.md` — Phases 1–5 with task checklist
|
||||
- ✅ `specs/multi-device-eq/spec.md` — Full delta spec (5 requirements)
|
||||
- ✅ `verify-report.md` — Completeness, spec compliance, TDD compliance
|
||||
- ✅ `state.yaml` — DAG state (archived status recorded)
|
||||
- ✅ `archive-report.md` — This document
|
||||
|
||||
**Artifacts NOT in openspec/**:
|
||||
- Engram artifacts remain in Engram at topic keys listed above (not moved; persisted independently)
|
||||
|
||||
---
|
||||
|
||||
## Issue Tracking and Resolutions
|
||||
|
||||
### Verification WARNING (1)
|
||||
|
||||
**WARNING-1: Spec Text / Implementation Divergence on Requirement 2**
|
||||
|
||||
- **Description**: Spec scenario literally states `presetsDispositivo['builtin_speaker']` is bootstrapped to the global preset with the implication the entry is created. Implementation (matching Design Decision #2) NEVER creates that entry — the `esBase` guard unconditionally skips the bootstrap block.
|
||||
- **Status**: ✅ RESOLVED (post-verify spec refinement)
|
||||
- **Resolution**: The on-disk spec file at `openspec/changes/eq-device-disconnect-revert/specs/multi-device-eq/spec.md` was edited by the orchestrator post-verify to correct the THEN clause: now states no entry is ever created and resolution always falls through to the hierarchy. This is functionally correct and safer than the literal wording.
|
||||
- **Proof**: Test D.2 asserts `presetsDispositivo.containsKey('builtin_speaker')` is `false` — proves no entry exists, and the edit aligns the spec with this proven behavior.
|
||||
|
||||
### Verification SUGGESTIONS (3)
|
||||
|
||||
**SUGGESTION-1**: Reconcile spec text per WARNING-1 resolution (already done in on-disk file; no action needed).
|
||||
|
||||
**SUGGESTION-2**: Apply-progress TDD Cycle Evidence table uses 3-column format (RED/GREEN/REFACTOR) instead of full 5-column strict-TDD template. Substance is present; format nitpick for future runs.
|
||||
|
||||
**SUGGESTION-3**: Pre-existing `cambiarPresetPrincipal()` hierarchy-bypass (applies raw global instead of re-resolving) observed during apply, noted as tracked tech debt. Out of scope for this change; recommend a follow-up ticket.
|
||||
|
||||
---
|
||||
|
||||
## Non-Blocking Items
|
||||
|
||||
**Task 5.1: On-Device Manual Sanity Check — PENDING**
|
||||
|
||||
- **Description**: Connect BT/USB device with own preset, disconnect (verify revert to base), reconnect (verify return to device preset)
|
||||
- **Status**: Intentionally unchecked
|
||||
- **Reason**: Non-automatable human gate requiring real hardware; verified via automated unit tests (D.1–D.5) to be theoretically sound
|
||||
- **Blocking the release?**: No. Can be executed anytime on real hardware as a confidence check; not required for PR merge.
|
||||
|
||||
---
|
||||
|
||||
## Tech Debt Carried Forward
|
||||
|
||||
**Pre-existing `cambiarPresetPrincipal()` Bypass**
|
||||
|
||||
- **Description**: The `cambiarPresetPrincipal()` method immediately calls `audio.aplicarPresetActivo(preset)`, bypassing `_resolverPresetActivo()` and applying the raw global preset directly without re-resolving the hierarchy. This could allow a station or matrix override to be transiently bypassed for one apply call.
|
||||
- **Scope**: Out of scope for this change; pre-existing behavior unrelated to disconnect/cold-start revert
|
||||
- **Recommendation**: Follow-up ticket to refactor `cambiarPresetPrincipal` to push through `_resolverPresetActivo()` for hierarchy consistency
|
||||
|
||||
---
|
||||
|
||||
## Review Workload Impact
|
||||
|
||||
| Metric | Value |
|
||||
|--------|-------|
|
||||
| Estimated changed lines (from task forecast) | 130–160 |
|
||||
| Actual changed lines (via git) | ~221 (9 lib + 214 test) |
|
||||
| 400-line budget risk | Low |
|
||||
| Chained PRs recommended | No |
|
||||
| Delivery strategy applied | ask-on-risk → single PR (no split needed) |
|
||||
|
||||
---
|
||||
|
||||
## Completeness Checklist
|
||||
|
||||
| Item | Status | Notes |
|
||||
|------|--------|-------|
|
||||
| Proposal read and logged | ✅ | #2301 |
|
||||
| Spec read and merged | ✅ | #2306 → openspec/specs/multi-device-eq/spec.md |
|
||||
| Design read and verified | ✅ | #2303, all 6 decisions followed |
|
||||
| Tasks read and tracked | ✅ | #2315, 15/16 complete |
|
||||
| Apply progress read | ✅ | #2323, Phases 1–4 complete |
|
||||
| Verify report read and resolved | ✅ | #2325, PASS WITH WARNINGS, WARNING resolved in spec |
|
||||
| All artifacts moved to archive | ✅ | Filesystem and Engram copies established |
|
||||
| State recorded (archived) | ✅ | state.yaml written with status and notes |
|
||||
| Archive report created | ✅ | This document (both filesystem and Engram) |
|
||||
|
||||
---
|
||||
|
||||
## SDD Cycle Complete
|
||||
|
||||
This change has successfully completed all phases:
|
||||
|
||||
1. ✅ **Explore** — Investigated device-event handling and revert semantics
|
||||
2. ✅ **Propose** — Defined scope and approach
|
||||
3. ✅ **Spec** — Formal 5-requirement specification with 6 scenarios
|
||||
4. ✅ **Design** — Architecture decisions and bootstrap guard detail
|
||||
5. ✅ **Tasks** — Breakdown with phases, forecast, and completeness tracking
|
||||
6. ✅ **Apply** — Implemented with Strict TDD (15/16 tasks automated, 1/16 manual gate)
|
||||
7. ✅ **Verify** — PASS WITH WARNINGS (43/43 tests, 0 CRITICAL, 1 WARNING resolved)
|
||||
8. ✅ **Archive** — All artifacts consolidated, spec merged, change moved to archive
|
||||
|
||||
**Ready for**: Next change or release planning
|
||||
|
||||
---
|
||||
|
||||
## Artifact Links
|
||||
|
||||
- **Main spec**: `openspec/specs/multi-device-eq/spec.md`
|
||||
- **Archived change folder**: `openspec/changes/archive/2026-07-10-eq-device-disconnect-revert/`
|
||||
- **Engram topic keys** (full content via mem_get_observation):
|
||||
- Proposal: `sdd/eq-device-disconnect-revert/proposal` (#2301)
|
||||
- Spec: `sdd/eq-device-disconnect-revert/spec` (#2306)
|
||||
- Design: `sdd/eq-device-disconnect-revert/design` (#2303)
|
||||
- Tasks: `sdd/eq-device-disconnect-revert/tasks` (#2315)
|
||||
- Apply Progress: `sdd/eq-device-disconnect-revert/apply-progress` (#2323)
|
||||
- Verify Report: `sdd/eq-device-disconnect-revert/verify-report` (#2325)
|
||||
- Archive Report: `sdd/eq-device-disconnect-revert/archive-report` (this document)
|
||||
|
||||
---
|
||||
|
||||
**Archived by**: SDD Archive Phase (haiku model)
|
||||
**Date**: 2026-07-10 (ISO format)
|
||||
**Project**: pluriwave
|
||||
**Mode**: Hybrid (Engram + OpenSpec filesystem)
|
||||
@@ -0,0 +1,76 @@
|
||||
# Design: EQ Device Disconnect / Cold-Start Revert
|
||||
|
||||
## Technical Approach
|
||||
|
||||
Keep the single handler `_onDispositivoCambiado(DispositivoAudio)` — the platform stream has no add/remove discriminator; it always emits the now-active device (disconnect emits the fallback, e.g. `builtin_speaker`). The handler already sets `_dispositivoActualId`, re-resolves via `_resolverPresetActivo()`, and applies via `audio.aplicarPreset()`. The change is NOT new logic — it is **removing the first-seen bootstrap collision** so revert-to-base resolves live through the 4-level hierarchy instead of being pinned by a forced device-level copy, then locking the corrected behavior with disconnect / cold-start / cycle tests. No parallel resolution path is added; `_resolverPresetActivo()` stays the sole resolver (spec REQ-2, REQ-4).
|
||||
|
||||
## Architecture Decisions
|
||||
|
||||
| # | Decision | Options | Choice + Rationale |
|
||||
|---|----------|---------|--------------------|
|
||||
| 1 | Handler shape | (a) keep single handler, unconditional resolve+apply · (b) split connect/disconnect dispatch · (c) add Kotlin `type` field | **(a)**. Stream carries only the active device; disconnect == "resolve for whatever is now active". Splitting needs a native payload change (out of scope) and buys nothing — resolution is already device-uniform. |
|
||||
| 2 | Bootstrap collision (base device) | (a) exclude base type from bootstrap · (b) bootstrap only BT/USB · (c) keep, tolerate redundant entry | **(a)** gate on `dispositivo.tipo != TipoDispositivo.altavozInterno`. `builtin_speaker` must always fall through to global (L4) so a later global-preset edit is not masked by a stale L3 entry. (b) is (a) plus wired-headset ambiguity; wired is legitimately per-device, so only the base speaker is excluded. (c) leaves the persisted mask bug. |
|
||||
| 3 | Revert semantics | (a) deterministic re-resolution · (b) snapshot/restore prior preset | **(a)** — locked by exploration. Hierarchy is a pure fn of `(station, device)`; it self-heals and is the ONLY mechanism that also covers cold start (no snapshot exists at boot). |
|
||||
| 4 | Apply guarantee | reuse `aplicarPreset()` choke-point vs state-only mutation | Reuse. `_presetActual = resuelto; await audio.aplicarPreset(resuelto)` already present — verified reaching native EQ (REQ-3). Keep idempotent. |
|
||||
| 5 | Cold start | trust seed path vs add parallel apply | Trust + test. `cargarPersistido` → `_sembrarDispositivoActual` → `obtenerDispositivoActual` (base fallback) → `_onDispositivoCambiado` already applies base resolution. No new code; add proof tests. |
|
||||
| 6 | Toggle-off invariance | — | Early return at L211 (`if (!_eqMultiDeviceEnabled) return;`) is untouched. Subscription is never created when off (`_configurarSuscripcionDispositivo`). Byte-for-byte 2-level preserved. |
|
||||
|
||||
## Data Flow
|
||||
|
||||
```
|
||||
Disconnect (BT removed) Cold start (no device)
|
||||
platform emits builtin_speaker cargarPersistido()
|
||||
│ ├─ _resolverPresetActivo() [deviceId null → global]
|
||||
▼ ├─ aplicarPreset(base)
|
||||
_onDispositivoCambiado(builtin_speaker) └─ _sembrarDispositivoActual()
|
||||
├─ guard _eqMultiDeviceEnabled └─ obtenerDispositivoActual() → builtin_speaker
|
||||
├─ _dispositivoActualId = builtin_speaker └─ _onDispositivoCambiado(builtin_speaker)
|
||||
├─ BOOTSTRAP: SKIP (tipo == altavozInterno) ← the fix
|
||||
├─ resuelto = _resolverPresetActivo() [matrix→station→device→GLOBAL]
|
||||
├─ _presetActual = resuelto
|
||||
└─ audio.aplicarPreset(resuelto) ──────────────► native EQ
|
||||
```
|
||||
|
||||
## File Changes
|
||||
|
||||
| File | Action | Description |
|
||||
|------|--------|-------------|
|
||||
| `lib/estado/estado_ecualizador.dart` | Modify | `_onDispositivoCambiado`: gate the first-seen bootstrap (L216-220) on `dispositivo.tipo != TipoDispositivo.altavozInterno`. Resolve+apply stay unconditional and unchanged. |
|
||||
| `test/estado/estado_ecualizador_test.dart` | Modify | Add disconnect-revert group (see Testing Strategy). Extend existing fakes only via helpers already present. |
|
||||
|
||||
## Interfaces / Contracts
|
||||
|
||||
No new public API, no persistence field, no migration. Bootstrap guard uses the existing `DispositivoAudio.tipo` discriminator:
|
||||
|
||||
```dart
|
||||
// First-seen device bootstrap: skip base speaker so it always resolves
|
||||
// live through the hierarchy (never pin a stale device-level entry).
|
||||
final esBase = dispositivo.tipo == TipoDispositivo.altavozInterno;
|
||||
if (!esBase && !_presetsDispositivo.containsKey(dispositivo.id)) {
|
||||
final presetBase = _resolverPresetActivo();
|
||||
_presetsDispositivo[dispositivo.id] = presetBase;
|
||||
await servicio.guardarPresetDispositivo(dispositivo.id, presetBase);
|
||||
}
|
||||
```
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
Existing fakes suffice — no new fake types. `FakeServicioDispositivoAudio.emitirDispositivo()` drives connect/disconnect; `obtenerDispositivoActual()` returns `builtin_speaker` when nothing emitted (cold-start path). Reuse the base-speaker `DispositivoAudio(id:'builtin_speaker', tipo: altavozInterno)`.
|
||||
|
||||
| ID | Layer | What to test |
|
||||
|----|-------|--------------|
|
||||
| D.1 | Unit | Disconnect: BT active → emit `builtin_speaker` → `presetActual` == resolved base (global when no station/matrix); `presetsAplicados.last` == base. |
|
||||
| D.2 | Unit | Disconnect does NOT create `presetsDispositivo['builtin_speaker']` (bootstrap skip proven — guards the L3 mask bug). |
|
||||
| D.3 | Unit | Cold start, nothing emitted: `cargarPersistido` applies base (global), `dispositivoActualId == 'builtin_speaker'`, and no stale device preset applied. |
|
||||
| D.4 | Unit | Cycle: connect BT (device preset) → disconnect (base) → reconnect BT lands on the same device preset each step; 5.5b invariant holds (reconnect never overwrites user preset). |
|
||||
| D.5 | Unit | Station-override interaction: station has own preset + BT connected → disconnect → resolution still honors the station preset for `builtin_speaker` (L2 beats L4), proving revert is deterministic, not flatten-to-global. |
|
||||
|
||||
Consistency: cases 5.1-5.8, 2.1-2.4 stay green unchanged. 5.5a (BT first-seen bootstrap) and 2.2 (BT startup bootstrap) still pass because both use `bluetoothA2dp`, which is NOT excluded by the guard. Full `flutter test` green before/after (Strict TDD, RED→GREEN per new case).
|
||||
|
||||
## Migration / Rollout
|
||||
|
||||
No migration. Any `presetsDispositivo['builtin_speaker']` entries already persisted by the old bootstrap remain harmless (L3 still resolves) and are not written going forward; optional one-line cleanup deferred — out of scope.
|
||||
|
||||
## Open Questions
|
||||
|
||||
- None blocking. Wired-headset (`auricularesCable`) is intentionally treated as per-device (bootstrap applies) — confirm in tasks if product wants it excluded too; default keeps current per-device behavior.
|
||||
@@ -0,0 +1,62 @@
|
||||
# Proposal: EQ Device Disconnect / Cold-Start Revert
|
||||
|
||||
## Intent
|
||||
|
||||
When an audio device disconnects — or the app opens with no external device connected — the EQ must return to the phone/base configuration, not stay on a stale device preset. Today `_onDispositivoCambiado()` (`estado_ecualizador.dart:210-227`) treats every device event identically: there is no explicit disconnect path, no cold-start guarantee, and ZERO disconnect test coverage. On disconnect it re-resolves the hierarchy for the now-active device, which only *coincidentally* looks like a revert when no station/matrix override interferes. This closes a correctness gap, not a regression.
|
||||
|
||||
## Scope
|
||||
|
||||
### In Scope
|
||||
- Explicit disconnect handling: re-resolve + APPLY the preset for the now-active device (typically `builtin_speaker`).
|
||||
- Cold-start guarantee: app open with no device connected applies the phone/base resolution, never a stale persisted device preset.
|
||||
- Verify the resolved preset reaches the native EQ via the existing `audio.aplicarPreset()` choke-point (not state-only mutation).
|
||||
- Test coverage: disconnect event, cold start without device, connect→disconnect→reconnect cycle, station/matrix override interaction.
|
||||
- Respect `_eqMultiDeviceEnabled`: when OFF, behavior is unchanged (2-level resolution).
|
||||
|
||||
### Out of Scope
|
||||
- Audio-focus ducking EQ re-apply (separate change `eq-audiofocus-reapply`, Part B).
|
||||
- Native Kotlin/Swift device detection (`MainActivity.kt`, `pluriwave/audio_devices`) — already emits add/remove correctly.
|
||||
- New persistence fields or migration; matrix/station/device preset storage is unchanged.
|
||||
|
||||
## Capabilities
|
||||
|
||||
### New Capabilities
|
||||
- None.
|
||||
|
||||
### Modified Capabilities
|
||||
- `multi-device-eq`: revert-on-disconnect and cold-start resolution become explicit, guaranteed requirements. "Revert" = deterministic re-resolution of the existing 4-level hierarchy (matrix → station → device → global) for the now-active device — NOT a snapshot/restore. This is stateless, self-healing, and the only mechanism covering the cold-start case (no snapshot exists to restore).
|
||||
|
||||
## Approach
|
||||
|
||||
Make the disconnect path explicit inside the device-event handler (`_onDispositivoCambiado`, or a split connect/disconnect dispatch). On any device event — connect, disconnect, or cold-start seed — set `_dispositivoActualId` to the now-active device, run `_resolverPresetActivo()`, assign `_presetActual`, and push it through `audio.aplicarPreset()`. Cold start already flows through `_sembrarDispositivoActual()` → `obtenerDispositivoActual()` (fallback `builtin_speaker`); the change guarantees the resolved base preset is APPLIED on that path, not just seeded. The first-seen bootstrap (`presetsDispositivo` write) must NOT hijack revert resolution for the base device.
|
||||
|
||||
## Affected Areas
|
||||
|
||||
| Area | Impact | Description |
|
||||
|------|--------|-------------|
|
||||
| `lib/estado/estado_ecualizador.dart` | Modified | Explicit disconnect/cold-start resolve+apply in the device handler |
|
||||
| `test/estado/estado_ecualizador_test.dart` | Modified | New disconnect, cold-start, reconnect-cycle, override-matrix cases |
|
||||
|
||||
## Risks
|
||||
|
||||
| Risk | Likelihood | Mitigation |
|
||||
|------|------------|------------|
|
||||
| Regress passing connect/4-level suite (Phase 5, tests 5.1–5.8, 2.x) | Med | Reuse `_resolverPresetActivo()`; run full `estado_ecualizador_test.dart` green before/after |
|
||||
| First-seen bootstrap overwrites base on disconnect | Med | Guard bootstrap so revert to `builtin_speaker` resolves from hierarchy, not a forced copy |
|
||||
| Double-apply / redundant native pushes on rapid events | Low | Idempotent apply choke-point; skip when resolved preset unchanged |
|
||||
|
||||
## Rollback Plan
|
||||
|
||||
Single-file logic change plus tests. Revert the commit(s) touching `estado_ecualizador.dart` and its test file; no persistence schema, native, or public-API change means rollback restores the prior coincidental-revert behavior with zero migration.
|
||||
|
||||
## Dependencies
|
||||
|
||||
- Shared exploration `sdd/eq-apply-lifecycle/explore` (Part A section). Locked decision: re-resolution, not snapshot/restore.
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- [ ] Disconnect event re-resolves and applies the base-device preset via `aplicarPreset()`.
|
||||
- [ ] Cold start with no device applies phone/base resolution, not a stale device preset.
|
||||
- [ ] connect→disconnect→reconnect cycle lands on the correct preset at each step, including station/matrix overrides.
|
||||
- [ ] With `_eqMultiDeviceEnabled=false`, behavior is byte-for-byte unchanged (2-level).
|
||||
- [ ] Full `estado_ecualizador_test.dart` suite stays green (no connect-path regression).
|
||||
+136
@@ -0,0 +1,136 @@
|
||||
# Delta for multi-device-eq
|
||||
|
||||
No prior `openspec/specs/multi-device-eq/spec.md` exists. This delta adds
|
||||
requirements as new (ADDED), scoped to disconnect/cold-start revert only.
|
||||
Connect-path and 4-level-hierarchy requirements are out of scope (already
|
||||
covered by the passing test suite).
|
||||
|
||||
## ADDED Requirements
|
||||
|
||||
### Requirement: Device-Event Resolve-and-Apply
|
||||
|
||||
The system MUST treat every device-change event — connect or disconnect —
|
||||
identically: set the now-active device as current, re-resolve via the
|
||||
existing 4-level hierarchy (matrix -> station -> device -> global), and
|
||||
APPLY via `ServicioAudio.aplicarPreset()`. The system MUST NOT branch on
|
||||
event type; no add/remove discriminator exists on the event, and none is
|
||||
required.
|
||||
|
||||
#### Scenario: Connect applies the device's own preset
|
||||
|
||||
- GIVEN `_eqMultiDeviceEnabled` is true and a station is active
|
||||
- WHEN a device-change event arrives for a device with a resolvable preset
|
||||
- THEN the resolved preset becomes `presetActual`
|
||||
- AND `audio.aplicarPreset()` is called with it (asserted via
|
||||
`FakeServicioAudio.presetsAplicados`)
|
||||
|
||||
#### Scenario: Disconnect resolves and applies the base preset
|
||||
|
||||
- GIVEN `_eqMultiDeviceEnabled` is true, a BT device is active with its own
|
||||
resolved preset, and `builtin_speaker` resolves to a distinct preset
|
||||
- WHEN a device-change event arrives carrying `builtin_speaker` (simulating
|
||||
disconnect-to-base via `FakeServicioDispositivoAudio.emitirDispositivo`,
|
||||
which carries no add/remove flag)
|
||||
- THEN `_dispositivoActualId` becomes `'builtin_speaker'`
|
||||
- AND the hierarchy resolution for `builtin_speaker` is applied
|
||||
- AND `audio.aplicarPreset()` records it as the last entry in
|
||||
`presetsAplicados` (proves push, not state-only mutation)
|
||||
|
||||
### Requirement: First-Seen Bootstrap Must Not Hijack Base-Device Resolution
|
||||
|
||||
The system MUST NOT force-copy a stale resolved preset into
|
||||
`presetsDispositivo['builtin_speaker']` on disconnect when it has no
|
||||
existing entry. The base speaker is excluded from first-seen bootstrap
|
||||
entirely: no `presetsDispositivo['builtin_speaker']` entry is ever
|
||||
auto-created, so the base device always resolves live through the
|
||||
hierarchy (a persistent device-level entry would mask later global-preset
|
||||
edits, since level 3 beats level 4 on every subsequent resolution).
|
||||
|
||||
#### Scenario: Disconnect to a never-seen builtin_speaker resolves from the hierarchy
|
||||
|
||||
- GIVEN `_eqMultiDeviceEnabled` is true, `presetsDispositivo` has no entry
|
||||
for `builtin_speaker`, a BT device is active resolved to `jazz`, and
|
||||
global `_presetPrincipal` is `rock`
|
||||
- WHEN the device-change event carries `builtin_speaker`
|
||||
- THEN no `presetsDispositivo['builtin_speaker']` entry is created, and
|
||||
resolution falls through to the global preset `rock`, NOT `jazz`
|
||||
- AND `audio.aplicarPreset()` is called with `rock`
|
||||
- AND a later change of the global preset while on `builtin_speaker`
|
||||
re-resolves to the new global value (no stale level-3 entry masks it)
|
||||
|
||||
### Requirement: Cold Start Applies Base Resolution, Never a Stale Persisted Preset
|
||||
|
||||
The system MUST guarantee that cold start with no external device connected
|
||||
applies the phone/base resolution for `builtin_speaker` through
|
||||
`audio.aplicarPreset()`, never a stale persisted preset for a
|
||||
previously-connected, now-inactive device.
|
||||
|
||||
#### Scenario: Cold start with no device connected applies the base preset
|
||||
|
||||
- GIVEN `_eqMultiDeviceEnabled` is true, `presetsDispositivo` has a stale
|
||||
entry for a BT device id that is NOT currently connected, and
|
||||
`obtenerDispositivoActual()` resolves to `builtin_speaker` (its default
|
||||
when no device was ever emitted)
|
||||
- WHEN `cargarPersistido()` runs (`_sembrarDispositivoActual()` ->
|
||||
`_onDispositivoCambiado()`)
|
||||
- THEN `_dispositivoActualId` becomes `'builtin_speaker'`
|
||||
- AND `presetActual` is the hierarchy resolution for `builtin_speaker`
|
||||
- AND `audio.aplicarPreset()` records that resolution, NOT the stale BT
|
||||
device's preset
|
||||
|
||||
### Requirement: Connect-Disconnect-Reconnect Cycle Lands on the Correct Preset at Each Step
|
||||
|
||||
The system MUST resolve and apply the correct preset at every step of a
|
||||
connect -> disconnect -> reconnect cycle, honoring station and matrix
|
||||
overrides, and MUST NOT let disconnect corrupt state such that reconnect
|
||||
resolves incorrectly.
|
||||
|
||||
#### Scenario: Full cycle honors station/matrix overrides at each step
|
||||
|
||||
- GIVEN a matrix override for `(stationUuid, btDevice.id)` = `bassBoost`,
|
||||
and `builtin_speaker` resolves to global `flat`
|
||||
- WHEN the device stream emits: BT device (connect) -> `builtin_speaker`
|
||||
(disconnect) -> BT device (reconnect)
|
||||
- THEN after connect, `presetActual` is `bassBoost`; after disconnect,
|
||||
`flat`; after reconnect, `bassBoost` again — each recorded as the last
|
||||
entry in `presetsAplicados`
|
||||
- AND the BT device's `presetsDispositivo` entry is UNCHANGED by the
|
||||
disconnect step (consistent with existing test 5.5b: reconnect must not
|
||||
overwrite a user-saved device preset)
|
||||
|
||||
### Requirement: Toggle-Off Behavior Is Byte-for-Byte Unchanged
|
||||
|
||||
When `_eqMultiDeviceEnabled` is false, device-change events MUST NOT alter
|
||||
resolution: no device subscription is active, no first-seen bootstrap runs,
|
||||
and resolution stays 2-level (station -> global) only.
|
||||
|
||||
#### Scenario: Toggle off ignores device events entirely
|
||||
|
||||
- GIVEN `_eqMultiDeviceEnabled` is false
|
||||
- WHEN a device-change event is emitted (any device, including
|
||||
`builtin_speaker`)
|
||||
- THEN `_onDispositivoCambiado` returns immediately with no state change
|
||||
- AND `presetEfectivo(stationUuid, deviceId)` still resolves via the
|
||||
2-level path exactly as before this change
|
||||
- AND `presetsAplicados` gains no new entry from the event
|
||||
|
||||
## Testability
|
||||
|
||||
All scenarios are Dart-testable via existing fakes in
|
||||
`test/helpers/fakes.dart` — no new fake infrastructure required:
|
||||
|
||||
- `FakeServicioDispositivoAudio.emitirDispositivo(dispositivo)` — pushes any
|
||||
`DispositivoAudio` through `onDispositivoCambiado`; used identically for
|
||||
simulated connect and disconnect (no discriminator on the real stream).
|
||||
- `FakeServicioDispositivoAudio.obtenerDispositivoActual()` — defaults to
|
||||
`builtin_speaker`; used for the cold-start scenario.
|
||||
- `FakeServicioAudio.presetsAplicados` — proves gains were PUSHED via
|
||||
`aplicarPreset()`, not just mutated in state. Every scenario asserts this
|
||||
list, not only `presetActual`.
|
||||
- `FakeServicioEcualizador` constructor params (`principal`, `porEmisora`,
|
||||
`presetsDispositivo`, `presetsMatriz`) — seed the hierarchy as the
|
||||
existing Phase 5 suite already does.
|
||||
|
||||
## Localization
|
||||
|
||||
No new l10n strings are introduced by this change.
|
||||
@@ -0,0 +1,17 @@
|
||||
schema: sdd-change-state
|
||||
change: eq-device-disconnect-revert
|
||||
status: archived
|
||||
archived_date: 2026-07-10
|
||||
verification_verdict: PASS WITH WARNINGS
|
||||
critical_issues: 0
|
||||
warning_issues: 1
|
||||
suggestion_issues: 3
|
||||
tasks_complete: 15
|
||||
tasks_total: 16
|
||||
notes: |
|
||||
Change archived successfully. Spec delta merged into main spec.
|
||||
Task 5.1 (manual on-device sanity check) remains PENDING as non-blocking human gate.
|
||||
Pre-existing cambiarPresetPrincipal() hierarchy-bypass carried forward as tracked tech debt.
|
||||
Verification WARNING (spec-text/implementation drift on Requirement 2) already resolved:
|
||||
on-disk spec file at openspec/changes/eq-device-disconnect-revert/specs/multi-device-eq/spec.md
|
||||
was edited post-verify to match safer implementation (no entry ever created for builtin_speaker).
|
||||
@@ -0,0 +1,54 @@
|
||||
# Tasks: EQ Device Disconnect / Cold-Start Revert
|
||||
|
||||
## Review Workload Forecast
|
||||
|
||||
| Field | Value |
|
||||
|-------|-------|
|
||||
| Estimated changed lines | ~130-160 |
|
||||
| 400-line budget risk | Low |
|
||||
| Chained PRs recommended | No |
|
||||
| Suggested split | Single PR |
|
||||
| Delivery strategy | ask-on-risk |
|
||||
| Chain strategy | pending |
|
||||
|
||||
Decision needed before apply: No
|
||||
Chained PRs recommended: No
|
||||
Chain strategy: pending
|
||||
400-line budget risk: Low
|
||||
|
||||
### Suggested Work Units
|
||||
|
||||
| Unit | Goal | Likely PR | Notes |
|
||||
|------|------|-----------|-------|
|
||||
| 1 | Bootstrap guard fix + proof tests (RED-GREEN-REFACTOR) | PR 1 (single PR) | Base: main. One logical change, well under 400-line budget |
|
||||
|
||||
## Phase 1: RED — Write Failing Tests
|
||||
|
||||
- [x] 1.1 Add group `'device disconnect / cold-start revert (Phase D)'` to `test/estado/estado_ecualizador_test.dart` after L334, reusing existing fakes (no new fake types)
|
||||
- [x] 1.2 D.1 disconnect resolves+applies base preset (spec Req 1, scenario "Disconnect resolves and applies the base preset")
|
||||
- [x] 1.3 D.2 disconnect does NOT bootstrap `presetsDispositivo['builtin_speaker']` from the disconnecting device (spec Req 2)
|
||||
- [x] 1.4 D.3 cold start with no device applies base resolution via `cargarPersistido()` only, not a stale device preset (spec Req 3)
|
||||
- [x] 1.5 D.4 connect→disconnect→reconnect cycle: correct preset at each step, matrix override honored, device preset unchanged by disconnect (spec Req 4, locks 5.5b)
|
||||
- [x] 1.6 D.5 station override still wins over global on disconnect-to-base (design Testing Strategy D.5)
|
||||
- [x] 1.7 Run `flutter test test/estado/estado_ecualizador_test.dart` — confirm D.1-D.5 FAIL; D.2 must fail on the real bootstrap-collision bug, not a tautology
|
||||
|
||||
## Phase 2: GREEN — Minimal Fix
|
||||
|
||||
- [x] 2.1 In `lib/estado/estado_ecualizador.dart` `_onDispositivoCambiado()` (L216-220), gate bootstrap on `dispositivo.tipo != TipoDispositivo.altavozInterno`
|
||||
- [x] 2.2 Leave L213 (`_dispositivoActualId` assignment) and L222-226 (resolve+apply) unchanged — single handler, no branching on event type
|
||||
- [x] 2.3 Run test suite — confirm D.1-D.5 PASS
|
||||
|
||||
## Phase 3: Regression Lock
|
||||
|
||||
- [x] 3.1 Run full `estado_ecualizador_test.dart` — all existing groups green (5.1-5.8, 2.1-2.4, rename API, guardarPresetDispositivo)
|
||||
- [x] 3.2 Confirm 5.5a and 2.2 (both `bluetoothA2dp`) unaffected — guard is base-device-scoped only
|
||||
- [x] 3.3 Confirm 5.5b and toggle-off 5.7a/5.8b pass unmodified — named consistency locks
|
||||
|
||||
## Phase 4: REFACTOR
|
||||
|
||||
- [x] 4.1 Add one-line comment above the bootstrap block explaining the `altavozInterno` exclusion (prevents L3 masking future L4 edits)
|
||||
- [x] 4.2 `flutter analyze` on both modified files — zero new warnings
|
||||
|
||||
## Phase 5: Manual Sanity (light, non-blocking)
|
||||
|
||||
- [ ] 5.1 On-device: connect BT/USB with own preset, disconnect (reverts to base), reconnect (returns to device preset) — one-time confidence check
|
||||
@@ -0,0 +1,131 @@
|
||||
## Verification Report
|
||||
|
||||
**Change**: eq-device-disconnect-revert
|
||||
**Version**: N/A (delta spec, no prior openspec/specs/multi-device-eq/spec.md)
|
||||
**Mode**: Strict TDD
|
||||
|
||||
### Completeness
|
||||
| Metric | Value |
|
||||
|--------|-------|
|
||||
| Tasks total | 16 |
|
||||
| Tasks complete | 15 |
|
||||
| Tasks incomplete | 1 (5.1, manual on-device sanity, intentionally left open as a non-blocking human gate) |
|
||||
|
||||
### Build and Tests Execution
|
||||
**Build**: not run (explicitly excluded from this verification per instruction; flutter build must never be run in this phase)
|
||||
|
||||
**Tests**: 43 passed / 0 failed (target file); 68 passed / 0 failed (target file plus 3 related regression files)
|
||||
```text
|
||||
flutter test test/estado/estado_ecualizador_test.dart --reporter expanded
|
||||
-> 43/43 passed, including D.1-D.5 (new), 5.5a/5.5b, 5.7a/5.8b, 2.1-2.4, rename API, guardarPresetDispositivo groups
|
||||
|
||||
flutter test test/estado/estado_ecualizador_test.dart test/servicios/servicio_audio_eq_reapply_test.dart test/servicios/servicio_ecualizador_test.dart test/pantallas/pantalla_ajustes_test.dart --reporter expanded
|
||||
-> 68/68 passed
|
||||
|
||||
flutter test --reporter compact (attempted full suite)
|
||||
-> TIMED OUT / hung at ~300s. Confirmed known unrelated flake: runaway log spam originating in
|
||||
test/estado/estado_alarmas_ejecuciones_test.dart and test/estado/estado_alarmas_snooze_test.dart
|
||||
(repeating log line once per simulated minute). Process terminated cleanly by tool timeout; verified
|
||||
no orphaned dart.exe processes afterward. Fell back to explicit targeted runs per instructions.
|
||||
Mode used: TARGETED FALLBACK, not full suite.
|
||||
```
|
||||
|
||||
**Coverage**: Diff-region (the 2 actual changed executable lines): 100 percent. Whole-file baseline (this test file alone): 59.3 percent (118/199 lines), pre-existing, not moved backward by this change. See Changed File Coverage below.
|
||||
|
||||
### Spec Compliance Matrix
|
||||
| Requirement | Scenario | Test | Result |
|
||||
|-------------|----------|------|--------|
|
||||
| Device-Event Resolve-and-Apply | Connect applies the active device preset | estado_ecualizador_test.dart, test 5.3b (pre-existing, unmodified) | COMPLIANT |
|
||||
| Device-Event Resolve-and-Apply | Disconnect resolves and applies the base preset | estado_ecualizador_test.dart, test D.1 | COMPLIANT |
|
||||
| First-Seen Bootstrap Must Not Hijack Base-Device Resolution | Disconnect to a never-seen builtin_speaker resolves from the hierarchy | estado_ecualizador_test.dart, test D.2 | PARTIAL, see WARNING-1 |
|
||||
| Cold Start Applies Base Resolution, Never a Stale Persisted Preset | Cold start with no device connected applies the base preset | estado_ecualizador_test.dart, test D.3 | COMPLIANT |
|
||||
| Connect-Disconnect-Reconnect Cycle Lands on the Correct Preset at Each Step | Full cycle honors station/matrix overrides at each step | estado_ecualizador_test.dart, test D.4 | COMPLIANT |
|
||||
| Toggle-Off Behavior Is Byte-for-Byte Unchanged | Toggle off ignores device events entirely | estado_ecualizador_test.dart, tests 5.3c and 5.7a (pre-existing, unmodified) | COMPLIANT |
|
||||
| (bonus, design Testing Strategy, not a distinct spec scenario) | Station override still wins over global on disconnect-to-base | estado_ecualizador_test.dart, test D.5 | COMPLIANT (supplementary coverage) |
|
||||
|
||||
**Compliance summary**: 6/6 formal spec scenarios compliant (5 fully compliant, 1 PARTIAL on literal scenario wording only; functional intent met and exceeded) plus 1 bonus scenario compliant.
|
||||
|
||||
Note: the verification task brief referenced "6 requirements / 7 scenarios". The actual persisted spec artifact (engram sdd/eq-device-disconnect-revert/spec #2306, byte-identical to openspec/changes/eq-device-disconnect-revert/specs/multi-device-eq/spec.md) contains 5 "Requirement:" headings and 6 "Scenario:" headings (Requirement 1 has 2 scenarios; Requirements 2-5 have 1 each). Counting design-driven bonus test D.5 as a 7th test case reconciles the "7" figure; there is no 6th formal requirement in the artifact.
|
||||
|
||||
### Correctness (Static Evidence)
|
||||
| Requirement | Status | Notes |
|
||||
|------------|--------|-------|
|
||||
| Device-Event Resolve-and-Apply | Implemented | Single handler _onDispositivoCambiado, unconditional resolve (_resolverPresetActivo) plus apply (audio.aplicarPreset) at L228-231, no event-type branch |
|
||||
| First-Seen Bootstrap Must Not Hijack Base-Device Resolution | Implemented, stronger than literal scenario text | esBase guard at L220-221 fully excludes altavozInterno from bootstrap; no entry is ever persisted for builtin_speaker, not even a correct-value one |
|
||||
| Cold Start Applies Base Resolution | Implemented | cargarPersistido calls _sembrarDispositivoActual which calls _onDispositivoCambiado with the builtin_speaker default; confirmed via source read L141-196 |
|
||||
| Connect-Disconnect-Reconnect Cycle | Implemented | No state mutation specific to disconnect beyond the guarded bootstrap; hierarchy is a pure function of station and device |
|
||||
| Toggle-Off Behavior Byte-for-Byte Unchanged | Implemented | L211 early return untouched; guard changes are unreachable when eqMultiDeviceEnabled is false |
|
||||
|
||||
### Coherence (Design)
|
||||
| Decision | Followed | Notes |
|
||||
|----------|-----------|-------|
|
||||
| 1. Single handler, unconditional resolve+apply, no connect/disconnect branch on event type | Yes | Confirmed via git diff and full source read; L213 assignment and L228-231 resolve+apply untouched |
|
||||
| 2. Bootstrap guard scoped to altavozInterno only, not all BT/USB/wired | Yes | esBase = dispositivo.tipo == TipoDispositivo.altavozInterno; wired/BT/USB still bootstrap on first-seen (confirmed via passing 5.5a, 2.2, and D.4 BT first-connect assertion) |
|
||||
| 3. Deterministic re-resolution, not snapshot/restore | Yes | No snapshot storage added; _resolverPresetActivo remains the sole resolver |
|
||||
| 4. Reuse aplicarPreset choke point | Yes | L229-230 unchanged |
|
||||
| 5. Cold start trusts existing seed path, no new code | Yes | cargarPersistido to _sembrarDispositivoActual to _onDispositivoCambiado, verified unchanged |
|
||||
| 6. Toggle-off invariance, L211 early return untouched | Yes | Line unchanged; 5.3c/5.7a/5.8b still pass |
|
||||
|
||||
### TDD Compliance
|
||||
| Check | Result | Details |
|
||||
|-------|--------|---------|
|
||||
| TDD Evidence reported | Yes | apply-progress #2323 includes a TDD Cycle Evidence table (3-column RED/GREEN/REFACTOR plus prose, not the full 5-column template, see SUGGESTION-2) |
|
||||
| All tasks have tests | Yes | 5/5 Phase-D tasks (D.1-D.5) have one dedicated test case each |
|
||||
| RED confirmed, tests exist | Yes | All 5 test cases verified present via direct file read of test/estado/estado_ecualizador_test.dart |
|
||||
| GREEN confirmed, tests pass | Yes | 5/5 pass on independent re-execution this session (43/43 in file; 68/68 in fallback regression run) |
|
||||
| Triangulation adequate | Yes | Each Phase-D scenario has exactly one purpose-built, non-overlapping test; no scenario needed multiple variants |
|
||||
| Safety Net for modified files | Yes | lib/estado/estado_ecualizador.dart (modified, not new); 43/43 full-file regression green, independently re-verified this session |
|
||||
|
||||
**TDD Compliance**: 6/6 checks passed
|
||||
|
||||
---
|
||||
|
||||
### Test Layer Distribution
|
||||
| Layer | Tests | Files | Tools |
|
||||
|-------|-------|-------|-------|
|
||||
| Unit | 5 (new, Phase D) | 1 (estado_ecualizador_test.dart) | package:test |
|
||||
| Integration/Widget | 0 new | pre-existing pantalla_ajustes_test.dart used only as unrelated regression check | flutter_test widget testing |
|
||||
| E2E | 0 | 0 | not installed |
|
||||
| Total (new to this change) | 5 | 1 | |
|
||||
|
||||
---
|
||||
|
||||
### Changed File Coverage
|
||||
| File | Line pct whole file | Diff-region coverage | Uncovered lines in diff region | Rating |
|
||||
|------|--------|----------|-----------------|--------|
|
||||
| lib/estado/estado_ecualizador.dart | 59.3 pct (118/199, baseline; file has many unrelated pre-existing methods not exercised by this single test file) | 100 pct (L220 esBase hit 2x; L221 guarded if hit 3x; L222-224 bootstrap body hit 3x via 5.5a/2.2/D.4; L228-231 resolve+apply hit continuously) | none | Excellent (diff region) |
|
||||
| test/estado/estado_ecualizador_test.dart | N/A, test file, not instrumented | - | - | - |
|
||||
|
||||
Source: coverage/lcov.info generated this session via flutter test --coverage test/estado/estado_ecualizador_test.dart (gitignored, not committed, not left in working tree changes).
|
||||
|
||||
---
|
||||
|
||||
### Assertion Quality
|
||||
All assertions verify real behavior. No tautologies, ghost loops, assertion-free tests, or CSS/implementation-detail-only checks found across D.1-D.5. Every test drives real production code (_onDispositivoCambiado, _resolverPresetActivo, cargarPersistido) and asserts on production-derived values (presetActual, presetsDispositivo, FakeServicioAudio.presetsAplicados), consistent with the spec instruction to assert presetsAplicados (proves push, not state-only mutation) rather than state alone.
|
||||
|
||||
Manually traced D.2 pre-fix versus post-fix behavior against the actual presetEfectivo hierarchy to independently confirm it is a genuine, non-tautological regression test (would have resolved presetActual == rock, stale, pre-fix versus pop, fresh, post-fix on the second builtin_speaker emit). This matches the claim already documented in apply-progress that D.2 is the only test among D.1-D.5 that targets the actual bootstrap-collision bug.
|
||||
|
||||
**Assertion quality**: All assertions verify real behavior.
|
||||
|
||||
---
|
||||
|
||||
### Quality Metrics
|
||||
**Linter/Analyzer**: No errors. flutter analyze reported zero issues (ran this session, full project, 63.6s)
|
||||
**Type Checker**: N/A separate tool; covered by flutter analyze (Dart static analysis)
|
||||
|
||||
### Issues Found
|
||||
|
||||
**CRITICAL**: None
|
||||
|
||||
**WARNING**:
|
||||
1. Spec scenario text versus implementation divergence, Requirement 2 "First-Seen Bootstrap Must Not Hijack Base-Device Resolution". The spec scenario literally states that the presetsDispositivo entry for builtin_speaker is bootstrapped to rock (global fallback), implying the key gets created with the correct value. The actual implementation, matching Design Decision 2 and its explicit reasoning ("Rejected: keep and tolerate, leaves persisted mask bug"), NEVER creates that entry at all. The esBase guard unconditionally skips the whole bootstrap block, so the presetsDispositivo containsKey check for builtin_speaker stays false forever, proven by test D.2. This is objectively safer than the literal scenario (D.2 proves it prevents the L3-masks-L4 bug that a bootstrap-with-correct-value approach would not prevent) and is fully justified by the design document rationale; it is not a functional defect. But it means a reader following the spec literally would expect that containsKey check to be true, which is false: a real drift between spec-as-written and spec-as-implemented. Recommendation: before archive, update the spec scenario THEN clause to state that the entry is never bootstrapped and always resolves live through the hierarchy, so the spec matches the safer, already-implemented, already-tested behavior.
|
||||
|
||||
**SUGGESTION**:
|
||||
1. Reconcile spec text per WARNING-1 recommendation before archive (listed there to avoid double-counting).
|
||||
2. The apply-progress TDD Cycle Evidence table uses a 3-column format (RED/GREEN/REFACTOR plus prose) instead of the full 5-column strict-TDD template (RED/GREEN/TRIANGULATE/SAFETY NET/REFACTOR). Substance is all present and was cross-verified independently in this session, but a future sdd-apply run should use the standard table shape for easier automated auditing.
|
||||
3. The apply-progress "Issues Found" section already flags, correctly, as out-of-scope for this change, that cambiarPresetPrincipal immediate re-apply bypasses _resolverPresetActivo and applies the raw global value directly rather than through the hierarchy. Carrying this forward as a tracked tech-debt item; no action needed in this change, but worth a follow-up ticket since it could theoretically let a station/matrix override be transiently bypassed for one apply call when the global preset changes.
|
||||
|
||||
### Verdict
|
||||
**PASS WITH WARNINGS**
|
||||
|
||||
0 CRITICAL, 1 WARNING (spec-text/implementation wording drift on Requirement 2, functionally sound and safer than written; recommend a spec-text correction before archive), 3 SUGGESTION. All 43 tests in the target file pass (including all 5 new Phase-D tests and every named consistency lock: 5.5b, 5.7a, 5.8b, 2.2), flutter analyze is clean, the code change matches the design Bootstrap Guard block exactly (verified via git diff and direct source read), and 15/16 tasks are complete with task 5.1 intentionally left as the non-blocking manual on-device gate.
|
||||
Reference in New Issue
Block a user