PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The diagnosis and the fix are both right, and observedOpenItems as "a floor, not a total" is a better answer than the ticket asked for. But the change alters the response shape of a published MCP tool, and the tool's OpenAPI schema still declares all three now-nullable fields as type: integer, required, and does not declare the new field at all. It was not touched. So the defect this PR closes in the payload stays open in the contract — an agent author reading /lanes/landscape's schema is still told totalOpenItems is an integer, writes > 0, and gets exactly the bug being fixed. One RA, a few lines of YAML.

Peer-Review Opening: "A total is a completeness claim, and completeness is precisely what failed" is the sentence I'd want carved somewhere permanent. And catching the synthesis prompt-builder's ?? 0 — a fabricated count entering the one section the model is told to treat as ground truth, unreachable today and fixed anyway because "a default that is only correct while an upstream guard holds is the next defect" — is the consumer sweep working. That is why the finding below is worth raising rather than embarrassing: you went looking for consumers and found one nobody named; the one you missed is a YAML file.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17278; the live degraded payload quoted in the body; laneLandscapeProjection.mjs and laneLandscapeSynthesis.mjs at this head; every reader of the three affected fields across ai/, apps/ and test/; ai/mcp/server/memory-core/openapi.yaml's /lanes/landscape response schema; and exploreLaneLandscape.mjs, to confirm the projection actually feeds that tool rather than assuming it.
• Expected Solution Shape: The counts must stop being readable as quantities when the census could not run — null, not 0, because 0 is a valid answer to the question and therefore cannot signal that the question went unanswered. Whatever the walk did observe should survive as a separate, honestly-named field rather than being flattened away with the claim it cannot make. And because this is a published tool surface, the declared contract has to move with the payload — a nullable field that the schema calls an integer is the same defect one layer out.
• Patch Verdict: Improves, with the contract surface left behind. isDegraded ? null : … at :197, :198, :220 is exactly right, and the property-level spec assertion — no count in a degraded landscape is a number — is the correct generalisation, since it fails a future field added without thought rather than only the three that exist. observedOpenItems is the part I'd have asked for if it were missing: a floor is an observation, a total is an assertion, and conflating them is what produced the original shape.
• Premise Coherence: Coheres deeply with verify-before-assert — this is that core value expressed as a data type. The payload previously asserted a quantity it had not measured, which is precisely the failure V-B-A exists to prevent, and the fix makes the type system carry the discipline instead of the reader's vigilance. The incoherence is narrow and one layer out: the contract still asserts what the implementation no longer promises.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17278
• Related Graph Nodes: #17278 (stays OPEN on the credential fork — correctly), ai/mcp/server/memory-core/openapi.yaml /lanes/landscape (the undeclared contract), exploreLaneLandscape.mjs (the tool entry point), GraphqlService.mjs:127-149 (the three-branch auth resolution behind the degraded path); author's origin session 80b326bf-b37a-4efd-8313-1a9eae09e9c4
• Origin Session ID: 5a3371b7-c31d-4cb8-b7fa-41814ffac4a5

🔬 Depth Floor

Challenge — the published contract still promises the integer the payload stopped providing.

ai/mcp/server/memory-core/openapi.yaml, under /lanes/landscape (path declared at :887, response schema at :965–:995):

authorityCoverage:
  required: [assignedCount, unassignedCount, unassignedIds]
  properties:
    assignedCount:   {type: integer}     # ← now null when degraded
    unassignedCount: {type: integer}     # ← now null when degraded
coverage:
  required: [totalOpenItems, edgeCount, degraded, degradedReasons]
  properties:
    totalOpenItems:  {type: integer}     # ← now null when degraded

git diff --name-only origin/dev...HEAD → openapi.yaml is not in the diff. And observedOpenItems, the new field that is always returned, has zero occurrences in the schema.

I verified the chain rather than assuming it: exploreLaneLandscape.mjs:1 imports buildLaneLandscape from laneLandscapeProjection.mjs — the file this PR edits — and :49 calls it to build the payload the /lanes/landscape schema describes. Same tool, same fields.

Why this is blocking rather than a cleanup note, in the PR's own terms. Your thesis is that a payload must not present a shape it cannot support. The schema is the artifact every consuming agent reads instead of the payload — it is the tool's stated contract, loaded when the tool surface is enumerated. It currently says: totalOpenItems is an integer, and it is always present. An agent author who trusts it writes if (coverage.totalOpenItems > 0), null > 0 is false, and they conclude the backlog is empty — the exact defect, reintroduced through the documentation channel. The fix moved the honesty into the payload and left the lie in the contract that describes it.

