feat(node): add a commit status reporting surface - #304
Conversation
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.
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.
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.
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.
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.
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.
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds signed commit-status APIs, pull-request status rollups, repository-scoped push-event polling, push-event persistence, pull-request head tracking, authorization guards, and database-backed tests. ChangesStatus claims and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant create_status
participant Db
participant commit_status
Client->>create_status: submit signed status claim
create_status->>Db: validate replay and caps
Db-->>create_status: inserted or existing claim
Client->>commit_status: request commit status
commit_status->>Db: load authorized latest claims
Db-->>commit_status: status projections
commit_status-->>Client: combined status
sequenceDiagram
participant GitClient
participant receive_pack
participant Db
participant list_repo_push_events
GitClient->>receive_pack: push ref updates
receive_pack->>Db: update PR heads and insert push events
list_repo_push_events->>Db: fetch events after cursor
Db-->>list_repo_push_events: ordered event page
list_repo_push_events-->>GitClient: events and next_cursor
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
crates/gitlawb-node/src/api/repos.rs (1)
1672-1702: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the recorder documentation onto
record_push_events.The doc block that starts at Line 1672 describes
record_push_events, but it is one continuous///run that ends atPUSH_WRITE_CHUNK. Rust attaches all of it to the constant, so the constant's rustdoc opens with the recorder's contract and the function at Line 1735 has no documentation.♻️ Proposed doc move
-/// Record one catch-up poll event per ref update of a push. -/// -/// This is the producer behind the repo-scoped push-event poll surface: a -/// subscriber whose webhook delivery failed can still find the work by polling -/// the repo's events since its last cursor, which makes delivery reliability a -/// read-side property instead of requiring retry machinery on the send side. -/// -/// The rows go into `repo_push_events`, never `received_ref_updates`: the -/// unauthenticated global feed reads the latter, so a local push written there -/// would publish a private repo's push metadata to anonymous callers. -/// -/// Every row of one push shares a single timestamp, which is why the read side -/// pages on `(created_at, id)` rather than the timestamp alone. A failure is -/// logged and skipped: the push itself already succeeded and the objects are on -/// disk, so refusing the response over a missed poll row would be the worse -/// trade. /// How many ref updates of one push go into a single database statement.Then place the removed block directly above
pub(crate) async fn record_push_events(.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/api/repos.rs` around lines 1672 - 1702, Split the continuous rustdoc block before the PUSH_WRITE_CHUNK-specific documentation. Keep the “How many ref updates…” section directly above PUSH_WRITE_CHUNK, and move the preceding recorder contract documentation directly above the record_push_events function so each symbol has the correct documentation.crates/gitlawb-node/src/api/pulls.rs (1)
226-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the discarded
list_refserror.
.ok()drops the error. Whenlist_refsfails,merged_source_headbecomesNone,COALESCEkeeps the storedhead_commit, and the merged pull request freezes at the value a racing push left — the exact stale value this block exists to replace. Nothing records that the resolve failed, so the wrong frozen head looks identical to a correct one.The sibling best-effort paths in this change log instead of swallowing:
rollup_headandrecord_push_eventsboth emittracing::warn!before falling back.🔍 Proposed change
- let merged_source_head = store::list_refs(&disk_path).ok().and_then(|refs| { - let want = format!("refs/heads/{}", pr.source_branch); - refs.into_iter() - .find(|(name, _)| *name == want) - .map(|(_, sha)| sha) - }); + let merged_source_head = match store::list_refs(&disk_path) { + Ok(refs) => { + let want = format!("refs/heads/{}", pr.source_branch); + refs.into_iter() + .find(|(name, _)| *name == want) + .map(|(_, sha)| sha) + } + Err(e) => { + tracing::warn!( + err = %e, + pr_id = %pr.id, + source_branch = %pr.source_branch, + "could not resolve the source head being merged; the stored head \ + is kept and may predate a racing push" + ); + None + } + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/api/pulls.rs` around lines 226 - 231, Update the merged_source_head resolution around store::list_refs to log a tracing::warn! with the list_refs error before falling back to None; preserve the existing successful branch lookup and fallback behavior.crates/gitlawb-node/src/server.rs (1)
186-204: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider separate rate-limit buckets for status writes.
status_write_routesreusesstate.rate_limiterfor the per-DID throttle andstate.create_ip_rate_limiterfor the per-IP brake.creation_routesuses those same two limiters, and both limiters key on the DID and the client IP respectively, not on the route. Status writes and creation writes therefore share one quota.The traffic shapes differ sharply. Repo, issue, and pull-request creation are occasional. Status writes are one call per CI context per commit, so a single active producer can drain the shared bucket and start getting 429 on repo creation for the same DID.
This file already treats bucket sharing as a decision that needs an explicit answer — the
peer_write_routescomment states the notify bucket is separate from the trigger bucket "so an unsigned notify flood cannot drain the signed trigger caller's quota". The same argument applies here, and the new comment does not address it.Adding
status_write_rate_limiterandstatus_write_ip_rate_limitertoAppStatewould isolate the two surfaces. If the sharing is intentional, please record the reasoning in the comment so the next reader does not have to rediscover the interaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/server.rs` around lines 186 - 204, Use dedicated per-DID and per-IP rate-limit buckets for status writes instead of reusing creation quotas: add and initialize status-specific limiters in AppState, then update status_write_routes to use them in rate_limit_by_did and the IpRateLimiter extension. If sharing remains intentional, explicitly document that decision and its rationale in the existing status-write comment.crates/gitlawb-node/src/api/status/mod.rs (1)
58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
*_CHARSbounds to*_BYTES.
boundreceivesmaterial.signature.len(), which is a byte count, and its message says "bytes". The three constants are namedMAX_SIGNATURE_CHARS,MAX_SIGNATURE_INPUT_CHARS, andMAX_SIGNING_STRING_CHARS.MAX_REQUEST_BODY_BYTESalready uses the accurate suffix.The names matter here because the same module measures the other limits differently:
validate_contextandvalidate_descriptionusechars().count(). A reader who trusts the_CHARSsuffix could switch these to a character count and silently loosen the persisted-row bound for multi-byte input.♻️ Proposed rename
-const MAX_SIGNATURE_CHARS: usize = 512; -const MAX_SIGNATURE_INPUT_CHARS: usize = 1024; -const MAX_SIGNING_STRING_CHARS: usize = 4096; +const MAX_SIGNATURE_BYTES: usize = 512; +const MAX_SIGNATURE_INPUT_BYTES: usize = 1024; +const MAX_SIGNING_STRING_BYTES: usize = 4096;The three call sites in
create_statusand the four references incrates/gitlawb-node/src/api/status/tests.rsneed the same rename.Also applies to: 512-520
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/api/status/mod.rs` around lines 58 - 64, Rename MAX_SIGNATURE_CHARS, MAX_SIGNATURE_INPUT_CHARS, and MAX_SIGNING_STRING_CHARS to their *_BYTES equivalents, preserving their existing numeric limits and byte-based len() checks in create_status. Update all corresponding references in the status tests, while leaving the character-count validation in validate_context and validate_description unchanged.crates/gitlawb-node/src/db/mod.rs (1)
2516-2528: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider gating
insert_status_claimbehindcfg(test).
insert_status_claimbypasses all three caps and the replay probe. It is annotated#[allow(dead_code)]because only tests call it. Two sibling primitives in this file solved the same problem differently:set_open_pr_heads(Line 2110) andinsert_repo_push_event(Line 3135) are both#[cfg(test)]. Matching that here removes the uncapped writer from the production surface instead of documenting it.The same applies to
list_status_claimsat Line 2643, which is also#[allow(dead_code)].♻️ Proposed change
- // The write handler uses the capped form below; this stays the uncapped - // primitive the db tests drive directly. - #[allow(dead_code)] + // The write handler uses the capped form below; this stays the uncapped + // primitive the db tests drive directly. Compiled only under test so the + // cap-bypassing writer cannot be reached from a production call site. + #[cfg(test)] pub async fn insert_status_claim(&self, claim: &StatusClaim) -> Result<i64> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/db/mod.rs` around lines 2516 - 2528, Gate the test-only database primitives `Db::insert_status_claim` and `Db::list_status_claims` with `#[cfg(test)]`, matching the existing pattern used by `set_open_pr_heads` and `insert_repo_push_event`; remove their `#[allow(dead_code)]` annotations while leaving their implementations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 1683-1687: Update the comment near the push polling response to
state that the read side pages on the database-assigned sequence number (`seq`),
not `(created_at, id)`. Preserve the existing explanation about shared
timestamps and logging missed poll rows, while removing the stale ordering-key
claim.
- Around line 3048-3101: Replace the thread-local statement counter and
current-thread LogCapture assumptions used by the affected #[sqlx::test] tests
with runtime-safe shared instrumentation, or configure those tests to run on a
guaranteed single-thread Tokio runtime. Update statements_since_last_check,
capture_logs, and their backing state so writes and logs remain observable when
execution resumes on different worker threads.
In `@crates/gitlawb-node/src/api/status/tests.rs`:
- Around line 1821-1842: Correct the doc comments for seed_branch_head and
seed_push_event to state that latest_push_sha_for_ref selects by insertion
sequence (seq DESC), not created_at or a UUID tiebreaker. Describe the
monotonically increasing timestamps only as fixture metadata, and remove claims
that timestamp ordering determines which push wins.
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 3227-3267: Update list_repo_push_events_keyset and the repository
push-insert path to acquire the same per-repository pg_advisory_xact_lock used
by insert_status_claim_capped before allocating/inserting push events, ensuring
seq order follows commit order within a repository. Correct the method
documentation to remove the claim that insert-time seq allocation is safe under
concurrent writers and describe the locking guarantee instead.
- Around line 1114-1132: Replace the thread-local PUSH_WRITE_STATEMENTS counter
with a process-global atomic and protect test reset/read operations with a test
mutex so counts remain consistent across Tokio worker threads. Update
count_push_write_statement and take_push_write_statements to use the shared
synchronized state, preserving the existing test-only behavior and
zero-after-read semantics.
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 145-147: Update the anchor search in the helper containing the
`rest` match so it advances to the second UTF-8 character boundary rather than
using byte offset 1; preserve the existing behavior of returning the found
boundary plus one character boundary. Add a regression test covering a valid
region whose first character is non-ASCII and whose anchor occurs after it.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/pulls.rs`:
- Around line 226-231: Update the merged_source_head resolution around
store::list_refs to log a tracing::warn! with the list_refs error before falling
back to None; preserve the existing successful branch lookup and fallback
behavior.
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 1672-1702: Split the continuous rustdoc block before the
PUSH_WRITE_CHUNK-specific documentation. Keep the “How many ref updates…”
section directly above PUSH_WRITE_CHUNK, and move the preceding recorder
contract documentation directly above the record_push_events function so each
symbol has the correct documentation.
In `@crates/gitlawb-node/src/api/status/mod.rs`:
- Around line 58-64: Rename MAX_SIGNATURE_CHARS, MAX_SIGNATURE_INPUT_CHARS, and
MAX_SIGNING_STRING_CHARS to their *_BYTES equivalents, preserving their existing
numeric limits and byte-based len() checks in create_status. Update all
corresponding references in the status tests, while leaving the character-count
validation in validate_context and validate_description unchanged.
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 2516-2528: Gate the test-only database primitives
`Db::insert_status_claim` and `Db::list_status_claims` with `#[cfg(test)]`,
matching the existing pattern used by `set_open_pr_heads` and
`insert_repo_push_event`; remove their `#[allow(dead_code)]` annotations while
leaving their implementations unchanged.
In `@crates/gitlawb-node/src/server.rs`:
- Around line 186-204: Use dedicated per-DID and per-IP rate-limit buckets for
status writes instead of reusing creation quotas: add and initialize
status-specific limiters in AppState, then update status_write_routes to use
them in rate_limit_by_did and the IpRateLimiter extension. If sharing remains
intentional, explicitly document that decision and its rationale in the existing
status-write comment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39c087d7-192c-482d-8697-f0637d3ae7cc
📒 Files selected for processing (11)
crates/gitlawb-core/src/http_sig.rscrates/gitlawb-node/src/api/events.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/pulls.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/api/status/mod.rscrates/gitlawb-node/src/api/status/tests.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/test_support.rs
| /// Statements executed on the push write path since the last call. The | ||
| /// counter is thread-local, so it measures this test alone with no | ||
| /// cross-test serialization to remember to take. | ||
| fn statements_since_last_check() -> usize { | ||
| crate::db::take_push_write_statements() | ||
| } | ||
|
|
||
| /// A `tracing` sink for the current thread, so "the drop is logged" is a | ||
| /// property the test observes rather than one it takes on trust. The | ||
| /// subscriber is installed for the lifetime of the returned value and | ||
| /// captures whatever this thread emits while it lives. | ||
| struct LogCapture { | ||
| buf: std::sync::Arc<std::sync::Mutex<Vec<u8>>>, | ||
| _guard: tracing::subscriber::DefaultGuard, | ||
| } | ||
|
|
||
| #[derive(Clone)] | ||
| struct LogSink(std::sync::Arc<std::sync::Mutex<Vec<u8>>>); | ||
|
|
||
| impl std::io::Write for LogSink { | ||
| fn write(&mut self, data: &[u8]) -> std::io::Result<usize> { | ||
| self.0.lock().expect("log buffer").extend_from_slice(data); | ||
| Ok(data.len()) | ||
| } | ||
| fn flush(&mut self) -> std::io::Result<()> { | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| impl<'a> tracing_subscriber::fmt::MakeWriter<'a> for LogSink { | ||
| type Writer = LogSink; | ||
| fn make_writer(&'a self) -> Self::Writer { | ||
| self.clone() | ||
| } | ||
| } | ||
|
|
||
| impl LogCapture { | ||
| fn contents(&self) -> String { | ||
| String::from_utf8_lossy(&self.buf.lock().expect("log buffer")).into_owned() | ||
| } | ||
| } | ||
|
|
||
| fn capture_logs() -> LogCapture { | ||
| let buf = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); | ||
| let subscriber = tracing_subscriber::fmt() | ||
| .with_writer(LogSink(buf.clone())) | ||
| .with_ansi(false) | ||
| .with_max_level(tracing::Level::WARN) | ||
| .finish(); | ||
| LogCapture { | ||
| buf, | ||
| _guard: tracing::subscriber::set_default(subscriber), | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the runtime flavor sqlx::test generates and that no push-write test spawns work.
set -euo pipefail
# The declared sqlx version and features.
fd -t f 'Cargo.toml' | xargs rg -n -C3 '^sqlx\b|sqlx\s*=' || true
# Where the counter is incremented, to confirm it is on the caller's thread.
rg -n -C6 'PUSH_WRITE_STATEMENTS' crates/gitlawb-node/src/db/mod.rs
# Any spawn inside the push write path.
rg -n -C3 'tokio::spawn|spawn_blocking' crates/gitlawb-node/src/api/repos.rsRepository: Gitlawb/node
Length of output: 4416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant test declarations and all uses of the thread-local helpers.
rg -n -C4 '#\[sqlx::test|statements_since_last_check|capture_logs|LogCapture|take_push_write_statements|count_push_write_statement' crates/gitlawb-node/src/api/repos.rs crates/gitlawb-node/src/db/mod.rs
# Inspect the exact push-write implementation and the test range without executing repository code.
sed -n '1080,1325p' crates/gitlawb-node/src/api/repos.rs
sed -n '2700,3340p' crates/gitlawb-node/src/api/repos.rs
# Check the locked sqlx version and whether the repository contains macro/runtime source.
rg -n -C2 'name = "sqlx"|name = "sqlx-macros"|version = "0\.8' Cargo.lock crates/gitlawb-node/Cargo.toml
fd -HI 'sqlx*' . /root/.cargo/registry/src 2>/dev/null | head -40 || trueRepository: Gitlawb/node
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 '#\[sqlx::test|statements_since_last_check|capture_logs|LogCapture|take_push_write_statements|count_push_write_statement' crates/gitlawb-node/src/api/repos.rs crates/gitlawb-node/src/db/mod.rs
sed -n '1080,1325p' crates/gitlawb-node/src/api/repos.rs
sed -n '2700,3340p' crates/gitlawb-node/src/api/repos.rs
rg -n -C2 'name = "sqlx"|name = "sqlx-macros"|version = "0\.8' Cargo.lock crates/gitlawb-node/Cargo.toml
fd -HI 'sqlx*' . /root/.cargo/registry/src 2>/dev/null | head -40 || trueRepository: Gitlawb/node
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Restrict the search to the relevant test block and show task-spawn calls only
# in that block.
sed -n '2740,3335p' crates/gitlawb-node/src/api/repos.rs | rg -n 'tokio::spawn|spawn_blocking|JoinSet|spawn_local|async fn|#\[sqlx::test|record_push_events|update_open_pr_heads|capture_logs|statements_since_last_check'
# Show the helper implementations and the exact write methods.
sed -n '1100,1135p' crates/gitlawb-node/src/db/mod.rs
sed -n '2150,2180p' crates/gitlawb-node/src/db/mod.rs
sed -n '3160,3200p' crates/gitlawb-node/src/db/mod.rs
# Inspect the lockfile entries without scanning the whole repository.
if [ -f Cargo.lock ]; then
awk '
/^name = "sqlx"$/ {show=1}
show {print}
show && /^$/ {exit}
' Cargo.lock
awk '
/^name = "sqlx-macros"$/ {show=1}
show {print}
show && /^$/ {exit}
' Cargo.lock
fi
# Search local Cargo metadata/cache for the sqlx test macro implementation.
find /root/.cargo/registry /usr/local/cargo/registry -type f \
\( -path '*sqlx-macros*' -o -path '*sqlx-core*' \) 2>/dev/null |
head -100 || trueRepository: Gitlawb/node
Length of output: 7857
🌐 Web query:
sqlx 0.8 #[sqlx::test] default runtime current-thread multi-thread documentation
💡 Result:
In SQLx 0.8, the #[sqlx::test] macro automatically executes your test in the configured asynchronous runtime based on the enabled cargo features [1][2]. Runtime Behavior: - When the runtime-tokio feature is enabled, #[sqlx::test] creates a multi-threaded Tokio runtime (via Builder::new_multi_thread) to execute the test [3]. - The macro does not allow you to manually switch between current-thread or multi-thread configurations via attribute arguments; it defaults to the behavior defined by the SQLx implementation for the chosen runtime feature [2][4]. - If multiple runtime features (e.g., runtime-tokio and runtime-async-std) are enabled, SQLx prefers the Tokio runtime [1][2]. Key Characteristics: - It behaves similarly to #[tokio::test] or #[async_std::test] but does not support the same configuration control arguments (such as those used to specify runtime threads or worker counts) [2][4]. - For advanced requirements—such as controlling the number of threads or using a specific runtime configuration—you should handle the runtime setup manually in your test function rather than relying solely on the #[sqlx::test] macro [2][4]. - As noted in the official documentation, if you encounter connection pool limits or timeout issues during parallel execution, you should manage your pool size explicitly using PoolOptions or adjust the cargo test runner's concurrency settings (e.g., --test-threads) [2]. For further implementation details, you can refer to the official sqlx::test documentation [1].
Citations:
- 1: https://docs.rs/sqlx/latest/sqlx/attr.test.html
- 2: https://github.com/launchbadge/sqlx/blob/main/src/macros/test.md
- 3: https://github.com/launchbadge/sqlx/blob/335eed45455daf5b65b9e36d44d7f4343ba421e6/sqlx-macros/src/lib.rs
- 4: feat: implement testing utilities transact-rs/sqlx#2001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify the warning/drop event asserted by the log-capture test.
rg -n -C8 'warn!|tracing::warn|drop|chunk|PUSH_WRITE_CHUNK' crates/gitlawb-node/src/db/mod.rs crates/gitlawb-node/src/api/repos.rs | sed -n '1,240p'
# Fetch the pinned SQLx 0.8.6 macro source as data, without building or running it.
python3 - <<'PY'
import urllib.request
urls = [
"https://raw.githubusercontent.com/launchbadge/sqlx/v0.8.6/sqlx-macros/src/lib.rs",
"https://raw.githubusercontent.com/launchbadge/sqlx/v0.8.6/sqlx-macros-core/src/test.rs",
"https://raw.githubusercontent.com/launchbadge/sqlx/v0.8.6/src/macros/test.md",
]
for url in urls:
print(f"\n--- {url} ---")
try:
text = urllib.request.urlopen(url, timeout=10).read().decode()
except Exception as exc:
print(f"FETCH_ERROR: {exc}")
continue
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if "multi_thread" in line or "new_" in line and "runtime" in line or "test" in line and "runtime" in line:
lo, hi = max(1, i-5), min(len(lines), i+8)
print("\n".join(f"{j}: {lines[j-1]}" for j in range(lo, hi+1)))
PYRepository: Gitlawb/node
Length of output: 21304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the implementation and assertions for the chunk-boundary scenario.
sed -n '2060,2225p' crates/gitlawb-node/src/db/mod.rs
sed -n '3180,3265p' crates/gitlawb-node/src/api/repos.rs
# Locate all warning events and exact log assertions in the relevant files.
rg -n -C5 'tracing::(warn|info|error)|warn!\(|logged|contents\(\)|contains\(' \
crates/gitlawb-node/src/api/repos.rs crates/gitlawb-node/src/db/mod.rsRepository: Gitlawb/node
Length of output: 50368
Replace the thread-local test instrumentation.
#[sqlx::test] with SQLx 0.8.6 and runtime-tokio-rustls uses a multi-thread Tokio runtime. The test future can resume on another worker after .await, so the counter and subscriber can miss writes or logs. Use runtime-safe instrumentation or a guaranteed single-thread runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gitlawb-node/src/api/repos.rs` around lines 3048 - 3101, Replace the
thread-local statement counter and current-thread LogCapture assumptions used by
the affected #[sqlx::test] tests with runtime-safe shared instrumentation, or
configure those tests to run on a guaranteed single-thread Tokio runtime. Update
statements_since_last_check, capture_logs, and their backing state so writes and
logs remain observable when execution resumes on different worker threads.
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.
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.
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.
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.
3af65dd to
d3074a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/auth/mod.rs`:
- Around line 37-48: Update require_signature and SignatureMaterial so signed
routes do not retain the full request body by default. Move body-carrying
material into the status-write middleware, or explicitly release it before
returning from routes that do not persist status claims, while preserving body
availability for the status-claim write path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9e3e3d7-3835-4e6d-8b4c-2cf71747d456
📒 Files selected for processing (1)
crates/gitlawb-node/src/auth/mod.rs
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.
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.
|
Worked all seven. Five landed in The seq keyset skipReal, and the sharpest of the set. I confirmed the window is open rather than assuming it: Taken the second remedy, with one addition. 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 a sequence until the previous row is visible. The wait is bounded ( That bound needed the caller fixed too. Worth recording how the defect is pinned, because the obvious test does not work. A barrier-driven concurrency test passes against the unfixed code: before this change the write was a single autocommit statement, so allocate and commit were one round trip and the interleaving window was never open to a caller of the public API. The reproduction owns both transaction boundaries directly and observes the strand: A allocates, B allocates and commits, the walk from zero leaves the cursor past A, A commits, and the next page comes back empty with both rows committed. I also corrected the doc comment, which claimed the sequence "cannot disagree with the order the rows actually became visible". True for a single writer, and the sentence that made this invisible. The body retention, with a correctionReal, and I have made the change, but the finding overstates it and I would rather say so than quietly ship a fix under a claim that does not hold. The part I can show: On top of that, my reading of the extractor is that the request is consumed to produce So the change is still worth making and is in: the field is optional now and populated only behind a marker the persisting route group applies. What it buys is removing the second handle across the remaining chain, and removing the retention outright on small-body signed routes. Lowering peak memory on a push needs a streaming receive-pack, which is separate work. The failure mode worth guarding here is layer order: 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 guard for that moves the layer inside rather than deleting it, and the production-router test still fails. The handler also refuses an absent body rather than storing an empty column. The three smaller onesThe stale cursor comment and both fixture comments are corrected. There was a fourth next to them, calling the timestamp format load-bearing for a cursor that no longer reads it; same root cause, so it went with them. The two comments in The scrape helper advances by a character now. You are right that it is not reachable today, since every current anchor is ASCII, but five guards share that helper and one that silently reports "not found" is worse than one that fails loudly. Both directions are guarded, so the fix did not reintroduce the zero-position collapse the byte offset originally prevented. Declined: the two thread-local findings
The suggested remedy also points the wrong way for this suite. There is a process-global If the runtime flavour ever changes, the concern becomes real, so the new concurrency work deliberately uses Full workspace suite is green at 1165. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Require a body-bound Content-Digest before persisting signed provenance
crates/gitlawb-node/src/auth/mod.rs:212
The status handler now stores the request body as evidence of the signed verdict, butrequire_signaturesubstitutes an empty string whenContent-Digestis absent and only compares the digest when the header exists. An owner can therefore sign the method/path plus an empty digest, omit the header, and submit any status body; the resulting claim asserts provenance that cannot re-verify the stored body. Require and verify the digest on this route (or make this PR depend on the digest-enforcement change) before recording the claim. -
[P1] Apply push side effects only to refs that receive-pack accepted
crates/gitlawb-node/src/api/repos.rs:1017
smart_http::receive_packregards a zero process exit as success and forwards the protocol response untouched, while receive-pack reports individual rejected refs asng <ref> ...pkt-lines even with that exit status. The new code then updates PR heads and writes poll events for every ref parsed from the request. A rejected non-fast-forward, hook, or unpack failure can consequently publish an uninstalled SHA and make the PR rollup target it. Parse the report-status and use only accepted refs (or derive the updates from the resulting repository state) for these side effects. -
[P1] Keep PR-head writes ordered with the git write lock
crates/gitlawb-node/src/api/repos.rs:952
The repository lock is released beforeupdate_open_pr_headsruns. If pushes A then B are accepted for the same branch, B can reach the unconditionalUPDATE pull_requests SET head_commit = v.shafirst and A can overwrite it afterwards. Since a non-null stored head bypasses the fallback, the rollup remains pinned to A even though the branch points at B. Serialize this update with the receive-pack order, or make the update conditional on an ordering/version that cannot move the head backwards. -
[P1] Make push-event cursors opaque and repository/node-bound
crates/gitlawb-node/src/api/events.rs:349
Every non-negative integer is accepted and passed directly to a per-reposeq > cursorquery. A high value issued by another repo/node, or retained across a restore, returns an empty 200 and is echoed, permanently skipping this repo's history while the subscriber believes it is caught up. The returned value is also the table-globalBIGSERIAL, so gaps let a reader measure activity in other repositories, including private ones. Use an opaque scoped cursor, or validate that it was issued for this repository and reject an invalid/ahead value visibly. -
[P2] Enforce the ref-count limit while parsing the receive-pack request
crates/gitlawb-node/src/api/repos.rs:874
The 10,000-ref bound is checked only afterparse_ref_updateshas scanned the entire request and allocated three strings for every valid pkt-line. Git routes allow a pack body up to the configured 2 GB, so a signed caller can force the allocation and CPU work that the new cap is meant to prevent before receiving the 400. Stop parsing as soon as the cap is exceeded (or otherwise bound the parser) before retaining each update. -
[P2] Apply the status body-size limit before signature middleware buffers it
crates/gitlawb-node/src/api/status/mod.rs:176
The advertised 8 KiB request bound is checked only afterrequire_signaturehas collected and hashed the full body; the route has no matching transport body limit. A large signed request is therefore allocated before authorization and only then rejected by this handler, defeating the stated bound under concurrent requests. Add a smallRequestBodyLimitLayeroutside the auth middleware (or make the middleware collection bounded) for this route.
2 participants
Summary
Adds a commit status surface so an external CI system can report a verdict against a commit and readers can see it, including a rolled-up state on a pull request head. The node executes nothing: this is the reporting half only.
Motivation & context
A repo here can be pushed to, reviewed, and merged, but there is nowhere to record whether the code is any good, so anyone who wants build results keeps a GitHub mirror. Every mirror is a reason a GitHub outage still reaches our users.
The gap was narrower than it looked. A completed push already fires an outbound
pushwebhook whose payload mirrors GitHub's shape, so the trigger half existed and the reporting half did not.I looked at running CI on the node and decided against it. Gitea, GitLab, sourcehut and Radicle all put the executor out of process, GitHub's own docs say self-hosted runners should almost never serve public repos, and every major free CI provider gated access in 2021 after cryptomining abuse. A node here is structurally the "public repo, operator's machine" case. That decision is settled, not open for this PR.
Kind of change
What changed
All in
gitlawb-node.status_claimstable ordered by a database-assigned sequence, a nullablehead_commitonpull_requests, andrepo_push_events.POST /api/v1/repos/{owner}/{repo}/statuses/{sha}, owner-gated, validated, rate-limited, with three write caps.GET .../commits/{sha}/statusandGET .../pulls/{number}/status, both behind the existing repo read gate.GET .../push-events, a cursor-paged catch-up surface so a checker that missed a webhook can still find the commit.head_commitmaintained on push and frozen at close or merge, so the rollup has an honest target.require_signaturenow attaches the verified RFC 9421 material as a request extension, so the write path can persist what a signature actually covered.Version 24 rather than 18:
pr-173andfix/issue-135-ipfs-cid-tree-gateboth claim through 23, and the runner keys only on version without comparing the recorded name, so a collision is a silent full skip rather than an error.Three design calls worth stating plainly rather than leaving to be inferred. The wire shape follows GitHub's commit status contract, which buys existing status-consuming clients and settles an inconsistency we already had at the push webhook. Writes are append-only claims and the visible status is a projection over them, which is what keeps a later move to signed attestations a substrate swap. Each claim stores the signature and the bytes it covered, because a claim nobody can re-verify after the request is gone is not history that substrate could adopt.
How a reviewer can verify
cargo test --workspace cargo clippy --workspace --all-targets -- -D warningsThe tests worth reading first are the ones that pin the security properties rather than the happy path:
Before you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare clean.env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsThe only signing-path change left here is additive and internal:
require_signatureattaches the material it already computed to the request extensions, so a handler can persist what a signature covered. No wire format, no verification behavior, nothing a client can observe.This branch previously also made
Content-Digestmandatory and refused duplicated covered components. Those are a protocol change, they are not needed by this feature, and they wanted their own scrutiny, so they now live in #306 behind #305. This branch has been rebased to drop them and depends on nothing there.Notes for reviewers
Known limitations, stated up front.
No first producer is named yet. Nothing in this PR runs CI, and no specific system is committed to reporting into it. It can merge complete and still not answer the question that motivated it until someone points a checker at it.
The rollup fallback only helps going forward. When a pull request has no stored head, it resolves from
repo_push_events, which only carries pushes that land after this deploys. A pull request whose branch was last pushed before then stays unresolved until it is pushed again. There is no backfill.Two contracts are not enforced by a guard yet. No verdict data reaches gossip, the GraphQL broadcast channel, or anchoring, and I checked that by reading rather than assuming, but the source-scrape test that would keep a future edit from adding one is deferred. So is a reference signing shim.
No
glor MCP coverage. The signing path already exists in the client, so this is a missing wrapper rather than a reachability problem, but the status domain has none of the roughly thirty tools the other domains have.Two behaviours I would rather you agree with than discover. A replayed byte-identical write is idempotent and returns the original claim instead of erroring, which also means two genuinely separate reports with identical bodies inside the same clock second collapse into one, since RFC 9421 signs
createdat second granularity. And the pull request rollup persists a resolved head during an unauthenticated GET, bounded in SQL so it fires at most once per pull request.head_commitnow appears on existing pull request responses through the record's serialization. Additive, and the head is already derivable by anyone who can list refs, but it is a response shape change rather than an invisible one.Summary by CodeRabbit
New Features
Bug Fixes