docs(dev): maintainer review companion for RFC-013 node identity

[aaltshuler]

Summary

Adds a maintainer review companion for RFC-013 (PR #287, node identity —
surrogate + mutable natural key), keeping the substantive findings in the dev
docs rather than only in PR threads. Linked from a new Design Reviews section
in docs/dev/index.md.

Findings recorded

1. Surrogate-reuse collision after a rename (not caught by Greptile/Cursor) —
   derive-then-freeze makes the surrogate a deterministic function of the initial
   @key, and id must be unique, so a rename frees the natural key but burns
   the surrogate: reinserting the freed name collides with the original's frozen
   id. A new failure mode vs today's delete+reinsert, and it conditions the
   headline "@key is now mutable" promise. Must address before Accepted.
2. Endpoint-resolution precedence (id-first vs natural-key-first) is
   undefined for the window where the two namespaces overlap.
3. Composite @key → scalar surrogate encoding must be deterministic and
   injective (composite_unique_key is separator-free specifically to avoid the
   collisions a flat scalar reintroduces).
4. Rollout step 4 atomicity — removing the @key-update rejection without
   cross-version uniqueness admits duplicate natural keys.
5. Internal ticket-id nit downweighted (consistent with the dev-track MR-*
   convention).

Merge ordering

The review doc links to rfc-013-node-identity-surrogate-key.md, which lands via
#287. Merge this after #287 so that intra-doc link resolves on main
(check-agents-md.sh validates index links, which pass independently — this is
only about the in-body cross-link).

🤖 Generated with Claude Code

Greptile Summary

This PR adds a maintainer review companion document for RFC-013 (node identity — surrogate + mutable natural key) and registers it under a new "Design Reviews" section in docs/dev/index.md. The companion records five substantive findings — most notably the surrogate-reuse collision after a rename (not caught by automated reviewers) — so design decisions and open edges are preserved alongside the proposal rather than lost in PR comment threads.

• docs/dev/index.md: Adds a "Design Reviews" table section with one entry linking to rfc-013-review.md; the file is introduced in this same PR so the CI link-integrity check (check-agents-md.sh) passes cleanly.
• docs/dev/rfc-013-review.md: New 110-line companion covering what RFC-013 gets right, five findings (three rated "must address before Accepted"), and a merge recommendation. The in-body cross-link to rfc-013-node-identity-surrogate-key.md is a dangling reference that CI will not catch — it resolves only after docs(rfc): RFC-013 node identity: surrogate + mutable natural key #287 lands. The identifier dec-918 is also committed without inline gloss, making it permanently opaque to anyone outside the internal decision-record system.

Confidence Score: 4/5

Safe to merge once the merge ordering with #287 is respected; the changes are docs-only with no code impact.

The companion document is well-reasoned and the index change is minimal. The two concerns — a dangling in-body cross-link that CI will not catch, and an unexplained internal identifier committed to the permanent record — are real but non-blocking. The cross-link risk is entirely dependent on merge ordering; if #287 lands first the link is fine. No code or behavior is affected.

docs/dev/rfc-013-review.md — the in-body cross-link on line 3 and the unexplained dec-918 reference on line 27 are worth a quick look before merging.

Important Files Changed

Filename	Overview
docs/dev/index.md	Adds a new "Design Reviews" section with one index entry pointing to rfc-013-review.md; the link target is added in this same PR so the CI link-check passes cleanly.
docs/dev/rfc-013-review.md	New maintainer review companion for RFC-013; findings are substantively sound, but the in-body cross-link to rfc-013-node-identity-surrogate-key.md is a dangling reference invisible to CI until #287 lands, and dec-918 is committed without inline context.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Author as RFC-013 Author (#287)
    participant Reviewer as Maintainer (this PR #289)
    participant CI as check-agents-md.sh
    participant Docs as docs/dev/

    Author->>Docs: proposes rfc-013-node-identity-surrogate-key.md
    Reviewer->>Docs: "adds rfc-013-review.md (cross-links to #287 file)"
    Reviewer->>Docs: registers entry in docs/dev/index.md

    Note over CI,Docs: CI validates index links only
    CI->>Docs: checks docs/dev/index.md → rfc-013-review.md ✅
    CI-->>Docs: does NOT check in-body link inside rfc-013-review.md ⚠️

    Note over Author,Docs: Merge ordering matters
    alt "#289 merged before #287"
        Docs-->>Docs: rfc-013-node-identity-surrogate-key.md missing → dangling link
    else "#287 merged first, then #289"
        Docs-->>Docs: both files present → link resolves ✅
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Author as RFC-013 Author (#287)
    participant Reviewer as Maintainer (this PR #289)
    participant CI as check-agents-md.sh
    participant Docs as docs/dev/

    Author->>Docs: proposes rfc-013-node-identity-surrogate-key.md
    Reviewer->>Docs: "adds rfc-013-review.md (cross-links to #287 file)"
    Reviewer->>Docs: registers entry in docs/dev/index.md

    Note over CI,Docs: CI validates index links only
    CI->>Docs: checks docs/dev/index.md → rfc-013-review.md ✅
    CI-->>Docs: does NOT check in-body link inside rfc-013-review.md ⚠️

    Note over Author,Docs: Merge ordering matters
    alt "#289 merged before #287"
        Docs-->>Docs: rfc-013-node-identity-surrogate-key.md missing → dangling link
    else "#287 merged first, then #289"
        Docs-->>Docs: both files present → link resolves ✅
    end

Loading

_{Reviews (1): Last reviewed commit: "docs(dev): maintainer review companion f..." | Re-trigger Greptile}

Greptile also left 2 inline comments on this PR.

Context used:

• Context used - CLAUDE.md (source)

[greptile-apps]

Dangling cross-link with no CI guard

The link to rfc-013-node-identity-surrogate-key.md on this line points to a file that does not yet exist in the repository — it is landing via #287. check-agents-md.sh only validates links originating from the three index files (AGENTS.md, docs/user/index.md, docs/dev/index.md); in-body links inside non-index docs like this one are invisible to it. If this PR is merged before #287 the link is silently broken in main with no CI failure, and the merge-ordering requirement is preserved only in the PR description (ephemeral). A brief inline note in the file — e.g., a > **Merge note:** requires #287 to be merged first callout — makes the dependency durable and survives the PR thread being closed.

[greptile-apps]

Opaque internal identifier committed without inline context

dec-918 is used here without any inline gloss, making the reference permanently unresolvable to anyone outside the internal decision-record system. Finding 5 explicitly flags iss-* / MR-* / dec-* slugs against AGENTS.md rule 5 but downweights them as consistent with the dev-track convention — however, MR-* and iss-* references elsewhere in docs/dev/ are generally either contextually inferrable or glossed inline, while dec-918 has no surrounding explanation. A one-phrase inline description (e.g., "the dec-918 boundary — the decision that Lance stable row ids are a derived accelerator, not the logical source of truth") would preserve the traceability benefit without requiring access to the internal tracker.

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!