Skip to content

Stabilize graph export contract - #34

Open
blast-hardcheese wants to merge 4 commits into
mainfrom
export-contract
Open

blast-hardcheese wants to merge 4 commits into
mainfrom
export-contract

Conversation

@blast-hardcheese

@blast-hardcheese blast-hardcheese commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • emit a fixed complete graph export schema, including canonical lifecycle and standing fields
  • add an explicit --active-only export projection that excludes retired and superseded predecessors
  • preserve only stored graph arcs so export/import cannot invert directed relationships
  • cover the directed supersedes export → import → --active-only regression for both JSON and JSONL

Validation

  • git diff --check
  • tests/test_import_export.py -v — 12 passed locally (through the repository serialized-test wrapper)
  • CI is running for d519c1d

@adaptcom adaptcom Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confidence Score: 4/5

Summary

Standardizes export fields and active-only filtering while preserving directed arcs. No change-caused defects found; targeted tests and CI pass, with unrelated local environment/timing failures.

Important Files Changed

File Overview
src/kindex/cli.py Adds active-only filtering and deterministic node ordering while preserving stored arcs
src/kindex/graph_transfer.py Emits complete fields with canonical lifecycle and standing defaults
tests/test_import_export.py Covers schema defaults, stored directionality, and JSON/JSONL supersession roundtrips

↻ Re-run review · View in Adapt

Comment thread src/kindex/cli.py Outdated

@jmc-wander jmc-wander left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. A complete, stable export shape and an --active-only projection are both worth having, but the reciprocal-edge inference can't merge as it stands.

Blocking: inferred reverse edges invert directed relationships.

  • The export adds a reverse edge of the same type for every one-sided edge, including directed ones such as supersedes, implements and depends_on.
  • That includes edges the writer created with bidirectional=False, and the new test asserts that behavior.
  • Repro on current main with this branch merged:
    1. Add decisions old and new.
    2. Add add_edge("new", "old", "supersedes", bidirectional=False).
    3. Export as JSON.
    4. Import into an empty store.
    5. Run kin export --active-only.
  • Result: Exported 0 nodes. The export gave old a supersedes edge back to new, so after import new looks superseded too and the current decision disappears.

Suggested change:

  • Export the edges the store actually holds; add_edge already writes the 0.8 reciprocal when bidirectional=True.
  • If older one-sided rows need reciprocals, limit the inference to symmetric types, never to supersedes or other directed relations.
  • Add a round-trip test covering export, import and then --active-only.

Also:

  • Please run the test suite before re-requesting review. tests/test_import_export.py and tests/test_supersede_lifecycle.py are the relevant ones.
  • The branch merges cleanly with current main.

@blast-hardcheese

Copy link
Copy Markdown
Collaborator Author

Addressed the requested directed-edge regression in ace95e1 and rebased onto current main. Validation: 98 affected transfer/import/lifecycle tests pass. Full pytest reached 2893 passed and 4 skipped; the two failures are in untouched code and environment-specific here (a Node >=24 prerequisite with Node 22 installed, and a Codex reminder-session expectation). CI will exercise the repository Node 24 matrix.

@blast-hardcheese

Copy link
Copy Markdown
Collaborator Author

@jmc-wander It looks like we're in a better place now. Please re-review, we've also rebased overtop v0.44.1 which had some improvements to the precedence order for kindex db lookup when multiple databases are present.

@jmc-wander

Copy link
Copy Markdown
Contributor

Independent review. Note up front: a tooling block stopped me reading the standing CHANGES_REQUESTED review and the Adapt findings, so treat this as independent rather than reconciled with them.

Tests pass on the branch: test_import_export.py 12 passed, test_release_metadata.py 7 passed. git merge-base(main, pr34) == main@e2aa36f, so the branch contains all of main.

Blocking

1. Edge ordering is not stabilized, so the contract is not stable. cli.py:2033 sorts nodes by id. Edges come from Store.edges_from (store.py:3675): ORDER BY e.weight DESC with no tiebreak, so equal-weight edges come back in physical row order.

Reproduced on this branch: one hub, six relates_to edges all at weight 0.5. Delete and re-insert two (what graph heal, graph merge, or any maintenance rewrite does) and the exported edge list goes from [t0,t1,t2,t3,t4,t5] to [t0,t3,t4,t5,t2,t1]. Two stores holding an identical graph export different bytes.

