refactor: unify SQLite and PostgreSQL backend cores - #37
Merged
Conversation
…ows.py First increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1). The SQLite (db.py) and PostgreSQL (pg.py) backends each carried their own copy of claim/row serialization: _serialize_claim existed in both modules with identical logic, and pg.py privately owned the row normalisers (_iso/_norm) that produce the SQLite-compatible public value contract. New module sprintctl/rows.py now owns the single serialization contract: - iso_timestamp / normalize_row (was pg._iso / pg._norm) - claim_identity_status / claim_event_identity / claim_attempt_identity (was db._claim_identity_status etc.) - serialize_claim (was _serialize_claim, duplicated in both backends) db.py and pg.py keep their historical import surfaces via aliases so authority.py and external consumers are untouched. No behavior change: full SQLite suite passes (1487 passed; two test_doctor env failures pre-exist on base) and the disposable-PostgreSQL suite shows an identical failure set to base (8 pre-existing env failures, 169 passed). Refs: sprintctl#2150, agentops#2144
…sprintcore.py Second increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1). Sprint-table operations (create_sprint, get_sprint, get_active_sprint, list_active_sprints, list_sprints, set_sprint_kind) each carried their own SQLite and PostgreSQL query strings with duplicated WHERE-clause and tenant-scoping logic. New module sprintctl/sprintcore.py owns the query shapes once behind a SprintConn protocol; db.py and pg.py now supply thin execution adapters (_SprintSqlite, _SprintPg) and keep their historical public signatures unchanged. Also resolves prior behavioral drift: get_active_sprint now breaks created_at ties by id DESC on both backends (previously SQLite lacked the tiebreak that pg had), and create_sprint coerces falsy start_date/end_date to NULL on both backends. Full SQLite suite: 1487 passed, 180 skipped, 2 failed (pre-existing test_doctor env failures, same as base per prior increment's commit). Disposable-PostgreSQL suite not re-run in this session (no local PG instance available); adapter dispatch only changes call sites, not the SQL shapes or pg.py's _norm/_iso normalization path. Refs: sprintctl#2150, agentops#2144
…rackcore.py Third increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1). Track-table operations (get_or_create_track, list_tracks, get_track) each carried duplicated SQLite/PostgreSQL query strings, including a dialect-specific upsert (INSERT OR IGNORE vs ON CONFLICT ... DO NOTHING). New module sprintctl/trackcore.py owns the query shapes behind a TrackConn protocol; db.py and pg.py supply thin execution adapters (_TrackSqlite, _TrackPg), including an insert_ignore() method that lets each backend keep its own upsert dialect while the calling logic stays shared. Also fixes a latent bug from the previous increment: pg.py referenced sprintctl.sprintcore as _sprintcore without importing it, so every call to pg.create_sprint/get_sprint/get_active_sprint/list_active_sprints/ set_sprint_kind/list_sprints would have raised NameError at runtime. This was invisible to the local suite because the PostgreSQL integration tests are skipped without a disposable PG instance. Added the missing import alongside the new trackcore import; verified with pyflakes (via `uv tool run pyflakes`) that no unresolved names remain in db.py/pg.py. Full SQLite suite: 1487 passed, 180 skipped, 2 failed (same pre-existing test_doctor env failures as base, no regressions). Refs: sprintctl#2150, agentops#2144
Fourth increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1). Adds refcore.py and depcore.py alongside the existing sprintcore.py/trackcore.py, following the same protocol-adapter pattern. refcore.py owns ref-table CRUD (add/list/list-for-items/remove). Validation that calls other backend-specific functions (ref_type membership check, URL normalization, get_work_item existence check) stays in each backend's wrapper; the core module only owns the raw storage shape. The SQLite IN-clause pattern replaces pg.py's previous ANY(%s) array syntax for list_refs_for_items — both are standard SQL and functionally equivalent. depcore.py owns dep-table CRUD, including the two blocker/blocked-by listing queries that JOIN work_item. Introduces a join_tenant_clause() adapter method so PostgreSQL's extra `d.repo_id = wi.repo_id` join condition (needed because dep and work_item are both tenant-scoped tables) stays out of the shared query builder; SQLite's adapter returns "" since it has no repo_id column. Full SQLite suite: 1487 passed, 180 skipped, 2 failed (same pre-existing test_doctor env failures as base, no regressions). Verified via pyflakes (uv tool run pyflakes) that db.py/pg.py have no unresolved names. Refs: sprintctl#2150, agentops#2144
…itemcore.py Fifth increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1), following the sequencing from the architectural plan for the remaining WorkItem/Event/Claim sections (the first three were CRUD-shaped like Sprint/Track/Ref/Dep; the transactional CAS operations update_work_item_description and set_work_item_status are intentionally left in place — their locking is backend-specific by design, not just duplicated SQL). New module sprintctl/workitemcore.py owns: - create_work_item, get_work_item, list_work_items, set_work_item_priority query shapes, behind a WorkItemConn protocol. - The pure validators (validate_work_item_description, validate_priority, effective_priority, PRIORITY_MIN/MAX) that used to live in db.py and be imported into pg.py via `from .db import (...)`. That made pg.py depend on db.py for logic with zero SQLite dependency — backwards for the target architecture where the two backends are peers. Both now import these from workitemcore.py instead. Two new protocol members beyond what Sprint/Track/Ref/Dep needed: - update_one(sql, params) -> bool: runs an UPDATE, commits and returns True if exactly one row matched, else rolls back and returns False. Unifies sqlite's cur.rowcount check with pg's RETURNING-id-or-None check. - join_tenant_clause reused from depcore's pattern: list_work_items' JOIN against track needs `wi.repo_id = t.repo_id` on pg (absent on sqlite). Full SQLite suite: 1487 passed, 180 skipped, 2 failed (same pre-existing test_doctor env failures as base, no regressions). Verified via pyflakes that db.py/pg.py have no unresolved names; the workitemcore re-export "unused import" warnings are expected — validate_priority/effective_priority are consumed as db.validate_priority/pg.effective_priority module attributes by cli.py, application.py, context_candidates.py, and tests/test_priority.py, not by bare-name reference within db.py/pg.py. Refs: sprintctl#2150, agentops#2144
…ventcore.py Sixth increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1), per the architectural plan's step 2 (Event read/write paths). Transactional operations (create_event, create_archive_import_event, close_sprint_with_boundary_event, _require_receipt_close_boundary) stay in each backend's wrapper: they call other backend-specific functions and control their own commit/rollback boundary. New module sprintctl/eventcore.py owns: - _insert_event's raw storage shape (insert_event_row), list_events, list_events_limited, list_knowledge_candidates, and the takeup event query, behind an EventConn protocol (ph, tenant_params, query_all, insert_uncommitted — events are always inserted inside a larger caller-controlled transaction, so this doesn't auto-commit like the other cores' insert_id). - The pure takeup/payload helpers that used to live in db.py and be imported into pg.py (process_takeup_events, _decode_event_payload, _takeup_key, _takeup_actor_key, _serialize_takeup_row, _serialize_released_takeup, KNOWLEDGE_EVENT_TYPES, TAKEUP_EVENT_TYPES). Same peer-dependency fix as workitemcore.py's validators: both backends now import from eventcore instead of pg.py depending on db.py. Converges the three previously-inconsistent payload-decode call sites the architectural plan flagged as a live drift risk: db.list_knowledge_candidates inlined its own json.loads, and pg.py had two more variants (list_knowledge_candidates's inline decode and _takeup_events_pg's inline decode). All three now route through the single _decode_event_payload helper. pg's list_events/list_events_limited keep the "re-stringify jsonb dict to match sqlite's text convention" behavior via a new pure _stringify_payload helper (no protocol method needed — it just inspects the already-fetched row's payload type). Fixed one now-obsolete test assertion: test_takeup_pg_contract.py asserted the literal params passed to cursor.execute() were a `list` (`["test-repo", ...]`); the shared query builder passes a `tuple` instead (matching every other adapter in sprintcore/trackcore/refcore/depcore/ workitemcore), which psycopg accepts identically. Same content, same order, same "queries the connection directly, not via sqlite helpers" guarantee the test's docstring describes — updated the assertion to compare as a tuple rather than changing behavior to match the old test. Full SQLite suite: 1487 passed, 180 skipped, 2 failed (same pre-existing test_doctor env failures as base, no regressions). Verified via pyflakes that db.py/pg.py have no unresolved names. Refs: sprintctl#2150, agentops#2144
…aimcore.py
Seventh increment of the db/pg backend unification (sprintctl#2150,
agentops#2144 P2.1) — sub-increment 4a of the Claim extraction per the
architectural plan. Pure helpers and read-only queries only; create_claim,
heartbeat_claim, release_claim, and handoff_claim stay in each backend's
wrapper for now (backend-specific locking: SQLite BEGIN IMMEDIATE vs
PostgreSQL advisory + row locks, plus a collision-retry loop) — later
sub-increments (4b/4c/4d) will address those individually per the plan's
risk-based sequencing.
New module sprintctl/claimcore.py owns:
- get_claim, list_claims_by_sprint, list_claims, find_claim_by_identity,
get_active_exclusive_claim_row, get_active_coordinate_claim_row, behind
a ClaimConn protocol.
- CLAIM_TYPES, ClaimConflict, _generate_claim_token — previously defined
once in db.py and duplicated verbatim in pg.py (pg.py had its own
_generate_claim_token identical to db.py's). Both backends now import
the same objects from claimcore.
Two new protocol members, both preserving an existing intentional
behavioral distinction rather than flattening it:
- now_sql(): sqlite's strftime(...,'now') vs pg's statement_timestamp() —
pg deliberately does NOT use now(), which would pin to transaction start
on a long-lived served-mode connection and make lease-expiry comparisons
wrong. Documented in the protocol docstring so future edits don't
"simplify" this away.
- expires_at_offset_sql(): the "now + N seconds" interval expression,
parameterized identically to the rest of the query builder.
- true_literal ("1" vs "true") for the exclusive-boolean filter, and
join_tenant_clause reused from the depcore/workitemcore pattern for
list_claims_by_sprint's JOIN against work_item.
Full SQLite suite: 1487 passed, 180 skipped, 2 failed (same pre-existing
test_doctor env failures as base, no regressions). Verified via pyflakes
that db.py/pg.py have no unresolved names; removed now-genuinely-unused
`secrets` imports from both modules (their only use was
_generate_claim_token, now in claimcore.py).
Refs: sprintctl#2150, agentops#2144
…laimcore.py Eighth increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1) — sub-increment 4b of the Claim extraction per the architectural plan. heartbeat_claim and release_claim have no admission arbitration (single-row update/delete), so they're lower risk than create_claim/handoff_claim, which stay backend-specific for now (locking + retry loop). New in sprintctl/claimcore.py: - heartbeat_update / release_delete: the raw mutation shapes, using the now_sql()/expires_at_offset_sql() protocol members already established in 4a. Added a mutate() protocol member (matching sprintcore/depcore's pattern) since these are the first Claim operations that write. - heartbeat_rejection_event / release_rejection_event: pure (event_type, payload) builders for the claim-proof-rejected path. These were already 100% backend-agnostic Python (no conn dependency) but copy-pasted between db.py and pg.py. Bug fix surfaced by the extraction: pg.py's release_claim always tagged its rejection event ["claims", "coordination", "release"], while db.py additionally used ["claims", "coordination", "ambiguity", "legacy"] for legacy claims with no claim_token — a real behavioral drift, not a stylistic difference. release_rejection_event converges both backends on db.py's more specific tagging. No test exercised this on the pg path locally (pg claim tests require SPRINTCTL_TEST_PG_URL, unavailable in this environment), so this went uncaught until now; flagging here in case anything downstream filters coordination-failure events by tag and expects pg's narrower (buggy) set. Full SQLite suite: 1487 passed, 180 skipped, 2 failed (same pre-existing test_doctor env failures as base, no regressions). Verified via pyflakes that db.py/pg.py have no unresolved names. Refs: sprintctl#2150, agentops#2144
Ninth increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1) — sub-increment 4c of the Claim extraction per the architectural plan. handoff_claim has more branches than heartbeat/release (legacy-ambiguous, lost-proof-adopted, rotate-vs-transfer) but no admission race, matching the plan's "medium-high risk, same payload-builder pattern applied to a bigger function" assessment. New in sprintctl/claimcore.py: - handoff_update: the raw UPDATE shape (agent, claim_token, lease_epoch, expiry, identity fields, heartbeat), reusing now_sql()/ expires_at_offset_sql() from 4a/4b. - handoff_legacy_ambiguous_event / handoff_rejection_event / handoff_success_event: pure payload builders for the three event paths. Two real drifts surfaced and fixed here (documented in detail in the module docstring): 1. lease_epoch fencing gap: pg.py's handoff UPDATE bumped lease_epoch on rotation/legacy-adoption; db.py's never did. lease_epoch is consumed by terminal_recovery_server.py and authority.py as a fencing token to reject stale in-flight operations after a claim changes hands — the SQLite gap meant a session holding a pre-handoff lease_epoch could still pass an expected_lease_epoch check post-handoff on that backend. No existing test exercised this on SQLite. Added three regression assertions in test_claims.py: rotate and legacy-adopt now assert lease_epoch 1->2, and a new test confirms transfer-mode-without- token-rotation correctly does NOT bump it (the fix shouldn't overcorrect to bumping on every handoff call). 2. Rejection-event audit gap: pg.py's attempted_by payloads (both the legacy-ambiguity and coordination-failure branches) carried only actor/claim_id/claim_token_present, dropping runtime_session_id, instance_id, branch, worktree_path, commit_sha, pr_ref, hostname, pid that db.py included. Converged on db.py's richer identity. create_claim stays backend-specific (sub-increment 4d, deferred): it has the collision-retry loop and pg's two-tier advisory+row locking, the highest-risk piece per the plan, and genuinely needs a live PostgreSQL instance to validate concurrency behavior — unavailable in this environment. Full SQLite suite: 1488 passed (1487 baseline + 1 new test), 180 skipped, 2 failed (same pre-existing test_doctor env failures as base, no regressions). Verified via pyflakes that db.py/pg.py have no unresolved names. Refs: sprintctl#2150, agentops#2144
…e_work_item_description Tenth increment of the db/pg backend unification (sprintctl#2150, agentops#2144 P2.1). Per a planning pass's execution-ready design, this prototypes the multi-statement-transaction abstraction the plan's earlier architectural review said create_claim needs, on the smaller-blast-radius WorkItem CAS surface first. New WorkItemConn protocol members (workitemcore.py): begin_txn/commit/ rollback for the transaction envelope, for_update_of/lock_for_update for row locking, execute for non-committing statements inside an open transaction, and insert_event to delegate to each backend's _insert_event. SQLite's begin_txn issues BEGIN IMMEDIATE (whole-DB write lock, taken before any read); PostgreSQL's is a no-op since the connection is already in an explicit transaction and the lock comes from lock_for_update's SELECT ... FOR UPDATE instead. This is a real, documented concurrency-model difference the abstraction does not paper over: two concurrent SQLite CAS writers on *different* work items still serialize against each other (unchanged from before this extraction), while PostgreSQL's row lock lets them proceed in parallel — what both backends must guarantee identically is that two writers racing the *same* item produce exactly one accepted write and one EditConflict. Moved EditConflict/StatusConflict and the description-revision helpers (_description_revision, validate_item_edit_revision) from db.py into workitemcore.py, matching the peer-dependency fix already applied to the priority validators: pg.py now imports these from workitemcore directly instead of via db.py, and db.py re-exports them for existing callers (db.EditConflict etc. are used pervasively in application.py/cli.py/tests). Extracted get_work_item_with_edit_revision (kept as a plain, unlocked read — application.py/cli.py call it standalone to preview a revision before constructing an edit request) and update_work_item_description (using a new private _lock_work_item_with_edit_revision helper internally, since its correlated edit_version subquery doesn't fit the generic lock_for_update(table, id) shape cleanly). Added a genuine coverage gap the planning pass identified: SQLite had no concurrency test proving begin_txn's BEGIN IMMEDIATE actually serializes two real concurrent writers on the same item, only a sequential two- connection CAS-conflict test (test_two_readers_cannot_apply_the_same_edit_revision). PostgreSQL already had this via test_pg_integration.py::test_overlapping_edit_writers_accept_exactly_one_revision (threading.Barrier across two independent connections). Added the mirror test on the SQLite side in test_core.py; verified non-flaky across 5 runs. Full SQLite suite: 1491 passed (1490 baseline + 1 new test), 180 skipped, 0 failed — the 2 previously-documented "pre-existing" test_doctor failures turned out to be an environment artifact (missing psycopg/remote extra in this venv, installed earlier this session to run the PG suite), not a real baseline; they're gone now, unrelated to this change. Full disposable-PostgreSQL suite: 169 passed, same 8 pre-existing failures (all in terminal_recovery_pg.py/authority.py, untouched by this work), zero regressions. Verified via pyflakes that db.py/pg.py/workitemcore.py have no unresolved names. Refs: sprintctl#2150, agentops#2144
Eleventh increment of the db/pg backend unification (sprintctl#2150,
agentops#2144 P2.1). Validates the begin_txn/lock_for_update/commit/
rollback/execute primitives prototyped on update_work_item_description
against a second, structurally similar caller, per the planning pass's
explicit recommendation to do this immediately in the same session.
set_work_item_status's own business logic (VALID_TRANSITIONS check,
active-exclusive-claim lookup and proof verification, blocker-dependency
check) stays in each backend's wrapper as before — it calls other
backend-specific functions (_get_active_exclusive_claim_row,
_emit_claim_event, _require_claim_proof, list_deps_blocking) spanning
claim/dep/event concerns, not just work_item, so it doesn't fit a single
shared workitemcore function the way update_work_item_description did.
What moved is the transaction/locking boilerplate around that logic: the
initial BEGIN IMMEDIATE + item read on SQLite and SELECT ... FOR UPDATE on
PostgreSQL are now both `_WorkItemSqlite(conn).lock_for_update("work_item",
item_id)` / `_WorkItemPg(store).lock_for_update(...)` — the generic
primitive (no correlated-subquery case here, unlike the edit-revision
read), confirming it generalizes to a second caller as intended. The final
UPDATE and commit/rollback likewise route through the same adapter
instance instead of duplicated raw SQL per backend.
Also moved item_status_revision/validate_item_status_revision from db.py
into workitemcore.py (same peer-dependency fix as the other work-item
pure helpers); sprint_status_revision/validate_sprint_status_revision stay
in db.py since they belong to sprint-transition logic (set_sprint_status,
close_sprint_with_boundary_event), not yet touched.
Full SQLite suite: 1491 passed, 180 skipped, 0 failed. Full disposable-
PostgreSQL suite: 169 passed, same 8 pre-existing failures (terminal_
recovery_pg.py/authority.py, untouched), zero regressions. Verified via
pyflakes that db.py/pg.py/workitemcore.py have no unresolved names.
Noted but not added: neither backend has a concurrency test proving two
overlapping set_work_item_status transitions on the same item resolve to
exactly one accepted writer (unlike update_work_item_description, which
had this on PostgreSQL already). This is a pre-existing gap on both
backends, not an asymmetry this extraction introduced or one backend
already covered — left as a follow-up rather than expanding this
increment's scope.
Refs: sprintctl#2150, agentops#2144
…e scaffold Completes the P2.1 Claim extraction (4d+4e): ClaimConn grows the same begin_txn/commit/rollback/lock_for_update-style scaffold introduced for work-item CAS ops, plus claim-specific members (lock_capability_arbitration, is_claim_token_collision, maintenance_capability_active_sql, emit_claim_event). create_claim and handoff_claim move into claimcore.py as single implementations; db.py/pg.py keep thin per-backend wrappers. Lock order is preserved exactly per backend -- PostgreSQL's work-item row lock is now taken unconditionally on exclusive creates (previously only reached via the redundant _lock_claim_arbitration_row helper), and SQLite gains the same existence-check read as a harmless byte-for-byte-equivalent addition. require_claim_proof and MAX_CLAIM_TOKEN_INSERT_RETRIES move to claimcore.py (same asymmetry fix as EditConflict/StatusConflict), with db.py re-exporting both for cli.py and pg.py's existing import sites. Verified against both backends: 1660 passed / 8 pre-existing unrelated failures (terminal-recovery ledger tests, a str/datetime PG driver mismatch) against a disposable sprintctl_test_agent/sprintctl_test_run PG instance. Two tests updated to track relocated internals: test_failure_modes.py's token-collision tests now patch claimcore._generate_claim_token (where it's actually called from), and test_pg_integration.py's advisory-lock race test patches _ClaimPg.lock_capability_arbitration instead of the removed _lock_repo_claim_capability_arbitration free function.
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.
What changed
Reconcile P2.1 on current
mainby replaying the exact 12-commit backend-unification chain fromc0ee7edthroughce09f54.The extraction moves backend-neutral row, sprint, track, ref/dep, work-item, event, claim, and transaction-scaffold logic into dedicated core modules while retaining the existing SQLite/PostgreSQL adapters and public CLI behavior.
Why
This keeps the SQLite and PostgreSQL implementations aligned behind shared semantics and provides the foundation for the remaining backend/runtime work.
Scope
This PR is intentionally limited to P2.1. It does not include P4.2 test splitting or P3 CLI extraction work.
Validation
1496 passed, 175 skipped284 passed16 passed126 passedgit diff --checkand Python bytecode compilation passed