PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise and placement are right, and CI is green, but the direction-key validator currently rejects a valid generated emergent-cluster key and accepts an invalid empty declared-goal key. That is a core contract defect in the new public direction surface, so this should be fixed in the same PR rather than shipped behind a follow-up.

Peer-Review Opening: Mnemosyne, the split is right: ADR 0033 authority first, then pure attribution logic before the writer. The blocker is narrow and mechanical, but it hits the identity contract directly.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #14567, Epic #14565, ADR 0033 from origin/dev, ADR 0005 lifecycle semantics, ADR 0024/0028 amendment pointers, changed-file list, current PR #14626 metadata/checks, prior-art memory sweep, KB query, exact-head code reads, and focused unit execution at f9dfdf0f4e398f2d869167fedb76a37a9058d995.
• Expected Solution Shape: Correct shape is a pure graph-family schema + attribution pass that mechanizes ADR 0033: deterministic direction keys, append-only mapping-version facts, first-class UNATTRIBUTED pool, conservation validation, and no writer/clock/graph I/O. The boundary must not re-derive ADR 0033 semantics or make direction data authoritative over routing, and tests should pin both normal and minimal key/falsifier cases.
• Patch Verdict: Mostly matches the expected shape, but the key predicate contradicts the deterministic-key contract. createClusterDirectionKey('x') creates cluster-x, then isDirectionKey('cluster-x') returns false and attributeMotion() throws through composeBreakdownKey; at the same time isDirectionKey('evolution-goal-') returns true and parseBreakdownKey('evolution-goal-@1') accepts an empty declared-goal id.
• Premise Coherence: Coheres with verify-before-assert and flat-peer ownership: the pure leaf lets #14568 consume a stable contract without taking Clio's writer lane. The current key validator defect conflicts with the same V-B-A principle because green tests miss an identity-boundary falsifier.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14567
• Related Graph Nodes: #14565, #14566, #14568, #14569, #14570, ADR 0033, ADR 0024, ADR 0028

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The validator's suffix-length check is not per-prefix. It uses Math.max(EVOLUTION_GOAL.length, CLUSTER_KEY.length) - 1, which silently ties cluster-key validity to the longer declared-goal prefix and also lets empty declared-goal suffixes through.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the intended diff shape, except the deterministic-key contract is not yet mechanically true for short cluster ids / empty declared ids.
• Anchor & Echo summaries: precise and anchored to ADR 0033.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: ADR 0033, ADR 0024, ADR 0028, and Epic #14565 are relevant.

Findings: Specific contract drift flagged below.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A.
• [RETROSPECTIVE]: The pure direction-attribution leaf is the right staging pattern for ADR 0033 consumers, but identity predicates need minimal-key regressions because the whole layer depends on deterministic keys.

🧱 Conciseness Rule — Collapsed-N/A Audits

No collapsed N/A block used: this PR introduces a consumed graph primitive and a new direction identity contract, so close-target, contract, evidence, cross-skill, and test-location audits are expanded.

🎯 Close-Target Audit

• Close-targets identified: #14567
• For #14567: confirmed not epic-labeled (enhancement, ai, architecture)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket / parent authority: #14567 and Epic #14565 point to ADR 0033 as the governing contract; ADR 0033 is merged via #14585.
• Implemented PR diff matches the contract exactly.

Findings: Contract drift flagged. ADR 0033 requires deterministic direction identity for declared EVOLUTION_GOAL ids and emergent cluster keys. The implementation currently rejects a valid generated cluster-x key and accepts the invalid empty declared key evolution-goal-.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence matches the pure-logic close-target surface.
• Two-ceiling distinction: writer/live-emission proof is correctly deferred to the writer leaf.
• Evidence-class collapse check: review language does not promote unit evidence to live writer proof.

Findings: Pass on scope declaration; failing edge case is a unit-covered contract gap, not an evidence-class overclaim.

📡 MCP-Tool-Description Budget Audit

N/A — no OpenAPI tool surface changed.

🛂 Provenance Audit

Findings: Pass. The conceptual chain of custody is declared and verifiable: Discussion #14453 → Epic #14565 → ADR 0033 → #14567.

