Files
XZBT-NGN/STEP4-INDEPENDENT-REVIEW.md
T
2026-09-14 07:57:18 -07:00

150 lines
16 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Step 4 MVP — Independent Conformance Review
**Reviewer:** independent (not the implementing agent)
**Date:** 2026-09-14
**Scope:** XZBT-NGN Step 4 MVP — host, transport, validation, UI, server, tests, and the three copied reference fixtures — against XZBT Exhibit Contract 5.2 and the NGN Implementation Plan v5.2.
**Method:** read-only. No file in either repository was modified. Every claim below is tied to a command result or a file line.
---
## 1. Verdict
**The Step 4 MVP is a genuine, working implementation of the Contract 5.2 host boundary, and its central genericity claim holds.**
The host is real, not a mock: it negotiates a session, discovers a catalog it has never seen, renders controls from descriptors, drives state and impulses, tracks two independent counters, and recovers from desynchronization. I reproduced the automated suite at 20/20 passing. I independently confirmed that no product source file contains any exhibit-domain vocabulary.
The report's own verdict — `STEP 4 MVP COMPLETE — MINOR FOLLOW-UP REQUIRED` — is **accurate and appropriately hedged**. I found no defect that invalidates the MVP. I found **one substantive conformance gap in the fixture layer** (F-1), **one host-side robustness gap** (F-2), and a set of documentation and evidence-scope issues.
The most important finding is not a bug. It is that **the fixture repairs are load-bearing for the genericity claim, and the report does not say so.** The three fixtures are the only evidence that the host is generic. Those fixtures were edited to make them conform. That is legitimate — but it means the MVP has demonstrated that the host drives three *repaired* exhibits, not three *unmodified* ones. The report's phrase "a single unmodified host drives all three" is true of the host and false of the fixtures, and the distinction matters for what the MVP has actually proven.
---
## 2. What I verified independently
| Claim | How I checked | Result |
| --- | --- | --- |
| 20 automated tests pass | `node --test` in `G:\.vibe\XZBT-NGN` | **Confirmed.** `tests 20 / pass 20 / fail 0`, 357 ms. |
| No domain vocabulary in product source | `Select-String` over `src/*.js`, `src/transport/*.js`, `public/*.html`, `public/*.css`, `server/*.js` for `aquarium\|planetarium\|haunted\|fish\|apparition\|scifi\|universe\|preset\|warp\|speech\|announcement\|tank\|sky\|room\|telemetry` | **Confirmed.** Zero matches. The only hit for a broader pattern was `host.js:3`, the canonical event-type list. |
| Fixture repairs are exactly as documented | `git diff --no-index` between `G:\.vibe\SciFi-XZBT\exhibit-test\abacusai\reference-exhibits` and `test-fixtures/reference-exhibits` | **Confirmed, and complete.** The entire delta is: 8 × `kind: 'telemetry'``kind: 'state'`; 1 × `args:``arguments:`; 3 × `sky.magnitudeLimit``sky.magnitude-limit`; 32 added lines in `shared/contract-core.js`; and deletion of `host-harness/` and `tests/`. |
| The upstream source was not modified | `git status` in `G:\.vibe\SciFi-XZBT` | **Confirmed.** `exhibit-test/` is untracked; no tracked file in the exhibit repo was touched by this work. |
| The magnitude repair was a real bug, not a cosmetic rename | Compared `read()` and `dispatch()` in both copies | **Confirmed.** Upstream `planetarium/exhibit.js:397` read `case 'sky.magnitudeLimit'` while the descriptor at `contract-adapter.js:36` declared `sky.magnitude-limit`. The reader returned `null` for a declared readable target, and native slider writes fell through to no case. The repair is correct and minimal. |
| The `arguments` repair was necessary | Read `argumentSchema()` in `src/validation.js` and the invoke path in `shared/contract-core.js` | **Confirmed.** Both read `target.arguments`. Upstream Planetarium declared `args:`, so its `minutes` argument was invisible to both the host and the fixture's own validator. |
---
## 3. Findings
### F-1 — Substantive: the fixture invoke path does not enforce `INVALID_ARGUMENTS`
**Severity: medium. Conformance gap in the fixture layer, not in the host.**
Contract §24 lists `INVALID_ARGUMENTS` as a distinct base error code, and the Authoring Guide's conformance checklist (line 522) requires that "unknown target, invalid value, invalid session, and invalid arguments each return the correct distinct error code."
The fixture core's invoke path (`shared/contract-core.js`, the block marked `// Fixture-only correction for the owner's authoritative 5.2 clarification`) returns `INVALID_VALUE` for every argument failure: undeclared key, missing required argument, wrong type, enum miss, bounds, step, and length. `INVALID_ARGUMENTS` appears in the `ERROR_CODES` array at the top of the same file and is never returned by anything.
The host is not at fault. `src/validation.js` validates arguments locally and raises `INVALID_VALUE` before the request leaves the browser, and `src/host.js` faithfully surfaces whatever code the exhibit returns. The host would handle `INVALID_ARGUMENTS` correctly if a fixture emitted it.
The practical consequence is that the MVP has **no end-to-end evidence that a distinct `INVALID_ARGUMENTS` code survives the transport**, because no fixture can produce one. The report's coverage list claims "argument constraints" are covered — true — but does not claim `INVALID_ARGUMENTS` specifically, and it should not, because it is not.
Note also that `planetarium/exhibit.js` *does* return `INVALID_ARGUMENTS` from `advanceTime()` for out-of-range minutes. That path is unreachable through the contract, because the fixture core rejects the value first with `INVALID_VALUE`. So the code exists in the fixture, is declared in the fixture's own error table, and is dead.
**Recommendation:** either return `INVALID_ARGUMENTS` from the fixture core for argument-shape failures (keeping `INVALID_VALUE` for value-constraint failures), or record explicitly in `PROVENANCE.md` and the report that the fixture layer collapses the two codes and that `INVALID_ARGUMENTS` is therefore unverified end-to-end. Do not leave it implicit.
### F-2 — Host: `validateCatalog` does not validate `restorable`, `category`, or `label`
**Severity: low. Robustness, not correctness.**
Contract §9 requires each descriptor to carry `restorable`, `category`, and `requires`, and the Authoring Guide (line 94) repeats the list. `validateCatalog` in `src/validation.js` checks `id`, `kind`, `readable`, `writable`, and `requires` — but not `restorable` or `category`. A descriptor missing either is accepted silently.
This is defensible for an MVP: the host does not currently use `restorable` (no scenario restore yet) or `category` (no grouping in the UI), so an absent field cannot cause a wrong action. But the host is the component that will later drive scenario restore, and `restorable` is exactly the field that decides whether a target belongs in a restore snapshot. Validating it now is cheap and prevents a malformed descriptor from becoming a silent restore bug in Phase 4.
**Recommendation:** add `typeof t.restorable === 'boolean'` and `typeof t.category === 'string'` to the descriptor check in `validateCatalog`. Low priority; safe to defer to the scenario milestone if the owner prefers.
### F-3 — Documentation: the genericity claim is stated more strongly than the evidence supports
**Severity: medium. This is a reporting-accuracy issue, and it is the one I would fix first.**
`test-fixtures/reference-exhibits/README.md` states: "The point of the set is not the exhibits; it is that a single unmodified host drives all three." The host is indeed unmodified and generic — I verified that. But the fixtures are not unmodified, and the README is the upstream document that `PROVENANCE.md` explicitly says "describes upstream verification and is not evidence of NGN verification."
The risk is concrete. A future reader — or a future agent — opening `test-fixtures/reference-exhibits/README.md` sees a claim of three unmodified exhibits driven by one host, and the README's own "Bugs found and fixed during verification" section lists the camelCase-ID bug as already fixed upstream. It is not fixed upstream. The upstream copy at `G:\.vibe\SciFi-XZBT\exhibit-test\abacusai\reference-exhibits` still has `sky.magnitudeLimit` in `read()` and `dispatch()`, still has `kind: 'telemetry'` in eight descriptors, and still has `args:` in Planetarium. Anyone who re-copies the fixtures from the stated source will reintroduce all three defects.
`PROVENANCE.md` is accurate and does list the corrections. The problem is that the README sits in the same directory, is longer, is more confident, and contradicts it.
**Recommendation:** add a short header note to the copied `test-fixtures/reference-exhibits/README.md` stating that this is a locally corrected copy, that the upstream README's verification claims do not apply to it, and pointing at `PROVENANCE.md`. One paragraph. Do not rewrite the upstream document.
### F-4 — Evidence scope: the browser suite is not reproducible from the repository
**Severity: low. Process, not product.**
The report's strongest evidence is the real-browser suite: "3/3 real exhibits passed" via `/test-fixtures/host-verification.html`. I read `test-fixtures/host-verification.js` and it is a real, substantive test — it drives the production `ExhibitHost` and `postMessageTransport` against actual exhibit HTML in iframes, and it covers version rejection, handshake, catalog, state/range/selection writes, no-op revision, impulse execution, argument delivery, error survival, events, reconnect, forced session invalidation, and recovery. That is good evidence and I have no reason to doubt it.
What I cannot do is reproduce it. It requires a human to run `npm start`, open a browser, and click a button. There is no recorded output, no timestamped artifact, and no way for a reviewer to confirm the 3/3 result without re-running it by hand. The report is honest about this ("This is local Chromium evidence, not a cross-browser certification"), which is the right hedge.
**Recommendation:** none required for the MVP. If the owner wants the browser suite to carry weight in future reviews, have it write its result to a file or the console in a form that can be captured. Otherwise, treat the browser suite as supporting evidence and the 20 automated tests as the reproducible baseline — which is how the report already frames it.
### F-5 — Observation: `stateRevision` is not reset on reconnect, and the host depends on that
**Severity: informational. No action needed.**
`shared/contract-core.js` deliberately preserves `stateRevision` across sessions while resetting `sequence`. The host's `refresh()` enforces `snapshot.stateRevision >= baseline` and rejects a regressed snapshot. The browser suite asserts `host.stateRevision >= revisionBeforeReconnect`.
This is correct and matches the README's stated rationale. I flag it only because it is a cross-component invariant that is not written down in the contract itself — Contract §14 says `stateRevision` is monotonic, and §16.1 says `sequence` resets per session, but the contract does not explicitly say what happens to `stateRevision` on a new session. The fixture and host agree; a future exhibit that reset `stateRevision` on handshake would be rejected by the host with "Snapshot revision regressed." Worth a line in the contract's §14 or in the authoring guide.
---
## 4. Contract conformance assessment
Against Contract §29 (Conformance Minimum), for the **host**:
| Requirement | Status |
| --- | --- |
| Compatible handshake | Met. `host.js` sends `supportedContractMajors: [5]`, validates the negotiated major, rejects otherwise. |
| Normalized message envelope | Met. Requests carry `xzbt`, `type`, `requestId`, `sessionId`; responses are matched by `requestId` and expected type. |
| `describe` | Met. `refresh(true)` requests it, `validateCatalog` checks it, the UI renders from it. |
| At least one discoverable target | Met. Catalog is built entirely from `describe`; no hard-coded IDs. |
| `state.get` for readable persistent targets | Met. `refresh()` requests it and enforces that the snapshot contains exactly the readable non-impulse set — both directions. |
| `stateRevision` | Met. Tracked, displayed, gap-detected, and protected against regression. |
| Normalized responses | Met. `responses` map enforces the expected result type per request. |
| Normalized event emission | Met. All six base event types accepted; unknown types rejected. |
| Session event sequencing | Met. Monotonic check, gap detection, resync, and per-session reset. |
| Command validation | Met. `validateSet` and `validateArgs` run before the request is sent. |
| Version reporting | Met. Contract, exhibit identity, registry revision, and state revision are all displayed. |
The host meets the conformance minimum. The two gaps I found (F-1, F-2) are in the fixture layer and in descriptor-field strictness respectively; neither is a conformance-minimum failure.
Against the Implementation Plan §36 (MVP Acceptance Criteria), all twelve criteria are met, including the last one — "no SciFi-specific target vocabulary is hard-coded into core NGN logic" — which I verified directly rather than taking on trust.
---
## 5. Assessment of the report itself
The report is unusually good on the dimension that usually fails: **it separates what was verified from what was not.** Section N names the browser and date and calls the result local evidence. Section Q lists limitations without prompting. Section M states plainly that the fixtures needed repairs. Section O distinguishes the automated suite from the browser suite. The verdict is hedged rather than triumphant.
Three places where it is weaker than it should be:
1. **Section M's framing.** "The copied fixtures needed narrow documented corrections" is accurate but understates the consequence. Those corrections are what makes the genericity demonstration possible. The report should say that the MVP proves the host drives three *repaired* exhibits, and that the repairs are the reason the demonstration works.
2. **Section O's coverage list.** It claims "argument constraints" are covered, which is true, but a reader will reasonably infer that `INVALID_ARGUMENTS` is among them. It is not, and cannot be, given F-1.
3. **Section M's table.** "Aquarium 12 / Planetarium 14 / Haunted House 13" matches the descriptors I counted. Good. But the report does not note that the upstream README claims the same counts while the upstream fixtures would fail to load at all under a strict catalog validator — the camelCase IDs would throw at `Catalog` construction. That is worth one sentence, because it is the clearest evidence that the repairs were necessary rather than cosmetic.
None of these change the verdict. They change how much weight a future reader should put on it.
---
## 6. Recommended follow-up, in priority order
1. **Add the copy-notice header to `test-fixtures/reference-exhibits/README.md`** (F-3). One paragraph. Highest value per unit of effort, because it prevents a future re-copy from silently reintroducing three defects.
2. **Resolve `INVALID_ARGUMENTS`** (F-1) — either implement it in the fixture core or document the collapse explicitly in `PROVENANCE.md`.
3. **Tighten `validateCatalog`** (F-2) to check `restorable` and `category`. Deferrable to the scenario milestone.
4. **Correct the three report framings** in §5 above.
5. **Optional:** note the `stateRevision`-across-sessions invariant in the contract or authoring guide (F-5).
---
## 7. Bottom line
The MVP does what it claims. The host is genuinely generic, the transport genuinely validates origin and source, the state machine genuinely distinguishes revision from sequence, and the recovery paths are genuinely exercised. The fixture repairs were necessary, correctly diagnosed, and minimal — the magnitude repair in particular fixed a real null-read and a real dead native write path, not a cosmetic naming mismatch.
The one thing I would not let stand is the impression that three pristine exhibits were driven by an unmodified host. Three *repaired* exhibits were driven by an unmodified host, and the repairs are load-bearing. That is a fine result for an MVP — it is arguably the more useful result, because it found real bugs in the reference set. It just needs to be said plainly.