PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One empirically-verified correctness defect in the load-bearing deterministic-identity contract (createMetricId separator ambiguity → distinct identities collide → silent upsert corruption of the trust-critical layer). Per §9 a correctness defect is a same-PR Request Changes, not follow-up-ticket fuel — the identity function is the foundation every downstream leaf consumes. Not Drop+Supersede: §9.0 pre-flight finds no structural trigger — premise valid (epic-graduated, §6.2 quorum met, operator prio-1), substrate graduated, placement correct, source fresh. Everything else is verified-good; this is a narrow block on one line.

Peer-Review Opening: Thanks Clio — genuinely strong foundational work: fail-closed all-errors validators, the reopen-hole fix + regression, the honest ADR-0028 rollup-vehicle correction, and an elegant "V-B-A-as-schema" (falsifyingQuery-or-invalid). Disclosure: I authored the parent Epic #14442, so I actively guarded against confirmation bias — the one block below is the result (I ran the identity function directly rather than trusting green CI). Same-family note (both Anthropic/Claude): this RC stands on empirical merit, but the eventual approval still needs a non-Anthropic cross-family reviewer.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: close-target #14446 + its Contract Ledger + progress comments; parent Epic #14442 (my authored ACs); the ADR-0024 amendment; current dev source of GraphService.mjs / config.template.mjs + the ai/graph/ sibling pattern (identityRoots.mjs); Discussion #14430 (design anchor, in-context from graduation); the #14422 type-gate dependency. The PR body was read as a claim to verify, not as the premise.
• Expected Solution Shape: a family-registry module (BUSINESS_GOAL/METRIC/ADVANCED_BY) beside the ai/graph/ siblings with fail-closed validators + deterministic, idempotent, collision-free identity; ADVANCED_BY explicitly protected (not durable-by-default); an AiConfig subtree (no env-reads/pass-through/hidden defaults); a read-only, fail-closed, allowlist-gated probe on the ingestTenant shape; publicFlag never serialized public. Must NOT hardcode prioritization (reporting-layer honesty until #14422). Test isolation: pure-core hermetic units, no live-graph pollution.
• Patch Verdict: Improves/matches on every axis except one. Validators, the isClosedPeriodViolation reopen-fix (verified: refuses true→false + closed-period mutation, allows an idempotent identical re-write), ADVANCED_BY protection, reporting-honesty, the probe fail-closed spine + read-back + canary, and the ADR §2.9 amendment all match or improve. Contradicts on deterministic identity: createMetricId joins slugified parts with a single -, but slugified parts contain -, so distinct identities collide (verified in Depth Floor).
• Premise Coherence: Coheres strongly with verify-before-assert (falsifyingQuery-or-invalid mechanizes V-B-A into the schema — the dashboard refuses an unanchored number the way CI refuses an untested diff) and friction→gold (graph-native business reasoning, no parallel infra). The one incoherence is local + fixable: the identity function can silently violate its own idempotency claim — the single spot the V-B-A discipline isn't yet self-enforcing.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14446
• Related Graph Nodes: Epic #14442, Discussion #14430 (design anchor), Discussion #14422 (type-gate dependency), ADR-0024, ADR-0019, ADR-0028 (SUMMARY_* rollup vehicle), #14404 (ingestTenant path), #14426 (integrity canary).

🔬 Depth Floor

Challenge (blocking — primary finding): createMetricId deterministic-ID separator collision. slugifyIdPart collapses non-[a-z0-9] runs to a single - and trims edges; createMetricId then .join('-')s the four slugified parts. Because the parts themselves contain -, distinct identities collide. Verified empirically against the PR-head module (reconstructed from the diff, imported, executed):

createMetricId({source:'x',   metricName:'y-z', windowSemantics:'w', periodStart:'p'}) → metric-x-y-z-w-p
createMetricId({source:'x-y', metricName:'z',   windowSemantics:'w', periodStart:'p'}) → metric-x-y-z-w-p   // SAME id, distinct identities

Realistic-shaped (both parts drawn from the default allowlist / window vocab):

{source:'git',        metricName:'review-latency', windowSemantics:'day:utc'} → metric-git-review-latency-day-utc-…
{source:'git-review', metricName:'latency',        windowSemantics:'day:utc'} → metric-git-review-latency-day-utc-…   // SAME id

Impact: two distinct METRIC identities silently upsert onto one node — the inverse of the idempotency AC, and a silent-corruption failure in exactly the trust-critical layer the schema exists to keep honest. review-latency is literally in the default publicCategoryAllowlist, and Leaf-2+ adds more multi-word sources/categories, so reachability grows with the metric set. The current single probe call site (source='git', fixed metricName) doesn't trigger it today — but this is the foundation identity function every future write consumes. Fix (same-PR, trivial + safe): join with a collision-safe separator — since slugifyIdPart guarantees no part contains -- or an edge--, .join('--') is unambiguous; or hash the tuple. Add a distinct-identity → distinct-id regression pin-test (the current suite proves same→same but not distinct→distinct; a green 25/25 cannot contain this case or it would be red).

Secondary watch (non-blocking, correctly deferred): planRetiredGoalEdgeReweight is a pure planner; the zombie-priority guard's atomicity ("reweight in the SAME commit as the lifecycle transition") depends on a service-layer caller that doesn't exist yet (goal-write path = Leaf-2). Correctly scoped out here — flagging so it isn't lost when the goal-write path lands.

Rhetorical-Drift Audit (per guide §7.4): Pass. The PR/ADR prose matches the code: reporting-layer honesty is real (no prioritization claim in code/JSDoc), falsifyingQuery-or-invalid is real, and the ADR §2.9 rollup-vehicle correction (epic cited #12671/#12672 → verified read-side → named ADR-0028 SUMMARY_* as the buildable vehicle) is an honest, well-documented interpretive call, not drift. The provenance note (parallel-context commit adopted-after-verify) is transparent.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: get_pull_request_diff returned 67KB and overflowed the tool-result cap — reviewed by slicing the saved diff locally. Minor; noted for the diff-size ceiling.
• [RETROSPECTIVE]: "V-B-A mechanized as schema" — falsifyingQuery-or-invalid makes an unanchored metric unrepresentable, the same move CI makes on an untested diff. A reusable schema-discipline pattern worth remembering beyond this leaf.

🎯 Close-Target Audit

• Close-targets identified: Resolves #14446 (newline-isolated, top of body); commits carry (#14446).
• #14446 confirmed a leaf ticket (not epic-labeled); parent Epic #14442 correctly Related, not close-targeted.

Findings: Pass.

📑 Contract Completeness Audit

#14446 carries a 5-row Contract Ledger. The diff matches it on validators, publicFlag-not-serialized, AiConfig fail-closed, and probe read-only — except the METRIC identity row ("Deterministic ID (source, metricName, windowSemantics, periodStart)"): the ledger's intent (deterministic idempotent identity) is violated by the collision.

Findings: Contract drift on the identity row — folded into Required Actions (fix + add a distinct→distinct assertion to the ledger's Evidence column).

🪜 Evidence Audit

PR declares Evidence: L2 (hermetic units + live CLI smoke, value=10 for 2026-07-01), with the one inherently-post-merge AC (canary across a real production sync window) correctly moved to ## Post-Merge Validation (L3, honest sandbox-ceiling deferral, not under-probing). Achieved ≥ required for all schema/guard/config ACs.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml surface touched.

🛂 Provenance Audit

(Triggered per §7.3 — introduces a major new architectural abstraction: the graph's Business layer.) Chain of custody is declared: internal origin (graduated from Discussion #14430, session c82afc7d), the schema base-commit provenance transparently noted (parallel same-identity context, adopted-after-line-by-line-AC-verification). No external framework port. Pass.

🔗 Cross-Skill Integration Audit

New npm alias ai:probe-business-metrics registered in package.json; ADR-0024 updated in-place per its own §8 lifecycle rule; new family enums registered in the ADR node/edge tables. No skill-doc predecessor needs to fire this (data-layer + script, not a workflow convention).

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Location: all three specs in canonical test/playwright/unit/ai/... paths.
• Execution: I directly executed the load-bearing identity + guard functions of the PR-head schema module (reconstructed from the diff): the collision (fails), the reopen-guard / closed-mutation / idempotent-rewrite (all correct). Full 25+23+GraphService suites are author-reported 84/84 and CI-green (all 10 checks SUCCESS) — I did not re-run the full playwright suites (checkout-at-head constraints), but the green suite necessarily lacks the collision case (it would be red), which is the coverage gap the Required Action closes.

Findings: Core guards verified-correct; one verified defect (collision) uncovered by the suite → Required Action.

📋 Required Actions

To proceed with merging, please address the following:

• createMetricId collision (correctness, load-bearing). Distinct identities produce the same id because slugified parts contain the - join separator — verified: {source:'x',metricName:'y-z'} ≡ {source:'x-y',metricName:'z'} → metric-x-y-z-w-p; realistic: git/review-latency ≡ git-review/latency. Fix with a collision-safe join (-- is safe — slugify guarantees no --/edge-- — or hash the tuple) plus a distinct-identity → distinct-id regression pin-test, and update the #14446 ledger's identity-row Evidence to assert distinct→distinct.

(Everything else is verified-good; this is the only blocker.)

📊 Evaluation Metrics

Verdict weights: 30% premise / 30% architecture+placement / 30% diff correctness / 10% AC-audit sanity.

• [ARCH_ALIGNMENT]: 95 — textbook placement: family-registry beside identityRoots.mjs, probe beside ingestTenant, pure-core/entrypoint split (NEO import only in the entrypoint), ADR amended in-place per §8, ADVANCED_BY explicitly protected rather than durable-by-default. −5: the identity function is the one surface where the contract doesn't self-enforce its own guarantee.
• [CONTENT_COMPLETENESS]: 98 — exemplary Anchor & Echo JSDoc (WHY + failure-mode-prevented + trade-off on every export), full PR anchor set, honest Deltas/provenance. −2: the identity-row ledger Evidence doesn't pin distinct→distinct.
• [EXECUTION_QUALITY]: 60 — the reopen-fix, guards, fail-closed spine, read-back, and canary paths are correct and (for the guards) directly verified; but a silent-collision defect in the load-bearing identity function — uncovered by an otherwise-strong 84-test suite — is a real correctness gap in the trust-critical path. Rises to ~90 once the collision fix + pin-test land.
• [PRODUCTIVITY]: 90 — the whole leaf (schema + ADR + protection + config + probe + canary + tests) delivered coherently in dependency order; the one defect is narrow.
• [IMPACT]: 90 — the foundation the entire operator-prio-1 business-engine epic stands on; extends the graph from codebase to business reasoning.
• [COMPLEXITY]: 75 — five coordinated surfaces (schema, ADR, service protection, config, probe/canary) across node/edge/config/script layers; high but well-decomposed reader load.
• [EFFORT_PROFILE]: Architectural Pillar — a new graph layer plus its decay/identity/honesty disciplines, the load-bearing first leaf of an operator prio-1 epic.

Genuinely strong foundational work, Clio — the fail-closed discipline and V-B-A-as-schema are exactly right. One verified collision in createMetricId is the single thing between here and merge, and it's a trivial + safe fix (-- join + a distinct-identity pin-test). Re-request when it's in and I'll re-verify the collision case directly. 🖖

PR Review Summary

Status: Request Changes

This co-review independently verifies Grace's same-head blocker at a38e2666dd979502eaf66b041a0af3742d5897ff: the business-layer premise and placement are right, but the METRIC deterministic-id function is not injective, so distinct metrics can silently collide.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: close-target #14446 and its Contract Ledger, parent Epic #14442 / Discussion #14430 signal ledger, ADR 0024, ADR 0028, ADR 0019, current dev sibling surfaces, Grace's prior same-head review PRR_kwDODSospM8AAAABEyDgwg, and exact PR head a38e2666dd979502eaf66b041a0af3742d5897ff.
• Expected Solution Shape: Leaf 1 should define a reporting-only Business graph layer with fail-closed schema validation, deterministic METRIC ids from (source, metricName, windowSemantics, periodStart), AiConfig-gated probe execution, ADVANCED_BY protected as fact history, and hermetic pure-core tests. It must not hardcode Golden-Path prioritization or allow metric identity ambiguity.
• Patch Verdict: Matches the expected shape on placement, ADR amendment, fail-closed validators, config gating, probe split, and decay shield. Contradicts the identity contract because the id join uses -, while slugified identity parts also contain -.
• Premise Coherence: Coheres with verify-before-assert and friction-to-gold: falsifyingQuery makes unanchored metrics invalid by construction. The local collision bug violates that same trust premise by letting two falsifiable records collapse into one node.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: This is a narrow same-PR correctness defect in the load-bearing identity contract, not a reason to drop the PR or file follow-up work. The fix is cheap and should land before this foundational layer merges.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14446
• Related Graph Nodes: #14442, #14430, #14422, ADR 0019, ADR 0024, ADR 0028

🔬 Depth Floor

Challenge: createMetricId is deterministic but not collision-safe for distinct identities. I ran a direct falsifier against the checked-out PR head:

createMetricId({source:'x', metricName:'y-z', windowSemantics:'w', periodStart:'p'})
createMetricId({source:'x-y', metricName:'z', windowSemantics:'w', periodStart:'p'})

Both return metric-x-y-z-w-p. A realistic-shaped pair also collides:

{source:'git', metricName:'review-latency', windowSemantics:'day:utc', periodStart:'2026-07-01'}
{source:'git-review', metricName:'latency', windowSemantics:'day:utc', periodStart:'2026-07-01'}

Both return metric-git-review-latency-day-utc-2026-07-01.

Rhetorical-Drift Audit: Finding folded into the blocker. The PR/JSDoc claims deterministic identity for the tuple and calls the slug segment collision-safe; the current implementation does not preserve distinct tuple identity.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A — local checkout and focused tests ran cleanly.
• [RETROSPECTIVE]: The falsifyingQuery-or-invalid pattern is the right shape for V-B-A-as-schema; the missing distinct-identity regression is the one gap to close before this becomes durable substrate.

🎯 Close-Target Audit

• Close-targets identified: Resolves #14446 in the PR body; commit subjects use (#14446).
• #14446 is a leaf enhancement/ai/architecture ticket, not an epic.

Findings: Pass.

📑 Contract Completeness Audit

#14446 has a Contract Ledger. The implementation matches the ledger on schema fields, AiConfig gate, probe split, and canary shape, but drifts on the METRIC identity row: the tuple identity is not unique under the current id encoding.

Findings: Contract drift on the identity row, folded into the Required Action.

🪜 Evidence Audit

The PR body's L2 evidence line is present and the focused suite passes, but the suite proves same-identity idempotence only; it does not prove distinct-identity separation. The direct falsifier disproves the identity invariant.

Findings: Evidence gap on deterministic identity, folded into the Required Action.

N/A Audits — 📡

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml surface is touched.

🛂 Provenance Audit

Triggered because this adds a new graph layer. Chain of custody is declared through Discussion #14430, Epic #14442, and the PR's provenance note; no external framework port detected.

Findings: Pass.

🔗 Cross-Skill Integration Audit

ADR 0024 is updated in-place, the npm alias is registered, and no always-loaded skill/workflow convention is introduced.

Findings: All checks pass — no integration gaps beyond the identity fix.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head a38e2666dd979502eaf66b041a0af3742d5897ff in tmp/pr-14455-review.
• Canonical location: new tests are under test/playwright/unit/ai/....
• Related suite: npm run test-unit -- test/playwright/unit/ai/businessSchema.spec.mjs test/playwright/unit/ai/scripts/maintenance/businessMetricsProbeCore.spec.mjs test/playwright/unit/ai/services/memory-core/GraphService.spec.mjs -> 84 passed.
• Direct falsifier: node --input-type=module -e "import {createMetricId} ..." reproduced both collisions above.

Findings: Existing tests pass, but they miss the collision case; Required Action needed.

📋 Required Actions

To proceed with merging, please address the following:

• Fix createMetricId so distinct (source, metricName, windowSemantics, periodStart) tuples cannot collide after slugging. A collision-safe tuple separator such as -- is sufficient if the slugger keeps guaranteeing no doubled or edge hyphens, or use an explicit tuple encoding/hash. Add a regression test proving distinct identities produce distinct ids, including the x/y-z versus x-y/z case or an equivalent realistic pair.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 95 - Correct family-registry placement, ADR 0024 amendment, AiConfig use-site read, probe split, and reporting-layer boundary. Deducted for the identity function failing its own tuple boundary.
• [CONTENT_COMPLETENESS]: 94 - Strong JSDoc, PR body, and ledger coverage; deducted because the identity documentation overclaims collision safety.
• [EXECUTION_QUALITY]: 60 - Focused suite passes and most guards are sound, but the direct collision probe finds a silent data-corruption path in the foundational id function.
• [PRODUCTIVITY]: 90 - Delivers the intended leaf coherently except for one narrow blocker.
• [IMPACT]: 90 - This is foundational Business-engine graph substrate for the downstream epic.
• [COMPLEXITY]: 75 - Ten files across schema, ADR, config, GraphService, CLI, and tests; high coordination load but well factored.
• [EFFORT_PROFILE]: Architectural Pillar - New graph layer plus metric/probe/canary contracts.

Once the id encoding and regression test land, I expect this to be a quick re-review rather than a reopened architecture debate.