feat: record and review classed observations over MCP - #1
Draft
jakewan wants to merge 10 commits into
Draft
Conversation
Walking skeleton proving the chain end to end: stdio transport, tool handler, SQLite store, read-back. Driven by TestAcceptanceRecordThenReview, written before any production code. Store uses SQLite in WAL mode with separate read and write handles. The handles take different DSNs: _txlock=immediate is write-only, since the driver consumes it solely on the BeginTx path and putting it on the read handle would make every read transaction take a write lock, serialising reviews against records. The split also avoids a permanent deadlock when List tallies classes while a page cursor is open. Observations are append-only, enforced by BEFORE UPDATE/DELETE triggers. No column refers to adjudication; the dependency direction is the invariant. Timestamps are stamped through formatTime, which converts to UTC before formatting. The trailing Z in the layout is a literal, not a zone token, so formatting a local time would label local wall-clock time as UTC and break the lexicographic-equals-chronological property the filters rely on. Paging is a keyset cursor on (recorded_at, id) rather than a timestamp filter, because recorded_at is not unique when sessions record concurrently. Dependencies are pinned to the versions the design was verified against (go-sdk v1.6.1, sqlite v1.53.0), matching the sibling MCP servers.
Adds the spec suite driving the store and tool behavior, including the guards that make the invariants fail loudly rather than erode quietly: - TestRecordRefusesOnlyMalformedInput asserts the refusal set exactly and carries accept-cases. A refusal-only table would stay green after a state-derived refusal was added, since the new refusal fires on inputs such a table never exercises. - TestObservationCarriesNoAdjudicationState asserts set-equality over the observation columns, so adding adjudication state to the row fails. - TestReviewPagesBackwardAcrossATimestampTie freezes the clock so every seeded row shares one recorded_at. Verified to fail with a timestamp-only cursor while the distinct-timestamp paging spec still passed, which is the false confidence it exists to remove. Fixes a cold-start race the cross-process spec surfaced: switching a new database into WAL mode needs a brief exclusive lock, and SQLite returns SQLITE_BUSY for that transition immediately instead of invoking the busy handler, so concurrent first-opens failed outright. Initialization now retries on a locked database. WAL mode persists in the header, so this only affects a store that does not exist yet. Documents that a clock passed to WithClock must be safe for concurrent use; Append calls it from every recording goroutine.
Serves the docket over stdio and resolves the store location from config, falling back to the XDG state path. A missing config is not an error here, which inverts the sibling servers' behavior deliberately: no field is required, so treating an absent file as fatal would make the server unusable out of the box for no gain. A config that exists but does not parse is still an error, since silently ignoring a typo in a file the operator named is worse than refusing to start. Adds `field-docket snapshot <path>`, which issues VACUUM INTO. The backup tool copies files and cannot run SQL, and copying a live WAL database can capture a mid-transaction state that will not open. VACUUM INTO refuses an existing destination, so the snapshot is written beside the target and renamed over it, which also means a reader never sees a partial file. Verified the invariant guards fail when eroded, rather than assuming they would: removing the null-arguments middleware panics on a nil map through jsonschema-go, adding an adjudication column fails the column-set assertion, and adding a refusal that consults the store fails on the accept-cases in the refusal-set table.
Merges the two donor repositories seam by seam rather than adopting either wholesale, because each is wrong in a different place. Build and install recipes take florilegium's -ldflags form, which injects internal/server.serverVersion. Overstory's bare `go build` would leave the locally-installed binary — the one MCP clients actually launch — reporting "dev" forever while tagged release builds injected correctly. CI takes overstory's shape (go mod verify, tidy -diff) minus its docs job, whose paths filter matches mise.toml and ci.yml and so would fire a book build against a docs/ directory this repo does not have. Adds florilegium's release-config job, which is the only real guard on the ldflags symbol: the linker ignores an -X naming a symbol that does not exist, so `goreleaser check` and any unit test both pass a rename that a real release fails on. Dropped --single-target there so the job exercises the full release matrix; the local recipe keeps it for speed. Actions are pinned to resolved commit SHAs. Overstory floats every action and florilegium's pins span five major versions, so neither donor's set transfers. mise pins gain goreleaser and git-cliff, which back `just release-check` and `just changelog` — the release path is reachable locally, not only from the tag workflow. The pre-push test hook runs -race: the concurrency specs assert this store's second invariant, and without the detector they pass while the race they exist to catch goes unreported. Full suite is 13s. govulncheck joins go.mod as a tool dependency so `go tool` runs a checksum-verified pin.
Both invariants lead in every agent-facing surface — CLAUDE.md, the Copilot review instructions, and CONTRIBUTING's scope section — because each is easier to erode by drift than to violate outright. A column named adjudicated_at and a refusal-only test table both look like ordinary changes at review time. The carried go-practices rule corrects one claim its donor asserts: that no null-guard middleware is needed. That is true for an *absent* arguments payload and false for a literal null, which passes the SDK's len(data) > 0 guard, nils the map, and panics in schema-default application. Carrying the donor text verbatim would have invited a later agent to delete tolerateNullArguments citing the rule. SECURITY.md documents what this server does that the donors do not: it keeps data. Permissions are the only confidentiality boundary, the append-only triggers make DROP TRIGGER the redaction path rather than tamper-proofing, and synchronous=NORMAL can lose the newest commits on power loss. Naming the modernc driver there records why the release matrix cross-compiles at all, and that SQLite advisories arrive as a Go module version rather than through system libraries. markdown-practices is not carried: it is path-conditioned to a docs book this repo does not have.
Snapshot wrote the whole observation corpus at 0644. VACUUM INTO opens its destination as a main database, which SQLite creates at its own default rather than at the source's mode — unlike the -wal/-shm sidecars, which do inherit. So the one path designed to carry this data off the machine was also the widest exposure of a store whose only confidentiality boundary is filesystem permissions. The intermediate now comes from os.CreateTemp, which additionally stops two overlapping snapshot runs from colliding on a fixed name where the second run's cleanup could delete the first's in-progress output. The single read transaction behind review's page/total/tally agreement had no guard. Breaking it left the entire suite green; the added spec fails within one run of moving the aggregates back onto the pool. migrate could not detect a store written by a newer binary — it skips migrations at or below the current version and would open a future-versioned store as if nothing were wrong, failing later with an opaque SQL error. Reachable because a store outlives the binaries that open it. clip cut on a byte boundary while claiming characters, so a multi-byte rune straddling the cap became U+FFFD on the wire. The per-entry cap also had no test at all, and the test harness's decode struct omitted the truncation flag entirely, which is why nothing noticed. Two shipped claims were false and are corrected rather than softened: SECURITY.md and the toolchain rule both asserted that CI installs through mise and verifies against mise.lock. No job does — the donor's only such consumer was its documentation job, which this repo dropped. And cliff.toml described preserving a hand-written [0.1.0] section that does not exist here; following it at the first release would have stranded the curated entries and published a one-line note. Also: errors.AsType at both sites the go-practices rule governs, source citations on the four external-system claims comment-conventions requires them for, "verbatim" corrected where fields are normalized, and just fmt now runs the formatter set CI actually enforces.
Switching to a unique intermediate name removed the fixed name's one virtue: a later run cleared it. With unique names nothing does, so a snapshot that wrote its copy and then failed to publish orphaned a full copy of the observations — at whatever mode SQLite chose — in a directory a backup tool may be watching. The spec induces the failure after VACUUM INTO has settled the file, by renaming onto an existing directory. A failure before that point leaves nothing behind whether the cleanup runs or not, so it would have passed against the missing cleanup and pinned nothing.
An instruction file carries the landed-state directive; where the convention came from changes no behaviour, and the reader of a public repo cannot resolve the reference.
The pin was carried over already one minor behind upstream, which meant shipping with the weekly toolchain-currency run red from the first scheduled firing. That run fails when any pin is behind, and the failure is the whole notification mechanism — mise outdated exits 0 either way, and GitHub only notifies on failed runs. A pin that starts out red teaches the maintainer to ignore the one toolchain signal no update bot covers. mise.lock regenerated in the same commit; the diff touches goreleaser entries only. The release config still validates and the snapshot build still resolves the serverVersion ldflags symbol, which is what the release-config CI job guards. CI needs no matching change: goreleaser-action pins the v2 line by range rather than an exact patch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
field-docket records classed observations to a per-machine store and reads them back. An agent session notices things — a rough edge, a surprising behavior, a place where guidance misfired — and those noticings die with the session, so anything built on them later rests on someone's recollection. This gives them somewhere to go: whether something has happened once or five times becomes a fact rather than an impression.
This is the initial implementation, scoped as a walking skeleton — the whole chain proven end to end rather than a full feature set. Two MCP tools, a SQLite store, an optional config file, and a
snapshotsubcommand for backups. The server holds evidence; it does not interpret it, and classes, scopes, and subjects are caller-defined strings it never branches on.Two invariants worth holding while reviewing
Both are easier to erode by drift than to violate outright, so both are enforced structurally rather than by convention.
Recording is separated from adjudication. Recording is append-only and never gated — no stored state can cause a write to be refused, skipped, or silently dropped, and
record_observationfails only on malformed input. Three mechanisms carry it: the observation table has no column referring to any judgment about a row; the dependency direction is fixed, so a future adjudication entity references observations and never the reverse; andBEFORE UPDATE/BEFORE DELETEtriggers reject row mutation at the SQL layer. A store that can decline to record is a store that quietly decides what matters.The store takes concurrent writes from several simultaneous processes. Three agent sessions against one SQLite file is the normal case, not an edge. This drove the storage choice rather than merely favoring it: bbolt holds an exclusive lock for the lifetime of an open handle, and stdio servers live for a whole session, so the second agent would block forever.
Decisions a reviewer would otherwise reverse-engineer
scope_kinddefaults toproject, notuser. The opposite default fails silently — an observation about a repository filed as user-level is invisible to every project-scoped review and unfixable in an append-only store. This direction turns that into a loud, retryable error whenscope_refis blank.enumonscope_kindis a boundary hint, not a storage constraint. A typo is rejected at the MCP boundary while the column itself accepts any string. ACHECKwould hard-code one caller's taxonomy into a general-purpose store, and widening it later means SQLite's twelve-step table rebuild in a database whose triggers exist to make row rewriting impossible._txlock=immediatesits on the write handle only; on a shared DSN every read transaction would take a write lock and serialize reviews against records.Zin Go's reference layout is a literal character, not a zone token, so a value formatted without.UTC()stamps local wall-clock time falsely labelled UTC — permanently, in a store that cannot be rewritten.Testing
Behavior specs driven outside-in from an acceptance test over an in-memory MCP session. Two are worth flagging: the cross-process concurrency spec re-execs the test binary, because in-process goroutines never exercise POSIX locking or WAL recovery — it surfaced a cold-start
SQLITE_BUSYthat no in-process test can see. And each spec pinning an invariant above was confirmed by breaking what it protects and watching it fail.Deliberately not here
Adjudication — recording what was decided about a body of observations — along with any pruning or retention mechanism, and the client-side rule that would tell an agent when to record. The server is installed and callable, but nothing invokes it unprompted.