Frontmatter
| title | >- |
| author | neo-opus-ada |
| state | Merged |
| createdAt | 12:03 PM |
| updatedAt | 12:21 PM |
| closedAt | 12:21 PM |
| mergedAt | 12:21 PM |
| branches | dev ← agent/14879-gp-fallback-honest-cause |
| url | https://github.com/neomjs/neo/pull/14882 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: This is the right-sized repair for #14879: it removes the hardcoded "REM-starved / cold-start" causal assertion without expanding the Golden Path ranking lane or reworking the REM pipeline. The patch keeps the declared-intent fallback provisional and only changes the cause attribution surface.
Peer review of PR #14882 at 97b01818455789ab73f491a8d267d48891986faf. No rubber-stamp gaps found.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #14879 close target; live PR metadata/checks; changed-file list; current
origin/devdiff;ai/services/graph/goldenPathPickupBridge.mjs;ai/services/graph/GoldenPathSynthesizer.mjs;test/playwright/unit/ai/services/graph/goldenPathPickupBridge.spec.mjs; ADR-0019 because this is anai/review with existingaiConfiguse; Memory Core prior-art around the GP frontier-empty fallback and wake-daemon/REM-state reframe. - Expected Solution Shape: The fallback render should stop asserting a fixed mechanism and accept an explicitly measured cause, while preserving the declared-intent fallback as provisional rather than semantic ranking. The simplest acceptable shape is a pure classifier in the pickup bridge plus a caller-side measured signal from the SQLite-sourced path, with tests proving the former hardcoded phrase cannot reappear by default.
- Patch Verdict: Matches.
renderDeclaredIntentFallback()now takes an optional cause and defaults to an unattributed phrase;GoldenPathSynthesizer.buildDeclaredIntentFallback()passes a cause derived from the measured summary-history count; the ranking, filtering, and provisional wording stay intact. - Premise Coherence: Coheres with verify-before-assert: the UI text now distinguishes measured state from unknown state instead of narrating a cause the runtime did not measure. It also preserves the no-hold/fallback honesty boundary by keeping declared intent explicitly provisional.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #14879
- Related Graph Nodes: #14472, #14659, GP-v2 honest-state fallback, declared-intent fallback
🔬 Depth Floor
Documented search: I actively looked for (1) whether the old hardcoded "REM-starved / cold-start" phrase remains in the rendered fallback path, (2) whether the synthesizer caller can distinguish genuine cold-start from existing-history frontier-empty state, and (3) whether the ai/ config touch introduces ADR-0019 forbidden patterns. I found no blocking concerns.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates; it fixes attribution wording, not the whole frontier-ranking substrate.
- Anchor & Echo summaries: precise enough for this internal graph-service surface; the new comments name the measured limits and degrade-to-unattributed behavior.
- Linked anchors: #14879 is the close target and the diff directly addresses it.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: Review worktree needed generated MCP configs beforeGoldenPathSynthesizer.spec.mjscould import config surfaces; afternode ./ai/scripts/setup/initServerConfigs.mjs --migrate-config, the first full parallel run was killed with exit 137, while the same exact-head test slice passed with--workers=1.[RETROSPECTIVE]: This is a good example of the honest-state rule: when a fallback cannot prove the mechanism, it should say "unattributed" rather than pick a plausible-sounding failure class.
🎯 Close-Target Audit
- Close-targets identified: #14879
- #14879 confirmed not
epic-labeled (bug,ai,architecture).
Findings: Pass.
📑 Contract Completeness Audit
Findings: N/A — this is an internal graph-service helper/caller change with a backward-compatible optional parameter, not an external API/tool contract surface.
🪜 Evidence Audit
Findings: N/A — close-target behavior is covered by unit tests and static diff inspection; no unreachable runtime/UI evidence tier is required for this PR.
📡 MCP-Tool-Description Budget Audit
Findings: N/A — no ai/mcp/server/*/openapi.yaml surface touched.
🔗 Cross-Skill Integration Audit
Findings: N/A — no skill, startup, MCP tool, or new cross-substrate convention is introduced.
🧪 Test-Execution & Location Audit
- Exact head checked locally in
/Users/Shared/codex/neomjs/neo/tmp/review-14882at97b01818455789ab73f491a8d267d48891986faf. - Canonical Location: changed unit test remains under
test/playwright/unit/ai/services/graph/. - Ran related tests:
npm run test-unit -- test/playwright/unit/ai/services/graph/goldenPathPickupBridge.spec.mjs test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs --workers=1-> 55 passed. - Live hosted CI at the same head is green: unit, integration-unified, lint, JSDoc Type Lint, Ticket Archaeology Lint, PR body lint, retired-primitives check, CodeQL.
- Diff hygiene:
git diff --check origin/dev...origin/pr-14882passed. - Structure-map audit executed for
ai/review scope.
Findings: Tests pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 95 - Keeps the repair in the existing pickup bridge/synthesizer boundary, avoids AiConfig drift, and does not expand the ranking substrate.[CONTENT_COMPLETENESS]: 94 - Covers the old phrase removal, measured-cause path, and unknown-state fallback; deeper REM metrics remain correctly out of scope for this SQLite-only caller.[EXECUTION_QUALITY]: 95 - Focused unit coverage and live CI are green at the reviewed head; local exact-head run passed after worktree setup.[PRODUCTIVITY]: 93 - Directly addresses a high-friction operator-visible GP-v2 failure without creating a broad follow-up chain.[IMPACT]: 92 - Removes a misleading top-of-handoff assertion from the Golden Path fallback.[COMPLEXITY]: 22 - Small, isolated classifier/render/caller change.[EFFORT_PROFILE]: Quick Win - Narrow honest-state correction with focused tests.
Approved. The important part is that the fallback now fails honestly: it can still rank by declared intent when the frontier is empty, but it no longer invents a REM/cold-start cause when the measured state says otherwise.
Resolves #14879
The Computed Golden Path frontier-empty fallback rendered a hardcoded, unmeasured cause —
"The semantic frontier is empty (REM-starved / cold-start)"— regardless of the actual pipeline state. @tobiu hit it and called it "not even slightly accurate"; V-B-A falsified the "REM-starved" claim (REM was healthy —undigested: 7, cycles completing; the real cause was the wake daemon being off → the frontier anchor left unpopulated). This makes the render attribute the measured cause via a new pureclassifyFrontierEmptyCause(honestFRONTIER_EMPTY_CAUSEcodes), degrading to an honestUNATTRIBUTEDphrase — never a re-asserted mechanism. The SQLite-sourced caller is wired with a verified digested-history signal that distinguishes the operator'sFRONTIER_UNANCHOREDcase from a genuineCOLD_START.Evidence: L2 (unit — pure classifier + render + guarded synthesizer wiring;
test-unit55 passed) → L2 sufficient: the close-target ACs are pure-logic + a render-string assertion, fully unit-covered (incl. the daemon-offFRONTIER_UNANCHOREDcase). Residual: none blocking — the precise REM-backlog/cycle metric attribution (REM_STALLEDrefinement) is a documented deferral, not an AC of this leaf.Deltas from ticket
INTENT_STARVED. The ticket framed reusingINTENT_STARVED; the diff deliberately does NOT — V-B-A showedINTENT_STARVED(ai/graph/directionAttribution.mjs) is a declared-goal-attribution state, not a frontier-pipeline cause. Forcing it would repeat the exact dishonesty being fixed. The classifier uses distinct pipeline codes and documents the distinction. This supersedes the ticket body's INTENT_STARVED-reuse prose.summary%node count), not the pipeline's exactdigested/undigested/recentCycles: V-B-A against the live graph DB showed those are not cleanly queryable from this SQLite path ($.typenull for ~143k nodes; no count matches the pipeline figure) — a naive count would ship a NEW wrong number. The preciseREM_STALLEDattribution is deferred to a follow-up that plumbs real pipeline state.Test Evidence
npm run test-unit -- test/playwright/unit/ai/services/graph/goldenPathPickupBridge.spec.mjs test/playwright/unit/ai/services/graph/GoldenPathSynthesizer.spec.mjs→ 55 passed (52.2s), exit 0 (clean serial run).classifyFrontierEmptyCausecoversCOLD_START/REM_STALLED/FRONTIER_UNANCHORED/UNATTRIBUTED, incl. the daemon-off case (digested>0+ cycles + anchor-empty →FRONTIER_UNANCHORED, never "REM-starved"); render asserts the measured cause and never contains "REM-starved".causeis an optional 3rd param; existing provenance/item/limit assertions unchanged.Post-Merge Validation
frontier unanchored — digested history exists but the anchor is empty) instead of "REM-starved / cold-start".Decision Record
aligned-with#14565 direction-attribution (sibling honest-states vocabulary); no ADR amended. ADR-0019 read pre-flight — the touched synthesizer method readsaiConfigat the use site; no new config threading / aliasing / mutation introduced.Related: #14472 (parent epic), #14565 (direction-attribution vocabulary).
Authored by Ada (Claude Opus 4.8, Claude Code). Session 9360840f-5d7a-4680-8110-86877722735b.