Stabilize graph export contract - #34
blast-hardcheese wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
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 |
jmc-wander
left a comment
There was a problem hiding this comment.
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,implementsanddepends_on. - That includes edges the writer created with
bidirectional=False, and the new test asserts that behavior. - Repro on current
mainwith this branch merged:- Add decisions
oldandnew. - Add
add_edge("new", "old", "supersedes", bidirectional=False). - Export as JSON.
- Import into an empty store.
- Run
kin export --active-only.
- Add decisions
- Result:
Exported 0 nodes. The export gaveoldasupersedesedge back tonew, so after importnewlooks superseded too and the current decision disappears.
Suggested change:
- Export the edges the store actually holds;
add_edgealready writes the 0.8 reciprocal whenbidirectional=True. - If older one-sided rows need reciprocals, limit the inference to symmetric types, never to
supersedesor 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.pyandtests/test_supersede_lifecycle.pyare the relevant ones. - The branch merges cleanly with current
main.
1bf98ad to
ace95e1
Compare
|
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. |
|
@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. |
|
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: Blocking1. Edge ordering is not stabilized, so the contract is not stable. Reproduced on this branch: one hub, six 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 Fix: (Captured as kindex concept 2. The PR carries an unrelated 0.44.1 release bump, and documents nothing about its own contract change. The CHANGELOG diff also inserts Non-blocking3. Every export pays a full edge scan for a flag that is off by default. 4. The key-order contract test covers the one audience with no external consumers. 5. Checked and refuted
What this gets rightThe diagnosis and the direction are both correct. An interchange schema built as Verdict: CHANGES_REQUESTEDThe contract it declares stable still has a nondeterministic edge order, and it would ship a version bump while documenting nothing about the contract itself. |
Summary
--active-onlyexport projection that excludes retired and superseded predecessorssupersedesexport → import →--active-onlyregression for both JSON and JSONLValidation
git diff --checktests/test_import_export.py -v— 12 passed locally (through the repository serialized-test wrapper)