fix(prompts): render locally recorded lessons and pitfalls

[rpoornac]

• Description: what and why

  Sections 5b (RELATED LESSONS) and 5c (KNOWN PITFALLS) of the specialist
  prompt read point["attrs"]["statement"] / point["attrs"]["description"],
  which is the shape a remote KB point arrives in. The local store flattens
  both on the way to disk — _normalise_lessons writes
  {statement, measured_impact} and
  _normalise_str_dicts(pitfalls, ("description", "severity")) writes
  {description, severity}, neither wrapped in attrs.

  A flat row therefore resolved attrs to {}, produced an empty statement,
  hit the if not statement: continue guard, and both sections fell through to
  their (none) placeholder. Every lesson and pitfall a local-mode session
  recorded was invisible to the next session — both halves of the KB's memory,
  what worked and what to avoid.

  Both readers now use the (point.get("attrs") or point) form already used for
  this same wrapped-vs-flat split at three sites in phases/prelude.py, so
  remote points and local rows both render. Fixing the reader rather than the
  writer keeps the on-disk format and the remote path untouched, and that
  renderer is already the designated tolerance point — it carries an explicit
  "tolerate shape drift" comment for bare-string rows and simply missed this
  shape.

  Nothing failed loudly, which is why it survived: the sections are advisory
  prose, and every existing test built its fixture in the wrapped shape by
  hand, so the writer was never on the other end of an assertion.

• Linked issue(s): close/fix refs

  None filed.

• Tests: added/updated? commands run?

  Two added, both in test_specialist_lessons_section.py. They build their
  input by calling the writer (_normalise_lessons / _normalise_str_dicts)
  instead of hand-writing a dict, which is what stops the two sides drifting
  apart silently again. Both fail on main with the section rendering (none).

  pytest src/hyperloom/inference_optimizer/tests/test_specialist_lessons_section.py
      14 passed   (includes the pre-existing guard that legacy {"raw": ...}
                   rows still do not leak, now that the fallback exposes them)
  
  pytest src/hyperloom -k "lesson or pitfall or warm or specialist or prompt"
      1323 passed
  
  pytest src/hyperloom scripts
      15745 passed, 8 failed — all 8 reproduce identically on a clean tree with
      this change stashed (pre-existing, subprocess "No module named 'hyperloom'"
      in this environment)
  
  ruff check / ruff format --check   clean
  

  Also verified as a real round trip: written through LocalRecipeStore.put_recipe,
  read back off disk, rendered into the prompt.

  --- 5b lessons, as the agent now sees it ---
  - **raise the MLA decode KV-split cap at conc=1**
      impact: gain_pct=30.00
  --- 5c pitfalls ---
  - **K=8 draft tokens overprovision at conc=1** (severity=regress)
  

• Breaking changes: yes/no (details if yes)

  No. Remote (wrapped) points render exactly as before; this only adds a
  fallback for rows that previously rendered as nothing.

• PR addresses single concern: yes/no (details if no)

  Yes — one defect, in the two renderers that share it.

  Noting one adjacent bug found while tracing this, deliberately not fixed
  here: SharedState.to_warm_start_summary renders the === Warm start ===
  section by reading entry.get("raw") or entry.get("symptom"), field names no
  writer produces. It is broken for the flat and wrapped shapes, so the root
  cause is different (wrong field names, not the wrapper), and it emits a
  misleading pitfalls (1): header with nothing beneath it. Worth its own
  change.

• Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed:

  No — root cause is in this repo.

[ZhengGong-amd]

The defect is real — locally recorded lessons and pitfalls never reach 5b/5c. But attrs = point.get("attrs") or point makes "a lesson row has two possible shapes" a permanent protocol, when the repo already has one answer: Recipe.to_dict / _normalise_lessons / _normalise_str_dicts write flat rows, writeback appends {statement, measured_impact} flat, knowledge_to_warm_recipe returns a flat top-level list, and test_t0_anchor_surfaces_pitfalls_and_lessons_from_existing_row already asserts state.warm_start_lessons[0]["statement"]. The wrapped attrs form only survives in hand-written test fixtures — so this change teaches the reader to accept a contract nothing currently writes, rather than deleting it.

Suggested direction:

