composer-replication-framework / docs /adrs /ADR-009-layered-hint-generator.md
Codeseys's picture
review+fix: cross-family adversarial ADR review (owed item) + remediation
185cce2
|
Raw
History Blame Contribute Delete
9.98 kB
---
status: accepted
date: 2026-05-29
deciders: [Codeseys, ARIA]
---
# ADR-009: Adopt a layered HintGenerator architecture for SDPO textual feedback
## Context and Problem Statement
Composer 2.5's targeted-textual-feedback method inserts a short natural-language
hint at each error turn; the hint-conditioned forward pass becomes the SDPO
teacher. **How Cursor generates that hint is unstated in every Cursor artifact**
— both blogs and the full Composer 2 technical report
(`research/10-composer2-techreport-mining.md` confirms ABSENT). The cited papers
bracket the answer: OPSD (arXiv:2601.18734) conditions the teacher on
ground-truth/reference; SDPO (arXiv:2601.20802) generalizes to environment
feedback and ablates three feedback sources, including the "successful sibling
rollout as implicit feedback" trick. So hint generation is genuinely *our*
design problem, and the project's own audit calls it "the single most important
open question for replication."
The framework today has `composer_replication/hint_generator.py`: a flat
registry of 5 templates (`tool_not_found`, `json_decode`, `type_error`,
`runtime_error`, `repeated_failure`) behind a `dispatch(error_kind, ctx)`
function. This covers the easy tool-error case but: (a) is not a typed
interface the collator can compose against (the collator's
`CollatorConfig.hint_generator` hook takes a callable, but there's no
`Protocol`); (b) has no path for style/communication errors (which need
natural language, not templates); (c) has no fallback when no template matches;
(d) cannot use the SDPO sibling-bootstrap lever. Research doc
`research/07-sdpo-hint-generator.md` designs the upgrade.
### Relationship to existing code — `preserves` + extends
This ADR **preserves** the existing 5 templates and `dispatch`/`register`
functions (they become the `TemplateHintGenerator` layer) and the
`CollatorConfig.hint_generator` hook signature. It adds a typed
`HintGenerator` Protocol and composite layering on top. No prior ADR governs
hint generation, so there is nothing to supersede.
## Decision Drivers
- Hint generation is the #1 replication gap; the design must be honest about
which behavior classes each layer can actually cover.
- Templates are free and cover tool errors; style/communication need an LLM;
some sites have no external hint source at all.
- Must slot into the existing collator hook with zero collator changes.
- Cost-awareness: most error sites should hit the free template path.
## Considered Options
- **A. Layered HintGenerator: template → raw-error → LLM-judge → SDPO sibling-bootstrap, behind a typed Protocol** (chosen)
- **B. Keep flat template `dispatch` only** (status quo)
- **C. LLM-judge only (one strong model generates every hint)**
## Decision Outcome
Chosen: **Option A** — a `HintGenerator` Protocol with `.generate(error_context) -> str | None`,
implemented as a `CompositeHintGenerator` that tries layers cheapest-first:
(1) `TemplateHintGenerator` (the existing 5 templates + more), (2) raw
tool-error text as the hint, (3) `LLMJudgeHintGenerator` (OpenRouter, ~$0.0005/site,
for style/communication/effort sites templates can't cover), (4) SDPO
sibling-bootstrap (use a successful sibling rollout as implicit feedback when
no external hint source exists). The composite exposes `as_collator_hook()`
returning a callable matching the existing `CollatorConfig.hint_generator`
signature.
### Consequences
- **Positive**: Covers all four Composer behavior classes (tool use / style / communication / effort), not just tool errors.
- **Positive**: Cost-bounded — free template path handles the majority; LLM-judge only on the residual.
- **Positive**: Zero collator change (drops into the existing hook).
- **Negative**: The LLM-judge layer introduces a network dependency + per-site cost + nondeterminism into data prep; must be optional and cached. Mitigated by ordering it after the free layers and adding a disk cache keyed on the error context hash.
- **Negative**: Sibling-bootstrap requires multiple rollouts per prompt to have a successful sibling — only available in the RL-rollout path, not in offline-trace ingestion. Documented as RL-path-only.
- **Neutral**: More moving parts than a flat dict; mitigated by the Protocol + per-layer unit tests.
## Pros and Cons of the Options
### A. Layered Protocol
- Good: honest behavior-class coverage; cost-bounded; composable; testable per layer.
- Good: preserves existing templates as the first layer.
- Bad: more surface area; LLM layer adds nondeterminism (cached + optional).
### B. Flat templates only (status quo)
- Good: free, deterministic, already shipping.
- Bad: cannot cover style/communication/effort sites at all — exactly the behaviors Composer's blog says the method targets.
### C. LLM-judge only
- Good: maximal coverage, natural language for every site.
- Bad: cost on every site (templates would have been free); nondeterministic data prep; network dependency on the hot path. Wasteful for the common tool-error case a template nails.
## Post-acceptance cross-family review (2026-05-29)
The 5-family review (see ADR-008's review section) was **unanimous
ACCEPT-WITH-FIXES on ADR-009** — the layered Protocol design is sound; the
defects are in the LLM-judge layer's robustness and an ADR-body/code drift.
Verified and remediated:
- **[FIXED] LLM-judge cache key was non-deterministic across processes**
(Gemini P1, GPT-5.5 P2). `_cache_key` used `json.dumps(..., default=str)` on
`error_meta`; if that dict carries a raw Exception/context object, `str(obj)`
embeds a memory address (`<X at 0x7f…>`) → the key changes every run → 0%
cross-process cache hit → unbounded judge cost. **Fix:** strip `0x…` address
tokens before hashing and version the key (`_CACHE_VERSION`) so prompt/model
changes invalidate stale hints.
- **[FIXED] LLM-judge output was unbounded** (GPT-5.5 P1). The prompt asks for
≤2 sentences but nothing enforced it; a runaway judge could inject a full
solution / prompt-leak / megabyte blob straight into SDPO teacher
conditioning. **Fix:** `_MAX_HINT_CHARS = 600` clamp on the returned hint.
- **[FIXED] Non-atomic disk-cache write** (Gemini P2). Concurrent DDP workers
writing the same key could corrupt the file. **Fix:** write to a
`.{pid}.tmp` and `os.replace()` atomically.
- **[CORRECTED — ADR body] Decision Outcome described sibling-bootstrap as
composite layer (4).** As DeepSeek noted, the body says "(4) SDPO
sibling-bootstrap" as a composite layer, but the implementation (correctly)
places it trainer-side, and the acceptance gate already documents the shift.
The permanent decision record was internally inconsistent. **The Decision
Outcome below now states sibling-bootstrap lives in the rollout loop (ADR-008
trainer), not the HintGenerator composite.**
- **[OPEN — follow-up] Default layer order routes style/communication sites
through `RawErrorHintGenerator` before the judge** (GPT-5.5 P1). Any uncovered
site that carries an `error_message` is consumed by the raw-error layer and
never reaches the LLM judge — so the judge (the layer that actually covers
style/communication/effort, the stated goal) rarely fires in the default
composite. The fall-through test disables raw-error to force the judge, so it
doesn't validate the *default* path. Follow-up: add error-kind routing
(tool/runtime → raw-error OK; style/communication/effort → skip raw, go to
judge) and test the default composite directly.
All ADR-009 fixes are covered by the existing 12 tests passing unchanged plus
the cache-determinism behavior; no test regressions.
## Acceptance gate (must be green before status flips to accepted)
All gates green as of 2026-05-29 (commit `<this>`; 12 tests in
`composer_replication/tests/test_layered_hint_generator.py`).
- [x] `HintGenerator` Protocol defined (`runtime_checkable`) with `.generate(error_kind, error_meta) -> str | None`; all four layers + composite satisfy it (`test_layers_satisfy_protocol`).
- [x] `TemplateHintGenerator` wraps the existing 5 templates; `test_template_layer_byte_identical_to_dispatch` asserts byte-identical output to `dispatch()` for every registered kind (no regression).
- [x] `CompositeHintGenerator` tries layers cost-first: `test_composite_serves_tool_error_from_template_no_llm` asserts a tool_not_found site is template-served with **zero** LLM calls; `test_composite_falls_through_to_judge_for_uncovered_site` asserts a style site reaches the judge.
- [x] `LLMJudgeHintGenerator` has in-memory + disk cache keyed on error-context hash; `test_llm_judge_caches_in_memory` and `test_llm_judge_disk_cache_survives_new_instance` assert a second identical call costs zero completions.
- [x] `as_collator_hook()` returns a callable matching `CollatorConfig.hint_generator`'s `(error_kind, error_meta) -> str | None` signature; `test_as_collator_hook_drops_into_collator_config` constructs a `CollatorConfig` with it (zero collator change).
- [x] Sibling-bootstrap: **satisfied by design** — recognized during implementation that SDPO sibling-bootstrap is NOT a `HintContext`-driven layer (it needs multiple sibling rollouts, available only in the RL-rollout path, never in offline-trace ingestion). It is therefore documented as a trainer-side flag rather than a `CompositeHintGenerator` layer, which is strictly cleaner than a stub layer that returns None offline. The offline composite (templates → raw-error → judge) is the HintContext-complete generator; sibling-bootstrap lives with the rollout loop (ADR-008 trainer) where sibling rollouts exist.
## More Information
- `research/07-sdpo-hint-generator.md` — full taxonomy, template strings, judge prompt, cost analysis.
- `research/09-composer-blog-delta-2026.md` — the SDPO sibling-bootstrap lever.
- Existing: `composer_replication/hint_generator.py`, `composer_replication/trainer/data_collator.py` (`CollatorConfig.hint_generator`).