generated from Labyricorn/labyricorn-project-template
256 lines
14 KiB
Markdown
256 lines
14 KiB
Markdown
# Step 4 MVP — Triage of Independent Review Findings
|
|
|
|
## A. Executive Triage Verdict
|
|
|
|
The Step 4 MVP is sound. The independent review confirmed 20/20 tests, generic host implementation, and no domain vocabulary in production source. None of the five findings invalidate the MVP.
|
|
|
|
After triage against the actual Contract 5.2 text, the current implementation, and the authoring guide:
|
|
|
|
- **One finding (F-1) is NOT ACTUALLY A DEFECT** — the fixture behavior matches the normative Contract text.
|
|
- **One finding (F-3) SHOULD BE FIXED NOW** — a small README header note to prevent future re-copy confusion.
|
|
- **One finding (F-2) is SAFE TO DEFER** — permissive but conformant; future restore work will need it.
|
|
- **One finding (F-4) is INFORMATIONAL ONLY** — the current evidence model is sufficient for an MVP.
|
|
- **One finding (F-5) is INFORMATIONAL ONLY** — already documented in the Authoring Guide.
|
|
|
|
**No contract blockers exist. No owner decisions are required.**
|
|
|
|
The milestone can be closed after one small documentation patch (F-3 README header).
|
|
|
|
---
|
|
|
|
## B. F-1 — `INVALID_ARGUMENTS` vs `INVALID_VALUE`
|
|
|
|
### Contract Basis
|
|
|
|
The review claims that Contract §24 and Authoring Guide Appendix A line 522 require `INVALID_ARGUMENTS` as a distinct error code for argument failures.
|
|
|
|
**Actual Contract text (§24):**
|
|
|
|
> Implementations **SHOULD** use stable machine-readable error codes.
|
|
> **Recommended** base codes include: [...] `INVALID_ARGUMENTS` [...]
|
|
|
|
This is SHOULD + "recommended", not MUST. The codes are a recommended palette, not mandatory assignments.
|
|
|
|
**Actual Contract text (§9.5) — the normative argument validation section:**
|
|
|
|
> Invoke `args` MUST be an object. Every argument with `required: true` MUST be present. **Undeclared keys MUST be rejected with `INVALID_VALUE`.** Values MUST match the declared type, and all declared enum, bounds, step, and length constraints MUST be enforced.
|
|
|
|
This is the only normative text that specifies which error code to use for argument failures, and it **explicitly mandates `INVALID_VALUE`** — including for undeclared keys, which are the most "shape-like" argument failure.
|
|
|
|
**Authoring Guide Appendix A line 522:**
|
|
|
|
> Unknown target, invalid value, invalid session, and invalid arguments each return the correct distinct error code.
|
|
|
|
This checklist item says "the correct distinct error code" — it does not say which code that is. Reading it together with §9.5, the "correct" code for argument failures *is* `INVALID_VALUE`, because that is what §9.5 normatively requires.
|
|
|
|
`INVALID_ARGUMENTS` is listed in §24 as an available code but has **no normative triggering condition anywhere in the Contract**. It exists as a palette option for exhibits that choose to distinguish argument-shape failures from value-constraint failures, but nothing requires it.
|
|
|
|
### Classification: **NOT ACTUALLY A DEFECT**
|
|
|
|
The fixture core returns `INVALID_VALUE` for all argument failures. This is exactly what Contract §9.5 mandates. The fixture is conformant.
|
|
|
|
The host already handles any error code an exhibit returns — if a future exhibit emits `INVALID_ARGUMENTS`, the host will surface it correctly. No host change is needed.
|
|
|
|
### Recommended Resolution
|
|
|
|
**No code change required.** The fixture is conformant with the normative Contract text.
|
|
|
|
Optionally, add one clarifying sentence to `PROVENANCE.md` noting that `INVALID_VALUE` is used for argument failures per §9.5 and that `INVALID_ARGUMENTS` remains available but has no normative trigger in Contract 5.2. This is strictly optional.
|
|
|
|
### Blocks MVP Closure: **No**
|
|
|
|
---
|
|
|
|
## C. F-2 — Descriptor Validation Strictness
|
|
|
|
### Field-by-Field Requirement Analysis
|
|
|
|
| Field | Contract §9 Base Descriptor | Normative Strength | Currently Validated | Classification |
|
|
| --- | --- | --- | --- | --- |
|
|
| `id` | Required, canonical grammar in §8 | MUST (§8.1) | ✅ Yes | — |
|
|
| `kind` | Required, four kinds in §10 | MUST (§10) | ✅ Yes | — |
|
|
| `readable` | Required boolean | MUST (§9, §13) | ✅ Yes | — |
|
|
| `writable` | Required boolean | MUST (§9, §12) | ✅ Yes | — |
|
|
| `requires` | Required array of capability IDs | MUST (§9, §17) | ✅ Yes | — |
|
|
| `restorable` | Present in base descriptor example | Illustrative example, not MUST | ❌ No | SHOULD validate — future restore logic needs it |
|
|
| `category` | Present in base descriptor example | Illustrative example, not MUST | ❌ No | Optional for MVP — no host feature uses it |
|
|
| `label` | Present in base descriptor example | Illustrative example, not MUST | ❌ No | Optional — host can default to `id` |
|
|
| `unit` | Present in base descriptor example as `null` | Illustrative, OPTIONAL | ❌ No | Optional |
|
|
|
|
**Key analysis:** Contract §9 says *"Each target returned by `describe` MUST include enough metadata for a generic host to inspect and operate it"* and then shows a "Base descriptor" example. The section does **not** use RFC 2119 MUST/REQUIRED language for `restorable`, `category`, `label`, or `unit` individually. The normative MUST statements in §9 are about the descriptor being sufficient for generic operation — which `id`, `kind`, `readable`, `writable`, and `requires` satisfy.
|
|
|
|
The Authoring Guide line 94 lists these fields together, but as an authoring recommendation, not a host validation mandate.
|
|
|
|
### Classification: **SAFE TO DEFER**
|
|
|
|
Current `validateCatalog` is permissive but not nonconformant. It checks everything needed for the host to operate: `id`, `kind`, `readable`, `writable`, `requires`, and kind-specific metadata (`min`/`max`/`step` for ranges, `options` for selections). The host does not currently use `restorable` or `category` for any behavior.
|
|
|
|
### Recommended Resolution
|
|
|
|
Defer `restorable` and `category` validation to the scenario/restore milestone (future Step 4 or Step 5 work), where `restorable` becomes load-bearing. Adding it now is harmless but not required.
|
|
|
|
`label` validation is genuinely optional — the host can and does fall back to `id`.
|
|
|
|
### Blocks MVP Closure: **No**
|
|
|
|
---
|
|
|
|
## D. F-3 — Fixture README / Genericity Framing
|
|
|
|
### Classification: **SHOULD FIX NOW** — documentation/evidence-integrity issue with concrete future risk
|
|
|
|
This is not a product defect but a real risk to future development:
|
|
|
|
1. The copied [README.md](file:///g:/.vibe/XZBT-NGN/test-fixtures/reference-exhibits/README.md) is an upstream document that describes upstream verification and lists bugs as "found and fixed during verification" — but they are **not fixed upstream**. The upstream repository at `G:\.vibe\SciFi-XZBT` still has all three defects.
|
|
|
|
2. A future developer or agent re-copying from the stated upstream source will silently reintroduce all defects.
|
|
|
|
3. [PROVENANCE.md](file:///g:/.vibe/XZBT-NGN/test-fixtures/PROVENANCE.md) is accurate, but the README is longer, more prominent, and contradicts it.
|
|
|
|
### Exact Recommended Wording
|
|
|
|
Add the following header note to the **top** of `test-fixtures/reference-exhibits/README.md`, before the existing content:
|
|
|
|
```markdown
|
|
> **Local copy notice.** This directory contains locally corrected copies of
|
|
> the upstream reference exhibits. The corrections are documented in
|
|
> `../PROVENANCE.md`. The text below is the original upstream README; its
|
|
> verification claims describe the upstream state, not this corrected copy.
|
|
> Re-copying from the upstream source without reapplying corrections will
|
|
> reintroduce known defects.
|
|
```
|
|
|
|
Do **not** rewrite the upstream README body. One blockquote header is sufficient.
|
|
|
|
### Blocks MVP Closure: **Should be done before closure, but is not a contract blocker**
|
|
|
|
---
|
|
|
|
## E. F-4 — Browser Evidence
|
|
|
|
### Sufficiency Assessment
|
|
|
|
The current evidence model is:
|
|
|
|
- **Reproducible baseline:** 20/20 automated Node.js tests (`npm test`), fully reproducible from the repository
|
|
- **Supporting evidence:** 3/3 real-browser verification via `/test-fixtures/host-verification.html`, manually triggered, with results described in the report
|
|
|
|
This is appropriate for an engineering MVP. The report correctly hedges: *"This is local Chromium evidence, not a cross-browser certification"* and *"No success claim relies on mocks alone."*
|
|
|
|
### Must the browser suite produce a machine-captured artifact?
|
|
|
|
**No.** The automated suite is the reproducible baseline. The browser suite supplements it with end-to-end evidence that cannot be captured without a real browser (rendering, audio context lifecycle, iframe postMessage). Its manual nature is inherent to what it tests.
|
|
|
|
### Should automated browser execution be deferred?
|
|
|
|
**Yes.** Automated browser execution (Playwright, Puppeteer, etc.) is CI/infrastructure work, not Step 4 MVP scope.
|
|
|
|
### Classification: **INFORMATIONAL ONLY**
|
|
|
|
### Blocks MVP Closure: **No**
|
|
|
|
---
|
|
|
|
## F. F-5 — `stateRevision` Across Session Reconnect
|
|
|
|
### Semantic Assessment
|
|
|
|
The current implementation preserves `stateRevision` across sessions while resetting `sequence`. The review notes this is an "unwritten" cross-component invariant.
|
|
|
|
**Actual document coverage:**
|
|
|
|
1. **Contract §14.4:** *"`stateRevision` tracks committed persistent-state history."* — History is not session-scoped.
|
|
2. **Contract §16.1:** *"Event `sequence` [...] is session-scoped and resets when a new `sessionId` is issued."* — Only `sequence` is declared session-scoped. `stateRevision` is not.
|
|
3. **Authoring Guide §H (line 200):** *"`stateRevision` tracks committed persistent-state history and **does not reset with a new session**"* — **This is already explicitly stated.**
|
|
4. **Authoring Guide Appendix A (line 519):** *"Events carry monotonic `sequence`, reset on new session; `stateRevision` does not reset with session."* — **Also already explicit.**
|
|
|
|
The invariant **is** written down — in the Authoring Guide, which is an authoritative project document. The Contract itself implies it through §14.4 (state *history* is not session-scoped) and by the asymmetry with §16.1 (only `sequence` is called session-scoped).
|
|
|
|
### Is the current host behavior reasonable?
|
|
|
|
**Yes.** A host that detects `stateRevision` regression on reconnect is correctly enforcing the monotonicity requirement of §14.
|
|
|
|
### Would an exhibit that resets `stateRevision` on every session violate the contract?
|
|
|
|
**Yes** — it would violate §14's monotonicity requirement and contradict the Authoring Guide's explicit statement.
|
|
|
|
### Classification: **INFORMATIONAL ONLY**
|
|
|
|
### Recommended Clarification Level: **No action needed**
|
|
|
|
The Authoring Guide already states this explicitly. A future Contract §14 revision could add one sentence for completeness, but this is not a gap that needs closing before the MVP.
|
|
|
|
### Blocks MVP Closure: **No**
|
|
|
|
---
|
|
|
|
## G. Required Owner Decisions
|
|
|
|
**NO OWNER DECISIONS REQUIRED**
|
|
|
|
---
|
|
|
|
## H. Minimal Completion Patch List
|
|
|
|
### Patch 1 (the only patch): README copy-notice header
|
|
|
|
| Attribute | Detail |
|
|
| --- | --- |
|
|
| **File** | `test-fixtures/reference-exhibits/README.md` |
|
|
| **Change** | Add a blockquote header note (5 lines) at the top of the file, before existing content |
|
|
| **Why** | Prevents future re-copy confusion; distinguishes local corrected copy from upstream claims |
|
|
| **Verification** | Visual review of the added header; confirm existing README body is unchanged; `git diff --check` |
|
|
|
|
**Exact content:**
|
|
```markdown
|
|
> **Local copy notice.** This directory contains locally corrected copies of
|
|
> the upstream reference exhibits. The corrections are documented in
|
|
> `../PROVENANCE.md`. The text below is the original upstream README; its
|
|
> verification claims describe the upstream state, not this corrected copy.
|
|
> Re-copying from the upstream source without reapplying corrections will
|
|
> reintroduce known defects.
|
|
|
|
```
|
|
|
|
No other patches are required for MVP closure.
|
|
|
|
---
|
|
|
|
## I. Safe Deferrals
|
|
|
|
| Item | Defer To | Reason |
|
|
| --- | --- | --- |
|
|
| `validateCatalog` adding `restorable`/`category` checks (F-2) | Scenario/restore milestone | Not needed until restore logic depends on `restorable`; current behavior is permissive but conformant |
|
|
| Automated browser test execution (F-4) | CI/testing infrastructure work | Infrastructure concern, not an MVP requirement |
|
|
| Contract §14 explicit `stateRevision` session-persistence sentence (F-5) | Future Contract revision | Already stated in the Authoring Guide; not blocking |
|
|
| Optional `PROVENANCE.md` note about `INVALID_VALUE` per §9.5 (F-1) | Any convenient time | Purely informational; the fixture is already conformant |
|
|
| Report framing refinements (review §5 recommendations) | Owner's discretion | Report accuracy improvements; do not block the MVP |
|
|
|
|
---
|
|
|
|
## J. Recommended Final Report Wording
|
|
|
|
The following paragraph should be used in future reports to describe the genericity result:
|
|
|
|
> A single unmodified XZBT-NGN host drove three reference exhibits — Aquarium (12 targets), Planetarium (14 targets), and Haunted House (13 targets) — copied from the upstream AbacusAI reference set and locally corrected to conform to Contract 5.2. The corrections addressed non-standard descriptor kinds, a misnamed metadata field, missing argument validation, and a stale internal ID mapping; they are documented in `test-fixtures/PROVENANCE.md`. The host contains no exhibit-domain vocabulary and required no fixture-specific logic. The upstream reference source was not modified.
|
|
|
|
This wording makes clear that:
|
|
|
|
- ✅ The NGN host remained generic and unmodified
|
|
- ✅ Fixture corrections were necessary and load-bearing
|
|
- ✅ Corrections were Contract-oriented (conformance repairs), not host-specific exceptions
|
|
- ✅ Upstream reference sources were not modified
|
|
- ✅ The corrections are documented and traceable
|
|
|
|
---
|
|
|
|
## K. Final Triage Verdict
|
|
|
|
**STEP 4 MVP REQUIRES MINOR COMPLETION PATCHES**
|
|
|
|
One documentation patch (F-3 README header) should be applied before changing the milestone verdict. All other findings are either not defects, informational, or safely deferred.
|
|
|
|
After that patch, the milestone verdict should become:
|
|
|
|
**STEP 4 MVP COMPLETE — READY FOR NEXT STEP 4 PHASE**
|