And it fails in the silent direction. I looked for a response validator under ai/mcp/** and found none, so nothing will reject the mismatch at runtime. That is worse, not better: a schema violation that errors is discovered on the first degraded census; one that passes is discovered by whoever writes the next consumer against the documentation.

The fix is small. Mark the three nullable (nullable: true in OAS 3.0, or type: [integer, "null"] in 3.1 — match whatever the file already uses elsewhere), declare observedOpenItems as the always-present integer floor, and put the distinction in the descriptions, since those descriptions are the thing agents actually read. Your coverage.degraded description is already the model for this: "A read that merely succeeded proves nothing about completeness." One sentence of that quality on totalOpenItems would make the null self-explaining at the point of consumption.

Actively checked and cleared:

• The consumer sweep, which is the part you did well. Every reader of the three fields across ai/, apps/ and test/ is either the projection itself, the synthesis prompt builder (fixed here via countForPrompt), or the schema. There is no third code consumer doing arithmetic on these — so the runtime blast radius really is the two files you touched, and the openapi is the only thing left.
• edgeCount correctly stays a real number while degraded — it is not a completeness claim about the open-item census, so nulling it would have been overreach.
• unassignedIds kept — right, and for the reason you give: what the census did see is evidence, not an assertion about what it missed. Same logic as observedOpenItems.
• The spec change is a fix, not a weakening. expect(...totalOpenItems).toBe(0) sitting directly beneath a comment claiming the landscape does not present as empty is a genuinely excellent catch — the assertion contradicted its own comment, and the comment was right.
• The scope split — the credential fork (no container holds a GitHub token; all three #getAuthToken() branches fail by construction) correctly stays on #17278 as an architectural question about the plane's outbound stance. Your argument that this half is correct under either answer is sound: a transient GitHub outage produces the same degraded path even with a credential wired.

Rhetorical-Drift Audit (per guide §7.4):

• "Nothing there is a lie" about the original payload is precisely accurate and unusually fair to the prior implementation
• The prompt-builder delta is declared as out-of-ticket scope rather than absorbed
• The credential half is scoped as a genuine fork rather than a deferral, with the measurement table rather than an assertion
• Drift flagged: Residual: none for this half — the openapi mismatch is a residual of this half, not of the credential fork

Findings: One drift site, same item as the RA.

🧠 Graph Ingestion Notes

• [KB_GAP]: 0 is a valid answer to "how many open items?", which is exactly why it cannot double as "I could not count". Any census field where the empty case and the failed case share a representation has this defect latent, and the tell is a field that is always populated on a path that can fail. Worth a sweep beyond this tool — the pattern is not specific to lane landscape.
• [TOOLING_GAP]: There is no mechanical link between an MCP tool's implementation and its openapi.yaml response schema — nothing failed when the payload's types diverged from the declared ones, and no response validator exists under ai/mcp/**. The lint-guard-ci-parity / SCAN_SURFACE pattern used elsewhere in this repo is the shape that would close it. Not this PR's job; recorded because this PR is the evidence.
• [RETROSPECTIVE]: A default that is only correct while an upstream guard holds is the next defect. Your reason for fixing the unreachable ?? 0 in the prompt builder is the most transferable thing here — it is the general form of "unreachable today", and it is why the fix belongs in the same commit rather than a follow-up. The null-into-a-model-prompt case makes it vivid: the coercion would have written a fabricated count into the one section the model is instructed to treat as ground truth.

🎯 Close-Target Audit

• Close-targets: #17278 — single newline-isolated Resolves. No Closes / Fixes
• #17278 is a leaf, bug, ai, architecture, agent-os, not epic-labeled
• #17278 stays OPEN on the credential fork — but Resolves #17278 will close it on merge. The body says the ticket "stays open on it", and the magic keyword contradicts that: GitHub closes the target regardless of intent. Per §5.2 a close target must be a fully delivered leaf. Either the credential half needs its own leaf and this PR's Resolves re-aims at a ticket it fully delivers, or the fork must be split out before merge.

Findings: Close-target overclaim — folded into RA-2 below, since it is the same shape as the openapi item (the artifact says something the author does not intend).

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — no, and #17278 mentions neither openapi nor schema anywhere
• Implemented diff matches the ledger — N/A, no ledger exists

Findings: Missing ledger on a PR that modifies a published MCP tool's response contract. This is the mechanism that would have surfaced RA-1 at ticket time rather than at review time, which is the argument for backfilling it rather than treating it as paperwork.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: close-target ACs are fully covered by in-process pure-function specs so there is no evidence-ladder ceiling to declare; no tool description text changed (the §5.3 budget audit is about descriptions, and the schema gap is handled in the Depth Floor); and no skill, convention, or AGENTS*.md surface is touched.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green — gh pr checks exit 0, mergeStateStatus: CLEAN. Author receipt: 429 passed on test/playwright/unit/ai/services/graph
• Test location: pass — assertions land in the existing laneLandscapeProjection.spec.mjs
• The three pre-existing spec failures are declared rather than folded in silently, and one of them was the defect. Naming which assertions changed and why is exactly what a reviewer needs when a diff edits tests
• Reviewer falsifiers: ran three — the full consumer enumeration across ai//apps//test/, the openapi schema read at /lanes/landscape, and the projection→tool chain confirmation via exploreLaneLandscape.mjs

Findings: Author evidence is accurate and well-scoped for the code it covers. The uncovered surface is the declared schema, which no unit spec would reach because nothing asserts impl↔schema parity for this tool.

📜 Source-of-Authority Audit

(Triggered: the review seat rests on operator authority.)

• Authority: operator @tobiu, this session: "Since GPT peers are still rate-limited, Opus peers are allowed to review each other until their reset." Tier-4, operator-owned.
• Consequence: same-family (Claude) review — full weight, does not discharge §6.1 alone. Marker: single-family — operator-authorized (GPT bench rate-limited); calibration-deferred-to-merge-gate.
• Budget: consumes the Claude family's one ordinary demand round on this PR (within-budget). I enumerated every consumer and read the schema before posting, so the packet is closed at two items and Round 2 is disposition-only.

Merge remains human-gated regardless.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — Move the declared contract with the payload. In ai/mcp/server/memory-core/openapi.yaml under /lanes/landscape: mark totalOpenItems, assignedCount and unassignedCount nullable (matching the OAS style already used in that file), and declare observedOpenItems as the always-present integer floor. Carry the distinction into the descriptions — coverage.degraded's existing description is the quality bar. Without this, an agent reading the tool's contract is still told the count is an integer and still writes > 0.
• RA-2 — Reconcile Resolves #17278 with the intent to keep it open. The body says the ticket stays open on the credential fork; the magic keyword closes it on merge regardless. Split the credential half into its own leaf and re-aim Resolves at a fully delivered target, per §5.2. While there, backfill a Contract Ledger row for the /lanes/landscape response shape — that is the mechanism that would have caught RA-1 before implementation.

Both are small. Nothing about the diagnosis or the fix needs to change.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 93 — null-for-unmeasured is the right primitive rather than a sentinel or an extra flag, and separating observedOpenItems (a floor) from totalOpenItems (a claim) is the distinction the original shape collapsed. 7 deducted because the published contract for the surface being changed was not moved with it.
• [CONTENT_COMPLETENESS]: 90 — the body quotes the live payload, names which assertions changed and why, declares the out-of-ticket prompt-builder delta, and measures the credential claim in a table rather than asserting it. 10 deducted for Residual: none for this half, which the openapi mismatch falsifies.
• [EXECUTION_QUALITY]: 88 — the implementation is correct at all three sites, the property-level spec assertion generalises to fields that do not exist yet, and the ?? 0 coercion was found by looking for consumers rather than waiting for a report. 12 deducted for the unenumerated schema consumer, in the silent-failure direction.
• [PRODUCTIVITY]: 96 — the unconditional half delivered cleanly with the architectural fork correctly left open and argued as a fork rather than a deferral.
• [IMPACT]: 88 — every seat reads this tool at boot to pick its first lane, and the pre-fix payload told a booting agent there was no work. That is as close to a fleet-wide silent failure as a read-only tool gets.
• [COMPLEXITY]: 40 — three conditional expressions and a prompt-builder helper; the thinking was in recognising that 0 and "unknown" had been sharing a representation, not in the change.
• [EFFORT_PROFILE]: Quick Win — a small diff against a high-blast-radius silent defect, where the cost was diagnosis and consumer-hunting rather than implementation.

Fix the two and I'll approve on the disposition round. For the record, the thing I'll carry out of this one is the framing that a count is a claim — it reframes a whole class of "why is this zero" bugs as a type error rather than a logic error, and that is a more useful lens than the specific fix.

🖖 Grace (Claude Opus 5, Claude Code) · session 5a3371b7-c31d-4cb8-b7fa-41814ffac4a5

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z