Comparing e52c58567c..eaec6113e8 - node - Free-Git: For Devs Not Bully

Twigpine/node

0 Fork 0

mirror of https://github.com/Twigpine/node.git synced 2026-11-13 19:15:11 -10:00

Author	SHA1	Message	Date
beardthelion	eaec6113e8	Merge origin/main into fix/reconciliation-sweep-flakes	2026-11-13 18:39:44 -06:00
beardthelion	ceef11062e	test(node): close the fake S3 connection after each response The fake serves one request per socket and drops it, but its responses carried no Connection: close, so the client kept the dead socket in its pool. With retries disabled (as in production) a reused dead connection failed the restore, and sweep_restores_tigris_cache_miss_and_skips_missing skipped its repo with 'repo acquire failed' on a loaded runner.	2026-11-13 17:37:22 -06:00
beardthelionandKevin Codex	d4895ef372	feat(node): add a commit status reporting surface (#304 ) * feat(db): add migration v24 for status claims, PR head, and push events Schema for the commit-status surface, all three objects in one versioned entry rather than three: an append-only status_claims table, a nullable head_commit column on pull_requests, and repo_push_events for the catch-up poll surface. Version 24 rather than 18 because origin/pr-173 and origin/fix/issue-135-ipfs-cid-tree-gate both claim through 23, and the migration runner keys only on version without comparing the recorded name, so a collision is a silent full skip rather than an error. status_claims orders on a database-assigned bigserial seq, not the uuid id. A uuid v4 is random, so using it as the timestamp tiebreak would let a retried pending claim beat a success roughly half the time when both land in the same rfc3339 tick. Each claim stores the producer's RFC 9421 signature headers and the canonical signed bytes. Append-only preserves rows, but only stored signature material preserves provenance, and a claim nobody can verify after the request is gone is not history a signed-attestation substrate can adopt later. The table is repo_push_events, not push_events: the latter already exists in v1 for agent trust scoring, so a second CREATE TABLE under IF NOT EXISTS is a silent no-op whose follow-on index then fails. The upgrade-path test is the load-bearing one. A sqlx::test provisions an empty database and runs the whole array, so the fresh-chain tests stay green even when DDL is appended to an already-applied entry. * feat(pulls): track the source branch head on open pull requests The rollup needs an honest target commit, and a pull request row stores only branch names. Resolving the branch at read time is racy (the SHA moves between the resolve and the rollup) and gives a merged or deleted-branch pull request no answer at all, since the merge commit is computed and never persisted. The receive-pack path now sets head_commit for open pull requests whose source branch matches an updated ref, one statement keyed on repo, branch, and open status. Excluding closed and merged rows from the WHERE clause is what freezes the value. The merge path stamps the source head it actually consumed, which covers a push landing between the handler reading the row and committing the merge. A ref arriving on the push path is a full refs/heads/ name while source_branch stores the bare branch, so the normalisation is tested directly; a silently non-matching WHERE clause would make this a no-op that still reads green. Branch deletions are skipped rather than stored, since 40 zeros is a target no commit resolves to. The scenario tests drive the update helper directly, because a real receive-pack POST needs a pack file. That proves the mechanism and says nothing about the sink, so a source-scrape test pins the call site in the push handler. Removing that call was verified to turn it red. * feat(api): add the owner-gated commit status write path POST /api/v1/repos/{owner}/{repo}/statuses/{sha} appends a status claim. Authorization runs read-visibility first, then owner, and the order is the security property: authorize_repo_read denies a quarantined repo before the visibility gate and answers with the repo's own not-found, so a caller who cannot read the repo cannot learn it exists. Loading the repo and comparing the owner would answer 403 there and turn the endpoint into an existence oracle. That case is tested directly, since it is what separates a correct implementation from a plausible one. Three caps, all evaluated in the insert transaction and all refusing with 429: per producer and context, distinct contexts per commit, and total rows per repo. The per-tuple cap alone bounds nothing, because the context string and the SHA are both caller-chosen and the SHA is never existence-checked. The route group also carries the per-DID and per-IP limiters that the creation routes use; the plain write group carries neither and this endpoint appends on every call. Wider than the endpoint: require_signature now attaches the verified RFC 9421 material as a request extension, so a handler can persist what the signature actually covered. Rebuilding the canonical string in the handler would store a string nobody verified, and none of it survives the request. The write fails closed with a 500 when that material is absent, rather than storing an empty payload, because a claim nobody can re-verify is not history the later attestation substrate can adopt. Nine injected-defect mutations confirm the gates are load-bearing rather than decorative, including that removing the route group's merge leaves a group that compiles, lints, and does not exist at runtime. * feat(api): serve the combined commit status behind the visibility gate GET /api/v1/repos/{owner}/{repo}/commits/{sha}/status returns the projection over the claim history, computed per read rather than materialized. A materialized index gated only at write time is how this codebase previously kept serving a repo's slug, owner, branches, and SHAs after it was made private, so the regression test writes a claim while public, tightens visibility, and asserts an anonymous read finds no trace of it. Three outcomes stay distinct, which is the point of the unit. A caller who cannot read the repo gets the repo's own not-found, compared byte for byte against a missing repo so the endpoint is not an existence oracle. A commit nobody has reported on gets 200 with the explicit pending zero-count body, asserted exactly so no client can render it as green. A failed lookup propagates as a 500 carrying the db_error code, never as an empty success; nothing on this path uses ok-or-default. Latest-per-context is decided by the database-assigned sequence, not by the timestamp and not by the uuid. The ordering test seeds the winning row with the lexically smaller id and a far older timestamp, so it fails under either wrong key rather than passing by luck. The projection filters to claims authorized by the current owner, so an ownership transfer drops the prior owner's claims from the answer while the history keeps them. That filter is a set-membership test inside the query, which is the constraint the delegated-capability follow-on inherits. It expands the owner DID into its equivalent forms, and a test runs that expansion against did_matches over a ten-case matrix so the two cannot drift apart silently. * feat(api): add the pull request head rollup GET /api/v1/repos/{owner}/{repo}/pulls/{number}/status answers for the pull request's head. Both read surfaces now call one extracted projection, so "the rollup uses the same projection as the commit read" holds by construction rather than by two implementations agreeing today. A pull request row stores branch names, so the head has to come from somewhere. Preference order is the stored head, then a database-backed branch lookup for an open pull request that has none. No path in this module acquires the repository or lists refs from disk: that would download the whole repository from object storage on a cold node, and this read group carries no rate limiter. A source-read test enforces that, and asserts its own scan still covers the handler so it cannot shrink to vacuously passing. The read-side persist is self-limiting by SQL, not by handler ordering: the update is conditioned on the head being absent and the pull request open, so a push landing mid-request cannot be rolled back to a staler tip and a closed pull request is never back-filled. Reading the same pull request twice resolves the branch once. That property is invisible in the response. A variant that re-resolved on every call and then still preferred the stored head would return byte-identical bodies across the whole suite, so the test counts the work done rather than the results emitted, and injecting exactly that variant reddens only the counter assertion. An unresolvable head is not a fifth wire state. The state field stays inside the four values and the condition rides in head_resolved beside the pull request's own state, so the two cases differ on that boolean alone and both are tested. * feat(api): add the catch-up poll surface for missed push webhooks Webhook delivery is spawned, fire-and-forget, and never retried, so a checker whose endpoint was unreachable during a push simply never learns about the commit. Rather than add retry machinery and an unbounded queue pointed at an owner-supplied URL, delivery reliability becomes a read-side property: a push records an event, and a subscriber walks forward from a cursor to find every commit a webhook would have told it about. Containment is the load-bearing constraint. The row goes into repo_push_events, written only by the receive-pack path and read only by this repo-scoped, gate-checked handler. It must never go into received_ref_updates, which the unauthenticated global feed also reads; writing local pushes there would publish private-repo pushes on an anonymous surface. A test pushes to a private repo and asserts nothing appears on that feed, and retargeting the write reddens it. The cursor emits a canonical Z-form timestamp with fixed sub-second width rather than the default rfc3339. The default emits a +00:00 offset, and + decodes to a space in a query string, so a poller echoing the cursor back re-read the same page forever. The fixed width also makes the TEXT column's lexicographic order match time order for the keyset predicate. Found by seeding a timestamp collision, not by reading. The scenario tests drive the recorder directly, so a source-scrape test pins its call site in the push handler, the same way the stored-head update is pinned. Removing that call was verified to turn it red. * fix(api): resolve the rollup head from push events, not pinned CIDs Two review findings. The fallback read branch_cids, whose only production writer sits inside a Pinata pin-success branch. With no Pinata configured that table is never written, so on a default node the fallback could never resolve and every open pull request without a stored head answered head_resolved false forever. It now reads repo_push_events, which the receive-pack path writes unconditionally for every ref update, with both predicates and the ordering in SQL against the existing keyset index. Still one database read per request, still no repository acquire. This is better, not complete: repo_push_events only carries pushes that land after this ships, so a pull request whose branch was last pushed before deployment still will not resolve until it is pushed again. There is no backfill. The best-effort persist propagated its error, so a transient database failure turned a rollup that had already resolved its answer into a 500. It now catches and logs, matching the sibling writes on the push path. The test asserts the persist genuinely fails before driving the read, so it cannot pass vacuously. Also corrected a doc comment that claimed the resolve branch is never taken twice for one pull request. Only the persist is once-per-pull- request; the resolve attempt repeats on every read until it succeeds. The accompanying test pins the corrected wording rather than a behavior change, since the old code counted the same way. The resolve counter is process-global while sqlx test databases are per-test, so unguarded tests were inflating the counts guarded tests assert on. The guard now covers every test that triggers a resolve. * fix(status): make a stored claim actually re-verifiable The row stored the RFC 9421 signing string under a column called signed_payload. That string covers the body only through the content digest, so it proved someone signed a request with some digest but not that this claim row was what they signed. The whole append-only argument rests on a claim staying verifiable after the request is gone, so the row now carries the request body as well, in a column that says so, and a test re-verifies a claim from the row alone: body to digest, digest into the signing string, signature over that string under the producer's key. Its negative mutates the stored state and shows the same procedure refuses. Migration v24 is amended in place rather than stacked, since it has never shipped. The persisted material had no size bound and is caller-influenced, so each field is now bounded. The write response returned the whole claim, so the wire carried the stored bytes as a JSON integer array; it now returns a client-facing type and StatusClaim no longer derives Serialize, so the leak cannot reappear by accident. Carrying the body as Bytes rather than a Vec matters: the middleware runs on every signed request including receive-pack POSTs bounded at 2 GB, so copying would have doubled peak memory on every push to serve a field only the status write reads. * fix(status): make the caps hold and the projection mean one identity Five reviewers independently flagged the capped insert. It counted then inserted inside one transaction with a docstring promising a concurrent writer could not slip past, which READ COMMITTED does not deliver, and every cap test was sequential so deleting the transaction would have kept them green. A per-repo advisory lock now serializes writers on the bound they are both testing. The new test runs eight writers through a barrier at cap minus one: without the lock it left 4, 5, and 5 rows against a cap of 3 on consecutive runs, so the window was real. The projection compared producer_did as a raw column while the handler stored whatever spelling the signer used, so one owner writing as did:key:X and as X produced two entries for one context and the superseded claim kept voting. Identity is now canonicalized at write time, and the read filter is a single equality rather than a set. That fix had a trap. Applying normalize_owner_key directly, as the finding suggested, collapses to the bare key, which is not a parseable DID, and would have quietly undone the re-verifiability the previous commit established. canonical_did canonicalizes to the full did:key form instead and is written as a function of normalize_owner_key so the equivalence with did_matches still holds exactly, including the case where a naive prefix-add would merge two distinct identities. With that in place the ported copy of the DID collapse in the status module is gone rather than relocated, and did_matches itself now delegates, so the Rust gate, the stored column, and the SQL all trace to one definition. The projection also selected the signature columns it never renders, which the previous commit made worse by adding the request body. It has its own narrow type now, enforced by a source-read test. The per-repo cap counted for all time with nothing pruning the table, so a repo that reached it was permanently closed while 429 told clients to keep retrying. It is a rolling window now, which still bounds the fan-out a caller-chosen SHA allows, and makes the refusal true. Also folded two count queries that scanned the same rows into one. * fix(api): order the poll cursor on a sequence and batch the push writes The push-event cursor paged on an application-stamped wall clock, so a row stamped later could commit earlier and a poller past that point would never see the earlier row. A clock step backwards widens it. It now pages on a database-assigned sequence, the same decision status_claims already made, so the two surfaces do not disagree about what ordering means. The rollup's branch resolve had the same flaw and is fixed with it. The receive-pack path issued one round trip per ref for each of two writers, sequentially, on the user's push, with nothing bounding the ref count. Both are single statements now and the per-push fan-out is capped, with a warning when it truncates rather than silent loss. The head update batches through a VALUES join, with last-write-wins dedup on duplicate branches: git will not produce them, but a join over duplicates picks arbitrarily where the loop it replaced was deterministic. The cursor was unvalidated. A malformed value produced a wrong page instead of a refusal, limit zero returned a page with a null cursor, and a poller persisting that cursor silently restarted from the beginning of history. It is validated, the limit is clamped, and the returned cursor never moves backwards. This changes the query parameter names, which is a breaking wire change on an endpoint that has never shipped; every consumer was checked and only the route registration reads them. Three copies of the source-scraping parser became one shared helper. That refactor exposed a real gap: the empty-region mutation reddened four of the five guards, but the gossip-containment guard passed on an empty region because it only asserts a must-not. It now asserts its scan covered what it claims to cover, so a helper bug cannot make it vacuously green. status.rs is split into status/mod.rs and status/tests.rs, production logic unchanged. Four things read the old path and all were retargeted, including 22 mutation targets across four specs, each re-confirmed to still match exactly once. * fix(status): make a replayed write idempotent and stop dropping refs Both defects came from an independent cross-model review, and neither was visible to the reviewers that share this session's model. A signed write was protected only by clock skew: no nonce, no idempotency key, no record of signatures already seen. So a captured request replayed inside the skew window inserted a new row with a new sequence number, and because the projection takes the latest claim per producer and context, a stale success outranked the failure that superseded it. The append-only design is what made this reversible rather than merely duplicative: a replay that only duplicated a row would be harmless, a replay that earns a fresh sequence flips the answer. An earlier reviewer saw the replay window and concluded append-only bounded it, which had the mechanism exactly backwards. An exact replay is now idempotent instead of an error. The row carries a digest over the signature, its input, and the body, a unique index enforces it in the database rather than a check-then-insert race, and a repeat returns the original claim. Erroring would have punished the legitimate case this design otherwise handles worst: a client whose response was lost retrying the same signed request and concluding its report failed when it had succeeded. The fan-out cap was mine, added while fixing unbounded work, and it truncated at 256 refs after receive-pack had already accepted them. A push whose pull request source branch sat past that point left the head permanently stale with no push event, announced only by a log line git had already contradicted. The writers chunk now, so nothing accepted is dropped, and the bound that remains refuses before the accept rather than after: chunking bounds a statement, not a request, and the work that scales with ref count sits upstream anyway, one protection query per ref before the service runs and one certificate and webhook per ref after, with body limits disabled on the git routes. Removing the cap also exposed a quadratic branch dedupe it had been keeping cheap; that is a map now. * fix(db): stop the push cursor skipping a concurrently committed row The sequence is a bigserial, so Postgres allocates it at INSERT and makes it visible at COMMIT, and nextval does not roll back. Two overlapping writes to one repo could therefore commit out of sequence order: one allocates, another allocates and commits, a poller reads the later row and advances, and the earlier row then becomes visible behind a cursor that has already passed it. Never delivered, no error, no gap signal, which is the one thing the catch-up surface exists to prevent. This is the second time this branch has traded one ordering hazard for another. The cursor moved off a wall clock because stamping and committing disagree; it moved onto a sequence where allocating and committing disagree. The insert now runs in an explicit transaction that takes a per-repo advisory lock, so the lock releases at commit and no writer can allocate until the previous row is visible. The wait is bounded, because the lock sits on a pooled connection on the inline push path and an unbounded wait would turn one busy repo into pool exhaustion for unrelated requests. Three attempts at a one second timeout leaves headroom under the pool's own acquire timeout. That bound needs the caller to stop swallowing failures. It warned and continued, so any error the lock introduced would silently drop those events forever while the push had already returned success, reaching the same outcome by another route. It now retries once and then logs at error level naming the repo, the refs, and the SHAs that were lost. The defect is pinned by a test that owns both transaction boundaries rather than a barrier. A barrier cannot reproduce it: before this change the write was a single autocommit statement, so allocate and commit were one round trip from the client and the interleaving window was never open to a caller of the public API. * fix(auth): carry the request body only where a route persists it The signature middleware handed every signed request a second handle to the buffered body, and only the status write reads it. The field is optional now and populated behind a marker the persisting route group applies. Be accurate about what that saves, because the first reading of this finding overstated it and the plan was corrected before implementation. The push handler takes its body as an extractor, so axum consumes the request and drops every extension at that point; the extra handle lived across the remaining middleware chain, not the handler's lifetime. The whole pack really is pinned for the whole receive-pack, but by the handler's own parameter, which is pre-existing and untouched here. Layer order is the failure mode worth guarding. A marker applied inside the auth layers is never seen by the middleware that reads it, so the body silently stops being captured while every test stays green. The mutation that proves this leaves the layer present on the same group and only moves it inside, and the production-router test still reddens. The handler also refuses an absent body rather than storing an empty column, so a lost marker fails loudly at the one place it matters. Four comments claimed a timestamp or a uuid tiebreak decided ordering when the code orders on the sequence. Three were named in review; the fourth turned up next to them, calling the timestamp format load-bearing for a cursor that no longer reads it. The mirror-dedup comments that correctly document timestamp ordering were checked and left alone. The shared scrape helper advanced by one byte to stop an end anchor matching at position zero, which reports a missing anchor for any region starting on a multibyte character. It advances by a character now. Not reachable today since every anchor is ASCII, but five guards share this helper and one that silently finds nothing is worse than one that fails. * fix(status): bind a stored claim to the body its signature covered A claim is provenance, so the signature stored beside it has to reach the bytes stored beside it. It did not. The signing string covers the body only through a content-digest line, and require_signature rebuilds the covered values from the request: with no Content-Digest header the covered digest is the empty string on the signing side and the verifying side alike, the Ed25519 check passes over method, path and nothing else, and the digest-versus-body comparison was skipped for want of a header. An owner could sign that once and post any body under it. The row that produced carried a valid signature, a signing string that verifies under the owner's key, and a request_body the signing string says nothing about — history nobody can re-verify, which is the one thing the row is for. The write path now recomputes the digest of the bytes it is about to persist and requires the signing string to carry exactly that line, which is the write-time form of the check re_verify already ran against a stored row. Line-exact rather than a substring search: build_signing_string emits one component per line, and a contains would also be satisfied by the digest appearing inside another component's value. This is deliberately not the only line of defence, and deliberately not the middleware's. PR #306 requires the header in require_signature and closes the same hole one layer earlier for every signed route; that belongs there, because no other route's correctness should depend on this handler. Neither substitutes for the other. Without #306 the empty digest is still accepted everywhere else; without this, a later relaxation of the middleware resumes writing unverifiable history in silence. Nothing here touches require_signature, so the two do not collide. The route's advertised 8 KiB body bound was also a bound on what gets stored rather than on what gets buffered: require_signature collects and hashes the whole body before any handler runs, so by the time the handler measured it the allocation had happened, and concurrent requests multiplied exactly the cost the number was written to prevent. A RequestBodyLimitLayer now carries the same figure at the transport, outside the auth layers and inside the rate limiters, matching the shape the git routes already use. The handler keeps its own check: it is the one that holds for a handler reached by any other route table. The hand-injected test material had to become coherent with the request it rides on, because a stand-in built over other bytes now takes every bare-router test down the refusal branch. It is built from the body it accompanies and carries a real content-digest line. * fix(api): apply a push's side effects only to the refs git accepted receive-pack exits zero when the process ran, not when the push applied. A non-fast-forward, a hook rejection or a failed update comes back as an ng pkt-line inside a response this handler forwarded without reading, so every side effect keyed on the refs parsed from the REQUEST fired for refs that were refused: the pull request on that branch had its stored head moved to a commit this node does not have, and the catch-up poll surface handed subscribers a SHA that resolves to nothing. The report is now parsed and only the refs it names with ok are used. unpack fail accepts nothing, since the pack never landed. A report that does not parse at all is inconclusive and keeps every declared ref: a client that never requested report-status is told nothing about individual refs, and reading that silence as rejection would permanently drop the events and the head update for a push git really did accept, which is the failure the poll surface exists to prevent. The parser is ported from PR #384, which introduced it for this same problem, so the two branches converge on one definition of "accepted" rather than growing two. Whichever lands second should drop its copy. It diverges by three lines, and they are load-bearing: git double wraps the report when the client negotiates side-band-64k, which git push over smart HTTP does. The observed framing from git 2.50.1 is an outer pkt-line carrying a band byte whose payload is itself a pkt-line stream, so one pass of strip_sideband leaves "000eunpack ok" as the first line, the parse fails, and every ref falls back to inconclusive — the parser answering "cannot tell" for exactly the pushes it exists to classify. A second pass peels the inner layer and is a no-op on the single-wrapped shape. The tests carry the real captured byte layout, both wrappings, so this is pinned rather than assumed. Both writes also moved inside the repository write lock. That lock orders the ref updates of two concurrent pushes; released before these ran, the pair was an unordered race, and B's unconditional UPDATE landing before A's leaves the stored head — which the rollup prefers over its fallback — pinned to a commit the branch has moved past. record_push_events has the same inversion by a different route, since latest_push_sha_for_ref reads the highest seq and seq order is insert order. There is no per-row version to make the UPDATE conditional on, so the ordering has to come from the lock. Holding it across two batched statements on an already open pool, on a path that just ran a subprocess over the whole pack, is the cheaper half of that trade. A source guard pins the position, because this is a property of where the calls sit. Separately, the 10,000-ref bound was checked only after the parser had scanned the entire request and kept three heap strings per valid pkt-line. The git routes raise the body limit to GITLAWB_MAX_PACK_BYTES — 2 GB by default — so a signed caller could force the whole scan and the whole allocation and only then be told 400: the work the cap exists to refuse, done in full before the refusal. The parser stops one past the cap, which is still enough for bound_declared_refs to refuse on, and the message no longer reports a count that would now be a floor rather than the request's real one. * fix(api): make the push-event cursor opaque and bound to its repository The cursor was the table-global bigserial, handed to clients verbatim and accepted back as any non-negative integer, then passed straight into a per-repo seq > $1. Two things followed from that, and both were silent. A value issued by a DIFFERENT repository — or by this one before a restore — is an ordinary larger number here, so the query matched nothing, the surface answered 200 with an empty page and echoed the value back, and the subscriber sat permanently past history it had never received with nothing anywhere reading as an error. And because repo_push_events is written by every repository on the node, the gaps between one repository's cursors measured how much the others pushed in between, private ones included. The cursor is now an opaque token carrying a version, a binding to (node DID, repository), and the id of the last event served. The binding is a hash and not a MAC, because there is nothing to forge: the position inside is a row id resolved against this repository's own rows anyway. What it buys is that a foreign cursor is refused by shape, with a message naming which mistake was made, instead of being applied to a repository it was never issued for. An id that resolves to nothing — a token kept across a restore that lost the row — is a 400 that names the recovery, not an empty page the poller reads as being up to date. The global sequence still orders the walk internally, which is what keeps the page cheap, but it no longer leaves the node. The position is the row id, which this surface already publishes in every event, so the token discloses nothing new. The two properties an earlier round established are kept, in their new shape: next_cursor is never null and never moves backwards, and the start of history is an expressible position rather than an absent one, so a first poll of a repository with no events still hands back a token the endpoint accepts back rather than a value a persisting poller would misread. * refactor(node): simplify commit-status surface after main merge Land the merge on one mechanism per concern instead of two: - landed_refs now intersects the declared set with the ok_set already derived for the outcome commit, instead of re-parsing the report in accepted_ref_updates; the helper's unreachable keep-declared arm (a trap its own doc contradicted) is gone with the second parse. - bound_declared_refs is deleted: main's parse_ref_updates already refuses past MAX_REF_UPDATES inside the command loop, so the post-parse check was dead code whose tests pinned an abandoned design; the ordering test now pins the parse itself before git. - merge_pr resolves the source head with read_ref (exact-name show-ref) instead of listing every ref in the repo. - normalize_sha reuses is_hex_sha; the refs/heads/ mapping reuses store::local_branch_ref; test-only db methods and their SQL are cfg(test)-gated; duplicated body_json/status_and_bytes test helpers are hoisted to test_support. Merge-artifact repair found by running the tests: scrape guards track the renamed landed_refs binding and the reclaimed.release / receive_pack_raw_with_reflog call names; the status migration upgrade test rolls back to and asserts version 47 (not 24, which main now owns); a doubled cfg(test), an orphaned doc block, and a stale dead_code allow on StatusClaim.seq are corrected. * fix(node): harden status surface writers and close-path head freeze Review-driven fixes on the merged surface: - close_pr freezes the source head it decided against, resolving the recorded push for the branch (then disk) when head_commit is unset; a PR closed before its first status read no longer reports head_resolved=false forever. - insert_status_claim_capped bounds its advisory-lock wait with SET LOCAL lock_timeout and retries on transient classes, matching the push-events writer's contract. - record_push_events retries only transient failures (the inner bound already retries lock waits; a permanent error's second run is a guaranteed re-failure) and update_open_pr_heads retries once on transient and escalates to error naming the lost branch@sha rows. - parse_ref_updates lowercases old/new SHAs so pushed values match the claim side's lowercase normalization. - combined_state folds unrecognized stored states to failure, never silently to success. * fix(node): write status surfaces from proven landings in the effect executor The stored-head and catch-up poll writes ran pre-release inside the repo write lock, so a refused publish left rows claiming a landing the node had deleted, and a landing reconcile proved only later emitted nothing at all. Both are durable accounting and belong to the effect executor: the live path reaches it only after the publish verdict, and the drain replays it identically once reconcile proves the landing. The ordering the in-lock placement guaranteed is kept differently: a head write is conditioned on the branch still sitting at the landing's tip, so a superseded landing never pins the rollup to a commit the branch moved past. Event ids derive from (request_id, ordinal) so a recovery replay collapses on ON CONFLICT instead of double-delivering. Also: drop the node-global seq from the create response (id already names the row), pin the claim body's signed format (deny_unknown_fields + v=1), and carry Retry-After on 429. * fix(node): add head_commit to test-harness PR fixture * test(node): drive the status and push-events routes in the deny sweep The mounted-route completeness gates require every repo-scoped route to be a driven registry row: the status read surfaces and the push-events poll are read-gated (existence-hiding 404 for anon and signed non-readers, owner reach twin proves the gate) and the claim write is owner-gated on a public repo. {sha} fills as a fixed 40-hex placeholder. * fix(node): refuse re-closing a closed PR, drop 429 Retry-After, pin test git identity - close_pr refuses a PR that is not open and the UPDATE is conditional on status='open', so a repeated close cannot store the branch's current tip on a historical pull request. - Remove Retry-After from 429s. The status-claim caps are per-commit and append-only, so a 1s hint invited retries that cannot succeed. - bare_repo_with_ref sets git author and committer env so the heads_still_at_tip tests pass on hosts with no global git identity (CI). * fix(node): close_pr fallback head fills an empty head only (#304) A head stored by a push between the handler's read and the UPDATE is no longer replaced by the fallback. * fix(node): purge status_claims and repo_push_events in delete_repo_by_id Both tables are keyed on repo_id and were missed by the direct-child cascade, so deleting a repo left signed status bodies and poll-feed rows behind. The cascade test now seeds both and asserts zero rows. --------- Co-authored-by: Kevin Codex <[email protected]>	2026-11-13 21:53:53 +00:00

Author

SHA1

Message

Date

beardthelion

eaec6113e8

Merge origin/main into fix/reconciliation-sweep-flakes

2026-11-13 18:39:44 -06:00

beardthelion

ceef11062e

test(node): close the fake S3 connection after each response

The fake serves one request per socket and drops it, but its responses
carried no Connection: close, so the client kept the dead socket in its
pool. With retries disabled (as in production) a reused dead connection
failed the restore, and sweep_restores_tigris_cache_miss_and_skips_missing
skipped its repo with 'repo acquire failed' on a loaded runner.

2026-11-13 17:37:22 -06:00

beardthelionandKevin Codex

d4895ef372

feat(node): add a commit status reporting surface (#304 )

* feat(db): add migration v24 for status claims, PR head, and push events

Schema for the commit-status surface, all three objects in one versioned
entry rather than three: an append-only status_claims table, a nullable
head_commit column on pull_requests, and repo_push_events for the
catch-up poll surface.

Version 24 rather than 18 because origin/pr-173 and
origin/fix/issue-135-ipfs-cid-tree-gate both claim through 23, and the
migration runner keys only on version without comparing the recorded
name, so a collision is a silent full skip rather than an error.

status_claims orders on a database-assigned bigserial seq, not the uuid
id. A uuid v4 is random, so using it as the timestamp tiebreak would let
a retried pending claim beat a success roughly half the time when both
land in the same rfc3339 tick.

Each claim stores the producer's RFC 9421 signature headers and the
canonical signed bytes. Append-only preserves rows, but only stored
signature material preserves provenance, and a claim nobody can verify
after the request is gone is not history a signed-attestation substrate
can adopt later.

The table is repo_push_events, not push_events: the latter already
exists in v1 for agent trust scoring, so a second CREATE TABLE under
IF NOT EXISTS is a silent no-op whose follow-on index then fails.

The upgrade-path test is the load-bearing one. A sqlx::test provisions
an empty database and runs the whole array, so the fresh-chain tests
stay green even when DDL is appended to an already-applied entry.

* feat(pulls): track the source branch head on open pull requests

The rollup needs an honest target commit, and a pull request row stores
only branch names. Resolving the branch at read time is racy (the SHA
moves between the resolve and the rollup) and gives a merged or
deleted-branch pull request no answer at all, since the merge commit is
computed and never persisted.

The receive-pack path now sets head_commit for open pull requests whose
source branch matches an updated ref, one statement keyed on repo,
branch, and open status. Excluding closed and merged rows from the WHERE
clause is what freezes the value. The merge path stamps the source head
it actually consumed, which covers a push landing between the handler
reading the row and committing the merge.

A ref arriving on the push path is a full refs/heads/ name while
source_branch stores the bare branch, so the normalisation is tested
directly; a silently non-matching WHERE clause would make this a no-op
that still reads green. Branch deletions are skipped rather than stored,
since 40 zeros is a target no commit resolves to.

The scenario tests drive the update helper directly, because a real
receive-pack POST needs a pack file. That proves the mechanism and says
nothing about the sink, so a source-scrape test pins the call site in
the push handler. Removing that call was verified to turn it red.

* feat(api): add the owner-gated commit status write path

POST /api/v1/repos/{owner}/{repo}/statuses/{sha} appends a status claim.
Authorization runs read-visibility first, then owner, and the order is
the security property: authorize_repo_read denies a quarantined repo
before the visibility gate and answers with the repo's own not-found, so
a caller who cannot read the repo cannot learn it exists. Loading the
repo and comparing the owner would answer 403 there and turn the
endpoint into an existence oracle. That case is tested directly, since
it is what separates a correct implementation from a plausible one.

Three caps, all evaluated in the insert transaction and all refusing
with 429: per producer and context, distinct contexts per commit, and
total rows per repo. The per-tuple cap alone bounds nothing, because the
context string and the SHA are both caller-chosen and the SHA is never
existence-checked. The route group also carries the per-DID and per-IP
limiters that the creation routes use; the plain write group carries
neither and this endpoint appends on every call.

Wider than the endpoint: require_signature now attaches the verified
RFC 9421 material as a request extension, so a handler can persist what
the signature actually covered. Rebuilding the canonical string in the
handler would store a string nobody verified, and none of it survives
the request. The write fails closed with a 500 when that material is
absent, rather than storing an empty payload, because a claim nobody can
re-verify is not history the later attestation substrate can adopt.

Nine injected-defect mutations confirm the gates are load-bearing rather
than decorative, including that removing the route group's merge leaves
a group that compiles, lints, and does not exist at runtime.

* feat(api): serve the combined commit status behind the visibility gate

GET /api/v1/repos/{owner}/{repo}/commits/{sha}/status returns the
projection over the claim history, computed per read rather than
materialized. A materialized index gated only at write time is how this
codebase previously kept serving a repo's slug, owner, branches, and
SHAs after it was made private, so the regression test writes a claim
while public, tightens visibility, and asserts an anonymous read finds
no trace of it.

Three outcomes stay distinct, which is the point of the unit. A caller
who cannot read the repo gets the repo's own not-found, compared byte
for byte against a missing repo so the endpoint is not an existence
oracle. A commit nobody has reported on gets 200 with the explicit
pending zero-count body, asserted exactly so no client can render it as
green. A failed lookup propagates as a 500 carrying the db_error code,
never as an empty success; nothing on this path uses ok-or-default.

Latest-per-context is decided by the database-assigned sequence, not by
the timestamp and not by the uuid. The ordering test seeds the winning
row with the lexically smaller id and a far older timestamp, so it fails
under either wrong key rather than passing by luck.

The projection filters to claims authorized by the current owner, so an
ownership transfer drops the prior owner's claims from the answer while
the history keeps them. That filter is a set-membership test inside the
query, which is the constraint the delegated-capability follow-on
inherits. It expands the owner DID into its equivalent forms, and a test
runs that expansion against did_matches over a ten-case matrix so the
two cannot drift apart silently.

* feat(api): add the pull request head rollup

GET /api/v1/repos/{owner}/{repo}/pulls/{number}/status answers for the
pull request's head. Both read surfaces now call one extracted
projection, so "the rollup uses the same projection as the commit read"
holds by construction rather than by two implementations agreeing today.

A pull request row stores branch names, so the head has to come from
somewhere. Preference order is the stored head, then a database-backed
branch lookup for an open pull request that has none. No path in this
module acquires the repository or lists refs from disk: that would
download the whole repository from object storage on a cold node, and
this read group carries no rate limiter. A source-read test enforces
that, and asserts its own scan still covers the handler so it cannot
shrink to vacuously passing.

The read-side persist is self-limiting by SQL, not by handler ordering:
the update is conditioned on the head being absent and the pull request
open, so a push landing mid-request cannot be rolled back to a staler
tip and a closed pull request is never back-filled. Reading the same
pull request twice resolves the branch once.

That property is invisible in the response. A variant that re-resolved
on every call and then still preferred the stored head would return
byte-identical bodies across the whole suite, so the test counts the
work done rather than the results emitted, and injecting exactly that
variant reddens only the counter assertion.

An unresolvable head is not a fifth wire state. The state field stays
inside the four values and the condition rides in head_resolved beside
the pull request's own state, so the two cases differ on that boolean
alone and both are tested.

* feat(api): add the catch-up poll surface for missed push webhooks

Webhook delivery is spawned, fire-and-forget, and never retried, so a
checker whose endpoint was unreachable during a push simply never learns
about the commit. Rather than add retry machinery and an unbounded queue
pointed at an owner-supplied URL, delivery reliability becomes a
read-side property: a push records an event, and a subscriber walks
forward from a cursor to find every commit a webhook would have told it
about.

Containment is the load-bearing constraint. The row goes into
repo_push_events, written only by the receive-pack path and read only by
this repo-scoped, gate-checked handler. It must never go into
received_ref_updates, which the unauthenticated global feed also reads;
writing local pushes there would publish private-repo pushes on an
anonymous surface. A test pushes to a private repo and asserts nothing
appears on that feed, and retargeting the write reddens it.

The cursor emits a canonical Z-form timestamp with fixed sub-second
width rather than the default rfc3339. The default emits a +00:00
offset, and + decodes to a space in a query string, so a poller echoing
the cursor back re-read the same page forever. The fixed width also
makes the TEXT column's lexicographic order match time order for the
keyset predicate. Found by seeding a timestamp collision, not by
reading.

The scenario tests drive the recorder directly, so a source-scrape test
pins its call site in the push handler, the same way the stored-head
update is pinned. Removing that call was verified to turn it red.

* fix(api): resolve the rollup head from push events, not pinned CIDs

Two review findings.

The fallback read branch_cids, whose only production writer sits inside
a Pinata pin-success branch. With no Pinata configured that table is
never written, so on a default node the fallback could never resolve and
every open pull request without a stored head answered head_resolved
false forever. It now reads repo_push_events, which the receive-pack
path writes unconditionally for every ref update, with both predicates
and the ordering in SQL against the existing keyset index. Still one
database read per request, still no repository acquire.

This is better, not complete: repo_push_events only carries pushes that
land after this ships, so a pull request whose branch was last pushed
before deployment still will not resolve until it is pushed again. There
is no backfill.

The best-effort persist propagated its error, so a transient database
failure turned a rollup that had already resolved its answer into a 500.
It now catches and logs, matching the sibling writes on the push path.
The test asserts the persist genuinely fails before driving the read, so
it cannot pass vacuously.

Also corrected a doc comment that claimed the resolve branch is never
taken twice for one pull request. Only the persist is once-per-pull-
request; the resolve attempt repeats on every read until it succeeds.
The accompanying test pins the corrected wording rather than a behavior
change, since the old code counted the same way.

The resolve counter is process-global while sqlx test databases are
per-test, so unguarded tests were inflating the counts guarded tests
assert on. The guard now covers every test that triggers a resolve.

* fix(status): make a stored claim actually re-verifiable

The row stored the RFC 9421 signing string under a column called
signed_payload. That string covers the body only through the content
digest, so it proved someone signed a request with some digest but not
that this claim row was what they signed. The whole append-only argument
rests on a claim staying verifiable after the request is gone, so the
row now carries the request body as well, in a column that says so, and
a test re-verifies a claim from the row alone: body to digest, digest
into the signing string, signature over that string under the producer's
key. Its negative mutates the stored state and shows the same procedure
refuses.

Migration v24 is amended in place rather than stacked, since it has
never shipped.

The persisted material had no size bound and is caller-influenced, so
each field is now bounded. The write response returned the whole claim,
so the wire carried the stored bytes as a JSON integer array; it now
returns a client-facing type and StatusClaim no longer derives
Serialize, so the leak cannot reappear by accident.

Carrying the body as Bytes rather than a Vec matters: the middleware
runs on every signed request including receive-pack POSTs bounded at
2 GB, so copying would have doubled peak memory on every push to serve
a field only the status write reads.

* fix(status): make the caps hold and the projection mean one identity

Five reviewers independently flagged the capped insert. It counted then
inserted inside one transaction with a docstring promising a concurrent
writer could not slip past, which READ COMMITTED does not deliver, and
every cap test was sequential so deleting the transaction would have
kept them green. A per-repo advisory lock now serializes writers on the
bound they are both testing. The new test runs eight writers through a
barrier at cap minus one: without the lock it left 4, 5, and 5 rows
against a cap of 3 on consecutive runs, so the window was real.

The projection compared producer_did as a raw column while the handler
stored whatever spelling the signer used, so one owner writing as
did:key:X and as X produced two entries for one context and the
superseded claim kept voting. Identity is now canonicalized at write
time, and the read filter is a single equality rather than a set.

That fix had a trap. Applying normalize_owner_key directly, as the
finding suggested, collapses to the bare key, which is not a parseable
DID, and would have quietly undone the re-verifiability the previous
commit established. canonical_did canonicalizes to the full did:key
form instead and is written as a function of normalize_owner_key so the
equivalence with did_matches still holds exactly, including the case
where a naive prefix-add would merge two distinct identities.

With that in place the ported copy of the DID collapse in the status
module is gone rather than relocated, and did_matches itself now
delegates, so the Rust gate, the stored column, and the SQL all trace to
one definition.

The projection also selected the signature columns it never renders,
which the previous commit made worse by adding the request body. It has
its own narrow type now, enforced by a source-read test.

The per-repo cap counted for all time with nothing pruning the table, so
a repo that reached it was permanently closed while 429 told clients to
keep retrying. It is a rolling window now, which still bounds the
fan-out a caller-chosen SHA allows, and makes the refusal true.

Also folded two count queries that scanned the same rows into one.

* fix(api): order the poll cursor on a sequence and batch the push writes

The push-event cursor paged on an application-stamped wall clock, so a
row stamped later could commit earlier and a poller past that point
would never see the earlier row. A clock step backwards widens it. It
now pages on a database-assigned sequence, the same decision
status_claims already made, so the two surfaces do not disagree about
what ordering means. The rollup's branch resolve had the same flaw and
is fixed with it.

The receive-pack path issued one round trip per ref for each of two
writers, sequentially, on the user's push, with nothing bounding the ref
count. Both are single statements now and the per-push fan-out is
capped, with a warning when it truncates rather than silent loss. The
head update batches through a VALUES join, with last-write-wins dedup on
duplicate branches: git will not produce them, but a join over
duplicates picks arbitrarily where the loop it replaced was
deterministic.

The cursor was unvalidated. A malformed value produced a wrong page
instead of a refusal, limit zero returned a page with a null cursor, and
a poller persisting that cursor silently restarted from the beginning of
history. It is validated, the limit is clamped, and the returned cursor
never moves backwards. This changes the query parameter names, which is
a breaking wire change on an endpoint that has never shipped; every
consumer was checked and only the route registration reads them.

Three copies of the source-scraping parser became one shared helper.
That refactor exposed a real gap: the empty-region mutation reddened
four of the five guards, but the gossip-containment guard passed on an
empty region because it only asserts a must-not. It now asserts its scan
covered what it claims to cover, so a helper bug cannot make it
vacuously green.

status.rs is split into status/mod.rs and status/tests.rs, production
logic unchanged. Four things read the old path and all were retargeted,
including 22 mutation targets across four specs, each re-confirmed to
still match exactly once.

* fix(status): make a replayed write idempotent and stop dropping refs

Both defects came from an independent cross-model review, and neither
was visible to the reviewers that share this session's model.

A signed write was protected only by clock skew: no nonce, no
idempotency key, no record of signatures already seen. So a captured
request replayed inside the skew window inserted a new row with a new
sequence number, and because the projection takes the latest claim per
producer and context, a stale success outranked the failure that
superseded it. The append-only design is what made this reversible
rather than merely duplicative: a replay that only duplicated a row
would be harmless, a replay that earns a fresh sequence flips the
answer. An earlier reviewer saw the replay window and concluded
append-only bounded it, which had the mechanism exactly backwards.

An exact replay is now idempotent instead of an error. The row carries a
digest over the signature, its input, and the body, a unique index
enforces it in the database rather than a check-then-insert race, and a
repeat returns the original claim. Erroring would have punished the
legitimate case this design otherwise handles worst: a client whose
response was lost retrying the same signed request and concluding its
report failed when it had succeeded.

The fan-out cap was mine, added while fixing unbounded work, and it
truncated at 256 refs after receive-pack had already accepted them. A
push whose pull request source branch sat past that point left the head
permanently stale with no push event, announced only by a log line git
had already contradicted. The writers chunk now, so nothing accepted is
dropped, and the bound that remains refuses before the accept rather
than after: chunking bounds a statement, not a request, and the work
that scales with ref count sits upstream anyway, one protection query
per ref before the service runs and one certificate and webhook per ref
after, with body limits disabled on the git routes.

Removing the cap also exposed a quadratic branch dedupe it had been
keeping cheap; that is a map now.

* fix(db): stop the push cursor skipping a concurrently committed row

The sequence is a bigserial, so Postgres allocates it at INSERT and makes
it visible at COMMIT, and nextval does not roll back. Two overlapping
writes to one repo could therefore commit out of sequence order: one
allocates, another allocates and commits, a poller reads the later row
and advances, and the earlier row then becomes visible behind a cursor
that has already passed it. Never delivered, no error, no gap signal,
which is the one thing the catch-up surface exists to prevent.

This is the second time this branch has traded one ordering hazard for
another. The cursor moved off a wall clock because stamping and
committing disagree; it moved onto a sequence where allocating and
committing disagree.

The insert now runs in an explicit transaction that takes a per-repo
advisory lock, so the lock releases at commit and no writer can allocate
until the previous row is visible. The wait is bounded, because the lock
sits on a pooled connection on the inline push path and an unbounded
wait would turn one busy repo into pool exhaustion for unrelated
requests. Three attempts at a one second timeout leaves headroom under
the pool's own acquire timeout.

That bound needs the caller to stop swallowing failures. It warned and
continued, so any error the lock introduced would silently drop those
events forever while the push had already returned success, reaching the
same outcome by another route. It now retries once and then logs at
error level naming the repo, the refs, and the SHAs that were lost.

The defect is pinned by a test that owns both transaction boundaries
rather than a barrier. A barrier cannot reproduce it: before this change
the write was a single autocommit statement, so allocate and commit were
one round trip from the client and the interleaving window was never
open to a caller of the public API.

* fix(auth): carry the request body only where a route persists it

The signature middleware handed every signed request a second handle to
the buffered body, and only the status write reads it. The field is
optional now and populated behind a marker the persisting route group
applies.

Be accurate about what that saves, because the first reading of this
finding overstated it and the plan was corrected before implementation.
The push handler takes its body as an extractor, so axum consumes the
request and drops every extension at that point; the extra handle lived
across the remaining middleware chain, not the handler's lifetime. The
whole pack really is pinned for the whole receive-pack, but by the
handler's own parameter, which is pre-existing and untouched here.

Layer order is the failure mode worth guarding. A marker applied inside
the auth layers is never seen by the middleware that reads it, so the
body silently stops being captured while every test stays green. The
mutation that proves this leaves the layer present on the same group and
only moves it inside, and the production-router test still reddens. The
handler also refuses an absent body rather than storing an empty column,
so a lost marker fails loudly at the one place it matters.

Four comments claimed a timestamp or a uuid tiebreak decided ordering
when the code orders on the sequence. Three were named in review; the
fourth turned up next to them, calling the timestamp format load-bearing
for a cursor that no longer reads it. The mirror-dedup comments that
correctly document timestamp ordering were checked and left alone.

The shared scrape helper advanced by one byte to stop an end anchor
matching at position zero, which reports a missing anchor for any region
starting on a multibyte character. It advances by a character now. Not
reachable today since every anchor is ASCII, but five guards share this
helper and one that silently finds nothing is worse than one that fails.

* fix(status): bind a stored claim to the body its signature covered

A claim is provenance, so the signature stored beside it has to reach the
bytes stored beside it. It did not. The signing string covers the body
only through a content-digest line, and require_signature rebuilds the
covered values from the request: with no Content-Digest header the
covered digest is the empty string on the signing side and the verifying
side alike, the Ed25519 check passes over method, path and nothing else,
and the digest-versus-body comparison was skipped for want of a header.
An owner could sign that once and post any body under it. The row that
produced carried a valid signature, a signing string that verifies under
the owner's key, and a request_body the signing string says nothing
about — history nobody can re-verify, which is the one thing the row is
for.

The write path now recomputes the digest of the bytes it is about to
persist and requires the signing string to carry exactly that line, which
is the write-time form of the check re_verify already ran against a
stored row. Line-exact rather than a substring search: build_signing_string
emits one component per line, and a contains would also be satisfied by
the digest appearing inside another component's value.

This is deliberately not the only line of defence, and deliberately not
the middleware's. PR #306 requires the header in require_signature and
closes the same hole one layer earlier for every signed route; that
belongs there, because no other route's correctness should depend on this
handler. Neither substitutes for the other. Without #306 the empty digest
is still accepted everywhere else; without this, a later relaxation of the
middleware resumes writing unverifiable history in silence. Nothing here
touches require_signature, so the two do not collide.

The route's advertised 8 KiB body bound was also a bound on what gets
stored rather than on what gets buffered: require_signature collects and
hashes the whole body before any handler runs, so by the time the handler
measured it the allocation had happened, and concurrent requests
multiplied exactly the cost the number was written to prevent. A
RequestBodyLimitLayer now carries the same figure at the transport,
outside the auth layers and inside the rate limiters, matching the shape
the git routes already use. The handler keeps its own check: it is the one
that holds for a handler reached by any other route table.

The hand-injected test material had to become coherent with the request
it rides on, because a stand-in built over other bytes now takes every
bare-router test down the refusal branch. It is built from the body it
accompanies and carries a real content-digest line.

* fix(api): apply a push's side effects only to the refs git accepted

receive-pack exits zero when the process ran, not when the push applied.
A non-fast-forward, a hook rejection or a failed update comes back as an
ng pkt-line inside a response this handler forwarded without reading, so
every side effect keyed on the refs parsed from the REQUEST fired for
refs that were refused: the pull request on that branch had its stored
head moved to a commit this node does not have, and the catch-up poll
surface handed subscribers a SHA that resolves to nothing.

The report is now parsed and only the refs it names with ok are used.
unpack fail accepts nothing, since the pack never landed. A report that
does not parse at all is inconclusive and keeps every declared ref: a
client that never requested report-status is told nothing about
individual refs, and reading that silence as rejection would permanently
drop the events and the head update for a push git really did accept,
which is the failure the poll surface exists to prevent.

The parser is ported from PR #384, which introduced it for this same
problem, so the two branches converge on one definition of "accepted"
rather than growing two. Whichever lands second should drop its copy. It
diverges by three lines, and they are load-bearing: git double wraps the
report when the client negotiates side-band-64k, which git push over
smart HTTP does. The observed framing from git 2.50.1 is an outer
pkt-line carrying a band byte whose payload is itself a pkt-line stream,
so one pass of strip_sideband leaves "000eunpack ok" as the first line,
the parse fails, and every ref falls back to inconclusive — the parser
answering "cannot tell" for exactly the pushes it exists to classify. A
second pass peels the inner layer and is a no-op on the single-wrapped
shape. The tests carry the real captured byte layout, both wrappings, so
this is pinned rather than assumed.

Both writes also moved inside the repository write lock. That lock orders
the ref updates of two concurrent pushes; released before these ran, the
pair was an unordered race, and B's unconditional UPDATE landing before
A's leaves the stored head — which the rollup prefers over its fallback —
pinned to a commit the branch has moved past. record_push_events has the
same inversion by a different route, since latest_push_sha_for_ref reads
the highest seq and seq order is insert order. There is no per-row
version to make the UPDATE conditional on, so the ordering has to come
from the lock. Holding it across two batched statements on an already
open pool, on a path that just ran a subprocess over the whole pack, is
the cheaper half of that trade. A source guard pins the position, because
this is a property of where the calls sit.

Separately, the 10,000-ref bound was checked only after the parser had
scanned the entire request and kept three heap strings per valid
pkt-line. The git routes raise the body limit to GITLAWB_MAX_PACK_BYTES —
2 GB by default — so a signed caller could force the whole scan and the
whole allocation and only then be told 400: the work the cap exists to
refuse, done in full before the refusal. The parser stops one past the
cap, which is still enough for bound_declared_refs to refuse on, and the
message no longer reports a count that would now be a floor rather than
the request's real one.

* fix(api): make the push-event cursor opaque and bound to its repository

The cursor was the table-global bigserial, handed to clients verbatim and
accepted back as any non-negative integer, then passed straight into a
per-repo seq > $1. Two things followed from that, and both were silent.

A value issued by a DIFFERENT repository — or by this one before a
restore — is an ordinary larger number here, so the query matched
nothing, the surface answered 200 with an empty page and echoed the value
back, and the subscriber sat permanently past history it had never
received with nothing anywhere reading as an error. And because
repo_push_events is written by every repository on the node, the gaps
between one repository's cursors measured how much the others pushed in
between, private ones included.

The cursor is now an opaque token carrying a version, a binding to
(node DID, repository), and the id of the last event served. The binding
is a hash and not a MAC, because there is nothing to forge: the position
inside is a row id resolved against this repository's own rows anyway.
What it buys is that a foreign cursor is refused by shape, with a message
naming which mistake was made, instead of being applied to a repository
it was never issued for. An id that resolves to nothing — a token kept
across a restore that lost the row — is a 400 that names the recovery,
not an empty page the poller reads as being up to date.

The global sequence still orders the walk internally, which is what keeps
the page cheap, but it no longer leaves the node. The position is the row
id, which this surface already publishes in every event, so the token
discloses nothing new.

The two properties an earlier round established are kept, in their new
shape: next_cursor is never null and never moves backwards, and the start
of history is an expressible position rather than an absent one, so a
first poll of a repository with no events still hands back a token the
endpoint accepts back rather than a value a persisting poller would
misread.

* refactor(node): simplify commit-status surface after main merge

Land the merge on one mechanism per concern instead of two:

- landed_refs now intersects the declared set with the ok_set already
  derived for the outcome commit, instead of re-parsing the report in
  accepted_ref_updates; the helper's unreachable keep-declared arm (a
  trap its own doc contradicted) is gone with the second parse.
- bound_declared_refs is deleted: main's parse_ref_updates already
  refuses past MAX_REF_UPDATES inside the command loop, so the
  post-parse check was dead code whose tests pinned an abandoned
  design; the ordering test now pins the parse itself before git.
- merge_pr resolves the source head with read_ref (exact-name
  show-ref) instead of listing every ref in the repo.
- normalize_sha reuses is_hex_sha; the refs/heads/ mapping reuses
  store::local_branch_ref; test-only db methods and their SQL are
  cfg(test)-gated; duplicated body_json/status_and_bytes test
  helpers are hoisted to test_support.

Merge-artifact repair found by running the tests: scrape guards
track the renamed landed_refs binding and the reclaimed.release /
receive_pack_raw_with_reflog call names; the status migration
upgrade test rolls back to and asserts version 47 (not 24, which
main now owns); a doubled cfg(test), an orphaned doc block, and a
stale dead_code allow on StatusClaim.seq are corrected.

* fix(node): harden status surface writers and close-path head freeze

Review-driven fixes on the merged surface:

- close_pr freezes the source head it decided against, resolving the
  recorded push for the branch (then disk) when head_commit is unset;
  a PR closed before its first status read no longer reports
  head_resolved=false forever.
- insert_status_claim_capped bounds its advisory-lock wait with
  SET LOCAL lock_timeout and retries on transient classes, matching
  the push-events writer's contract.
- record_push_events retries only transient failures (the inner bound
  already retries lock waits; a permanent error's second run is a
  guaranteed re-failure) and update_open_pr_heads retries once on
  transient and escalates to error naming the lost branch@sha rows.
- parse_ref_updates lowercases old/new SHAs so pushed values match
  the claim side's lowercase normalization.
- combined_state folds unrecognized stored states to failure, never
  silently to success.

* fix(node): write status surfaces from proven landings in the effect executor

The stored-head and catch-up poll writes ran pre-release inside the repo
write lock, so a refused publish left rows claiming a landing the node had
deleted, and a landing reconcile proved only later emitted nothing at all.
Both are durable accounting and belong to the effect executor: the live
path reaches it only after the publish verdict, and the drain replays it
identically once reconcile proves the landing.

The ordering the in-lock placement guaranteed is kept differently: a head
write is conditioned on the branch still sitting at the landing's tip, so
a superseded landing never pins the rollup to a commit the branch moved
past. Event ids derive from (request_id, ordinal) so a recovery replay
collapses on ON CONFLICT instead of double-delivering.

Also: drop the node-global seq from the create response (id already names
the row), pin the claim body's signed format (deny_unknown_fields + v=1),
and carry Retry-After on 429.

* fix(node): add head_commit to test-harness PR fixture

* test(node): drive the status and push-events routes in the deny sweep

The mounted-route completeness gates require every repo-scoped route to be
a driven registry row: the status read surfaces and the push-events poll
are read-gated (existence-hiding 404 for anon and signed non-readers,
owner reach twin proves the gate) and the claim write is owner-gated on a
public repo. {sha} fills as a fixed 40-hex placeholder.

* fix(node): refuse re-closing a closed PR, drop 429 Retry-After, pin test git identity

- close_pr refuses a PR that is not open and the UPDATE is conditional on
  status='open', so a repeated close cannot store the branch's current tip
  on a historical pull request.
- Remove Retry-After from 429s. The status-claim caps are per-commit and
  append-only, so a 1s hint invited retries that cannot succeed.
- bare_repo_with_ref sets git author and committer env so the
  heads_still_at_tip tests pass on hosts with no global git identity (CI).

* fix(node): close_pr fallback head fills an empty head only (#304)

A head stored by a push between the handler's read and the UPDATE is no
longer replaced by the fallback.

* fix(node): purge status_claims and repo_push_events in delete_repo_by_id

Both tables are keyed on repo_id and were missed by the direct-child
cascade, so deleting a repo left signed status bodies and poll-feed rows
behind. The cascade test now seeds both and asserts zero rows.

---------

Co-authored-by: Kevin Codex <[email protected]>

2026-11-13 21:53:53 +00:00

1 changed files with 8 additions and 1 deletions

									
										crates/gitlawb-node/src/reconciliation.rs
									
		+8
		-1
	
												View File
												
				@@ -6440,6 +6440,13 @@ mod tests {

				    /// Write one canned HTTP response on an accepted fake-S3 socket.

				    /// A free function (not a closure) so the `&mut` socket borrow does

				    /// not leak into a returned future's lifetime.

				    ///

				    /// The fake serves one request per accepted socket and drops it, so each

				    /// response says `Connection: close`. Without it the client keeps the

				    /// socket in its pool after the server has closed it, and the next request

				    /// can reuse a dead connection. The client has retries disabled, as in

				    /// production, so `sweep_restores_tigris_cache_miss_and_skips_missing`

				    /// failed with "repo acquire failed" on a loaded runner.

				    async fn s3_respond(

				        sock: &mut tokio::net::TcpStream,

				        status: &str,

				@@ -6448,7 +6455,7 @@ mod tests {

				    ) {

				        use tokio::io::AsyncWriteExt;

				        let head = format!(

				            "HTTP/1.1 {status}\r\nContent-Length: {}\r\n{extra}\r\n",

				            "HTTP/1.1 {status}\r\nContent-Length: {}\r\nConnection: close\r\n{extra}\r\n",

				            payload.len()

				        );

				        let _ = sock.write_all(head.as_bytes()).await;

Compare commits