Archive QA feedback under feedback/

Keeps the 2026-10-30 QA report in the repo for later reference.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

feedback/vli-qa-feedback-2026-10-30.md

@@ -0,0 +1,99 @@
+# VisualLearningIdea — QA feedback for the coding agent
+
+**Session date:** 2026-10-30 · **Method:** full contract re-verification of the MCP tools via live calls, plus a read of repo HEAD (`app.py`, `SPEC.md`, `vli/grammar.py`, `vli/lexicon.py`, `vli/assets.py`, `vli/scene.py`, `vli/pipeline.py`, `vli/html.py`, `vli/simplify.py`).
+
+**TL;DR:** The `refine_scene` contract is now **fully verified** — including the last open cell (reject on an Objaverse-tier item drops cleanly to a primitive). Everything previously tested still behaves as documented. The feedback below is 4 priority items, 3 robustness/polish items, and one build-parity question. The single most impactful fix is #1 (reroll determinism); the cheapest high-value fix is #2 (q0 labels, one line).
+
+---
+
+## 1. Verified contract (complete as of today)
+
+| Change | Status | Evidence |
+|---|---|---|
+| `freeze: true/false` | ✅ | prior session; ledger round-trips `frozen` flag |
+| `use: n` (0-indexed, swap) | ✅ | prior session |
+| `reroll` swaps with the pool | ✅ | reproduced today: seed 0 → chosen ↔ `alternatives[6]`, `4459b90d… ↔ b86b2f4d…`, pool unchanged |
+| `reject` (simready → objaverse → primitive) | ✅ | prior session |
+| **reject on Objaverse item → primitive** | ✅ **new** | today: `rejected_tiers: ["objaverse"]`, chosen = box, height re-derived to 0.3 m (`source: default`), renders clean |
+| `shape` (primitive) | ✅ | prior session |
+| `height_m` | ✅ | prior session; reject correctly resets only non-user heights |
+| seed re-chooses configuration | ✅ | seed 0 and seed 1 re-verified today; placement details in item 6 |
+| reject on primitive-tier → generation | ⚠️ reachable dead end | today: readable error, no fallback (item 3) |
+
+Seed-0 baseline reproduced bit-identically from the first session: uid `4459b90d0e87486b96739feb2a40e0ee` (4893 faces, license `by`), yaw −82.87678304500669°, sun 26.638940957447787°/−145.04170934144128°, sky `#b8aaa5`, cat z-offset 0.013124391962400764.
+
+---
+
+## 2. Priority items
+
+### P1-1 · Reroll at a fixed seed is a toggle, not a sampler
+
+- **Evidence:** two sessions days apart, reroll at seed 0 always swaps chosen with `alternatives[6]` (`4459b90d ↔ b86b2f4d`, same z-offset 0.0215100129063485 both times). Seed 1 always picks slot 3 (`7c3383a2…`). Same input ⇒ same pick, forever.
+- **Cause:** `pipeline.apply_changes` builds `rng = np.random.default_rng(seed)` fresh on every call, so a fixed seed always draws the same `k = rng.integers(len(alternatives))`.
+- **Consequence:** to see a *different* asset the caller must change the seed — but the seed also re-rolls placement and sun (verified: seed 1 moved the cat to yaw 162.17° and the sun to 30.77°/134.59°, sky `#adacb3`). Asset exploration and configuration exploration are coupled through one number, which the docstring presents as one knob.
+- **Suggested fix (pick one):**
+  1. derive the pick from item state so it walks the pool: `k = int(chosen["uid"][:8], 16) % len(alternatives)` — deterministic per state, changes as swaps change state;
+  2. carry a counter in the ledger (e.g. `ledger["revision"] += 1` inside `apply_changes`; seed the changes-rng from `(seed, revision)`);
+  3. at minimum, document the toggle behaviour in the `refine_scene` docstring so MCP callers know to vary the seed (and that this also moves the scene).
+
+### P1-2 · `q0` "which cat?" is unanswerable — 8 identical labels
+
+- **Evidence:** `q0.alternatives = ["cat", "cat", … ×8]` on every ledger; unchanged after rerolls; `default_chosen: "cat"`.
+- **Cause:** `pipeline.settle_all` fills question labels via `candidate_label()`, which returns `cand.get("category")` for Objaverse candidates; the uid (present in every candidate dict) is never surfaced.
+- **Impact:** SPEC says determiners open a "which?" question for the discussion to arbitrate; as shipped the question carries zero information — arbitration is only possible by reading `n1.decision.chosen.uid`.
+- **Suggested fix (one line in `settle_all`):** for `unresolved` alternatives only, disambiguate, e.g. `f"{label} [{uid[:6]}]"` → `cat [4459b9]`, `cat [b86b2f]`… Keep `use` positional — labels are display-only. Optionally append loaded face count later. (Bonus: `check_words`' `maps_to` already helps availability; consider adding the uid there too.)
+
+### P1-3 · Generation tier is a user-reachable dead end
+
+- **Evidence (today, end-to-end):** `refine_scene` with `[{"id":"n1","reject":true}]` on a primitive-tier item → `Error: no usable generation asset for 'cat' (generation tier is not wired up yet)`.
+- **Good:** this is a readable `gr.Error` (via `assets.load_generation` → `UnusableAsset` → `assets.settle` → `app._fail`), *not* a raw `NotImplementedError` traceback; `app.generate_mesh` (the ZeroGPU stub) is never called, as documented.
+- **Bad:** the `refine_scene` docstring advertises `reject` → generation, so a user/model can hit this dead end, and the mutated ledger is discarded with the error (the caller must resend their previous ledger to recover).
+- **Suggested fix:** pre-empt in `apply_changes`/`next_tier` — when the *next* tier would be `generation`, raise a clean `ValueError` **before mutating** the decision, e.g. `"'cat' has no tier left after primitive; generation is not wired up — use shape/height instead"`. Keep the stub itself as is.
+
+### P1-4 · The canonical example sentence errors as-is
+
+- **Evidence:** `translate_to_scene("the cat sat and the sun shone")` → `Not available (no asset): sun … map each to a primitive`. It only works with `primitives_json={"sun":"sphere"}`. This sentence is the example in the `check_words` *and* `translate_to_scene` docstrings and the UI placeholder, so a first-time MCP caller (e.g. HuggingChat) hits an error on the advertised happy path.
+- **Cause:** intended, not a regression — the `simready_phrases` sun-visor guard correctly stopped indexing a lone "sun", and LVIS provides no usable sun category. The code is right; the docs lag.
+- **Suggested fix:** swap the canonical example to a sentence that needs no primitives. Verified today: **"the cat sat and the lamp glowed"** → `ok: true` (lamp: 1 SimReady + 79 Objaverse, `maps_to: "desk_lamp"`), and a desk lamp as the light source reads nicely. Alternatively keep the cat/sun sentence but append `… pass primitives_json={"sun":"sphere"}` wherever it appears.
+
+---
+
+## 3. Robustness / polish
+
+### P2-5 · Build parity: scene-page face budget is not in effect
+
+- **Evidence:** today's scene page for the 4893-face cat inlined **4893 faces** in its `mesh` blob. Repo `vli/html.py` sets `SCENE_FACE_BUDGET = 400` and `_geometry()` unconditionally decimates above budget (`simplify.decimate` → `simplify_quadric_decimation`); with repo HEAD, 4893 inline is impossible. Meanwhile `b86b2f4d` shipped exactly 400 — consistent with its native count (no decimation path running).
+- **Ask:** confirm the running build matches repo HEAD (rebuild/restart if stale). If it *does* match, debug the `_geometry` budget path. Untested related path: `ladder_page`'s `LADDER_MAX_FACES = 1000` skip.
+
+### P2-6 · Placement couples to asset bounds — document it
+
+- **Evidence:** same seed 0, same yaw/sun/sky, but the cat's z-offset differs per asset: 0.0131 (`4459b90d`), 0.0215 (`b86b2f4d`), 0.0205 (box). Cause: `scene.configure` draws the same uniforms for the same seed, but multiplies the row z-jitter by the mesh's half-width, and the sun position scales with reach computed from widths. So "same seed = same scene" only holds while assets don't change (exactly what confused the previous session's "seed restored, but not bit-identically").
+- **Ask:** either make the row z-jitter fixed in metres (so small objects don't wiggle proportionally to their own width), or add one docstring line: *placement/sun are seeded but scale with each asset's bounds; swapping an asset shifts offsets*.
+
+### P2-7 · Vocabulary can drift silently between rebuilds
+
+- `lexicon._simready_packages()` calls the unauthenticated GitHub tree API (60 req/h, shared Space IP) and silently falls back to `SIMREADY_FALLBACK` (only `nvidia/simready-assets`) on failure; `_cached_json` is per-container. The word list — the product's core promise ("the UI offers only available words") — can change without a code change.
+- **Ask:** make drift visible and/or impossible: commit the built package index (or serve it from a pinned dataset revision), and/or expose dictionary stats in `check_words` (e.g. total words, simready package count) so callers can detect a reduced dictionary.
+
+### P3-8 · Candidate pool capped at 9 of 87
+
+- `_object_decision` and `next_tier` take `cands[1:9]` → chosen + 8 alternatives, while "cat" has 87 Objaverse candidates (lamp: 79). `use`/`reroll` can never reach the rest, and a random 9 includes junk (one of the nine was a 400-face lump that reads as a rock; another, `7c3383a2`, is excellent — the pool has both).
+- **Ask:** raise the cap for large categories, or accept `{"id":"n1","use_uid":"…"}` / an offset parameter; at minimum document the cap. (Fixing #2's labels makes the 9 visible; wider access makes them matter.)
+
+### P3-9 · Small DX nits
+
+- `use` with an out-of-range index (or on empty alternatives) raises a raw `IndexError` → unhelpful message. Wrap: `"n1 has no alternative at index k (N alternatives)"`.
+- Fallback primitives render neutral grey (`base_color` default `#b4b4b4`): the rejected cat-box is colourless in an otherwise tinted scene. Optional: tint primitives deterministically per word (hash → hue) so primitive-tier scenes read better.
+
+---
+
+## 4. Session probe log (exact)
+
+1. `translate "the cat sat and the sun shone", {"sun":"sphere"}, seed 0` → n1 = `4459b90d…` (4893 faces, license `by`), yaw −82.8768°, sun 26.6389°/−145.0417°, sky `#b8aaa5`, z 0.01312 — matches the original session bit-for-bit ✅
+2. `reroll`, seed 0 → n1 = `b86b2f4d…` (400 faces, pale olive), z 0.02151, slot-6 swap — identical to previous session ✅
+3. `reject n1` → primitive box, `rejected_tiers: ["objaverse"]`, height 0.3 (default), q0 → `default_chosen: "box"`, `alternatives: []` ✅ *(closes the last untested cell)*
+4. `reroll`, seed 1 → n1 = `7c3383a2…` (399 faces, reads clearly as a cat), yaw 162.1669°, sun 30.7664°/134.5948°, sky `#adacb3` ✅ *(also demonstrates placement/asset seed coupling)*
+5. `reject n1` (primitive tier) → `Error: no usable generation asset for 'cat' (generation tier is not wired up yet)` ⚠️ *(documented gap, now confirmed end-to-end)*
+6. `check_words "the cat sat and the lamp glowed"` → `ok: true`, lamp `available_in {simready: 1, objaverse: 79}`, `maps_to: "desk_lamp"` ✅
+
+**Suggested follow-ups for the agent's own QA:** `simplify_item` end-to-end (ladder rungs + `LADDER_MAX_FACES` skip), NC/ND license skip recording with a live `decision.skipped` entry, `use` back-swap idempotence after a skip shifted indices, and `check_words` on a low-memory/fallback build to observe dictionary drift.