Skip to content

fix(kb): stop storing what_worked/what_failed rows as empty strings - #1561

Open
rpoornac wants to merge 1 commit into
mainfrom
fix/kb-experience-rows-lossless
Open

rpoornac wants to merge 1 commit into
mainfrom
fix/kb-experience-rows-lossless

Conversation

@rpoornac

Copy link
Copy Markdown
Collaborator

What breaks

what_worked / what_failed rows reach the KB with every value replaced by "".

The Coordinator is the only producer of these rows and stamps them with {name, extra_server_args, extra_envs, gain_pct, source} (writeback._build_recipe_payload). The store then normalised each row down to the arbor pair — _normalise_str_dicts(what_worked, ("description", "measured_impact")) — a vocabulary the Coordinator never writes. Nothing translated between the two, so every value was dropped:

sent:   {"name": "mtp_on", "extra_server_args": "--speculative-num-steps 3",
         "extra_envs": {...}, "gain_pct": 4.2, "source": "https://pr/123"}
stored: {"description": "", "measured_impact": ""}

This is not a missing field. The row count survived, so the KB looked populated while carrying nothing.

What that cost

prelude._warm_recipe_proven_items reads name and skips rows without one, so it always returned []. The already_proven list handed to the scout in phases/internal therefore carried only the current session's accepted variants. Anything proven in an earlier session was invisible, and the scout re-researched it — the KB's central promise, not paying out.

The fix

Finding and Failure now carry the producer's fields as hyperloom superset fields — the same convention as the existing Pitfall.severity, which arbor consumers ignore — and a new _normalise_experience preserves them instead of discarding them.

Two compatibility properties hold by construction:

  • description falls back to name, so an arbor consumer gets operator-readable text where it previously got "".
  • Superset fields serialise only when populated, so a row carrying just the arbor pair is byte-identical to before. The wire shape is unchanged for rows that never had these fields.

Why no test caught it

Both halves of the seam were already covered: one test asserts writeback stamps name, another asserts prelude reads it. But the prelude test hand-injected the row, so nothing exercised the persistence step between them — exactly where the data died.

The two added tests write through LocalRecipeStore and assert on what comes back out. Against main they fail with the bug in plain view:

assert worked[0].get("name") == "mtp_on"
E   AssertionError: assert None == 'mtp_on'
E    +  where {'description': '', 'measured_impact': ''}.get

assert {p["name"] for p in proven} == {"mtp_on", "fp8_kv"}
E   AssertionError: assert set() == {'fp8_kv', 'mtp_on'}

Scope

The explore dedup pre-fill stays inert for now: it fingerprints from extra_server_args / extra_envs, which the Coordinator does not record on what_failed rows. Storage no longer stands in the way; that remaining fix is producer-side.

Verification

  • 569 passed across the recipe_kb / sedimentation / warm-start suites.
  • Full suite: 22696 passed. The 9 failures are pre-existing on main at the same base (2cc382a83) in test_agentx_repair, test_external_multi_node, test_preflight_auth_override and test_supervisor — none touched here. The one that differed between worktrees (test_dotenv_fallback_ignores_arbitrary_cwd_dotenv) was traced to a stray .env in the local worktree: it fails there with main's code too.
  • ruff check src/hyperloom clean.

The Coordinator is the only producer of these rows and stamps them with
`{name, extra_server_args, extra_envs, gain_pct, source}`
(`writeback._build_recipe_payload`). The store normalised each row down to
the arbor pair — `_normalise_str_dicts(what_worked, ("description",
"measured_impact"))` — a vocabulary the Coordinator never writes, and
nothing translated between the two, so every value was dropped:

    sent:   {"name": "mtp_on", "extra_server_args": "--speculative-num-steps 3",
             "extra_envs": {...}, "gain_pct": 4.2, "source": "https://pr/123"}
    stored: {"description": "", "measured_impact": ""}

Not a missing field — the row count survived and every value was "".
`prelude._warm_recipe_proven_items` reads `name` and skips rows without
one, so it always returned [], and the `already_proven` list the scout is
handed in `phases/internal` carried only the current session's accepted
variants. Work proven in an earlier session was invisible, so the scout
re-researched it.

`Finding` and `Failure` now carry the producer's fields as hyperloom
superset fields — the same convention as `Pitfall.severity`, which arbor
consumers ignore — and `_normalise_experience` preserves them.
`description` falls back to `name` so an arbor consumer gets readable text
where it previously got "". A row carrying only the arbor pair still
serialises byte-identically, so the wire shape is unchanged for rows that
never had these fields.

Both halves of this seam were already tested: one test asserts writeback
stamps `name`, another asserts prelude reads it. But the prelude test
hand-injected the row, so nothing exercised the persistence step between
them — which is exactly where the data died. The added tests write
through LocalRecipeStore and assert on what comes back.

The explore dedup pre-fill stays inert for now: it fingerprints from
extra_server_args/extra_envs, which the Coordinator does not record on
what_failed rows. Storage no longer stands in the way; that fix is
producer-side.

Co-authored-by: Cursor <cursoragent@cursor.com>
@rpoornac
rpoornac requested a review from a team as a code owner September 17, 2026 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant