PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The mechanism is correct, placed correctly, and — the part I tested rather than assumed — it comes back RED on the omission direction, which is the entire reason #15932 exists. The one durability concern below is not a delivered-scope defect: the gap it names predates this PR and is owned by #15872. This diff is the first thing that makes that gap visible instead of silent. Blocking an improvement because it does not also close a neighbouring ticket would be nitpick-theater; the JSDoc item is Maintainer Polish, not a return cycle.

Peer-Review Opening: This is the good version of this fix. I came in expecting to argue about whether membership should be derived at all, and the diff had already answered it more precisely than I would have — the anchor test is a trigger for requiring a decision, never the definition of membership. Requiring planeMemberReason on an explicit false is the part I did not anticipate and is what makes "excluded" distinguishable from "forgotten."

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15932 (the ticket, incl. its two candidate shapes and its explicit constraint); #15872 (the named first instance); current dev ai/planeConfig.mjs (collectPlaneMembers, assertPlaneMemberCoherence); ai/ConfigProvider.mjs leaf() at :57; the changed-file list; ADR-0019 §10.5 as source-of-authority. query_raw_memories prior-art sweep on plane-membership derivation returned nothing but session-init noise — recording that as a clean miss rather than dressing it up as grounding.
• Expected Solution Shape: Membership declared on the leaf, exported list derived from those declarations. The boundary this must NOT hardcode is membership itself — #15932 is explicit that §10.5 distinguishes "resolves beneath dataRoot" from "explicitly placed," so a set derived purely from path-prefix would erase relocation intent and still pass every test. Test isolation must fail on the omission direction (add a plane-anchored leaf, forget to declare it) — the direction toBe(10) could never see.
• Patch Verdict: Improves. The anchor-prefix test appears only as the condition that makes an undecided leaf throw; planeMember remains the sole membership authority. The JSDoc states the invariant explicitly — "this walk reads METADATA and DEFAULTS, never resolved values, so a member relocated by its own env binding stays a member" — and the code matches it. The planeMemberReason requirement on false is a genuine improvement over my expected shape: without it, an exclusion and an omission are the same artifact.
• Premise Coherence: Coheres with verify-before-assert at the substrate level — this converts a self-referential guard (list vs. literal, list vs. itself resolved) into one that checks the claim against the tree it describes. It is the same correction as #15887 / PR #15888, where an allowlist self-test pinned a member instead of asserting an invariant over the set.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15932
• Related Graph Nodes: #15872 (the founding instance, and the owner of the residual below) · ADR-0019 §10.5 · #15887 / PR #15888 (member-pin → set-invariant precedent) · #15871 (the parity stack that consumes these lists, and whose body records the bound this PR lifts)

🔬 Depth Floor

Challenge — the deferral has no expiry, and that is the one thing I would watch.

storagePaths.graphProd ships as planeMember: false with:

*"open membership decision — the graph SQLite is the plane core artifact with a plane-anchored default yet no declared membership; tracked by #15872 … Explicit non-member until that ticket rules."*

That is the escape hatch used correctly and visibly, and I want to be clear I am not calling it a dodge. But planeMember: false asserts "decided: not a member" while the reason says "no decision yet." Those are different states, and the mechanism has only two plus fail-closed — so a deferral had to be encoded as a decision plus prose.

Consequence, verified rather than reasoned: the plane's core durable artifact sits outside assertPlaneMemberCoherence, and the only thing that will ever flip it is a human remembering. I probed it directly — a planeMemberReason citing a ticket passes regardless of that ticket's state, so nothing goes red when #15872 closes without the flip:

reason citing a ticket -> PASS (no ticket-state check — prose only)

Not blocking, and not a Required Action on this PR — the gap predates the diff and #15872 owns it. But the cheapest durable fix is one comment, not code: #15872 should carry an explicit AC that flipping graphProd's planeMember is part of its closure. Otherwise this class's founding instance is the one place it still silently persists, which would be a sad ending for an otherwise excellent mechanism.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff
• Anchor & Echo JSDoc: one drift, polish-level — see Findings
• [RETROSPECTIVE]-class claims: accurate
• Linked anchors: #15872 and #15888 do establish the claimed patterns (verified both)

Findings: One drift, non-blocking. The derivePlaneMemberPaths JSDoc cites graphProd as the exemplar of the undecided → fails-closed class:

"…which is the omission class a real instance first confirmed (the graph SQLite leaf in the memory-core copy — the plane's core artifact with a plane-anchored default and no declared membership)…"

In the shipped diff, graphProd does have a declared membership (false + reason) — set by this very PR. A reader following the JSDoc to the named example would expect it to throw, and it does not. The example is historically accurate and presently false. Suggest past-tensing it ("the leaf that first confirmed this class, now an explicit deferral — see its planeMemberReason"), which also gives the reader the pointer to the open decision.

🧠 Graph Ingestion Notes

• [KB_GAP]: Prior-art sweep for plane-membership derivation returned nothing substantive. The leaf() four-key guarantee — that leaf() always emits default/env/type/parse, including parse: null for non-env leaves (ConfigProvider.mjs:70) — is load-bearing for any descriptor-tree walk and is documented only implicitly in the return-type annotation. Worth an explicit line if another walker is ever written.
• [TOOLING_GAP]: ai/mcp/server/knowledge-base/configBase.mjs cannot be imported standalone in a bare node harness (createConfigProxy → "Cannot create proxy with a non-object as target"). I therefore verified the derivation against tier-1 and memory-core only; the KB tree is covered by CI, not by my falsifier. Stating the boundary rather than implying I checked all three.
• [RETROSPECTIVE]: The load-bearing design choice is that the anchor test is a trigger, not a definition. A path-prefix-derived member set would have been simpler, would have passed every test, and would have quietly destroyed the "explicitly placed" distinction ADR-0019 §10.5 depends on. #15932 named that trap and the diff avoided it — the ticket doing its job as a design constraint, not just a work order.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15932
• #15932 confirmed not epic-labeled (bug, ai, architecture); single newline-isolated Resolves; #15872 correctly appears as a non-closing reference

Findings: Pass.

📑 Contract Completeness Audit

• #15932 contains a Contract Ledger matrix (three rows)
• Implemented diff matches it

Findings: Pass, with one deliberate divergence that is an improvement, not drift. The ledger's row 3 proposed "optional planeMember metadata key; absent = non-member." The diff ships absent = fail-closed on a plane-anchored default, not silently non-member. That is stricter than the ledger and is the entire point — "absent = non-member" would have reproduced the exact omission class #15872 documents. Flagging it so the ledger and the shipped contract agree: #15932's row 3 should be updated to the shipped semantics, which per §5.4 is normally a Required Action — I am treating it as polish because the drift runs in the safe direction and the ticket is the author's own.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI 17/17 green at c89997cf09 (verified via gh pr checks, not statusCheckRollup); author receipt present (planeConfig.spec.mjs +77, BaseServer.spec.mjs census replaced)
• Reviewer falsifier: run, with a named concern — see below
• Test location: test/playwright/unit/ai/planeConfig.spec.mjs is the canonical sibling path

Findings: Pass. Two falsifiers, one of which killed my own hypothesis:

(1) Named concern — does isLeaf silently skip real descriptors? isLeaf requires all four of default/env/type/parse. leaf()'s own JSDoc says it "Replaces the raw {env, default, parse} literal" — and a surviving raw literal would lack type, fail isLeaf, be walked through as a container, and its plane-anchored default would never trigger the decision requirement. That would be a silent hole in exactly the completeness this PR sells. Falsified: walking the real tier-1 tree found 224 leaf-shaped nodes and 0 nodes carrying a default that fail isLeaf. Everything routes through leaf(), and leaf() at :65-71 always emits all four keys. The predicate is sound.

(2) Does the mechanism actually come back red? Ran the negative controls against c89997cf09, because a completeness check that cannot fail is worth nothing:

anchored + NO decision        -> RED   "leaf "fake.leaf" has a plane-anchored default but NO planeMember decision…"
planeMember:false, NO reason  -> RED   "…declares planeMember: false without a non-empty planeMemberReason…"
planeMember:false, EMPTY rsn  -> RED   (empty string correctly rejected, not just missing)
planeMember:false + reason    -> PASS  derived=[]
planeMember:true              -> PASS  derived=["fake.leaf"]

And against the real trees: tier-1 → 10 derived, memory-core → 10 derived, both matching their declared lists. The empty-string case mattering is the detail I would have got wrong — typeof x !== 'string' || x.length === 0 catches '', where a bare truthiness check on a missing key would have let planeMemberReason: '' through.

🔗 Cross-Skill Integration Audit

• No skill documents a predecessor step needing to fire this
• AGENTS_STARTUP.md §9 unaffected
• No new MCP tool
• New convention is documented — ADR-0019 gains a line (the one-line doc delta in the diff), which is the correct home since §10.5 owns the member contract

Findings: All checks pass — no integration gaps. Placement verified against siblings: derivePlaneMemberPaths sits at planeConfig.mjs:245, between collectPlaneMembers (:190) and assertPlaneMemberCoherence (:320) — the three-function member contract now reads in dependency order.

📋 Required Actions

No required actions — eligible for human merge.

Two polish items, neither blocking and both fine to fold or decline:

1. Past-tense the graphProd example in the derivePlaneMemberPaths JSDoc (it now has a declared membership, set by this PR).
2. Update #15932's Contract Ledger row 3 from "absent = non-member" to the shipped "absent + plane-anchored = fail closed."

And one thing that is not for this PR: #15872 should gain an explicit AC that flipping graphProd's planeMember is part of its closure, so the deferral expires mechanically rather than by memory.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — placement verified against siblings in the same file, in dependency order; pure and injectable with no Neo import, matching collectPlaneMembers. 4 deducted because the two-state planeMember boolean cannot express the deferral the PR itself needed, forcing a real third state into prose.
• [CONTENT_COMPLETENESS]: 92 — JSDoc is genuinely excellent: it states the invariant, names the failure class, and explains why the walk reads metadata rather than resolved values. 8 deducted for the one drift (the graphProd example the PR's own diff invalidated).
• [EXECUTION_QUALITY]: 97 — I actively attacked the isLeaf predicate and the fail-closed path and could not break either; the empty-string planeMemberReason guard is a correctness detail that is easy to miss. 3 deducted for the ledger/implementation divergence being undeclared, even though it runs in the safe direction.
• [PRODUCTIVITY]: 100 — #15932 offered two shapes and asked the implementer to pick during intake. It picked the larger, better one (membership on the leaf), and delivered the constraint the ticket named as the reason not to fully derive.
• [IMPACT]: 82 — converts a self-referential guard into a real invariant across three config bases, and closes the direction that empirically fails. Below architectural-pillar because it changes no runtime behavior when the lists are already complete.
• [COMPLEXITY]: 64 — one recursive walk with three declarative branches and clear failure messages; the load comes from needing ADR-0019 §10.5's placed-vs-resolved distinction in your head to see why the anchor test is a trigger and not a definition.
• [EFFORT_PROFILE]: Quick Win — ~91 lines of mechanism plus its spec, removing a whole class of silent omission across three files.

The thing worth keeping from this PR is the discipline of making an exclusion cost something. Requiring a reason on planeMember: false means the escape hatch leaves a record, and that record is what let me find the one real residual in ten minutes instead of never — the deferral on graphProd is legible precisely because the mechanism refused to let it be silent. That is a pattern worth reusing anywhere we have an opt-out.

Reviewed by Ada (@neo-opus-ada, Claude Opus 5, Claude Code) at c89997cf09. Cross-family (Kimi-authored / Claude-reviewed). Session 5664b1bb-99b3-4e0f-b464-163a1c8bfb16.