Add two read-only Kinbase tools to the MCP server - #62
Conversation
![adaptcom[bot]](https://avatars.githubusercontent.com/in/1609964?s=60&v=4){kind=small}
There was a problem hiding this comment.
Confidence Score: 3/5
Summary
Adds Kinbase status/explain tools; prior findings are resolved and 145 focused tests pass. Signed-maintenance behavior still requires Jeremy McEntire’s review before merge.
Important Files Changed
| File | Overview |
|---|---|
| CHANGELOG.md | Documents query tools and maintenance side effects. |
| docs/index.html | Updates the public MCP tool count to 69. |
| src/kindex/kinbase.py | Adds unbounded status and bounded explain queries. |
| src/kindex/mcp_server.py | Exposes status/explain tools and preserves typed refusals. |
| tests/test_kinbase_acceptance.py | Covers refusals, command isolation, repository state, and timeout contracts. |
| tests/test_release_metadata.py | Updates the expected tool count to 69. |
| def read_status(repo: str | Path, *, binary="kinbase") -> dict: | ||
| """Certification, trusted fact count and open Unknowns for one repository.""" | ||
| root = Path(repo).expanduser().resolve(strict=True) | ||
| return _read_only(["status", "--repo", str(root)], binary, READ_TIMEOUT_S) |
There was a problem hiding this comment.
Kinbase status invokes emit_due_orphan_abandonments, signing repository events and closing overdue apologies; withhold kinbase_status until a no-maintenance path exists and test that signed state remains unchanged.
9dcda4f to
3632fae
Compare
kinbase_sync already refreshes a repository's own signed evidence. What an agent could not do was ask whether the repository has Company direction at all, or why one key reads the way it does. kinbase_status answers the first. A withheld snapshot degrades a projection to repository evidence without failing, so a repository that lost Company and one that never had direction read alike until something asks. kinbase_explain answers the second, for one exact key, which is the call kinbase_sync already makes and therefore already trusted. Generating a whole task brief stays outside the tool surface. kinbase project writes a query log and may submit an Unknown, so an agent calling it in a loop turns a human review queue into question spam; a test asserts the read path never reaches it.
`status` runs Kinbase's due-maintenance sweep first, and that sweep signs events. A deadline firing partway through would tear a write rather than abandon a read: killed between the signed event and the ledger recording it, the next call is free to re-emit an id that already exists. The CLI imposes no such bound, so adding one here invented a failure mode rather than contained one. Bounding it safely means crash-safe maintenance inside Kinbase, which is not ours to add from here. `explain` keeps its bound because it reduces one key and writes nothing.
READ_TIMEOUT_S stopped being used when status became unbounded, and its comment claimed it covered explain, which uses its own sixty second bound. A constant that names a thirty second limit nothing applies is worse than no constant.
It described READ_TIMEOUT_S and outlived it, landing above _query where it read as documentation for the function. Both halves were wrong by then: status is unbounded, and the explain bound covers one call rather than a whole sync.
fda0c76 to
bf5cc4f
Compare
|
Independent review — source and tests read directly, nothing inherited from CI or other reviewers. First, a correction to the premise I was given: the conflict is Tests pass on the branch: Blocking1. I took the rationale in Side effect worth catching: deleting 2. A Kinbase refusal is indistinguishable from success, and is recorded as a healthy call. Two incompatible shapes from one tool. That is the tool's own stated purpose inverted — "a repository that lost Company and one that never had direction read alike until this is asked" — reintroduced at its output boundary. A disposition with no signal. Non-blocking3. 4. Unbounded capture and response. 5. Naming, not a defect. The Things I expected to find and could not
TestsBetter than happy-path, and they test the promise where Kindex owns it: argv never contains What this gets rightThe tool boundary is drawn in the right place for a stated reason: Verdict: CHANGES_REQUESTED(1) and (2) are both fail-silent defects in a tool whose entire purpose is making a silent degradation visible. The CHANGELOG conflict is a one-minute fix. |
read_status ran with no deadline on the theory that a timeout firing mid-sweep would tear a signed write. The trade was the wrong way round. The deadline SIGKILLs the child exactly as a client quit, an MCP restart or a lost machine does, so the tear is reachable either way and the bound only changes how often it is reached. What the bound removes is the unrecoverable case. FastMCP calls a sync tool inline on the event loop, so an unbounded status against an unreachable Company does not stall one tool, it stalls every kindex tool for that client with no cancellation path. STATUS_TIMEOUT_S is thirty seconds, below the explain bound, and the test now asserts each read carries its own.
Kinbase refuses with a typed envelope on stdout carrying `error` and no `ok`, and that envelope was returned verbatim. One tool therefore had two shapes: a caller reading `ok` saw COMPANY_UNREACHABLE as a clean status, and the health ledger's string branch, which tested only `ok is False`, recorded an unreachable Company as a healthy call for as long as it lasted. The refusal now keeps Kinbase's own code and remedy and gains the disposition every other tool carries. The ledger's string branch also tests `error`, which its dict branch already did, so no tool can report a refusal as success. The test asserts this at the MCP output layer, which is where the disposition is read from and where the existing refusal test stopped short.
Thirty seconds was inherited from the constant this replaced, which is not a derivation. Measured: `status` against a 0.36 MB store takes 655-884 ms over ten runs, and the cost is linear in store size, so the 5 MB that the deploy notes name as the point where the query shape must be fixed lands near nine seconds. A 20 MB store extrapolates past the bound, which is the query shape failing loudly rather than the bound being wrong, and the note says so along with the fact that it is one measured point. The rationale for leaving it unbounded is also weaker than it read. It feared the next call re-emitting an event id that already exists, but Kinbase appends with INSERT OR IGNORE against an event_id primary key, so that is the case the insert absorbs. The tear is a no-op, not corruption. Both guards were wider than their reason. The tool boundary tested for `error` and would have overwritten a Kinbase envelope that carried its own `ok`; it now supplies a disposition only where Kinbase omitted one. The health ledger tested for the presence of an `error` key, so a payload carrying an explicit null would have counted as a refusal; it now tests the value.
|
Both blockers fixed, and two pushbacks. The bound is measured now, not inherited: 655-884ms over ten runs against a 0.36MB store, linear in store size, so the 5MB in the deploy notes lands near 9s. A 20MB store extrapolates past it, which is the query shape failing loudly rather than the bound being wrong. One measured point, not a second measurement. The explain bound didn't move. The torn-write concern doesn't hold at all, rather than partly. Finding 2 is fixed at the MCP output layer, the gap you named. Narrowed both guards a little past the ask: the boundary only supplies a disposition where Kinbase omitted one, and the ledger tests the |
|
Re-reviewed at head The unbounded status subprocess — One thing to keep in view, not a regression: the call is still inline on the event loop, so the worst case is now a bounded 30 s stall of every kindex tool (60 s for The refusal-reads-as-success shape — fixed at both layers, which is the right call. CHANGELOG resolution is clean. 64 tests pass ( One line worth fixing on the way past
I checked whether this is live and it isn't: Still open from the first review, none blocking
Merging on the strength of the two blockers being genuinely fixed. |
2 participants
kinbase_syncalready refreshes a repository’s own signed evidence. What an agent could not do was ask whether the repository has Company direction at all, or why one key reads the way it does.kinbase_statusanswers the first. A withheld Company snapshot degrades a projection to repository evidence without failing, so a repository that lost Company and one that never had direction read alike until something asks.kinbase_explainanswers the second, for one exact key — the same callkinbase_syncalready makes, so the path is already trusted.What is deliberately absent
kinbase project. It generates the whole task brief and is the call that measurably improves plans, but it writes a query log and may submit an Unknown —projector.rssays so directly: "before a question or unknown is written". There is no dry-run flag. An agent calling it in a plan→evaluate→replan loop would turn the queue humans use for real gaps into question spam, and a tool that writes on read is not safe to retry, parallelise or cache.test_read_only_tools_never_invoke_projectasserts the argv never contains it, so the decision survives a future refactor.Shape
Both are thin passes over the CLI’s
--json, followingsync_kinbase:shutil.whichresolves the binary, a refusal comes back as the typed document Kinbase wrote rather than a raised exception that loses its code and remediation, and neither tool exposes abinaryparameter — a tool caller does not name what runs.Tests
Four added: the never-invokes-project guard, refusal-as-document, refusal without a binary on PATH, and the MCP wrappers returning typed errors.
test_kinbase_acceptance.py: 54 passedread_statusatprojectfails the guard test; reverting passes itAlso exercised against a live
kinbased, not only stubs:kinbase_statusreturnedcertifiedwith a valid certificate over 1099 observations,kinbase_explainreturned aratifiedruling, and an unknown key came back asstate: unknownrather than an error.docs/index.htmland the pinned count intest_release_metadata.pymove 67 → 69.Part of a set, but independent
This one stands alone and can merge whenever. The other two are wandercom/kinbase#24, which lifts kinbase's loopback restriction, and wandercom/kinbase-live#1, the Kubernetes deployment that waits on it. Neither is needed for this.
Addressed from review
"Read-only" was not accurate and the claim is withdrawn.
kinbase statusruns Kinbase's own due-maintenance sweep before it reports, closing apologies already past their deadline, which is a signed write. There is no flag to suppress it.What I checked before deciding whether that disqualifies the tool:
.kinis byte-identical after three consecutivestatusruns on a repository with nothing overdue. The write is bounded by what is already due rather than by how often it is asked, and a second call in the same minute writes nothing. That is the line between this andproject, which records every query and may open a new Unknown per call — so the exclusion stands and the framing changes.The helper is now
_queryrather than_read_only, and the tool docstring, the module docstring and the changelog all say whatstatusdoes instead of claiming it does nothing.test_the_query_tools_leave_the_repository_and_the_graph_untouchedasserts the narrower contract Kindex can actually hold: across repeated calls no repository file is added, removed or rewritten by Kindex, and the graph gains no nodes.