chore: sync the fork with upstream main - #6
Merged
Conversation
The /api/v1 discovery document listed GET /{owner}/{repo}/api/commit/{sha}/merge-queue,
which has no route, and pointed docs at https://git.example.com/api on every host. The
endpoint line is gone and docs is now derived from the request base like the other URLs
in the document (AGENTS.md section 5: nothing in crates/ knows a hostname).
web/API.md section 5 and the API page said every JSON write needs admin. Creating a
repository and starting an op need write (admin.rs, ui.rs); deleting a repository and
writing policy or settings need admin (admin.rs, policy.rs, settings.rs). The page also
had no row for settings and dropped the /api segment from every repository path.
API.md, docs/CONTRACT.md and the SDK comment promised an [integrations] settings section
that walgit-config rejects; the section 6 checklist gave browser_base as /api/v1 instead
of /api-browser/v1; the overview shape pointed at a Go file that does not exist; the
bundle design doc still named /services/install.sh.
Tests: api_v1 asserts docs is derived from the discovery base, and a new test pins the
write gates (a write token gets 403 on PUT and DELETE of policy and settings, an admin
token gets 204 and 200). The walgit-config settings test asserts [integrations] is
refused.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
StaticToken.token at crates/walgit-config/src/lib.rs:212 carried no serde default while token_env on the next line did, so an entry that named only an environment variable failed to deserialize with "missing field token". Every shipped example writes that shape, including README.md:19, walgit.example.toml:48 and walgit.standalone.toml:36, so the quick start config was rejected by "walgit --config walgit.toml config check". The field now defaults to the empty string, and Config::validate at lib.rs:1491 still refuses an entry that names neither token nor token_env, so a config with no way in stays fail-closed as GOAL.md asks. tests::auth_modes_validate_fail_closed parses the README entry and asserts the entry with neither form still errors; it fails on the old struct with the same TOML parse error the binary printed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The header comment at walgit.example.toml:6 said "walgit config check walgit.toml", but the check subcommand takes no positional argument and running it that way prints "error: unexpected argument 'walgit.toml' found" and exits 2. The path comes from the global --config flag declared at crates/walgit-cli/src/lib.rs:42-48, so the comment now reads "walgit --config walgit.toml config check". I ran that form against walgit.example.toml and walgit.standalone.toml with the built binary and both print "config OK". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The auth mode table at README.md:143 described mode none as everyone being anon with write, which understates what the mode does. AuthMode::None returns a principal with write true and admin true at crates/walgit-server/src/auth.rs:672-677, and AGENTS.md:78 states the same in the security contract, so a reader of the README alone would not know that a loopback run also hands out settings and policy.json writes. The row now says write and admin. This is a documentation correction with no code change, so the code cited above is the proof. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
client_authorization in crates/walgit-server/src/auth.rs:897 read X-Walgit-Authorization before it checked anything else, so a client talking to walgit directly could put a credential in that header and have it taken as the client credential. AGENTS.md D39 (2) and section 1.3 say the forwarded copy counts only when an edge announced client-authorization in X-Walgit-Capabilities on that request, and that nothing is assumed when walgit is hit with nothing in front of it. The function now consults edge_owns_authorization first and returns the plain Authorization header value when no capability was announced, reaching for the forwarded copy only behind an announcing edge. Behaviour behind an edge is unchanged, a missing forwarded copy there still meaning the client sent no credential. The unit test edge_owned_authorization_is_not_the_client now covers the direction that was missing: Authorization Bearer a with X-Walgit-Authorization Bearer b and no capability header yields a, and the same two headers with the capability yield b. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The S3 backend turned every failure that was not a precondition failure
into `StoreError::Other`, so nothing on the S3 write path could ever
produce `StoreError::Retryable`. GCS classifies the same conditions —
`gcs::is_retryable` covers Unavailable/DeadlineExceeded/ResourceExhausted/
Internal/Aborted, HTTP 503/504/429/500, and connect/IO faults — which made
this a "GCS only" behaviour of the kind AGENTS.md rules out.
Two readers of `StoreError::is_retryable` were affected:
- `coord::cas_update`, the manifest CAS that is the only commit point.
On `Retryable` it sleeps with jittered backoff and re-reads; on
anything else it returns `CoordError::Store` and gives up. A throttled
manifest PUT therefore failed the push outright on S3. The SDK's own
retries (standard mode, three attempts) run underneath this and do not
replace it — what reaches walgit is what the SDK could not absorb.
- `smart::wal_err`, which maps retryable store errors to 503 and
everything else to 500. On S3 a transient bucket fault reached the git
client as a hard 500 instead of the 503 its comment describes.
Add `is_retryable` for `SdkError` — dispatch failures and timeouts, the
transient AWS error codes, and the transient HTTP statuses for
S3-compatible stores that do not use AWS codes — and route every
`SdkError` site through it: put, list, head, delete, and the multipart
create/upload/copy/complete/abort paths.
Tested with a fake S3 bound on an ephemeral port, driving real `SdkError`
values through the classifiers with SDK retries disabled: throttling, a
server fault, an unrecognised transient status, and an unreachable
endpoint are retryable; access denied is not; a failed precondition stays
a failed precondition.
The Rust stage installed protobuf-compiler, which provides protoc but not the well-known .proto files that walgit/v1/wal.proto imports, so the walgit-proto build script failed with "google/protobuf/timestamp.proto: File not found" and the image never built (tobi#21). On Debian those files ship in libprotobuf-dev. jeonck checked both packages in the stage's own base image; the image has not been rebuilt on this machine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…buf-dev Install libprotobuf-dev in the Containerfile build stage
…n-needs-capability Read the forwarded client credential only when an edge announced it
…assification Classify transient S3 failures as retryable
Make the API docs and the discovery document match the server
Accept a static token entry that names only token_env
AuthConfig::default set issuer to https://accounts.google.com, so a config with mode = "oidc", an allowlist, an OAuth client and a session secret but no issuer passed config check and the server then ran discovery against Google, fetched Google's JWKS and validated every ID token as a Google identity (crates/walgit-server/src/auth.rs:150 and :777). The default is now an empty string, which the check at crates/walgit-config/src/lib.rs:1511-1515 rejects, so oidc mode fails closed until the operator names the issuer as AGENTS.md section 1.3 requires. That check sits inside the mode == Oidc arm opened at lib.rs:1502, so none and token mode are untouched. tests::auth_modes_validate_fail_closed proves it: a minimal oidc config without an issuer now fails on the issuer message, the two cases that used to ride on the Google default carry an explicit issuer, and two new assertions show none and token mode validate with the issuer empty. The walgit.example.toml comment near line 53 says the setting is required, and the ID token fixture in auth.rs sets the issuer it signs with. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… works again CI exports CARGO_TERM_COLOR=always, so rustc diagnostics carry ANSI escape prefixes and the warnings recipe's anchored '^warning:' grep never matches: a warning-bearing tree passed the gate. Strip the escapes with a sed that embeds the ESC via a bash $'…' literal (BSD and GNU sed both accept it) before matching. Verified end to end: with a planted unused variable the old recipe exited 0; after the fix it exits 1 and shows the warning, with and without colors.
The third command of the test recipe, justfile line 86, named eight walgit-server test binaries, and crates/walgit-server/tests/ holds three more. No other recipe named them: ci at line 118 is warnings clippy test e2e, and e2e at line 90 runs only --test e2e, so events.rs, follow.rs and policy.rs never ran on a laptop or in CI even though they compile under just clippy. This appends --test events --test follow --test policy to that command. The three suites pass today, seven tests in about five seconds, and .github/workflows/ci.yml line 58 runs just test, so the change reaches CI without editing the workflow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
.github/workflows/ci.yml pinned node-version: 22 at lines 46 and 78, while every other place that builds the SPA asks for Node 24: README.md line 118, flake.nix line 69 and Containerfile line 17. CI ran just web-build on a different major than a contributor's laptop or the OCI image, so a problem that shows up on only one of them could pass unnoticed. Both setup-node steps now say 24. GitHub Actions does not run locally here, so the check is that the workflow now agrees with those three files rather than a test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
README.md line 158 called just ci "all of the above" after a list of three commands, but the recipe at justfile line 118 is ci: warnings clippy test e2e and AGENTS.md section 5 names those same four gates. A reader following the README skipped the clippy gate on a laptop and met it for the first time in CI. The block now lists just clippy next to the other three and says what just ci actually runs. This is documentation, so the check is that the list matches justfile line 118. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
just dev-store ran the rootless podman socket bootstrap on every platform, but justfile:53 fell back to /run/user/$(id -u) when XDG_RUNTIME_DIR is unset and justfile:57 called setsid, and neither exists on macOS, so the recipe died before podman compose up. The bootstrap now runs only when uname -s reports Linux, and setsid is dropped because nohup with a redirect already detaches the service. On any other system the recipe makes one podman info probe and, when that probe fails, prints a single line asking the user to start the container runtime first (podman machine start on macOS) before exiting 1. Proved by extracting the recipe body to a file and running bash -n on it, then running that body on this macOS host, where it printed the one line and exited 1 without reaching podman compose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
justfile:33 skipped the SPA build whenever web/dist/index.html existed, but crates/walgit-server/build.rs:19-21 drops a placeholder index.html into web/dist on any cargo build of a fresh clone, so after the first cargo run just dev-local never built the real UI and served that placeholder page instead. The guard now tests web/dist/repos.js, which only a real Vite build produces and which Containerfile:23 already treats as the proof of one, and the message says the SPA is unbuilt rather than missing. Proved by moving the real web/dist aside and running the compiled walgit-server build script directly, which wrote the 220 byte placeholder index.html; against that tree the old condition skipped the build and the new one runs it, while against the restored Vite output the new condition stays quiet. The recipe body also passes bash -n after extraction, with and without just's {{config}} interpolation expanded.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tests/e2e.sh runs under set -euo pipefail, and bash 3.2, the /bin/bash macOS ships, calls the expansion of an empty array an unbound variable. The EXIT trap at line 73 therefore aborted on the PIDS expansion before rm -rf "$TMP" and leaked the temp directory, and the same expansion at lines 49, 51 and 56 stopped the run at its first health check. PIDS now takes the :- guard that tests/git-bundle-filter.sh:24 already uses, while the three argument lists take the +alternate form instead, because the :- form hands curl and git a blank argument they reject with "option : blank argument where content is expected". Proved on this macOS host with bash 3.2.57 by running tests/e2e.sh with WALGIT_E2E_BASE_URL pointed at a memory-backend server on loopback: before the change it died at line 56 with "AUTH_CURL_ARGS[@]: unbound variable", after it reaches the synth step and its mktemp directory is gone once the script exits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tests/e2e.sh line 126 passed --config "$TMP/walgit.toml" to synth, but that file is written only by the heredoc at lines 92 to 108 inside the local-server branch, so a run with WALGIT_E2E_BASE_URL set died on its first command with "config file ... not found" and exit 2. synth reads nothing out of the config, since crates/walgit-cli/src/lib.rs:493 dispatches it with out, size, commits, files and seed alone, so the call now passes --config /dev/null, which crates/walgit-cli/src/lib.rs:451-455 names as the way to ask for defaults on purpose. That is one token against moving a seventeen line heredoc, and it also keeps remote mode from writing a config whose listen address and cache directory no server in that mode ever reads. Proved by running tests/e2e.sh with WALGIT_E2E_BASE_URL pointed at a memory-backend walgit on loopback, where it now passes every step from synth through DELETE repo, and by running the script in local mode, which still passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
classify_put_error/classify_list_error still took references from main while classify_error and the new tests took ownership, so the crate did not compile. Take ownership throughout so a SlowDown on the manifest CAS is Retryable on S3 the way it already is on GCS.
CI pins rustc 1.97.1; clippy::manual_assert_eq is new there and `just clippy` is -D warnings.
`incremental_has_prerequisites` took the first 20 lossy-UTF-8 lines of the bundle file, which walks into PACK bytes. A binary line that started with '-' was then compared to the base tips and failed CI.
CI's `just test` second command spent ~4.5 minutes compiling test binaries inside `timeout 300`, then SIGTERM'd wal.rs mid-run (exit 124). The suites themselves finish in seconds once built.
A ref-only push carries a 32-byte pack with zero objects, and receive-pack skipped the connectivity check whenever ingest returned Ok(None). A command line naming an object the server does not have was therefore accepted, the ref was published to the WAL, and every clone that walked it failed with "missing object" (tobi#37, reported with a repro by czk-aa). The check now runs whenever unpack succeeded and any update names a non-zero tip. With wal.check_connectivity the existing walk covers the tips and everything new under them and stops at existing refs, so a legitimate ref-only push costs a few lookups; with it turned off the tips are still looked up one by one before anything is published. The e2e test posts the exact empty pack from the report against both settings and expects ng refs/heads/ghost, then pushes main to a new branch, which sends the same zero-object pack, and expects it to land. The test fails on main at the ng assertion and passes here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CSbhRh6UEjFrgBYYzucvDe
The two statvfs helpers, disk_usage in walgit-wal/src/registry.rs and disk_avail in walgit-server/src/rebuild.rs, widen the block-count fields with `as u64`. Those fields are u32 on macOS and u64 on Linux, so on a Mac the strict set fails with cast_lossless and the gate tobi#15 added never passes locally, while `u64::from` would be a useless conversion on Linux where CI runs. No spelling satisfies both platforms, so each helper carries a targeted allow with a comment saying why, the shape the Cargo.toml lint notes ask for. Checked with cargo clippy --workspace --all-targets -- -D warnings on macOS, now clean; the change is a no-op on Linux. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CSbhRh6UEjFrgBYYzucvDe
The web object faulter read objects from a remote pack set asynchronously and then called `write_loose_object` for each of them on the same tokio worker, so one request for a commit diff could spend seconds stating paths, deflating and renaming files while it held a runtime thread. `fault_many` now collects the reads of each chunk of 32 and hands the whole chunk to one `tokio::task::spawn_blocking`, and `fault` writes its single object through the same helper. Behaviour is unchanged: the same objects land in the loose store, ids already faulted are still skipped, and the error text is still `fault object <oid>: <e>`, with a join failure reported as an internal error. AGENTS.md principle VI says never block the async runtime, and `spawn_blocking` is the pattern the rest of the crate already uses. Checked with `cargo clippy -p walgit-server --all-targets --no-deps` (clean apart from the pre-existing `rebuild.rs:141` cast lint), the `web_api`, `web_ui` and `api_v1` suites, `cargo test -p walgit-server --test e2e -- remote`, the e2e `blocking_work_in_the_install_path_does_not_stall_requests`, and `cargo fmt --all -- --check`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CSbhRh6UEjFrgBYYzucvDe
gce_machine_type in crates/walgit-server/src/instance.rs forked curl inside a OnceLock::get_or_init, and info() reaches it from every readiness and health handler and the UI footer, so the first such request on an SSD host held a tokio worker for up to 300 ms on a subprocess. That is the tell principle VI names, a Command::new(...).output() on the async runtime. The probe is now a reqwest GET of the same URL with the same header, timeout and parsing, awaited once in serve() before the listener accepts, and only on the host that calls itself ssd (WALGIT_INSTANCE_KIND=ssd, or maintenance.disk = "ssd" with no explicit kind), which is the one shape whose info() prints a machine type and the only place the old code probed. Handlers now read the cell and nothing else; when the probe never ran, info() falls back to the cpu and memory shape exactly as it did when curl failed. Checked with cargo clippy -p walgit-server --all-targets --no-deps -- -D warnings, cargo test -p walgit-server --lib (the new test asserts a non-SSD host caches nothing and touches no network), cargo test -p walgit-server --test drain and cargo fmt --all -- --check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CSbhRh6UEjFrgBYYzucvDe
The field is documented as the pause between passes; the loop slept it at the top of the body, so a freshly started maintainer idled for one interval and a maintainer that did not live that long never ran a pass at all. The draining check stays at the top of the loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Check pushed tips even when the pack is empty
Let just clippy pass on macOS
…-the-runtime Resolve the GCE machine type at startup, not in a handler
…orker Write faulted objects off the async worker
Spend the maintenance interval between passes, not before the first
Upstream now satisfies the strict clippy gate on its own, so carrying a local style pass only guarantees a conflict on every future sync. What is left is the fork's own reason to exist: the canary image pipeline, the pinned actions, the container protobuf fix and the test-tier watchdog. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Across walgit-bundle, walgit-cli, walgit-git, and related crates, the bulk of the diff is Clippy-driven cleanup: fewer
Reviewed by Cursor Bugbot for commit c0cbaae. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Important Review skippedToo many files! This PR contains 112 files, which is 12 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (112)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cast_precision_loss = "allow"goes with it: upstream's code is clean without the relaxation, so keeping it would only lower the gate for no code that needs it.