LabyricornandClaude Opus 5
c4332363a9
Revision 0.9 adds section 20, the cadence and event subsystems contract, and carries two corrections the implementation forced. Section 6.1 now states that a duration is the authored literal or a non-negative finite number already in milliseconds, since a DurationSpec may be the resolved output of a ValueSpec or a bounded TimeSpec, with the one documented exception of an automation track's `at`, which 19.1 keeps literal-only so that point ordering stays decidable at import. Section 20.11 documents the rejection of an undeclared input name in an event action's `with` map as ERR_UNKNOWN_FIELD — the section's own convention for that shape of error, replacing an invented code that appeared nowhere in the registry. The review record is committed with the code it describes: the two code triages that found these defects, the reconciliation plan that sequenced the fixes, and a follow-up debt record listing what was deliberately left open — the unchecked JSON Schema artifact, degenerate path arcs, post-effect transient allocation, the window-traffic fixture's per-copy wrap bounds, and the unstated `ownership: "persistent"` value on a sound action. None of the five blocks phase 6; all five are written down rather than dropped. Devlog entries are backfilled for the two milestones that had none: phase 3c slice 2, the audio lifecycle and voice ceilings, and slice 4d, the renderer core. The implementation status summary now reflects the reconciled state rather than the in-flight one. 231 tests pass. tools/verify-spec-contract.py reports 46 declared diagnostic codes with every used code resolving and its two long-standing unresolved cross-references unchanged. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01ShxxFqFmCUDQnQvFNm4TKy
117 lines
15 KiB
Markdown
117 lines
15 KiB
Markdown
# XZBT 0.1 Phase 5 — Cadence and Event Subsystems: code review triage
|
||
|
||
**Scope:** independent verification of the Phase 5 implementation (`src/runtime/cadence.js`, `src/runtime/cadence-validation.js`, `src/runtime/actions.js`, and the `cadence`/`events`/`SoundCadence` schema definitions) against Format Specification revision 0.9, §20 ("Cadence and Event Subsystems Contract"). Requested because Phase 5 was reported complete (`docs/evidence/phase5/2026-09-06-phase5-events-cadence.md`, 229/229 tests passing) and needed review against the spec text itself rather than against its own self-authored evidence doc.
|
||
|
||
**Method:** read §20 normatively line by line, then checked the runtime code, the JSON Schema, and `test/phase5-cadence.test.mjs` against it directly — the same method as `01-triage.md` and `02-triage.md`. `npm test` was re-run and confirmed green (229/229) independently of the evidence doc's claim. `tools/verify-spec-contract.py` was re-run and confirms the *specification text* is internally consistent (46 declared codes, 43 used, all resolve; 2 unresolved cross-refs are pre-existing and unrelated to §20). That script checks codes used *in the spec* against the §7 table — it does not, and cannot, check codes used *in the code* against the spec, which is where the defects below live. All Phase 5 changes are currently uncommitted in the working tree (`git status` shows them as modified/untracked against HEAD `0af58da`).
|
||
|
||
**Result: the automatic-scheduling algorithm (§20.5) itself appears correctly implemented in the code path that actually runs (`CadenceSubsystem.advance` → `fireClass`). The defects are concentrated in (1) two invented diagnostic codes that do not exist anywhere in the specification, one of which masks a real, verifiable permission-check bug, and (2) four of the fourteen acceptance traces exercising a parallel/duplicate implementation instead of the production code path they are supposed to certify.** Nothing here is a matter of interpretation — every finding below is anchored to an exact spec line and an exact code line.
|
||
|
||
---
|
||
|
||
## Tier 1 — Confirmed defects
|
||
|
||
### F1. `ERR_SOUND_USAGE_MISMATCH` does not exist in the specification; the spec requires `ERR_UNSUPPORTED_TARGET` for this exact violation
|
||
|
||
**Locations:** `src/runtime/actions.js:189,192`; `test/phase5-cadence.test.mjs:365-415`; `docs/evidence/phase5/...md` §1, Trace 6; `docs/IMPLEMENTATION_STATUS.md:158`.
|
||
|
||
§20.2.2 states plainly: *"A sound action executed from a manual context naming a sound whose `usage` does not include `"manual"` is rejected with `ERR_UNSUPPORTED_TARGET`."* §20.12 repeats this for all three contexts (manual/scenario/cadence) and ends: *"Violations produce `ERR_UNSUPPORTED_TARGET`."* §20.14's diagnostic table lists `ERR_UNSUPPORTED_TARGET` for exactly this case and does **not** list `ERR_SOUND_USAGE_MISMATCH` anywhere. A full-text search of the 3,576-line specification confirms `ERR_SOUND_USAGE_MISMATCH` appears zero times outside the implementation and its own tests.
|
||
|
||
The implementation throws this invented code instead of the mandated one, and the evidence doc, `IMPLEMENTATION_STATUS.md`, and the test file all cite it as if it were the normative code. This is not a naming nit — a runtime or validator that checks diagnostics against the §7/§20.14 registry (as `tools/verify-spec-contract.py` does for the spec text) would flag this as an unrecognized code if pointed at the implementation.
|
||
|
||
**Fix:** replace `ERR_SOUND_USAGE_MISMATCH` with `ERR_UNSUPPORTED_TARGET` at both throw sites in `actions.js`, and update the test/evidence text accordingly.
|
||
|
||
### F2. The usage-permission check it's protecting is itself broken — a sound can be triggered from a context its `usage` array does not authorize
|
||
|
||
**Locations:** `src/runtime/actions.js:184-193`; demonstrated by the implementation's own test, `test/phase5-cadence.test.mjs:410-415`.
|
||
|
||
```js
|
||
const isActionCaller = callerUsage === 'manual' || callerUsage === 'scenario';
|
||
const permitsAction = allowedUsage.includes('manual') || allowedUsage.includes('scenario');
|
||
if (isActionCaller && !permitsAction) { throw ERR_SOUND_USAGE_MISMATCH; }
|
||
if (!allowedUsage.includes(callerUsage) && !permitsAction) { throw ERR_SOUND_USAGE_MISMATCH; }
|
||
```
|
||
|
||
`permitsAction` is true whenever the sound's `usage` contains **either** `"manual"` **or** `"scenario"` — it never checks which one the caller actually needs. Walk the second `if` with `callerUsage = 'manual'` and `allowedUsage = ['scenario']` (a sound declared scenario-only): `!allowedUsage.includes('manual')` is `true`, but `!permitsAction` is `false` (since `'scenario'` is present) — the whole condition is `false`, so **no error is thrown** and the sound plays. §20.12 requires this to be rejected: a scenario-only sound must not be playable from a manual sound action.
|
||
|
||
This is not a hypothetical — it's exactly what `test/phase5-cadence.test.mjs:410-415` (part of the Trace 6 test) does. It calls `executor.executeAction({ type: 'sound', sound: 'manualOrScenario' })` with no context override, so `callerUsage` defaults to `'manual'`. The target sound `manualOrScenario` is declared with `usage: ['scenario']` only (line 380 of the test file) — not `'manual'`. The test asserts this **succeeds** (`res2 === true`, zero diagnostics, one voice played). The test's own name ("manualOrScenario") suggests the author believed this sound was reachable from either context, but its declared `usage` says otherwise — the test is asserting the bug's behavior, not the spec's.
|
||
|
||
**Fix:** the check should simply be `if (isActionCaller && !allowedUsage.includes(callerUsage)) throw ERR_UNSUPPORTED_TARGET` (per F1). Delete the `permitsAction` short-circuit entirely — it has no basis in §20.12, which conditions the check on the *caller's* context, not on whether the sound permits *some* action context. Add a test case with `usage: ['manual']` invoked from a scenario context and `usage: ['scenario']` invoked from a manual context — the two cases the current Trace 6 test never exercises.
|
||
|
||
### F3. `ERR_UNKNOWN_PARAMETER` does not exist in the specification, and §20.11 does not actually require this rejection
|
||
|
||
**Locations:** `src/runtime/actions.js:154-156`; `test/phase5-cadence.test.mjs:272-280`; evidence doc §1, Trace 3.
|
||
|
||
§20.11 defines exactly two failure modes for an event's `with` map: type mismatch (`ERR_TYPE_MISMATCH`) and a missing required input with no default (`ERR_SCHEMA_VALIDATION`). It says nothing about rejecting a `with` key that isn't a declared input — and §20.15's own Trace 3 description (*"isolates inputs across concurrent invocations"*) doesn't mention it either. The implementation added this check on its own initiative, which is a defensible strictness choice, but it invented a diagnostic code to go with it (`ERR_UNKNOWN_PARAMETER`) that appears nowhere in the 46-code §7/§20.14 registry. The spec's own convention for "you named a field that doesn't exist here" is `ERR_UNKNOWN_FIELD` — used for exactly this shape of error everywhere else in §20 (the `cadence` container, `sounds.<id>.cadence`, `events.<id>`, `inputs.<id>`).
|
||
|
||
**Fix:** either drop the check (it's not required) or rename it to `ERR_UNKNOWN_FIELD` for consistency with the rest of the section, and add a line to §20.11 documenting the behavior since it's new normative surface, not an interpretation of existing text.
|
||
|
||
---
|
||
|
||
## Tier 2 — Test coverage gap: four of fourteen traces certify the wrong code path
|
||
|
||
### F4. Traces 7, 8, 9, and 10 exercise `CadenceSubsystem.triggerSound()` / `calculateEligiblePool()`, not the production scheduling path `advance() → fireClass()`
|
||
|
||
**Locations:** `test/phase5-cadence.test.mjs:440-599` (Traces 7, 8, 9, 10); `src/runtime/cadence.js:141-180` (`triggerSound`), `182-206` (`calculateEligiblePool`), `273-383` (`fireClass`, the method actually called from `advance()`).
|
||
|
||
`CadenceSubsystem` has three separate, independently-written implementations of overlapping logic:
|
||
|
||
1. `fireClass(cls)` — called from `advance()`, which is called from `update()`, which is the only method `performance.js` ever calls. **This is the only code path a real exhibit exercises.** It does its own cooldown check, its own overlap check, its own weight evaluation, its own anti-repetition lookup, and its own recency-history update.
|
||
2. `triggerSound(soundId, timeMs)` — a second, hand-rolled implementation of cooldown and overlap checking and recency-history update, called from nowhere in production code (`grep` across `src/runtime/` confirms zero call sites outside the test file).
|
||
3. `calculateEligiblePool(className, soundDefs, timeMs)` — a third, pure calculation helper that takes an externally-supplied `soundDefs` array (bypassing `fireClass`'s own eligibility filtering by class/usage/`when`/cooldown/overlap entirely) and only computes weights and multipliers.
|
||
|
||
Cooldown (Trace 7), overlap (Trace 8), anti-repetition multipliers (Trace 9), and pool relaxation (Trace 10) are all tested exclusively via `triggerSound()` and `calculateEligiblePool()` (confirmed by grep: no test calls `.advance()` or `.update()` in any of these four test bodies). Traces 11–14 correctly use `.update()`/`.advance()`.
|
||
|
||
This matters because `fireClass()` re-implements the same cooldown/overlap/weight/multiplier logic independently, and nothing currently checks that its version agrees with `triggerSound()`'s. It doesn't, in one respect (F5 below) — and the test suite, as written, cannot catch a divergence between the two because it only ever calls one of them per behavior.
|
||
|
||
**Fix:** rewrite Traces 7–10 to drive the scheduler through `update()`/`advance()` with a document containing appropriately-configured sounds and a controlled RNG seed, the same way Traces 11–14 already do. This is very likely a straightforward test-code change, not a runtime change, since `fireClass()`'s own logic (independently re-derived from the spec text above) reads as correct.
|
||
|
||
### F5. `triggerSound()` and `fireClass()` disagree on recency-history semantics (found because of F4, not by the suite)
|
||
|
||
**Locations:** `src/runtime/cadence.js:170-174` vs. `:372-376`.
|
||
|
||
§20.5.9 requires: *"Append the winning sound ID to the front of the class recency queue, trimming the queue to maximum depth 4."* Read together with §20.5.5's "N selections ago" phrasing, this is a plain FIFO of the last four **firings** (not the last four *distinct* sounds) — a sound selected twice in a row should occupy two of the four slots, not be moved to the front of an existing entry.
|
||
|
||
- `fireClass()` (line 373: `cls.recencyHistory.unshift(chosen.id)`) implements this literally — no dedup, matching the spec text.
|
||
- `triggerSound()` (lines 170-172) instead searches for an existing occurrence and splices it out before unshifting (`indexOf` + `splice` + `unshift`), i.e. "move existing entry to the front" rather than "record a new firing." This is a different, incompatible interpretation of the same requirement, and it only exists in the code path the test suite actually calls (F4) — meaning Trace 9's "verify multipliers 0.0/0.25/0.50/0.75" claim is validated against the non-shipping interpretation.
|
||
|
||
In practice the two produce identical multiplier results when only one sound ever repeats, and only diverge when two-plus sounds interleave with repeats within a 4-firing window — a case neither `triggerSound()`'s nor `fireClass()`'s tests currently construct.
|
||
|
||
**Fix:** once F4 is addressed and Trace 9/10 exercise `fireClass()` directly, delete `triggerSound()` (dead code with no production caller and an incorrect implementation) rather than reconciling it — there's no reason to keep two schedulers.
|
||
|
||
---
|
||
|
||
## Tier 3 — Minor / documentation
|
||
|
||
### F6. `ActionSpec.ownership` and `sound` action `ownership` schema/validator accept `"persistent"`, which §20.12 does not mention
|
||
|
||
**Locations:** `schema/xzbt-0.1.schema.json` (`ActionSpec.ownership` enum); `src/runtime/cadence-validation.js:316`.
|
||
|
||
§20.12 documents exactly two values for a sound action's `ownership`: `"performance"` and `"scenario"`. Both the schema and `cadence-validation.js`'s allow-list also accept `"persistent"`. This may be intentional carry-over from the general Action Model 0.1 ownership vocabulary (§10.1 mentions persistent resource ownership generally), but §20.12 doesn't say so for the `sound` action specifically, and it isn't in the four ownership fields called out in the earlier phase's "Key decisions" pattern this project has used to record such extensions. Worth a one-line confirmation in §20.12 if it's deliberate, since it's currently unstated.
|
||
|
||
### F7. `docs/IMPLEMENTATION_STATUS.md` and the evidence doc report the invented codes as if normative
|
||
|
||
Both documents (`IMPLEMENTATION_STATUS.md:158`; evidence doc §1 and Trace 3/Trace 6 rows) present `ERR_UNKNOWN_PARAMETER` and `ERR_SOUND_USAGE_MISMATCH` as part of the delivered contract surface, which will need correction once F1/F3 are resolved — these are downstream of the code fix, not independent defects.
|
||
|
||
---
|
||
|
||
## What is *not* a defect (checked and confirmed correct)
|
||
|
||
- The core §20.5 selection algorithm as implemented in `fireClass()` — eligible-pool filtering by class/usage/`when`, cooldown exclusion, overlap exclusion, weight evaluation, the exact `[0.0, 0.25, 0.50, 0.75]` multiplier table applied by recency-queue position, pool relaxation when total effective weight is zero, and reschedule-by-uniform-sample-divided-by-intensity — all match §20.5 line for line.
|
||
- §20.6 minimum-gap and priority servicing (`advance()`'s `CADENCE_PRIORITY` loop with the `minGap` guard and single-fire-per-window `break`) matches §20.6, including deferred-class retention.
|
||
- §20.7 intensity clamping to `[0, 1]`, the `<= 1e-6` pause threshold, and ambient continuing to run under paused intensity — matches §20.7 exactly, including the ordering (`maintainAmbience()` runs before the intensity gate returns).
|
||
- §20.13.3's dispatch-budget accounting for `type: "event"` actions consuming **two** units (one for "attempting an action," a second explicit `consumeUnit` for "entering an event") looks like a double-count at first read but is the correct implementation of both stated rules in §20.13.3, not a bug.
|
||
- Event nesting depth: `depth > 16` throws at depth 17, matching the spec's "up to depth 16... depth 17 raises" boundary exactly.
|
||
- `docs/evidence/phase5/...md`'s claims about test counts (229/229), exhibit validation, and the build hash were independently reproduced.
|
||
|
||
---
|
||
|
||
## Recommended sequencing
|
||
|
||
1. F1 + F2 together (one code change: fix the permission check and use the correct diagnostic code) — this is the only finding with a real behavioral consequence (a security/contract-boundary leak, not just a wrong error name).
|
||
2. F3 (either remove or rename the invented code) — small, but blocks calling Phase 5's diagnostic surface conformant.
|
||
3. F4 rewrite (test-only) — restores actual confidence in Traces 7–10; do this before relying on "14/14 traces pass" as acceptance evidence.
|
||
4. F5 (delete dead code) — falls out of F4.
|
||
5. F6/F7 — documentation, whenever convenient.
|
||
|
||
No specification or implementation file was changed in producing this triage.
|