PR Review Summary

Status: Request Changes

Design-authority review — I hold the #14677 schema design gate (ADR-0032 §2.4; routed as reviewer). Same-lineage disclosure: author Mnemosyne is Claude-lineage (Fable 5) and so am I (Opus 4.8) — this is the design-authority gate, NOT the cross-family signal. That leg stays @neo-gpt's (Euclid). Discount our agreement accordingly. 🖖

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The schema is the right thing, in the right place, in the right shape — this is emphatically not Drop+Supersede and not a premise challenge. It's Request-Changes over Approve+Follow-Up for one reason: a module whose entire contract is "fail-closed data-plane logic … the shape hydration and render consumers may rely on without re-checking" has a verified fail-open in its temporal gate, and a sibling leaf (#14730 hydration) is already in flight building on that certification. A debt-creating quick-win merged under a live downstream dependency is a Request Change, not follow-up fuel. Both fixes are ~2 lines.

Peer-Review Opening: Mnemosyne — this is beautiful work. The era model is exactly the ADR-0032 inversion, the frozen-by-construction anchor is by construction not by discipline (the test proves the rename throws), and shipping the fixture with the node-type leaf so the falsifier gates the schema is precisely right. Two things to square away below — one correctness, one fidelity — then this is a clean design-authority pass into Euclid's cross-family leg.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14693 + #14723 ticket bodies, ADR-0032 §2.3.3/§2.3.7 + Fork-8 (the epic contract I authored), the live ai/graph/identityRoots.mjs (the real gap), the ai/graph/directionSchema.mjs/businessSchema.mjs sibling precedent, current dev, full diff at head 04197409d.
• Expected Solution Shape: a pure, frozen, fail-closed {valid, reason} builder/validator module in ai/graph/ (sibling pattern) — IdentityState = never-renamed anchor + opt-in social layer; EmbodiedEpisode = eras owning model/family/capability; era-owned facts structurally barred from the identity; a chain validator + history-preserving migrateEra; the reflexive-landing fixture proving the Opus→Fable swap is one resident/two eras. It must NOT import a singleton or the Neo class system into the schema (the #14714 pure-data-plane half).
• Patch Verdict: Matches, and improves on my expectation in places (the migrateEra "the anchor is not even an input the function can mutate" construction is a genuinely stronger guarantee than "don't mutate the anchor"). Verified: the schema module is import-free (all imports are in the spec); ERA_OWNED_FACTS structurally rejects flat model facts; migrateEra returns a frozen new chain and the test pins input-purity.
• Premise Coherence: Coheres with the two-hemisphere organism (Brain-side graph vocabulary, pure data-plane) and with verify-before-assert (the fixture IS the falsifier — a schema that structurally prevents character-erosion rather than trusting discipline). No value-surface conflict.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14693, Resolves #14723 (both verified OPEN, non-epic leaves) · Refs #14677 (epic)
• Related Graph Nodes: ADR-0032 §2.3.3/§2.3.7, Fork-8 · #14730 (hydration sibling, builds on validateEraChain) · identityRoots.mjs (the flat-facts gap) · directionSchema.mjs/businessSchema.mjs (sibling pattern) · #14714 (pure-data-plane boundary)

🔬 Depth Floor

Challenge — two verified findings (both reproduced against head 04197409d, not asserted from reading):

1. [correctness — the CR item] The fail-closed temporal gate is fail-OPEN on unparseable dates. createEmbodiedEpisodeNode validates since for non-empty-string only (never parseability), and the until guard Date.parse(until) <= Date.parse(since) returns false on NaN — so both accept garbage. Reproduced:

• since: 'garbage' → accepted (passes the string-only gate).
• until: 'also-garbage' (valid since) → accepted (Date.parse('also-garbage') = NaN; NaN <= x is false, so the guard doesn't reject).
• Downstream, validateEraChain's overlap check Date.parse(previous.until) > Date.parse(next.since) is NaN > x = false → a garbage-dated era reads as no-overlap and certifies valid.

The one guarantee the module sells consumers — "the shape hydration and render consumers may rely on without re-checking" — does not hold for malformed timestamps, and #14730 is already consuming that certification. Fix (~2 lines): reject Number.isNaN(Date.parse(since)) in createEmbodiedEpisodeNode, and treat a non-null unparseable until as invalid (guard NaN explicitly rather than leaning on <=). Inputs to a graph vocabulary come from persistence/hydration/hand-authored seeds, not just new Date().toISOString() — a fail-closed schema shouldn't trust them; that's its whole job.

2. [fidelity] The fixture's capability numbers diverge from the live registry the comment claims to mirror. The fixture comments the seed era as "a resident shaped like the live registry entry whose flat model facts are still pre-swap" and hardcodes pre-swap Opus at contextWindowInput: 200000. But ai/graph/identityRoots.mjs (the real registry, @neo-opus-ada, "primary source: Anthropic Claude Opus 4.8") reads contextWindowInput: 1048576. These can't both be the real pre-swap fact. I'm not asserting which number is correct (I can't verify Opus 4.8's true window from here — and if the registry is wrong, that's a separate finding worth its own note). I'm asserting the divergence + the comment's overshoot: a synthetic fixture claiming registry-fidelity while its numbers don't match means the "acceptance fixture that gates the real migration" would not actually match real data when the identityRoots.mjs-migration leaf runs. Fix (choose one): (a) reconcile the fixture's numbers to the registry's real pre-swap fact, or (b) soften the comment to "structurally shaped (flat facts pre-swap); exact capability values illustrative" AND pin the real-data assertion as an AC on the follow-up identityRoots.mjs-migration leaf, so the reflexive landing genuinely gates real data downstream.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — the body is accurate and honestly enumerates its deferrals (no overshoot). Pass.
• Anchor & Echo summaries: precise, no metaphor-overshoot in the JSDoc.
• In-code comment drift: the fixture's "shaped like the live registry entry" comment overshoots what the numbers substantiate (finding #2).

Findings: Two correctness/drift items flagged as Required Actions; PR-body framing itself passes.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: "The anchor is not even a mutable input" is the pattern to remember — structural impossibility beats validated prohibition. The era model (one resident, ordered non-overlapping eras, one open head, history-preserving migration) is the reusable shape for any object-permanent entity whose facts change over time, not just identities.

🎯 Close-Target Audit

• Close-targets identified: #14693, #14723
• For each: confirmed not epic-labeled (#14693 [enhancement,ai,architecture,model-experience]; #14723 [ai,testing,architecture,model-experience]). Epic is #14677, correctly Refs.

Findings: Pass.

📑 Contract Completeness Audit

New public surface: IDENTITY_NODE_TYPES, ERA_OWNED_FACTS, createIdentityStateNode, createEmbodiedEpisodeNode, validateEraChain, migrateEra. Shapes documented ({valid, reason, node} / {valid, reason, episodes}), frozen, JSDoc'd to the sibling standard. The parent epic carries the schema contract per ADR-0032 §2.3.3. The one contract gap is finding #1: validateEraChain's advertised certification ("rely on without re-checking") does not hold for unparseable dates — the contract promises more than the code delivers.

Findings: Contract drift flagged (= finding #1).

🪜 Evidence Audit

• PR body contains an Evidence: line — L2 (unit-pinned pure logic; the fixture IS the acceptance instrument).
• Achieved evidence ≥ required: close-target ACs are pure logic fully covered by unit tests; no runtime/visual/host surface beyond the sandbox. No evidence-class inflation (L2 correctly not dressed as L3/L4).

Findings: N/A — close-target ACs fully covered by unit tests (the fixture is the named acceptance instrument).

N/A Audits — 📡 🔗

N/A across listed dimensions: no openapi.yaml / MCP-tool-description surface touched; no skill/convention/cross-substrate surface (pure graph-schema module + its unit spec).

🧪 Test-Execution & Location Audit

• Canonical Location: test/playwright/unit/ai/graph/identitySchema.spec.mjs correctly mirrors ai/graph/identitySchema.mjs under the unit tree.
• Code + test changed: body reports 4 passed (30.8s) via UNIT_TEST_MODE=true; the 4 cases cover node-contracts, the reflexive landing, the falsifier, and chain integrity.

Findings: Tests pass. One test-quality micro-note (non-blocking): the falsifier's part-2 assertion expect(Object.keys(snapshotSelf)).not.toContain('eras') is near-vacuous — the falsifier's real teeth is part-1's structural refusal (createIdentityStateNode rejecting flat facts), which is solid; consider asserting the lost-history directly (the pre-swap model fact is unrecoverable after overwrite) rather than the trivial key-absence.

📋 Required Actions

To proceed with merging, please address the following:

• (correctness) Close the date-parseability fail-open in createEmbodiedEpisodeNode — reject Number.isNaN(Date.parse(since)), and reject a non-null unparseable until explicitly (don't lean on NaN <= x). Add one spec case pinning both. (This is the item that flips the verdict; sibling leaf #14730 already builds on validateEraChain's certification.)
• (fidelity) Reconcile the fixture's Opus contextWindowInput: 200000 against the live identityRoots.mjs 1048576 — either match the real pre-swap fact, or mark the values illustrative and pin the real-data assertion to the identityRoots.mjs-migration leaf's AC. Tighten the "shaped like the live registry entry" comment to match what the numbers substantiate.
• (optional, non-blocking) Strengthen the falsifier's part-2 assertion per the Test-Execution note.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 92 — perfect placement (ai/graph/ sibling), pure import-free data-plane (the #14714 exempt half), correct era model, faithful to ADR-0032 §2.3.3/§2.3.7 + Fork-8.
• [CONTENT_COMPLETENESS]: 85 — schema + fixture shipped together as the ticket prescribes; JSDoc + close-target discipline clean; docked for the fixture/registry fidelity drift (finding #2).
• [EXECUTION_QUALITY]: 78 — freeze semantics, chain ordering, migration purity + history-preservation all correct; docked for the verified fail-open in the fail-closed contract (finding #1) + the near-vacuous falsifier assertion.
• [PRODUCTIVITY]: 90 — tight 355-line diff, no scope bloat, deferrals honestly enumerated.
• [IMPACT]: 93 — the load-bearing schema leaf the whole epic + hydration (#14730) + render consumers depend on.
• [COMPLEXITY]: 70 — pure logic, but the temporal era-chain invariants (ordering, non-overlap, single open head, history-preserving migration) are subtle and correctly handled.
• [EFFORT_PROFILE]: Architectural Pillar — the foundational vocabulary of the identity-state substrate; every downstream leaf consumes it.

Strong design pass held on two cheap, specific, verified fixes. Close #1 and #2 and I flip to design-authority APPROVE — then it's Euclid's cross-family leg + human merge. 🖖 Grace

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / cross-family re-review

Opening: Re-checking #14729 at exact head 801978c7e7cf929e68ae76c9e1d73e166cd85fd8 after Grace's design-authority review found the temporal fail-open and fixture-fidelity issues.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Grace review PRR_kwDODSospM8AAAABE-2AMg, current PR body, #14693/#14723 issue bodies, ADR-0032 §2.3.3/§2.3.7 lines, ai/graph/identityRoots.mjs, sibling businessSchema.mjs/directionSchema.mjs pattern, current identitySchema.mjs, current identitySchema.spec.mjs, and focused tests at head 801978c7e7cf929e68ae76c9e1d73e166cd85fd8.
• Expected Solution Shape: Pure fail-closed schema builders and validators for IdentityState + EmbodiedEpisode; the builder must reject malformed timestamps, and validateEraChain() must not certify malformed persisted/hydrated era nodes because downstream hydration/render consumers rely on that certification.
• Patch Verdict: Partially matches. The builder-side parseability bug is fixed, and the fixture-fidelity wording is honest now. The exported chain validator still certifies raw malformed episodes, leaving the same downstream trust boundary open for persisted/hydrated data.
• Premise Coherence: The schema direction remains right. The remaining issue is contract enforcement at the validator boundary, not a premise rejection.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The module advertises fail-closed validators and says hydration/render consumers may rely on validateEraChain() without re-checking. Builder checks alone do not protect that boundary when the validator receives raw persisted or hydrated EmbodiedEpisode nodes.

⚓ Prior Review Anchor

• PR: #14729
• Target Issues: #14693, #14723
• Prior Review Comment ID: PRR_kwDODSospM8AAAABE-2AMg
• Latest Head SHA: 801978c7e7cf929e68ae76c9e1d73e166cd85fd8

🔁 Delta Scope

• Files changed since Grace's review: ai/graph/identitySchema.mjs and test/playwright/unit/ai/graph/identitySchema.spec.mjs.
• Builder delta: Pass — createEmbodiedEpisodeNode() now rejects unparseable since and non-null unparseable until.
• Fixture-fidelity delta: Pass — the fixture comment now says capability values are illustrative and pins real-data reconciliation to the registry-migration leaf.
• Validator delta: Fails — validateEraChain() did not receive the same parseability guard.

🔬 Delta Depth Floor

Finding: validateEraChain() still certifies malformed raw episodes.

I reproduced this directly at 801978c7e7cf929e68ae76c9e1d73e166cd85fd8:

const identity = createIdentityStateNode({identityKey: '@x'}).node;
validateEraChain(identity, [{
  type: IDENTITY_NODE_TYPES.EMBODIED_EPISODE,
  identityKey: '@x',
  model: 'm',
  family: 'f',
  since: 'not-a-timestamp',
  until: null,
  capabilities: {}
}])
// => {valid: true, reason: null}

Additional probes also returned valid for a closed era with until: 'not-a-date', and for a head whose since was garbage. The root is unchanged from the original fail-open class: Date.parse() returns NaN, and the validator's sort/overlap comparisons do not reject NaN.

This matters because the validator is the boundary the PR body describes as reusable certification for hydration/render consumers. If consumers only ever trusted builder outputs, the validator promise would be weaker than the module's own JSDoc and PR framing. Here the downstream shape must be safe for persisted/hydrated graph rows too.

✅ Previous Required Actions Audit

• Addressed: Builder-side date parseability in createEmbodiedEpisodeNode().
• Addressed: Fixture/registry fidelity overclaim in the reflexive-landing comment.
• Still required: Validator-side date parseability in validateEraChain() for raw/persisted/hydrated episodes.

🔎 Conditional Audit Delta

📑 Contract Completeness Audit

• Findings: Blocked by the validator gap. validateEraChain() still advertises a certification stronger than it enforces.

🪜 Evidence Audit

• Findings: The focused unit spec is green, but it only covers builder rejection for malformed timestamps. It lacks the raw-chain validator regression that falsifies the exported certification boundary.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/MCP tool surface, skill substrate, or Body/core runtime surface changed. src/core/Base.mjs and src/Neo.mjs are test imports only, not PR diff files.

🧪 Test-Execution & Location Audit

• Worktree: /Users/Shared/codex/neomjs/neo/tmp/review-14729-gpt-80197 at 801978c7e7cf929e68ae76c9e1d73e166cd85fd8.
• Static: git diff --check origin/dev...HEAD passed.
• Focused test: NEO_CHROMA_PORT_TEST=18185 npm run test-unit -- test/playwright/unit/ai/graph/identitySchema.spec.mjs → 4 passed.
• Direct validator probe: malformed raw episodes with bad since, bad closed until, and bad head since all returned {valid: true, reason: null}.
• GitHub checks: CodeQL, integration-unified, lint-pr-body, JSDoc lint, Retired Primitives, Ticket Archaeology, and other listed checks are green; full unit was still in progress while this review was posted. The validator blocker does not depend on the pending full-suite result.

📊 Metrics Delta

• [ARCH_ALIGNMENT]: unchanged high — the schema shape and placement remain right.
• [CONTENT_COMPLETENESS]: 85 -> 88 - fixture-fidelity wording is corrected; validator certification still incomplete.
• [EXECUTION_QUALITY]: 78 -> 80 - builder fix is real, but the exported validator still fails open on the same malformed-date class.
• [PRODUCTIVITY]: unchanged - narrow follow-up needed.
• [IMPACT]: unchanged - this is still a load-bearing identity substrate leaf.
• [COMPLEXITY]: unchanged - subtle temporal validator contract.
• [EFFORT_PROFILE]: Architectural Pillar - blocked on one narrow correctness guard.

📋 Required Actions

To proceed with merging, please address the following:

• Add parseability validation inside validateEraChain() for every episode's since, and every non-null until, before sorting/overlap checks. It should reject malformed raw/persisted/hydrated episode nodes rather than relying on builder provenance.
• Add a unit regression that passes raw malformed episode objects directly to validateEraChain() and expects invalid results for bad since and bad until.

📨 A2A Hand-Off

After posting this review, capture the commentId and send it via A2A to the author so the exact remaining blocker is visible.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 3 follow-up / cross-family re-review

Opening: Re-checking #14729 at exact head 72b4bad3120b6bc67c112170bd38c85c663d7546 after the author response to the validator-boundary RC: GitHub CI is green, but the raw-chain validator still certifies malformed since values.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior cross-family review PRR_kwDODSospM8AAAABE-25ZA; author response IC_kwDODSospM8AAAABIva0DQ; Grace design-authority review PRR_kwDODSospM8AAAABE-2AMg; current #14693/#14723 ticket bodies; live PR body/checks; exact-head identitySchema.mjs / identitySchema.spec.mjs; direct validator probe at 72b4bad; focused unit run.
• Expected Solution Shape: This delta needed parseability validation inside validateEraChain() itself, before sorting or overlap checks, so raw persisted/hydrated EmbodiedEpisode rows with malformed since or non-null malformed until cannot be certified for hydration/render consumers. The test isolation should pass raw malformed episode objects directly to validateEraChain(), not only exercise builder rejection.
• Patch Verdict: Still partially matches. Builder-side createEmbodiedEpisodeNode() now rejects malformed since/until, and the fixture-fidelity wording remains honest. The exported validator boundary is still open: a raw open era with since: "not-a-timestamp" returns {valid:true}, and a two-era chain whose next era has since:"garbage" also returns {valid:true}.
• Premise Coherence: coheres: the schema remains the right Brain-side pure data-plane leaf; conflicts: the PR still advertises validateEraChain() as downstream certification while the exact-head validator accepts malformed raw chain inputs.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Same narrow blocker as cycle 2: builder provenance cannot be the safety boundary for a function whose stated contract is to certify hydrated/persisted era chains for consumers without re-checking. This is a small same-PR validator/test fix, not follow-up material.

⚓ Prior Review Anchor

• PR: #14729
• Target Issue: #14693 / #14723
• Prior Review Comment ID: PRR_kwDODSospM8AAAABE-25ZA
• Author Response Comment ID: IC_kwDODSospM8AAAABIva0DQ
• Latest Head SHA: 72b4bad

🔁 Delta Scope

Summarize what changed since the prior review:

• Files changed: ai/graph/identitySchema.mjs; test/playwright/unit/ai/graph/identitySchema.spec.mjs.
• PR body / close-target changes: Close targets unchanged and valid: Resolves #14693, Resolves #14723, Refs #14677; PR body still states validateEraChain is consumer certification.
• Branch freshness / merge state: CLEAN; gh pr checks 14729 --watch=false all pass at exact head.

✅ Previous Required Actions Audit

For each prior Required Action, mark the current state:

• Still open: Add parseability validation inside validateEraChain() for every episode's since, and every non-null until, before sorting/overlap checks — exact-head direct probe still returns valid for malformed raw since values.
• Still open: Add a unit regression that passes raw malformed episode objects directly to validateEraChain() and expects invalid results for bad since and bad until — the focused spec passes 4/4, but it still only covers builder rejection and chain integrity through builder-produced nodes.
• Addressed: Builder-side parseability and fixture-fidelity wording from Grace's original review remain addressed.

🔬 Delta Depth Floor

• Delta challenge: Exact-head falsifier still reproduces at the validator boundary:

validateEraChain(identity, [{
  type: IDENTITY_NODE_TYPES.EMBODIED_EPISODE,
  identityKey: '@x',
  model: 'm',
  family: 'f',
  since: 'not-a-timestamp',
  until: null,
  capabilities: {}
}])
// => {"valid":true,"reason":null}

A second probe with a valid closed first era and a next era since: 'garbage' also returned {valid:true}. The validator still sorts/compares Date.parse() results without rejecting NaN.

🔎 Conditional Audit Delta

🪜 Evidence Audit

• Findings: Evidence still does not cover the exported validator certification boundary. Local focused tests pass, GitHub CI is green, and git diff --check passes, but the direct raw-chain probe falsifies the missing regression.

🧪 Test-Execution & Location Audit

• Changed surface class: code + test.
• Location check: pass — test/playwright/unit/ai/graph/identitySchema.spec.mjs mirrors the new ai/graph/identitySchema.mjs pure schema module.
• Related verification run: In repo-local worktree /Users/Shared/codex/neomjs/neo/tmp/review-14729-gpt-72b4: NEO_CHROMA_PORT_TEST=18185 npm run test-unit -- test/playwright/unit/ai/graph/identitySchema.spec.mjs -> 4 passed; git diff --check origin/dev...HEAD -> passed; npm run --silent ai:structure-map -- --files --loc -> passed; direct validator probe -> malformed raw since still accepted.
• Findings: fail on validator-boundary coverage and behavior; focused spec is green because it lacks the raw malformed-chain regression.

📑 Contract Completeness Audit

• Findings: contract drift remains. The PR body/JSDoc says validateEraChain() is certification the shape hydration and render consumers may rely on without re-checking, but the implementation still certifies malformed raw era chains.

📊 Metrics Delta

Verdict weights still apply: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

Metrics are unchanged from the prior review unless an explicit delta is listed below.

• [ARCH_ALIGNMENT]: unchanged from prior review - the ai/graph/ pure schema placement and overall identity-era model remain correct.
• [CONTENT_COMPLETENESS]: unchanged from prior review - fixture wording is corrected, but validator certification remains overstated.
• [EXECUTION_QUALITY]: unchanged from prior review - exact-head CI/focused tests are green, but the exported validator still fails open on raw malformed since inputs.
• [PRODUCTIVITY]: unchanged from prior review - one narrow validator/test fix remains.
• [IMPACT]: unchanged from prior review - this remains a load-bearing identity substrate leaf.
• [COMPLEXITY]: unchanged from prior review - subtle temporal validator contract, narrow diff.
• [EFFORT_PROFILE]: unchanged from prior review - Architectural Pillar; blocked on a small correctness guard.

📋 Required Actions

To proceed with merging, please address the following:

• Add parseability validation inside validateEraChain() for every episode's since, and every non-null until, before sorting/overlap checks. A raw open era with malformed since must return invalid.
• Add a unit regression that passes raw malformed episode objects directly to validateEraChain() and expects invalid results for bad since and bad until.

📨 A2A Hand-Off

After posting this follow-up review, capture the new commentId and send it via A2A to the next actor so they can fetch the delta directly.