From 2adc950f23b38343730ce71198eef172e1846f11 Mon Sep 17 00:00:00 2001 From: linhdmn Date: Mon, 21 Sep 2026 23:10:11 +0700 Subject: [PATCH] =?UTF-8?q?docs(prd):=20the=20framework=20question=20was?= =?UTF-8?q?=20never=20a=20decision=20=E2=80=94=20D9,=20and=20the=20parity?= =?UTF-8?q?=20test=20that=20makes=20it=20falsifiable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `design.md` §18 carries the book's framework chooser, and it names LangGraph for exactly this shape: branching, state, HITL persistence. agentloop is that case. The choice never reached §15 — D1 asks "Runtime: Go vs Python", a language question, and it closed 2026-09-19, after M1's loop had shipped. The book's escape hatch is the 3% parity suite; no parity harness existed in any `.go` file, so custom won by default rather than by measurement. Recorded, not repaired quietly, in §3.1: - the decision was unfalsifiable, and Appendix F's F1 said the same thing about M3 from the other direction; - `AgentBase` — called "the abstraction layer" by §3.1, §4.3 and DUPLICATION-AUDIT's inheritance manifest — exists only as a comment at `internal/tools/registry_impl.go:3`. The seams that do exist are `loop.ModelClient`, `tools.ToolRegistry` and `xdev.Client`; - the reversal path was never costed. Then the missing half, in code: `internal/eval/parity.go` (`CompareParity`) pairs two reports by `CaseID` and returns `Ship` only when no case lost more than `ParityTolerance` (0.03) *and* the candidate is cheaper — the book's App. C bar. Pairing is by id, not index: a case that vanished is reported `Unpaired` and fails, because a dropped case is not a case that did not regress. Four tests, all failing when the tolerance is removed. §15 gains D9 carrying the question as **open** with that comparison as its acceptance; `docs/check-prd.py` gains two properties (the parity harness must be named where §11.4 requires it, and D9 must exist and cite its chooser) plus two selftest mutations. The first mutation found a real blind spot: the filename also appears in §3.1, D9 and the footer, so an unscoped search passed while §11.4 itself named no implementation — the check is now scoped to the section, and `--selftest` reports all 14 breakages caught. --- docs/PRD.md | 14 +++++- docs/check-prd.py | 20 ++++++++ internal/eval/parity.go | 87 +++++++++++++++++++++++++++++++++++ internal/eval/parity_test.go | 89 ++++++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+), 2 deletions(-) create mode 100644 internal/eval/parity.go create mode 100644 internal/eval/parity_test.go diff --git a/docs/PRD.md b/docs/PRD.md index fc0f229..018611e 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -114,6 +114,14 @@ That is what the decisions below implement — `AgentBase` is the abstraction la | Code intelligence | **LeanKG** over HTTP: ladder + graph verbs via `POST /api/v1/query`, memory via `/api/v1/memory/banks/{bank}/memories` | implements P33 Semantic Recall and P32 Landmark Memory over a real graph instead of re-embedding files: Ch.8's “large codebases are navigated with search plus selective retrieval, never full-context loading” only holds if the retrieval layer can answer structure questions (`impact`, `callers`, `context`), which a vector store cannot | **Runtime risk, stated honestly:** the book's code shapes (asyncio fan-out, Python SDK tool-use) do not port line-for-line; Go buys the deployment envelope and the portfolio's operational habits, and costs the SDK's reference implementations. Two things make this a recorded decision rather than a preference. First, the book itself disagrees with "start custom" in Ch.2 — its landscape chapter says *past a 30% workaround share, go custom*, and App. C scores frameworks on a 12-dimension matrix instead of dismissing them. We are past that share: the portfolio already maintains the three services this loop binds to. Second, the trade is only defensible because §11 (evals) measures it — if Go's loop plumbing delays the eval suite past milestone 2, the decision is wrong and gets revisited in §15. Second, every row above is *inherited* from the manifest in `design.md` §3.1 — the one line of code that resolves each inheritance is named there, and the four seams the audit found are listed at the bottom of this section. +**The framework question was never a decision, and that is a finding (2026-09-21).** This section quotes the book's chooser — *start custom rather than framework-first* (Ch.7) — and `design.md` §18 carries the rest of the same rule: *branching/state/HITL-persistence → LangGraph*, wrap a framework in the abstraction from day one, migrate once workarounds exceed 20–30% **and only when the custom build scores within 3% on the same eval suite** (App. C). agentloop *is* the LangGraph case: branching, state, HITL persistence. So the framework was the named candidate, not an afterthought — and the choice never reached §15. D1 asks *"Runtime: Go vs Python"*, a **language** question, and it is stamped closed 2026-09-19, after M1's loop had already shipped (2026-09-18). Three things followed, and each is recorded here rather than repaired quietly: + +1. **The decision is unfalsifiable.** The book's escape hatch is the 3% parity suite; no parity harness existed anywhere in the codebase, so custom won by default rather than by measurement. Appendix F's F1 said the same thing about M3 from the other direction. Fixed in the same commit as this paragraph: `internal/eval/parity.go` (`CompareParity`) is that harness, paired by `CaseID` so a dropped case fails instead of passing, and §11.4's requirement is now backed by a test that fails when the tolerance is removed. +2. **`AgentBase` does not exist as code.** §3.1 and §4.3 call it "the abstraction layer" and `docs/DUPLICATION-AUDIT.md` builds an inheritance manifest on it, but the only occurrence in any `.go` file is a comment at `internal/tools/registry_impl.go:3`. The seams that do exist are two small interfaces (`loop.ModelClient`, `tools.ToolRegistry`) plus `xdev.Client`. The *intent* is satisfied; the *name* is documentation that reads as a component. +3. **The reversal path was never costed.** Reversing now means re-implementing the loop, `budget.Guard`, the tracer, and the approval gate behind a framework to compare them — which is exactly the "prototype if <4 weeks, wrap from day one" work that was skipped. + +**What this does not say:** the book's headline advice (Ch.7, start custom for production) was followed, and its own landscape chapter (Ch.2) agrees past a 30% workaround share — which the portfolio is, since it already maintains the three services this loop binds to. The finding is narrower and more uncomfortable: a *recorded* decision was never made, and the test that would have made it reversible was never built. **§15's D9 now carries it as an open decision with that test as its acceptance.** + **Why hybrid (Ch.4–5):** predictability is the router's decision variable. @@ -391,7 +399,7 @@ Every config change runs the full suite with 3 runs per case, `p<0.05`, watching ### 11.4 Deploy gate and the REFINE loop -`EvalRunner` runs in CI; a deploy is blocked on the full-suite gate. **Config changes also run the paired parity comparison** (the same 50 cases under both configurations, scored per case): a cost win only counts if no case falls below the incumbent by more than its tolerance — this is the harness M3's acceptance needs, and without it that acceptance is untestable. each case runs on a fresh agent with `max_steps=10, max_cost=$1.00` (the book's harness caps; our per-run defaults stay §17's 9 steps / $1.00) and emits a JSON report (pass rate, avg/p95 latency, avg cost, per-category, failures). Every production incident's **first** fix step is a new regression case (Record → Extract → Formalize → Iterate → Normalize → Expand), +10 cases/week. +`EvalRunner` runs in CI; a deploy is blocked on the full-suite gate. **Config changes also run the paired parity comparison** (the same 50 cases under both configurations, scored per case): a cost win only counts if no case falls below the incumbent by more than its tolerance — this is the harness M3's acceptance needs, and without it that acceptance is untestable. **Implemented 2026-09-21:** `internal/eval/parity.go` (`CompareParity`) pairs two reports by `CaseID` and returns `Ship` only when no case lost more than `ParityTolerance` (0.03) *and* the candidate is cheaper — the book's App. C bar, in code. Pairing is by id, not index: a case that vanished is reported `Unpaired` and fails the comparison, because a dropped case is not a case that did not regress. It is the acceptance for §15's D9 and the missing half of M3's cost claim. ### 11.5 Baseline calibration (the goal G4 loop) @@ -475,7 +483,7 @@ These are the ways the book's own priors (Ch.1–13), accepted wholesale, would | **Go runtime tax** — loop plumbing delays the eval suite | the product's differentiator (measurement) ships late | M6 gate is the eval suite in CI before any domain template; §15 revisits the runtime if M2 slips | | **Auto-approval erosion** — gates approved without reading | agents get blanket permission harmlessly, then not harmlessly | timeout denies, <10% interrupt target, sampling, median-approve-time fatigue guard | | **Semantic-cache false positives** — a cached answer served to a distinct question | confidently wrong answers at scale | threshold validated against *our* measured false-positive rate, per-template, never a copied default; cache only above the validated threshold | -| **Infinite loop with cost** — the loop bounds themselves are the failure | worst case: spend breaks the service | pre-action enforcement, kill switch tested every deploy, per-day ceiling independent of per-run | +Every row here is a decision `design.md` §18 left open plus the two this PRD introduced (D3 tenancy, D7 memory backend). None of them blocks M0's exit except D1 — the runtime — and each is written as a recommendation with the section that argues for it, so a reviewer can disagree by citing a section rather than by rewriting one. **D9 is the exception to "closed":** the framework-vs-custom question `design.md` §18 actually poses was never decided, and it stays open until the parity comparison in §11.4 has been run against a real candidate (§3.1 records the finding). | **Tool surface growth** — 5 tools becomes 40 | selection accuracy collapses, prompt cost grows | hard cap of 15 visible tools with a router beyond it; new tool requires a removal or an eval justification | | **System One backend coupling** — onegw `systemone` Kind is merged (`9faea01`); open: PR #110 combo reorder, Laya sidecar not built, Jev↔Laya agreement on the guard corpus unmeasured. API shape verified 2026-09-21 (Bearer, `/v1/systemone`, state+questions). | if either backend drifts or local RAM OOMs, screens fail closed or burn budget | keep agentloop backend-agnostic; require shape parity tests; default CI to Laya; prod primary Jev until agreement ≥ target; RSS ceilings in runbook ([`docs/JEV-INTEGRATION.md`](JEV-INTEGRATION.md) §7–8) | | **Guardrail latency** — a ~740 ms/call screen at every step boundary adds seconds to a 10-step run | real-time UX degrades; budget burns faster | screen once per phase boundary, not per tool call; count screen cost in BudgetGuard; allow operators to disable the output screen for non-critical runs (logged) | @@ -488,6 +496,7 @@ Every row here is a decision `design.md` §18 left open plus the two this PRD in | # | Decision | Recommendation | Owner | By | |---|---|---|---|---| | D0 | Who reviews and owns this PRD (no name in the file today) | the author signs the Sources section and turns D1–D7 into dated decisions | **you** (signed 2026-09-19) | before any code — **closed** | +| D9 | **Framework vs custom loop** — LangGraph is the book's named candidate for this shape (branching/state/HITL-persistence, `design.md` §18); D1 answered the *language* question, not this one | **custom, on measurement not assertion** — and the measurement did not exist until this row: `internal/eval/parity.go` (`CompareParity`, §11.4) is the App. C "within 3%" bar in code. Acceptance for this decision: run both through the same suite, paired by case, and **ship the candidate if `Ship` is true** (no case below tolerance, candidate cheaper). Until that comparison has been run against a real LangGraph prototype, this row is **open** and the custom loop is the incumbent, not the winner | **you** | when a LangGraph prototype can run §11.2's suite — before any further loop-infrastructure milestone | | D1 | Runtime: Go vs Python | **Go** (§3.1) — the loop itself is stdlib-only, so the real question is the tools' SDKs; revisit if M2 slips | — | M0 exit — **closed 2026-09-19** | | D2 | Tracing backend: Langfuse self-hosted vs OTel-only | Langfuse self-hosted; OTel exporter as a secondary sink | — | before M2 — **closed 2026-09-19** | | D3 | Tenancy: single-tenant v1 vs isolation now | single-tenant v1, seam named (§7.4) | — | before M5 — **closed 2026-09-19** | @@ -740,6 +749,7 @@ Written the way an unfriendly reviewer would write it, then answered. Every find **Numbers** (all priors, all in §17, none of them specs): 9 steps · 120 s · $1.00/run · 90% forced synthesis · 70% context ceiling · compress every 5 · 2,000-token results · 3 identical `(tool,args)` = cycle · retry 3 with jitter, never a write or a 400 · CB 5/60 s/2 · eval pass 0.8 + latency + cost caps · <10% interrupts · 90-day traces. +* Last updated: 2026-09-21 (**The framework question was never a decision — now it is one.** `design.md` §18 carries the book's chooser and names LangGraph for exactly this shape (branching/state/HITL-persistence), but the choice never reached §15: D1 asks "Runtime: Go vs Python", a language question, and it closed 2026-09-19 — after M1's loop had shipped. The book's escape hatch, the 3% parity suite, did not exist in any `.go` file, so custom won by default. §3.1 now records the finding (including that `AgentBase` exists only as a comment at `internal/tools/registry_impl.go:3`), §11.4's parity requirement is backed by `internal/eval/parity.go` (`CompareParity`, paired by `CaseID`, `Ship` = no case below tolerance ∧ cheaper) with four tests that fail when the tolerance is removed, and **§15's D9 carries the decision as open** with that comparison as its acceptance.) **Proof.** Containment suite (5 cases, M1 gate) → 50-case four-category suite (M6 gate, +10/week, deploy blocked below 85%) → production incidents become cases. Identity: the durable write test (§11.2 case 2, both retry shapes). Every default's provenance is in §17; every default moves only with an eval run. **Build order.** M0 docs → M1 containment core → M2 guards + tracing → M3 planning + tiering → M4 memory → M5 HITL → M6 evals + console → M7 multi-agent (conditional on §10's gate). The eval suite is the moat; M6 exists so it cannot ship last by accident. diff --git a/docs/check-prd.py b/docs/check-prd.py index 12edf62..ab6ff51 100644 --- a/docs/check-prd.py +++ b/docs/check-prd.py @@ -162,6 +162,21 @@ def check(text: str) -> list[str]: if "Config changes also run the paired parity comparison" not in text: fail.append("the parity harness behind M3's cost acceptance is not specified") + # 8b. the parity harness is not merely *specified* — §11.4 says where it + # lives. Scoped to the section on purpose: the filename also appears in + # §3.1, D9 and the footer, so an unscoped search passed while §11.4 itself + # named no implementation (found by --selftest, which is why it exists). + if "### 11.4" in text and "### 11.5" in text: + s114 = text[text.index("### 11.4"):text.index("### 11.5")] + if "internal/eval/parity.go" not in s114: + fail.append("§11.4 specifies the parity harness but names no implementation") + # D1 answers the language; without a row for the framework, the choice is + # invisible to a reviewer reading §15 — which is how it went undecided. + if not any(ln.startswith("| D9 |") for ln in text.splitlines()): + fail.append("no D9 row: the framework-vs-custom decision has no home in §15") + elif "design.md` §18" not in text[text.index("| D9 |"):text.index("| D9 |") + 700]: + fail.append("D9 does not cite the chooser it comes from (design.md §18)") + # 9. provenance for both source sweeps is recorded if "QC pass (2026-09-18)" not in text: fail.append("the claim-verification sweep is not recorded in §16") @@ -202,6 +217,11 @@ def check(text: str) -> list[str]: ("the pattern ledger is renamed", "## 20. Appendix D", "## 20. Appendix Z"), ("the parity harness is dropped", "**Config changes also run the paired parity comparison**", "**Config changes also run a cost comparison**"), ("the SSE event names are dropped", "`state`, `step`, `approval`, `done`", "several event types"), + # Anchored on the §11.4 sentence, not the filename: the filename also + # appears in §3.1/D9/footer, so a first-occurrence replace left §11.4 + # unnamed and the check still passed — a blind spot --selftest reported. + ("the parity implementation is unnamed", "**Implemented 2026-09-21:** `internal/eval/parity.go`", "**Implemented 2026-09-21:** the parity harness"), + ("D9 loses its home", "| D9 |", "| DX |"), ] diff --git a/internal/eval/parity.go b/internal/eval/parity.go new file mode 100644 index 0000000..7bfdcb6 --- /dev/null +++ b/internal/eval/parity.go @@ -0,0 +1,87 @@ +// Package eval's parity half: the paired comparison that decides whether a +// cheaper configuration is actually cheaper. +// +// PRD §11.4 requires it and M3's acceptance depends on it: "a 5+-step task is +// ≥40% cheaper than single-tier ReAct with no case scoring below the baseline +// by more than its eval tolerance". Cost is measurable on its own; the second +// half is what stops a cheaper loop that quietly got worse from passing. Until +// this file existed, that half was prose — the PRD's own Appendix F (F1) said +// so: "parity is not measured anywhere in §11.2". +// +// The same comparison is the book's framework-migration bar (App. C): run both +// configurations through the same suite and ship the candidate only within +// tolerance. That is what makes §15's D9 falsifiable rather than asserted. +package eval + +import "fmt" + +// ParityTolerance is how far a single case may fall below the baseline before +// the comparison fails, in score points. It is a *tolerance*, not a target: +// the book's ship bar is "within 3%", and a case that loses more than this is +// a regression the headline cost number is not allowed to hide. +const ParityTolerance = 0.03 + +// ParityResult is the paired verdict for one configuration pair. +type ParityResult struct { + Baseline string `json:"baseline"` + Candidate string `json:"candidate"` + Paired int `json:"paired"` + Unpaired []string `json:"unpaired,omitempty"` + Regressions []string `json:"regressions,omitempty"` + CostDelta float64 `json:"cost_delta"` // fraction, negative = candidate cheaper + Parity bool `json:"parity"` // no case lost more than the tolerance + Cheaper bool `json:"cheaper"` // candidate cost is strictly lower + Ship bool `json:"ship"` // parity AND cheaper — the App. C bar +} + +// CompareParity pairs two reports case by case and reports whether the +// candidate is a legitimate improvement. +// +// Pairing is by CaseID, not by index: two reports of different lengths or +// orders would otherwise compare case 3 against case 7 and call it parity. +// A case present in only one report is reported as unpaired and fails the +// comparison — a case that silently vanished is not a case that did not +// regress. +func CompareParity(baseline, candidate *Report, tolerance float64) ParityResult { + res := ParityResult{ + Baseline: baseline.SuiteID, + Candidate: candidate.SuiteID, + } + if tolerance <= 0 { + tolerance = ParityTolerance + } + + byID := make(map[string]Result, len(candidate.Results)) + for _, r := range candidate.Results { + byID[r.CaseID] = r + } + seen := make(map[string]bool, len(baseline.Results)) + + for _, b := range baseline.Results { + c, ok := byID[b.CaseID] + if !ok { + res.Unpaired = append(res.Unpaired, b.CaseID+" (missing from candidate)") + continue + } + seen[b.CaseID] = true + res.Paired++ + if c.Score < b.Score-tolerance { + res.Regressions = append(res.Regressions, + fmt.Sprintf("%s: %.3f → %.3f (lost %.3f, tolerance %.3f)", + b.CaseID, b.Score, c.Score, b.Score-c.Score, tolerance)) + } + } + for _, c := range candidate.Results { + if !seen[c.CaseID] { + res.Unpaired = append(res.Unpaired, c.CaseID+" (new in candidate)") + } + } + + if baseline.AvgCostUSD > 0 { + res.CostDelta = (candidate.AvgCostUSD - baseline.AvgCostUSD) / baseline.AvgCostUSD + } + res.Cheaper = candidate.AvgCostUSD < baseline.AvgCostUSD + res.Parity = len(res.Regressions) == 0 && len(res.Unpaired) == 0 + res.Ship = res.Parity && res.Cheaper + return res +} diff --git a/internal/eval/parity_test.go b/internal/eval/parity_test.go new file mode 100644 index 0000000..42673d5 --- /dev/null +++ b/internal/eval/parity_test.go @@ -0,0 +1,89 @@ +package eval + +import "testing" + +// The parity comparison is the check M3's acceptance has been missing: "≥40% +// cheaper at eval parity" is only testable if a case that lost quality can +// fail the comparison. Each test below is a way the verdict must say no. +func TestParityRejectsACheaperButWorseCandidate(t *testing.T) { + base := &Report{SuiteID: "baseline", AvgCostUSD: 0.40, Results: []Result{ + {CaseID: "a", Score: 0.90}, + {CaseID: "b", Score: 0.85}, + }} + // Cheaper by 50%, but case b lost 0.10 — more than the 0.03 tolerance. + cand := &Report{SuiteID: "candidate", AvgCostUSD: 0.20, Results: []Result{ + {CaseID: "a", Score: 0.90}, + {CaseID: "b", Score: 0.75}, + }} + + got := CompareParity(base, cand, ParityTolerance) + if got.CostDelta >= 0 { + t.Errorf("CostDelta = %v, want negative (candidate is cheaper)", got.CostDelta) + } + if got.Parity { + t.Error("Parity = true, want false — a case lost more than the tolerance") + } + if got.Ship { + t.Error("Ship = true, want false — the App. C bar is parity AND cheaper") + } + if len(got.Regressions) != 1 { + t.Fatalf("Regressions = %v, want exactly one", got.Regressions) + } +} + +func TestParityShipsWithinTolerance(t *testing.T) { + base := &Report{SuiteID: "baseline", AvgCostUSD: 0.40, Results: []Result{ + {CaseID: "a", Score: 0.90}, + {CaseID: "b", Score: 0.85}, + }} + cand := &Report{SuiteID: "candidate", AvgCostUSD: 0.24, Results: []Result{ + {CaseID: "a", Score: 0.90}, + {CaseID: "b", Score: 0.83}, // lost 0.02, inside the 0.03 tolerance + }} + + got := CompareParity(base, cand, ParityTolerance) + if !got.Parity || !got.Ship { + t.Errorf("Parity/Ship = %v/%v, want true/true (0.02 < 0.03 tolerance, 40%% cheaper)", + got.Parity, got.Ship) + } + if got.Paired != 2 { + t.Errorf("Paired = %d, want 2", got.Paired) + } +} + +// A case that disappeared is not a case that did not regress. Comparing by +// index would have paired a against a and silently dropped b. +func TestParityFailsOnAnUnpairedCase(t *testing.T) { + base := &Report{SuiteID: "baseline", AvgCostUSD: 0.40, Results: []Result{ + {CaseID: "a", Score: 0.90}, + {CaseID: "b", Score: 0.85}, + }} + cand := &Report{SuiteID: "candidate", AvgCostUSD: 0.20, Results: []Result{ + {CaseID: "a", Score: 0.90}, + }} + + got := CompareParity(base, cand, ParityTolerance) + if got.Parity || got.Ship { + t.Error("a dropped case must fail the comparison, not pass it") + } + if len(got.Unpaired) != 1 { + t.Errorf("Unpaired = %v, want exactly one", got.Unpaired) + } +} + +// A candidate that costs more is not a migration — even at perfect parity. +func TestParityDoesNotShipAPricierCandidate(t *testing.T) { + base := &Report{SuiteID: "baseline", AvgCostUSD: 0.20, Results: []Result{{CaseID: "a", Score: 0.9}}} + cand := &Report{SuiteID: "candidate", AvgCostUSD: 0.30, Results: []Result{{CaseID: "a", Score: 0.9}}} + + got := CompareParity(base, cand, ParityTolerance) + if !got.Parity { + t.Error("Parity = false, want true — no case regressed") + } + if got.Cheaper || got.Ship { + t.Errorf("Cheaper/Ship = %v/%v, want false/false", got.Cheaper, got.Ship) + } + if got.CostDelta <= 0 { + t.Errorf("CostDelta = %v, want positive", got.CostDelta) + } +}