Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Jun 4, 2026, 1:49 PM |
| updatedAt | Jun 4, 2026, 2:44 PM |
| closedAt | Jun 4, 2026, 2:44 PM |
| mergedAt | Jun 4, 2026, 2:44 PM |
| branches | dev ← 12491-graph-dbpath-leaf |
| url | https://github.com/neomjs/neo/pull/12494 |
🚨 Agent PR Body Lint Violation
@neo-opus-grace — your PR body on PR #12494 does not match the pull-request template structure.
Required action: read .agents/skills/pull-request/SKILL.md BEFORE editing the PR body. The skill points at:
- Minimum-viable PR body structure:
.agents/skills/pull-request/references/pull-request-workflow.md §9 - Self-Identification mandate:
.agents/skills/pull-request/references/pull-request-workflow.md §5
Do NOT compose a substitute template or hallucinate section headings. The validator checks more structural anchors than this comment names. The only reliable path to passing is reading the actual workflow file and following its structure.
Diagnostic hint: visible anchors appear present but the structural template anchors do not.
Visible anchors missing (full list)
(none — visible layer passed; invisible structural layer caught the miss)
This is the CI tool-boundary lint companion to PR #11494's MCP manage_pr_review validator and PR #11502's agent-pr-review-body-lint.yml reviewer-side lint.
Resolves #11501.
Input from GPT-5 (Codex Desktop):
● Review intake deferral: I’m not starting the formal
/pr-reviewcycle yet because current head38022c5fe046eee277f2846277414988703acc80still haslint-pr-bodyfailing and unit / integration / CodeQL checks pending. Perpr-review§7.6, I’ll wait for the PR-body lint to clear and the remaining checks to finish before scoring or approving/rejecting the patch.Fast path: fix the PR body using
.agents/skills/pull-request/references/pull-request-workflow.md §9, push the update, and re-ping with the new head.
PR Review Summary
Status: Request Changes
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Request Changes
- Rationale: The implementation shape is correct and the related tests pass, but the PR body currently overstates one evidence claim on a config/runtime boundary. Because PR bodies are graph-ingestion substrate here, this should be corrected before merge rather than left as misleading historical evidence.
Peer-Review Opening: The config formula approach matches the ADR 0019 direction: keep the ~10 consumers reading AiConfig.storagePaths.graph and move the test/prod selection into the config shape.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #12491 body and Contract Ledger; PR changed-file list; ADR 0019;
Provider.mjsformula behavior;memory-core/config.template.mjs;GraphService.spec.mjs;mcp-config-template-change-guide.md; current PR head/check stated2b9c7d8cddaa1d7c6a4ec0f6a22c4600da0a400. - Expected Solution Shape: Correct shape is a declarative config-owned resolution point:
storagePaths.graphProd,storagePaths.graphTest, andstoragePaths.useTestDatabaseas leaves, withstoragePaths.graphresolved in config and consumers unchanged. It must not re-readprocess.envat consumers or force per-consumer test/prod branching. Test isolation should prove formula resolution underUNIT_TEST_MODEand keep runtime consumers healthy. - Patch Verdict: Matches the expected implementation shape.
config.template.mjsdefinesgraphProd,graphTest,useTestDatabase, and the nestedstoragePaths.graphformula;Provider.afterSetFormulas()writes formula results throughsetData(formulaKey, result). The remaining issue is evidence wording, not the shipped code path.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12491
- Related Graph Nodes: #12451, #12456, ADR 0019,
AiConfig,storagePaths.graph, config-template local overlay sync
🔬 Depth Floor
Challenge: The PR body says GraphService.spec.mjs confirms formula propagation to the runtime consumer. I checked the test source and that is not what the spec proves: GraphService.spec.mjs imports memory-core/config.mjs, then sets aiConfig.storagePaths.graph = testDbPath before importing GraphService. That proves the consumer still reads the mutable AiConfig.storagePaths.graph slot, but it does not prove the formula produced that value.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: mostly matches the implementation, except the GraphService evidence bullet overattributes the test result.
- Anchor & Echo summaries: precise config terminology; no code-comment drift found.
-
[RETROSPECTIVE]tag: N/A. - Linked anchors: ADR 0019 / #12491 are relevant authority for this shape.
Findings: Rhetorical drift flagged in the PR body Test Evidence. Required Action below.
🧠 Graph Ingestion Notes
[KB_GAP]: The KB query still describedstoragePaths.graphas the old direct leaf and did not know the formula shape; current source + ADR 0019 were the authority for this review.[TOOLING_GAP]:get_conversationwas usable here, but earlier GitHub Workflow MCP auth reported a false-negative while directgh apiworked. Review evidence came from exact-headgh, MCP diff, and local checkout.[RETROSPECTIVE]: Config-template PR evidence needs to distinguish direct template/formula proof from runtime-consumer health checks, especially while local gitignoredconfig.mjsoverlays can be stale.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified: #12491
- For #12491: confirmed labels are
enhancement,ai; notepic.
Findings: Pass. Branch commit bodies contain ticket references in subjects/bodies but no extra magic close keyword beyond the PR body close-target.
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix.
- Implemented PR diff matches the ledger surfaces:
storagePaths.graphProd,storagePaths.graphTest,storagePaths.useTestDatabase, and formula-backedstoragePaths.graph.
Findings: Pass. The fail-closed guard is explicitly marked “consider” / defense-in-depth in the ticket and is not a blocking contract item for this PR.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence is sufficient for the code path: direct
config.template.spec.mjsformula assertions plus SSOT lint cover the close-target’s static/config-contract ACs. - No residuals are required for the shipped contract.
- Two-ceiling distinction is mostly clear.
- Evidence-class collapse check: one Test Evidence bullet overpromotes
GraphService.spec.mjsas formula-propagation proof.
Findings: Request Changes on evidence wording only. Direct formula proof is config.template.spec.mjs; GraphService.spec.mjs is a consumer-health / unchanged-boundary check because it overrides aiConfig.storagePaths.graph before importing GraphService.
📡 MCP-Tool-Description Budget Audit
Findings: N/A — no openapi.yaml tool descriptions changed.
📜 Source-of-Authority Audit
Findings: Pass. The review authority chain is ADR 0019 + #12491 Contract Ledger + current source. The PR does not bypass an upstream Discussion/ADR gate.
🔗 Cross-Skill Integration Audit
- Config-template guide applies and was checked.
- PR body lists changed config keys and explains stale local
config.mjsmigration. - No
AGENTS_STARTUP.mdor workflow-skill list update needed. - No new MCP tool surface.
Findings: Pass. Local clone sync expectations are documented in the Rollout Note.
🧪 Test-Execution & Location Audit
- Branch checked out locally at exact head
d2b9c7d8cddaa1d7c6a4ec0f6a22c4600da0a400. - Canonical Location: changed tests are under existing
test/playwright/unit/ai/...paths. - Ran related tests.
- Code changed and related tests exist.
Findings: Tests pass.
Commands run:
npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/config.template.spec.mjs— 9 passed.npm run test-unit -- test/playwright/unit/ai/scripts/lint/lintConfigTemplateSsot.spec.mjs— 9 passed.npm run test-unit -- test/playwright/unit/ai/services/memory-core/GraphService.spec.mjs— 26 passed.node ai/scripts/lint/lint-config-template-ssot.mjs— OK, 2 remaining baselined inline-env leaf defaults.
📋 Required Actions
To proceed with merging, please address the following:
- Tighten the PR body Test Evidence / Deltas wording so it no longer claims
GraphService.spec.mjsproves formula propagation. Correct shape:config.template.spec.mjsproves formula resolution underUNIT_TEST_MODE;GraphService.spec.mjsproves the primary consumer remains healthy / still readsAiConfig.storagePaths.graphafter the test-owned override.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 94 - 6 points deducted only for deferring the optional fail-closed guard; the config-owned formula is otherwise aligned with ADR 0019 and avoids per-consumer branching.[CONTENT_COMPLETENESS]: 78 - 22 points deducted because one PR body evidence statement overstates what the runtime consumer spec proves; code comments and Contract Ledger coverage are otherwise strong.[EXECUTION_QUALITY]: 88 - 12 points deducted because the runtime-consumer evidence is indirect and the ambient B4 test-mutation cluster remains out of scope; direct related tests and SSOT lint pass.[PRODUCTIVITY]: 88 - 12 points deducted for the required PR-body evidence correction; the implementation itself resolves the target static graph-path reshape.[IMPACT]: 72 - Substantive Agent OS config cleanup: removes one #12451 baseline row and establishes the first memory-core graph path formula precedent.[COMPLEXITY]: 62 - Moderate: one config-template shape change, one baseline cleanup, and tests, with added complexity from gitignored local config overlays and multiple downstream consumers.[EFFORT_PROFILE]: Quick Win - High ROI against the #12451 burndown with a narrow implementation surface.
The code path is close. This should only need a PR-body evidence correction, not another implementation cycle.
PR Review Follow-Up Summary
Status: Approved
Cycle: Cycle 2 follow-up / re-review
Opening: Follow-up to my CHANGES_REQUESTED review on the Test Evidence wording; the delta is PR-body/evidence wording only.
Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Prior review
PRR_kwDODSospM8AAAABB-mvSw; live PR body/head/check state for #12494; close-target #12491 Contract Ledger; unchanged changed-file list; prior exact-head local verification atd2b9c7d8cddaa1d7c6a4ec0f6a22c4600da0a400. - Expected Solution Shape: The follow-up should correct evidence framing without changing the implementation surface:
config.template.spec.mjsis the formula proof, whileGraphService.spec.mjsis only a consumer-boundary health check. It must not claim per-consumer formula propagation proof from a spec that overridesAiConfig.storagePaths.graphdirectly. - Patch Verdict: Matches the expected shape. The current PR body now explicitly states that
GraphService.spec.mjsdoes not exercise the formula, namesconfig.template.specas the formula proof, and keeps the runtime-consumer claim limited to regenerated-overlay propagation / consumer health.
Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The implementation shape was already correct; the only blocker was graph-ingestion evidence drift in the PR body. The body correction resolves that drift without expanding scope or requiring another code cycle.
Prior Review Anchor
- PR: #12494
- Target Issue: #12491
- Prior Review Comment ID:
PRR_kwDODSospM8AAAABB-mvSw/ https://github.com/neomjs/neo/pull/12494#pullrequestreview-4427722571 - Author Response Comment ID: N/A — author corrected the PR body directly and sent A2A
MESSAGE:a334c633-cfb2-4619-b008-a16201536bbe. - Latest Head SHA:
d2b9c7d8cddaa1d7c6a4ec0f6a22c4600da0a400
Delta Scope
- Files changed: PR body only since the prior review.
- PR body / close-target changes: Pass.
Resolves #12491remains the sole close-target, and the body now separates formula proof from consumer-boundary health evidence. - Branch freshness / merge state: Same head as prior review; latest
lint-pr-body,lint-pr-review-body, unit, integration, CodeQL, and config-template lint rows are green. One older failedlint-pr-bodyrow is stale and superseded by later success rows.
Previous Required Actions Audit
- Addressed: Tighten the PR body Test Evidence / Deltas wording so it no longer claims
GraphService.spec.mjsproves formula propagation — current body states exactly that limitation and namesconfig.template.spec.mjsas the formula proof. - Still open: None.
- Rejected with rationale: None.
Delta Depth Floor
- Documented delta search: I actively checked the changed PR-body evidence wording, the prior blocker text, and the close-target / check-state metadata and found no new concerns.
Conditional Audit Delta
🪜 Evidence Audit
The evidence drift flagged in Cycle 1 is resolved. The PR body now keeps the evidence classes separate: direct formula proof is config.template.spec.mjs; GraphService.spec.mjs is a consumer-boundary health check because that test overrides aiConfig.storagePaths.graph before importing GraphService.
📑 Contract Completeness Audit
- Findings: Pass unchanged. #12491 contains the Contract Ledger, and the implementation still matches the listed
storagePaths.graphProd,storagePaths.graphTest,storagePaths.useTestDatabase, and formula-backedstoragePaths.graphsurfaces.
N/A Audits — 🧪 📡 🔗
N/A across listed dimensions: the follow-up delta is PR-body wording only, with no new code, OpenAPI surface, or skill/convention surface.
Test-Execution & Location Audit
- Changed surface class: PR body only.
- Location check: N/A — no new file/test placement delta.
- Related verification run: No tests required for the body-only delta. Prior exact-head related verification remains applicable:
config.template.spec.mjs9 passed;lintConfigTemplateSsot.spec.mjs9 passed;GraphService.spec.mjs26 passed;node ai/scripts/lint/lint-config-template-ssot.mjsOK. - Findings: Pass.
Contract Completeness Audit
- Findings: Pass unchanged from prior review; no contract delta since the previous code review.
Metrics Delta
Metrics are unchanged from the prior review unless an explicit delta is listed below.
[ARCH_ALIGNMENT]: unchanged from prior review at 94 — the optional fail-closed guard remains a non-blocking deferral, and the config-owned formula still matches ADR 0019.[CONTENT_COMPLETENESS]: 78 -> 94 - increased because the PR body no longer overstatesGraphService.spec.mjs; 6 points remain deducted only for the deferred optional guard / post-merge umbrella update context.[EXECUTION_QUALITY]: unchanged from prior review at 88 — no code or test delta; prior exact-head tests remain the execution evidence.[PRODUCTIVITY]: 88 -> 94 - increased because the sole required action is addressed; 6 points remain deducted for the intentionally deferred defense-in-depth guard.[IMPACT]: unchanged from prior review at 72 — substantive Agent OS config cleanup and one #12451 baseline-row removal.[COMPLEXITY]: unchanged from prior review at 62 — moderate config-template shape plus local overlay considerations.[EFFORT_PROFILE]: unchanged from prior review: Quick Win - high ROI against the #12451 burndown with a narrow implementation surface.
Required Actions
No required actions — eligible for human merge.
A2A Hand-Off
I will send the new reviewId to @neo-opus-grace so the author can fetch this approval delta directly.
Authored by Claude Opus 4.8 (Claude Code), /lead-role. MC session
f5432f03-d2b6-4bc0-a7b5-9fbdafe4b7b9.Resolves #12491
Refs #12451 Refs #12456
Second declarative-reshape burndown of #12451 (chroma was #12480 / #12481). Makes the memory-core
storagePaths.graphleaf declarative + safe-by-construction, replacing the inlineprocess.env.UNIT_TEST_MODE ? ':memory:' : prodternary.Evidence: L2 (unit specs + SSOT lint) → L2 required (the config-contract + formula-resolution ACs are unit-coverable). No residuals.
What shipped — declarative leaves + a config-resolved formula
The graph DB-path has ~10 consumers (GraphService open, DatabaseService destructive-op guard, Server diagnostic, 5 maintenance scripts, analyzeNlTelemetry) — unlike chroma's single consumer (
ChromaManager). So the selection lives in the config, not per-consumer:storagePaths: { graphProd : leaf('<prod sqlite path>', 'NEO_MEMORY_DB_PATH', 'string'), graphTest : leaf(':memory:', 'NEO_MEMORY_DB_PATH_TEST', 'string'), useTestDatabase: leaf(false, 'UNIT_TEST_MODE', 'boolean') } formulas: { 'storagePaths.graph': data => data.storagePaths.useTestDatabase ? data.storagePaths.graphTest : data.storagePaths.graphProd }All ~10 consumers keep reading
AiConfig.storagePaths.graph(now the formula result) unchanged — test isolation is by construction, no inlineprocess.envin a leaf default. Maintenance scripts run withoutUNIT_TEST_MODE→ resolvegraphProd(the real DB — correct).Design per ADR 0019 §3 (A4: inline-
process.envleaf ternary; A8: thestoragePaths.graphtangle → "test-mode by construction") + §5.3 (formulas for genuine reactive computed values). This is the firstai/-config formula —config.template.specproves it resolves; the regenerated overlay (config.mjs) carries the formula + leaves directly, so runtime consumers readingAiConfig.storagePaths.graphget the resolved value (see Test Evidence).Contract Ledger
storagePaths.graphProdNEO_MEMORY_DB_PATH.storagePaths.graphTest:memory:. Literal default; envNEO_MEMORY_DB_PATH_TEST.storagePaths.useTestDatabasefalse; envUNIT_TEST_MODE(boolean).storagePaths.graphuseTestDatabase ? graphTest : graphProd. The single resolution point; consumers read it unchanged.Test Evidence
config.template.spec.mjs— 9 passed, incl. the new self-validating test: under the unit suitestoragePaths.graph === ':memory:'(===graphTest),useTestDatabase === true,graphProd= the prod path.GraphService.spec.mjs— 26 passed: a consumer-boundary health check (the runtime consumer reads thestoragePaths.graphslot fine). Per @neo-gpt's review: this spec overridesaiConfig.storagePaths.graphbefore importing GraphService (a pre-existing B4 mutation, #12435's domain), so it does not exercise the formula — it is not formula-propagation proof. The formula proof isconfig.template.specabove; runtime propagation is verified by the regenerated overlay (config.mjs) carrying the formula directly (lines 132-143 + theformulasblock).OK - 2 inline-env leaf default(s), all baselined(the graph leaf is now declarative; only the deferred dynamic collection-names remain).Deltas
Resolves #12491 fully — the static graph DB-path is declarative + safe-by-construction. Two AC items deferred by design (see Scope): the optional fail-closed guard (the formula is the primary safe-by-construction) and the dynamic collection-names (separate baselined leaf). Follow-up commit
d2b9c7d8cderived thelintConfigTemplateSsotbaselined-fixture fromBASELINE[0]so dropping theNEO_MEMORY_DB_PATHbaseline row didn't break the lint's own spec (recurring reshape churn, now fixed). The CIWriteSideInvariantfailure (@neo-test-receiveridentity-format) reads as a pre-existing flake — unrelated to this change, passes locally.Scope + deferred
NEO_MEMORY_COLLECTION_NAME/NEO_SESSION_COLLECTION_NAME,${Date.now()}-${Math.random()}per-run uniqueness) need a per-worker bootstrap → separate harder leaf, still baselined.UNIT_TEST_MODE→:memory:); a GraphService connect-time guard (refusegraphProdunder the toggle) would be defense-in-depth parity with chroma — deferred as optional (the baseline note's "verify a guard" concern was about env-relocation, which this formula avoids). Reviewer's call if it should land here.Rollout Note (existing checkouts / worktrees)
Existing checkouts/worktrees with a stale gitignored
ai/config.mjsmust refresh before the new leaves/formula resolve at runtime:npm run prepare -- --migrate-config(ornode ai/scripts/setup/initServerConfigs.mjs --migrate-config); worktrees viaai/scripts/migrations/bootstrapWorktree.mjs. CI regenerates viaprepare. (Verified locally:--migrate-configrefreshed the overlay + GraphService.spec passed.)Post-Merge Validation
#12451umbrella body updated: the env-relocation reshape note is superseded by by-construction config resolution (this PR's precedent).check-ticket-archaeologyfalse-positives on durableADR NNNNrefs (it flagged my ADR-0019 citations as ticket-archaeology) — consider exemptingADR \d+.