1. Drop the or point fallback and the two narrating comments in _section_lessons / _section_pitfalls.
2. Read the flat fields directly (point["statement"], point["description"]), consistent with Recipe.to_dict, writeback, and the T0 test.
3. If legacy wrapped rows must still be tolerated, unwrap once in recipe_kb_t0 where warm_start_lessons / warm_start_pitfalls are assigned, and assert there that the snapshot carries no attrs wrapper. prelude's (recipe.get("attrs") or recipe) is a recipe-level shell, not a row-level one — it isn't the precedent it looks like.
4. Update the contract docs that caused this: _section_lessons / _section_pitfalls docstrings ("KB kind=lesson points"), _render_measured_impact ("attrs.measured_impact"), and the SharedState.warm_start_pitfalls comment ("list of KB point dicts").
5. Rewrite the section fixtures as flat rows (or feed them T0's output). Keep the writer-built fixture idea from this PR — that part is the right instinct; it just needs to lock the real shape.
6. Please don't fix to_warm_start_summary the same way in a follow-up. It reads raw / symptom, which no writer produces; the fix is to make it read the normalised description, not to add a fourth accepted shape.

Also worth noting: a non-empty input list can still render as (none) with no log and no error — that silence is why this survived. Worth addressing at the boundary (reject malformed rows in T0) rather than adding a warning in the renderer.

[rpoornac]

You're right, and the correction goes further than the patch: I justified the or point fallback by claiming the wrapped form was "the shape a remote KB point arrives in." There is no remote KB. It was removed end to end, and it was already gone from the commit I branched off — so I wasn't reasoning from a stale tree, I was reasoning from a shape I never verified had a producer. grep '"attrs"' across non-test code returns three hits in prelude.py, all recipe-level, and the two renderers I was editing. That's the whole population. Thanks for pushing back instead of taking the diff at face value.

And the prelude precedent is exactly the mistake you name: (recipe.get("attrs") or recipe) unwraps a recipe, and I read it as license for a row-level shell. Same expression, different object, no precedent.

Reworked along your six points:

1–2. Flat reads. _section_lessons / _section_pitfalls read point["statement"] / point["description"] directly. The or point fallback and both narrating comments are gone, along with the attrs local — the meta fields (validated_count, source_session_ids, framework_version) now read off the row too.

3. One unwrap, at the boundary. recipe_kb_t0._experience_rows normalises both lists where warm_start_lessons / warm_start_pitfalls are assigned. Legacy wrapped rows are unwrapped there and nowhere else, so nothing downstream knows two shapes.

Silence. Same helper drops rows missing the required field and logs the count and the field name. This is the part of your review I'd underweighted: the bug wasn't that the reader was wrong, it was that being wrong cost nothing — a non-empty list rendered (none) with no log, no error, and a plausible-looking prompt. A renderer warning would have been the wrong place; it doesn't know what the row was supposed to be. The boundary does.

4. Contract docs. Fixed all four you listed — the section docstrings, _render_measured_impact, and the SharedState.warm_start_pitfalls / warm_start_lessons comments — plus _format_version_note, whose lesson_attrs parameter was carrying the same assumption in its name.

5. Fixtures. Flat now, including a wrapped fixture in test_specialist_prompt_builder_coverage_unit.py that I hadn't touched in the first pass — it failed the moment the reader stopped accepting two shapes, which is a fair summary of your whole point. The writer-built fixtures stay, and there are three new tests on _experience_rows: flat passthrough, legacy unwrap, and drop-with-log.

6. to_warm_start_summary. Agreed, and #1466 currently makes exactly the mistake you're warning about — it reads description (right) but keeps a raw/attrs fallback (wrong). I'll rework it to read the normalised row and drop the fallbacks; it becomes simpler now that T0 guarantees the shape. It'll be a follow-up on that PR, not smuggled in here.

Rebased onto current main. Full suite: 16,281 passed, 8 failed — the same 8 that fail on a clean main checkout (test_external_multi_node, test_agentx_repair, test_preflight_auth_override, test_profile_and_kernel_handlers), none of them anywhere near this path. The 1,566 tests matching lessons/pitfalls/warm-start/prompt/T0 all pass.

One thing I did not touch, so it's a deliberate omission rather than an oversight: both renderers still accept a bare str row (if isinstance(point, str)), which pre-dates this PR. Now that T0 normalises, that branch is unreachable from the real path and is a third shape by your argument. Happy to delete it here or leave it for a separate change — your call.

[rpoornac]

Correcting one thing I said above, since you may act on it: "There is no remote KB" was too broad. The Cortex/gbrain KB backend is gone — recipe_kb/ is canonical_id, dispatcher, local_store, schema, with no remote — but knowledge/remote_recipe/ is live, and knowledge_to_warm_recipe passes lessons / pitfalls straight through from the remote record into the warm-start row.

It doesn't change the fix, and it doesn't rescue the fallback I removed. No code in this repo writes an attrs wrapper on an experience row, which is the claim that mattered. But because the remote projection is a passthrough, a remote record is the one thing that could deliver an unexpected shape, and that argues for the boundary unwrap over the reader-side one rather than against it: remote rows reach warm_start_lessons through the same history_source.get("lessons") path, so _experience_rows covers them, and anything malformed now gets logged instead of rendering (none).

I found this while working through your review on #1466, which turns on knowledge_to_warm_recipe never returning best_config — same module I'd written off.

[rpoornac]

Closing the sub-clause in your point 3 that I'd skipped — "assert there that the snapshot carries no attrs wrapper." My tests asserted on _experience_rows' return value, not on the snapshot T0 assigns, which is not the same guarantee. test_t0_anchor_surfaces_pitfalls_and_lessons_from_existing_row now asserts "attrs" not in state.warm_start_pitfalls[0] / [lessons][0] on the snapshot itself.

Writing the on-disk case for it turned up something that sharpens where the tolerance belongs. I seeded a wrapped row directly into recipe.json expecting T0 to unwrap it; it dropped it instead. Recipe.from_dict reads statement off each lesson, so a wrapped row on disk comes back as Lesson(statement="") — the local store cannot hand T0 anything to unwrap. The only path that can is the remote projection: knowledge_to_warm_recipe copies lessons straight out of the record without going through Recipe.from_dict.

So the unwrap is remote-only, and I've said so in the helper docstring rather than calling it "legacy", which was a guess dressed as a reason. The disk case is now tested for what it does do, which is the part that matters here: the unparseable row is dropped with warm_start_lessons: dropped 1 row(s) missing 'statement' instead of reaching the prompt as (none).

Same 8 pre-existing failures as clean main; CI green on the previous push and re-running now.