I checked the plain export→import→export round-trip first and it is byte-stable — but only because SQLite happens to return ties in rowid order and import preserves insertion order. That is undefined behaviour being relied on, and the delete/reinsert case breaks it. The comment at graph_transfer.py:109 claims "unchanged JSONL snapshots remain byte-stable across CLI versions"; sorting nodes and not edges closes half the door.

Fix: ORDER BY e.weight DESC, e.to_id, e.type.

(Captured as kindex concept 7e85978e6338 — the defect outlives this PR, it lives in Store.edges_from.)

2. The PR carries an unrelated 0.44.1 release bump, and documents nothing about its own contract change. pyproject.toml, README.md, server.json, src/kindex/__init__.py, docs/index.html, docs/.well-known/mcp/server-card.json, both plugin.json files and AGENTS.md all differ from main (git merge-base --is-ancestor 87f7fe3 main → no). Merging an export fix would silently cut 0.44.1.

The CHANGELOG diff also inserts ## [0.44.1] - 2026-09-23 above main's existing ### Fixed block, which retroactively reclassifies main's unreleased kin doctor and store-selection fixes as 0.44.1, leaves [Unreleased] empty, and says nothing about the export contract or the new --active-only flag. A contract declared stable and shipped with no changelog entry is invisible to the consumers meant to rely on it. Per the project's own checklist the version bump is a deliberate release step, not a PR side effect.

Non-blocking

3. Every export pays a full edge scan for a flag that is off by default. cli.py:2013-2018 computes superseded_ids over all nodes unconditionally; the if getattr(args, "active_only", False) guard is at line 2019, after it. Line 2035 then calls edges_from again per node, so a plain kin export issues 2N edge queries instead of N. Move the comprehension inside the if.

4. The key-order contract test covers the one audience with no external consumers. test_import_export.py:149 asserts the exact 28-key tuple — a real drift-catching test and the best thing in the PR — but only at --audience private. The public/org branch (graph_transfer.py:169-178) pops intent, prov_why, prov_activity, prov_method, so the promised key set does not hold for the shapes that actually leave the machine, and nothing asserts it. No edge-ordering assertion either.

5. --active-only misses cross-audience supersedes edges. cli.py:2013-2018 builds superseded_ids only from edges of nodes already filtered to the export audience, so a private→team supersedes edge is invisible in a team export and the retired predecessor exports as active. Narrow: Store.supersede (store.py:2529) inherits audience and sets status='superseded' in the same transaction, so the ordinary path is caught by the status filter. Only a hand-made kin link X Y supersedes can straddle audiences.

Checked and refuted

  • Does it break .kin/knowledge.jsonl readers (including factory)? No. export_record has exactly one caller (cli.py:2018 ← cmd_export). Factory's file comes from kin repo-memory publish → repo_memory.py, a separate serializer with a different shape. No impact. (Side note: canonical_status/canonical_standing therefore normalize only one of the two export paths.)
  • Wall-clock leakage via weight decay. Tried to force drift by back-dating decay.last_run 400 days; weights stayed at 0.5 (cold-start stamping plus 4-dp write suppression). Unproven, and weight was exported before this PR anyway.
  • Round-trip identity for verified nodes. Non-identical at gen1→gen2 (verified_by becomes null plus extra.imported_verification), but deliberate per the module docstring, and it converges: gen2 == gen3 exactly. A caveat on "stable", not a defect.
  • canonical_status freezing nodes. A node with extra.superseded_by but an active status now exports as superseded, and _refuse_dead refuses edits on those even with force — but that state only arises from legacy corruption, which is what the function is for.

What this gets right

The diagnosis and the direction are both correct. An interchange schema built as {key: node[key] for key in ... if key in node} is a projection of whichever columns an older writer happened to load — the shape varies silently with the store's migration level, which is not a contract at all. Replacing it with an explicit literal, defaulting every field, and pinning key order in an assertion converts an accident into a promise with a mechanical guard. canonical_status/canonical_standing repair legacy blanks at the boundary rather than migrating the store, which is the right place. Sorting nodes by id is a genuine determinism win. And test_cli_roundtrip_active_only_preserves_superseding_successor catches a nasty class — a one-way arc retiring its own successor on replay. --active-only defaults off, so existing callers are unaffected.

Verdict: CHANGES_REQUESTED

The contract it declares stable still has a nondeterministic edge order, and it would ship a version bump while documenting nothing about the contract itself.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants