Skip to content

fix(witan): order graph commit ids by parsing them, not as text - #463

Merged
blarghmatey merged 1 commit into
mainfrom
omnigraph-013-commit-id-order
Oct 8, 2026
Merged

blarghmatey merged 1 commit into
mainfrom
omnigraph-013-commit-id-order

Conversation

@blarghmatey

Copy link
Copy Markdown
Member

What are the relevant tickets?

N/A (witan task tk-stop-comparing-graph-commit-ids-as-strings-0-12--03da2d, part of the omnigraph 0.13 upgrade, spec in #461 D7)

Description (What does it do?)

task_claim trusts its post-write verification read only once the read's graph_commit_id has caught up with the claim's own write. It decided that with < and >= on the two strings, which works on 0.11 because every id is a ULID.

From omnigraph 0.12 only a graph's first commit (and anything written before omnigraph upgrade) is a bare ULID. Every later id is hb1.<block>.<slot>.<nonce> with a decimal slot, so hb1.X.9.… sorts after hb1.X.10.…. From the tenth write in a block the check would trust a stale read, or retry a fresh one to its attempt cap.

This adds witan/commit_ids.py with commit_reached(seen, floor) and uses it in both comparisons:

  • two commits of one block order by integer slot (exact: the publisher keeps a commit in its parent's block only when its slot is the parent's plus one);
  • a bare ULID is older than any hb1 id;
  • otherwise the commits' own ULIDs decide, which is the assumption the string comparison of 0.11 ids already made.

The spec suggested ordering by graph_manifest_version. A mutate response carries it but a read's envelope carries only graph_commit_id (checked on the 0.13.0 CLI and in upstream's ReadOutput), so the id is all the verification read has.

An id of neither shape counts as not caught up and logs witan.task_claim.verify.unorderable_commit. The comparison runs after the claim is written, so raising there would report a failure for a claim the caller holds.

The helper lives in witan-council because task_claim is its only caller. Putting it in witan-core would need a core release and a floor bump in the same change.

Safe on 0.11, so it can merge ahead of the pin.

How can this be tested?

  • just test-witan-council on omnigraph 0.11.0: 1446 passed.
  • The new and changed tests (tests/test_commit_ids.py, the claim-verification tests in tests/test_tasks.py) also pass against the 0.13.0 binary, with fix(witan-core): write timestamps at millisecond precision #462's timestamp fix applied locally so that 0.13 accepts the writes: 21 passed. On 0.13.0 test_real_commit_ids_order_the_way_the_writes_happened sees hb1.<block>.1.… through hb1.<block>.12.…, and sorting those as strings gives the wrong order.
  • The stubbed catch-up tests now use slots 9 and 10 of one block, where the old comparison gives the wrong answer.
  • just docs-check passes.

https://claude.ai/code/session_01Mfmuz4DTn557AV8V17o98A

task_claim trusts its verification read only once the read's commit has
caught up with the claim's own write, and it decided that with a string
comparison because 0.11's commit ids are ULIDs. From omnigraph 0.12 an id
is hb1.<block>.<slot>.<nonce> with a decimal slot, so hb1.X.9 sorts after
hb1.X.10. From the tenth write in a block a stale read would be trusted, or
a fresh one retried to the attempt cap.

A read's envelope carries only the id (no graph_manifest_version), so this
parses the two ids. Commits of one block order by integer slot. A bare
ULID (the bootstrap commit, or one from before `omnigraph upgrade`) is
older than any history-block id. Otherwise the commits' own ULIDs decide,
which is what comparing 0.11's ids did.

An id of neither shape counts as not caught up and is logged. The
comparison runs after the claim is written, so raising there would report
a failure for a claim the caller holds.

Claude-Session: https://claude.ai/code/session_01Mfmuz4DTn557AV8V17o98A
Copilot AI balanced review requested due to automatic review settings October 8, 2026 18:05

Copilot AI 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.

🟢 Approval recommended

The focused fix addresses numeric slot ordering with regression coverage and no identified blocking issues.

0 open findings

What changed in this PR

Fixes task_claim verification for omnigraph 0.12+ commit IDs, supporting the planned 0.13 upgrade while retaining 0.11 compatibility.

Changes:

  • Parses commit IDs to compare history-block slots numerically.
  • Uses parsed ordering in verification, with bounded retries for unknown formats.
  • Adds regression coverage and updates documentation.
File Description
packages/​witan-core/​witan_core/​omnigraph.py Corrects commit-ordering documentation.
mcp/​servers/​witan/​witan/​server.py Integrates parsed ordering into claim verification.
mcp/​servers/​witan/​witan/​commit_ids.py Adds commit parsing and comparison.
mcp/​servers/​witan/​tests/​test_tasks.py Covers catch-up retries and unknown formats.
mcp/​servers/​witan/​tests/​test_commit_ids.py Tests ordering rules and real commit sequences.
mcp/​servers/​witan/​changelog.d/​20261008_132356_blarghmatey_omnigraph_013_commit_id_order.md Records the verification fix.
docs/​glossary.md Explains version-dependent commit formats.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@blarghmatey
blarghmatey merged commit 8bd786b into main Oct 8, 2026
21 checks passed
@blarghmatey
blarghmatey deleted the omnigraph-013-commit-id-order branch October 8, 2026 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants