Promotes the android-auto-media capability spec to openspec/specs/ and moves both completed changes into openspec/changes/archive/.
180 lines
13 KiB
Markdown
180 lines
13 KiB
Markdown
# Verification Report: auto-media-art-quality
|
|
|
|
**Mode**: Strict TDD (adversarial, fresh re-execution — apply-progress claims independently re-verified, not trusted)
|
|
**Date**: 2026-07-19
|
|
**Verdict**: PASS WITH WARNINGS
|
|
|
|
## Completeness
|
|
|
|
| Source | Claim | Verified |
|
|
|---|---|---|
|
|
| tasks.md | 22/22 tasks `[x]` | Confirmed by reading tasks.md directly — all checkboxes marked |
|
|
| apply-progress.md | 22/22 tasks complete, single-batch delivery | Consistent with tasks.md and live code state |
|
|
|
|
## Test Execution (independent re-run, not trusted from apply report)
|
|
|
|
Command: `flutter test test/servicios/navegacion_auto_test.dart --concurrency=1 --timeout=60s`
|
|
|
|
Result: **25/25 passed** (`+0` through `+24`, `All tests passed!`). Matches apply-progress's claimed 25/25 exactly — re-run independently in this session, not copy-pasted from the report.
|
|
|
|
## Spec Compliance Matrix
|
|
|
|
| Spec Scenario | Covering Test(s) | Status |
|
|
|---|---|---|
|
|
| Valid, reachable remote logo | `artUriPara: favicon https valido...`, `itemEmisora: usa el favicon remoto...` | PASS |
|
|
| No logo (Case A: null/empty) | `artUriPara: favicon null/vacio cae al drawable rotativo` | PASS |
|
|
| Logo present but unreachable (Case B) | `artUriPara: favicon malformado cae al mismo comportamiento rotativo` | PASS for the **malformed/non-http(s) subset only** — see WARNING-1 |
|
|
| Fallback art matches phone-UI per-station selection | `indiceArtePara` group, `parity: phone/auto art order` group | PASS — formula/order diff-verified against source, see below |
|
|
| Fallback art is on-brand, not launcher icon | Static: `default_station_art.png` deleted, 4 `station_art_*` drawables byte-identical to `assets/images/` source | PASS |
|
|
| Root returns 3 folders, non-playable | `ConstructorArbolAuto.raiz` group (pre-existing, unchanged) | PASS |
|
|
| Empty folder returns empty list | `ConstructorArbolAuto.hijos: lista vacia...` (pre-existing) | PASS |
|
|
| Cold-start browse safety | `FuenteEmisorasAutoLocal` try/catch paths (pre-existing, unchanged) | PASS (not touched by this change) |
|
|
| Known codec+bitrate -> subtitle | `subtituloCalidad` matrix + `itemEmisora: setea displaySubtitle...` | PASS |
|
|
| Unknown codec/bitrate -> subtitle omitted, never literal "null" | `subtituloCalidad` matrix (incl. bitrate<=0, whitespace-codec) + null-substring test + `itemEmisora: omite displaySubtitle...` | PASS |
|
|
|
|
## Adversarial Checks (per orchestrator instructions)
|
|
|
|
### 1. Favicon validity gate — edge cases independently re-verified
|
|
|
|
Read `lib/servicios/navegacion_auto.dart:31-42`. Independently ran a throwaway Dart script (not trusting the apply report) to confirm the claimed Dart `Uri` quirk:
|
|
|
|
Uri.tryParse('http://') -> hasAuthority=true, host="", scheme=http
|
|
|
|
This confirms the apply-progress claim that a naive `uri.hasAuthority` check would wrongly accept `'http://'`. The shipped code uses `uri.host.isNotEmpty` (not `hasAuthority`), which correctly rejects it — confirmed both by source inspection and by the passing `faviconUsable` test case (`'http://': false`).
|
|
|
|
Also independently re-ran `Uri.tryParse` against the other edge cases in the task list (`cdn.example.com/logo.png`, `/relative/path/logo.png`, `not a url at all $$$ ///`, `ftp://cdn.example.com/logo.png`) — all parse with an empty or non-http(s) scheme, so `scheme == 'http' || scheme == 'https'` alone already rejects them (the `host.isNotEmpty` check specifically fixes the `'http://'` case). All 10 table-driven cases in the `faviconUsable` test pass.
|
|
|
|
**Gap (see WARNING-1)**: the gate is a **static URL-shape check only** — it cannot detect a syntactically-valid `https://` URL that 404s or points at a non-image response (spec's Case B literally lists "dead link, unreachable host, or non-image response" as triggers). This is a deliberate, documented design choice (design.md Decision 1), not an implementation bug — but it is a real narrowing of the spec's literal text.
|
|
|
|
### 2. Rotation algorithm parity — diffed, not assumed
|
|
|
|
Read both implementations side by side.
|
|
|
|
`lib/widgets/tarjeta_emisora.dart:361-370` (`_fallbackArtFor`):
|
|
|
|
const arts = ['assets/images/station_art_aurora.png', '...cosmic...', '...pulse...', '...nova...'];
|
|
final index = seed.codeUnits.fold<int>(0, (a, b) => a + b) % arts.length;
|
|
|
|
`lib/servicios/navegacion_auto.dart:21,47-48` (`indiceArtePara` + `_nombresArte`):
|
|
|
|
const _nombresArte = ['aurora', 'cosmic', 'pulse', 'nova'];
|
|
int indiceArtePara(String seed) => seed.codeUnits.fold<int>(0, (a, b) => a + b) % _nombresArte.length;
|
|
|
|
**Confirmed identical**: same fold expression, same modulus source (4 in both, since both lists have length 4), same element order (aurora, cosmic, pulse, nova). This is a provable, not assumed, match — both files were read and compared directly, not inferred from apply-progress prose.
|
|
|
|
### 3. Native drawables — byte-identical, dangling refs checked
|
|
|
|
`cmp` run on all 4 pairs — all reported IDENTICAL:
|
|
|
|
assets/images/station_art_aurora.png <-> android/.../drawable/station_art_aurora.png
|
|
assets/images/station_art_cosmic.png <-> android/.../drawable/station_art_cosmic.png
|
|
assets/images/station_art_pulse.png <-> android/.../drawable/station_art_pulse.png
|
|
assets/images/station_art_nova.png <-> android/.../drawable/station_art_nova.png
|
|
|
|
`android/app/src/main/res/drawable/default_station_art.png` confirmed **absent** (directory listing shows only the 4 new PNGs plus the pre-existing `ic_stat_pluriwave.xml`/`launch_background.xml`).
|
|
|
|
Grepped the whole repo for `default_station_art` — 8 hits, **all in SDD artifact prose** (`openspec/changes/auto-media-art-quality/{apply-progress,tasks,design,proposal}.md` and the prior `android-auto-media` change's artifacts). Zero hits in `lib/` or live `android/` resource/manifest files.
|
|
|
|
### 4. `displaySubtitle` fallback matrix — assertions inspected, not just names
|
|
|
|
Independently re-ran the test file and read the actual assertions (not just test names) in `group('subtituloCalidad', ...)`:
|
|
|
|
| Input | Assertion | Result |
|
|
|---|---|---|
|
|
| codec=mp3, bitrate=128 | `'128 kbps · MP3'` | matches design table |
|
|
| codec=null, bitrate=128 | `'128 kbps'` | matches |
|
|
| codec=mp3, bitrate=null | `'MP3'` | matches |
|
|
| codec=mp3, bitrate=0 | `'MP3'` (bitrate<=0 -> unknown) | matches design's stated Radio Browser convention |
|
|
| codec=null, bitrate=null | `isNull` | matches — not `''`, not `'null'` |
|
|
| codec=null, bitrate=0 | `isNull` | matches |
|
|
| codec=' ' (whitespace), bitrate=null | `isNull` | matches — whitespace-only codec treated as unknown |
|
|
| codec=' mp3 ', bitrate=128 | `'128 kbps · MP3'` | matches — trimmed correctly |
|
|
|
|
Plus a dedicated test asserting `result?.contains('null') ?? false` is `isFalse` across 4 representative cases — directly defends the "never render literal null" requirement rather than relying only on exact-match equality. All assertions call real production code (`subtituloCalidad`); none are tautological, none are ghost loops.
|
|
|
|
### 5. AI attribution / debug prints / dead code
|
|
|
|
- `git diff` on both changed files scanned for `print(`, `debugPrint`, `console.log`, `TODO`, `FIXME`, `Anthropic`, `Claude`, `Co-Authored`, `Generated by` — **zero matches**.
|
|
- Grepped `lib/` for `_defaultArtUri` and `_artUriPara` (the two retired identifiers) — **zero matches** anywhere, confirming full removal, not just from the one call site.
|
|
|
|
### 6. Working tree state
|
|
|
|
`git status --porcelain` output:
|
|
|
|
D android/app/src/main/res/drawable/default_station_art.png
|
|
M lib/servicios/navegacion_auto.dart
|
|
M test/servicios/navegacion_auto_test.dart
|
|
?? android/app/src/main/res/drawable/station_art_aurora.png
|
|
?? android/app/src/main/res/drawable/station_art_cosmic.png
|
|
?? android/app/src/main/res/drawable/station_art_nova.png
|
|
?? android/app/src/main/res/drawable/station_art_pulse.png
|
|
?? openspec/changes/auto-media-art-quality/
|
|
|
|
Confirmed: unstaged modifications/deletion plus untracked new files only. **No commit was made** by the apply agent, matching its claim.
|
|
|
|
### 7. Test count cross-check
|
|
|
|
Re-ran `navegacion_auto_test.dart` independently in this session (see "Test Execution" above): **25/25 passed**, matching apply-progress's claimed 25/25 exactly. Not trusted from the report — executed fresh in this session.
|
|
|
|
## TDD Compliance
|
|
|
|
| Check | Result | Details |
|
|
|---|---|---|
|
|
| TDD Evidence reported | Yes | "TDD Cycle Evidence" table present in apply-progress.md, one row per phase |
|
|
| All tasks have tests | Yes | Every behavioral task (1.1-6.2) has a corresponding test group in `navegacion_auto_test.dart`; Phase 7 tasks correctly marked static-review-only |
|
|
| RED confirmed (tests exist) | Yes | All claimed test groups exist in the live test file |
|
|
| GREEN confirmed (tests pass) | Yes | 25/25 passing on independent re-run |
|
|
| Triangulation adequate | Yes | Each behavior has 2+ distinct test cases with varying expected values |
|
|
| Safety Net for modified files | Yes | `navegacion_auto.dart` modified — full test file (25 tests, incl. 8 pre-existing) re-run and green, no regression |
|
|
|
|
**TDD Compliance**: 6/6 checks passed
|
|
|
|
### Assertion Quality
|
|
|
|
| File | Line(s) | Assertion | Issue | Severity |
|
|
|---|---|---|---|---|
|
|
| `test/servicios/navegacion_auto_test.dart` | 120-129 | `if (indiceArtePara(a.uuid) != indiceArtePara(b.uuid)) { expect(...) }` | Conditional/guarded assertion — could execute zero assertions if the two literal seeds hashed to the same index | SUGGESTION |
|
|
|
|
Independently computed `indiceArtePara('uuid-a') == 1` and `indiceArtePara('uuid-bbbb') == 0` — confirmed the guard condition is true for the literals actually used today, so the assertion does execute. Minor fragility only (a future seed-literal edit could silently disable the check); the same property is fully and unconditionally enumerated by the `parity: phone/auto art order` test (Phase 6.2), so coverage is not actually at risk.
|
|
|
|
No tautologies, no ghost loops over possibly-empty collections, no assertion-free tests found.
|
|
|
|
**Assertion quality**: 0 CRITICAL, 0 WARNING, 1 SUGGESTION
|
|
|
|
## Design Coherence
|
|
|
|
| Design Decision | Code Match |
|
|
|---|---|
|
|
| Static favicon validity gate (no network), malformed-subset-only | Matches exactly — `faviconUsable` is a pure sync function, no I/O |
|
|
| Port `_fallbackArtFor` verbatim (formula + order) | Confirmed identical via direct source diff |
|
|
| Retire `default_station_art.png` + `_defaultArtUri` (rotation is total) | Confirmed — both removed, zero dangling references |
|
|
| `displaySubtitle` format, omit when both unknown | Matches design's table exactly, incl. bitrate<=0 and whitespace-codec edges |
|
|
| `_nombresArte` stays private; order verified indirectly via `artUriPara` | Documented deviation in apply-progress, judged reasonable |
|
|
|
|
No design deviations that break a spec requirement.
|
|
|
|
## Issues Found
|
|
|
|
### CRITICAL
|
|
|
|
None.
|
|
|
|
### WARNING
|
|
|
|
**WARNING-1 — Case B coverage is narrower than the spec's literal text (documented/accepted, flagged for archive sign-off)**
|
|
|
|
Spec text (`specs/android-auto-media/spec.md`, "Station's logo URL is present but unreachable (Case B)") lists the triggering conditions as "dead link, unreachable host, or non-image response." The shipped `faviconUsable` gate is a **static URL-shape validator only** — a syntactically valid `https://cdn.example.com/dead-link.png` that returns 404, or a URL resolving to a non-image response, still passes the gate and is handed to the OS art loader as-is, which may still render a broken/blank tile. This contradicts the spec's stated "the car head unit MUST NOT display an empty, broken-image, or indefinitely-loading art tile for that station" for those specific sub-cases.
|
|
|
|
This is not an implementation bug — design.md Decision 1 explicitly considered and rejected live-reachability checking (HTTP HEAD, TTL cache, etc.) due to TOCTOU risk and the untested handler-path constraint, and states the proposal's "Split Judgment explicitly permits this narrowing of A." The gap is real and testably confirmed (only the malformed/non-http(s) subset is covered; genuine live-404 or non-image-response cases cannot be tested without network I/O, and are not tested). Recommend explicit archive-time sign-off that this narrowing is accepted, since the literal spec scenario text is broader than what ships.
|
|
|
|
### SUGGESTION
|
|
|
|
**SUGGESTION-1 — Guarded/conditional assertion in `artUriPara` "uuids distintos" test**
|
|
|
|
See Assertion Quality table above. Low priority; the same guarantee is already fully covered by the unconditional `parity: phone/auto art order` test.
|
|
|
|
## Next Steps
|
|
|
|
- `sdd-archive` is appropriate — no CRITICAL issues block archival. Recommend the archive step (or the user) explicitly acknowledge WARNING-1 (spec's Case B literal scope vs. shipped static-only gate) as an accepted, documented limitation before closing the change.
|
|
- Phase 7/8's static-review-only items (native drawable resolution via `android.resource://`, real Android build/DHU verification) remain unverified by this report for the same reason apply-progress could not verify them: no Android build environment available in this session. A real device/DHU pass is still recommended before shipping, per both apply-progress and design.md's own open question.
|