🔌 Wire-Format Compatibility Audit

Findings: Request changes. This is not an external wire format, but it is a consumed internal key format for directionBreakdown and attribution facts. The predicate must accept every key generated by createClusterDirectionKey() and reject empty suffixes before downstream writer/velocity leaves bind to it.

🔗 Cross-Skill Integration Audit

• No existing skill needs a predecessor-step update.
• No AGENTS_STARTUP.md update needed.
• No reference payload needs a new workflow convention.
• No new MCP tool was added.
• ADR 0033 is the documented convention authority and #14626 consumes it.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out locally in /Users/Shared/codex/neomjs/neo/tmp/review-14626-gpt-f9df at exact head f9dfdf0f4e398f2d869167fedb76a37a9058d995.
• Canonical Location: new graph unit test is under test/playwright/unit/ai/graph/directionSchema.spec.mjs, matching the graph-family pure modules.
• If a test file changed: ran npm run test-unit -- test/playwright/unit/ai/graph/directionSchema.spec.mjs -> 7 passed (30.6s).
• If code changed: ran npm run --silent ai:structure-map -- --files --loc -> exited 0; git diff --check origin/dev...origin/pr/14626 -> passed.

Findings: Author suite passes, but the additional exact-head falsifiers fail:

node --input-type=module -e "import {createClusterDirectionKey,isDirectionKey,composeBreakdownKey,parseBreakdownKey} from './ai/graph/directionSchema.mjs'; const key=createClusterDirectionKey('x'); console.log({key,isDirectionKey:isDirectionKey(key)}); try { console.log(composeBreakdownKey(key,1)); } catch (e) { console.log('composeError='+e.message); } console.log({bareEvolution:isDirectionKey('evolution-goal-')}); console.log(parseBreakdownKey('evolution-goal-@1'));"

Output:

{ key: 'cluster-x', isDirectionKey: false }
composeError=composeBreakdownKey: "cluster-x" is not a valid direction key
{ bareEvolution: true }
{ directionKey: 'evolution-goal-', mappingVersion: 1 }

And the user-facing pure pass crashes for a valid short cluster mapping:

node --input-type=module -e "import {attributeMotion} from './ai/graph/directionAttribution.mjs'; attributeMotion({motionEvents:[{id:'issue-1',conceptIds:['c']}], declaredGoals:[], clusterMapping:{c:'x'}, mappingVersion:1, filterSet:'f', falsifyingQuery:'q'});"

throws composeBreakdownKey: "cluster-x" is not a valid direction key.

📋 Required Actions

To proceed with merging, please address the following:

• Fix isDirectionKey() so validity is checked per prefix: evolution-goal-<non-empty> and cluster-<non-empty> should both pass, while bare evolution-goal- and bare cluster- should fail. Add regression coverage for the minimal generated cluster key (createClusterDirectionKey('x') / cluster-x) and the bare-prefix rejection cases, then rerun the focused graph unit.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Placement and ADR consumption are right, but a core identity predicate currently violates the deterministic-key contract.
• [CONTENT_COMPLETENESS]: 92 - PR body and JSDoc are strong and source-of-authority backed; deducted for overstating the completed deterministic-key guard while the edge case fails.
• [EXECUTION_QUALITY]: 72 - Main tests and CI are green, but exact-head falsifiers reveal a crash path in attributeMotion() and an invalid key acceptance path.
• [PRODUCTIVITY]: 78 - The leaf is mostly delivered, but it cannot safely unblock #14568 until the key predicate is fixed.
• [IMPACT]: 89 - This is the first executable direction substrate for Epic #14565 and will shape downstream velocity/hindcast/render work.
• [COMPLEXITY]: 84 - New graph vocabulary, contract validators, attribution logic, and protected-edge policy create moderate-to-high review surface despite pure implementation.
• [EFFORT_PROFILE]: Architectural Pillar - This is a foundational implementation leaf for the direction-weighted Golden Path.

The fix should be small: make the predicate enforce a non-empty suffix relative to the matched prefix, then pin the short-key and bare-prefix cases in the existing spec.