Commit Graph
594 Commits
Author SHA1 Message Date
Evan JarrettandClaude Opus 5 c36e90f6b7 hold/admin: swap out the deleted crew row instead of sending 204
The delete handler returned 204 No Content for htmx requests, on the
theory that an empty body plus hx-swap="outerHTML" would make the row
disappear. htmx's default responseHandling maps 204 to swap:false, so
it never swapped at all: the record was gone from the PDS but the row
stayed on screen until a manual refresh.

Return an empty 200, which htmx does swap.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ai43R3s33cBGMybGp2gUcG
2026-09-01 11:03:54 -05:00
Evan JarrettandClaude Opus 5 2a58ccebd8 hold/gc: name the third ownership state instead of encoding it as a lie
264d332 fixed the behaviour but encoded it badly. manifestBelongsToHold
returned (true, false) for an unreachable hold — "yes, but not really" — a
return value that contradicts itself, and isPredecessorHold both applied the
fail-open policy and handed back the raw material for that policy. The
behaviour was right and the shape was wrong.

The underlying problem is that ownership has three states and the return type
had two:

  ours        - this hold's manifest, or a confirmed predecessor's
  not ours    - the hold answered, and it is someone else's
  unknown     - the hold did not answer; don't delete, but do not adopt

For the first two, "is it ours" and "should its blobs stay referenced" have
the same answer, so one bool worked and the design was never stressed. They
diverge only on unknown. Every version so far has had to collapse unknown
onto one of the other two: before 95d4f7c onto "not ours", which deleted a
live predecessor's blobs, and after it onto "ours", which adopted foreign
manifests and put ten phantom missing layer records on hold01. Same shape
error, opposite sides. That conflation is original, not something 95d4f7c
introduced: manifestBelongsToHold has fed knownManifests since the function
was written.

So name the state. manifestClaim has three values, classifyManifest and
classifyPredecessorHold report what they found and apply no policy, and the
one decision that matters — an unknown claim is carried for blob protection
but never adopted — now sits in the open at the call site instead of two
functions deep, which is how it leaked into ownership to begin with.

No behaviour change from 264d332; the three outcomes and the blob protection
are identical. The regression test was re-verified against this shape: it
fails, reporting the adoption, when claimUnknown is allowed to adopt.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KWoKzpgtBJ33sCyGxJGR7x
2026-09-01 10:41:04 -05:00
Evan JarrettandClaude Opus 5 264d332bbd hold/gc: stop an unreachable hold's manifests from being adopted
95d4f7c made the predecessor check fail open so a five-second blip against
a live predecessor could not drop its blobs out of the referenced set. That
was right, but the boolean it flipped does two jobs: manifestBelongsToHold
decides both "keep these blobs referenced" and "this manifest is ours", and
for an unreachable hold those want different answers.

The consequence showed up on hold01 the moment it started running this
code. Five stale dev manifests pointing at did:web:localhost%3A8080 and
did:web:172.28.0.3:8080 were adopted into knownManifests, and since hold01
had never stored them, every one of their ten layers was reported as a
missing layer record. Worse than the noise: reconcileMissingRecords acts on
exactly that list, so a Reconcile would have written io.atcr.hold.layer
records asserting hold01 stores blobs for a localhost hold.

These DIDs are loopback and RFC1918, so they can never resolve from a
server. This is not a transient outage that clears itself on the next run.

manifestBelongsToHold and isPredecessorHold now return (value, definitive),
matching the idiom checkPredecessor already uses. An indefinite answer still
carries the manifest so its blobs stay referenced, but marks it ProtectOnly,
and analyzeRecords protects its digests without adding it to knownManifests
— the same shape the in-grace takedown branch above it already uses.

Left alone deliberately: the legacy holdEndpoint path still treats a resolve
failure as a definitive "not ours". That predates 95d4f7c and fails closed
rather than open, so it is a different bug with a different blast radius.

The regression test was verified to fail without the fix, reporting the
adoption rather than a build error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KWoKzpgtBJ33sCyGxJGR7x
2026-09-01 10:31:04 -05:00
Evan JarrettandClaude Opus 5 fd8e4b0bde docs: correct stale credential-helper and workspace layout in CLAUDE.md
The documented build command `go build -o bin/docker-credential-atcr
./cmd/credential-helper` has not worked since the helper was split into
per-brand modules: that path holds no Go files, so the command fails with
"no Go files in .../cmd/credential-helper".

Point it at cmd/credential-helper/atcr, grouped with the scanner under a
"separate modules" heading since both need the cd-and-build form, and note
that `make build-credential-helper` is the same build with version/commit
ldflags stamped.

The workspace section claimed two modules; there are five. The two
credential helpers and deploy/upcloud were missing, and the main module no
longer contains the credential helper. Added why the helpers are split out
at all: `go install atcr.io/cmd/credential-helper/atcr@latest` has to
resolve without the main module's dependency tree, which is what the
`require atcr.io vX.Y.Z` pin in each is for.

Both documented commands verified against the current tree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KWoKzpgtBJ33sCyGxJGR7x
2026-09-01 09:07:23 -05:00
Evan JarrettandClaude Opus 5 15871ad188 deps: update all modules, bump go and builder images to 1.26.7
Update every direct dependency across all five workspace modules to
latest. Notable jumps: syft v1.43.0 -> v1.51.1, grype v0.111.1 ->
v0.118.0, stereoscope v0.1.23 -> v0.3.1, indigo -> 2026-09-01,
aws-sdk-go-v2/service/s3 v1.99.1 -> v1.110.0, grpc v1.80.0 -> v1.83.2,
x/crypto v0.50.0 -> v0.55.0.

Three deps needed more than a version bump:

go-libipfs could not be updated at all. The repo was renamed to boxo, so
every tag past v0.7.0 declares `module github.com/ipfs/boxo` and cannot
be required under the old path. sqlite_store.go already imported
go-block-format alongside it and used the archived package exactly once,
inside a function already returning blockformat.Block, so it was relying
on structural interface satisfaction. Collapsing to the native type drops
the archived dependency entirely.

go-didplc moved its package from the repo root into a didplc/ subdir in
v0.2.2. Package name is unchanged and every symbol we use (RegularOp,
OpEnum, OpService, Client.DirectoryURL, Submit) is intact, so this is an
import path change only.

The go-diskfs replace in scanner/go.mod had inverted. It pinned v1.7.0
because syft v1.43 passed diskfs entries as os.FileInfo; syft v1.51.1
fixed that upstream and now requires v1.9.4, so the workaround had become
the thing breaking the build. Removed per its own "Remove when syft ships
a fix" note, closing anchore/syft#4796 for us.

The indigo bump needed no code changes: of the 21 packages we import only
5 changed, and the repo/MST/CAR-store core is byte-identical. It does
bring a util/ssrf fix blocking 6to4 addresses (2002::/16), which we
inherit through atproto/auth/oauth.

Go 1.26.7 across go.work, all five go.mod files, the four Dockerfiles,
the three tangled workflows, and the stale references in
docs/DEVELOPMENT.md. Verified golang:1.26.7-trixie resolves on
mirror.gcr.io, which is what the Dockerfiles actually pull from.

Makefile's TRIXIE_BUILDER_IMAGE stays on the floating golang:1-trixie.

make test, make lint, and make test-race all pass, as do the scanner
module's tests and the integration-tagged build.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KWoKzpgtBJ33sCyGxJGR7x
2026-09-01 09:02:33 -05:00
Evan JarrettandClaude Opus 5 606338b33e appview: fix mockup-code contrast in both themes
daisyUI dims the gutter prefix with `opacity: .5`, which multiplies
against whatever opacity the line's own text color carries. On
bg-base-300 a plain `$` measured 3.20:1 in light and 4.19:1 in dark,
and the `#` on a line already dimmed to /70 compounded to 2.15:1. The
text needs 4.5:1, so this failed in dark mode too, just less visibly
than the light-mode report that surfaced it.

Give the prefix an absolute muted color instead of a multiplying
opacity. The override has to sit unlayered: daisyUI ships this selector
in `@layer daisyui`, declared after `@layer components`, so a rule in
components loses on layer order however specific it is. A first attempt
inside components left daisyUI's `opacity: .5` live on top of the new
70% color, which made the `$` worse (0.5 -> 0.35 effective) rather than
better.

The hero tagline drops its /70 and now reads at the same weight as the
docker commands above it. install.html's two comment lines were at /50,
which failed on the text itself (3.20:1 light), and move to /70 where
they still read as comments.

Measured in Chromium against the built stylesheet, compositing each
pseudo-element color over its real background on a canvas: every prefix
and comment is now 5.84:1 light / 6.71:1 dark, against 14.03:1 / 12.02:1
for the command text, so the gutter stays visibly de-emphasized.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LpqkSRqpcnAjsSZgFTUN5z
2026-08-31 17:05:15 -05:00
Evan JarrettandClaude Opus 5 ee9ccc1fb8 appview: drop crossorigin from the imgs.blue preconnect
Connections are keyed by credentials mode. The crossorigin attribute
opened the imgs.blue socket in anonymous-CORS mode, but every request
to that origin is a plain <img src> avatar fetch (BlobCDNURL /
resizeImage) in no-cors mode, so nothing could reuse it. The browser
opened a second connection anyway and Lighthouse flagged the hint as
unused while still listing imgs.blue as a preconnect candidate worth
~300ms of LCP.

The attribute is correct for the font preloads below, which is where it
was likely copied from; comment the distinction so it stays put.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LpqkSRqpcnAjsSZgFTUN5z
2026-08-31 16:57:49 -05:00
Evan JarrettandClaude Opus 5 777bd15149 build: race-check the billing package too
test-race runs `go test -race ./...`, which is untagged, so pkg/billing prints
"no test files" and the money path has never been through the race detector.

This is the same gap batch 13 found in `test`, where the only -tags billing line
in the Makefile was a build line and gate_test.go had never executed in CI. That
one was fixed by adding test-billing; test-race was left behind.

It is not a theoretical gap. UpdateCrewTierOnAllHolds fans out to every managed
hold concurrently and joins the errors, and RefreshHoldTiers reads and writes
holdTierCache under a mutex from a background worker while request handlers read
it. Those are the two places in the package where a race would actually live.

Passes: 2.059s, no races reported.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rmjvU2gSRL9wFnmqVsWaF
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 521cf143e5 appview: cover the half of 13edb71 that had no tests
That commit throttles two writes and states a statement order. Only one of the
three claims was defended.

touchLastSeen is the hotter of the two writes — it ran once per indexed record,
so a busy firehose meant a database round trip per event for a timestamp read
in hours or days. Deleting its throttle outright left every existing test green,
as did keying it globally instead of per DID, which would let one busy account
suppress every other account's first write. Both now fail.

The statement order is the third claim: UpdateLastUsed stamps the throttle
before the write rather than after, so a slow or failing write cannot let every
concurrent caller through to queue another attempt behind it. That matters
because this runs on the authentication path, once per layer during a push, and
the pile-up is worst exactly when the database is least able to absorb it.

A failing write makes the ordering observable without timing anything: with the
stamp after the write every call retries, with it before only the first does.
Dropping the table leaves no row to inspect, so the attempts are counted through
the warning the function already logs. Under the reordering it reports 10
attempts across 10 calls.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rmjvU2gSRL9wFnmqVsWaF
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 ff0942196e db: cover the orphan drop in 0034, the case production actually presents
TestMigration0034PreservesLayersAndReferences seeds no orphan and says why: the
live foreign key refuses to create one, so the migration's join through
manifest_id is "insurance for a database whose foreign keys were off at some
point, not for anything reachable now".

Production is that database. It carries 160 layers and 22 manifest_references
pointing at manifests.id values that no longer exist, left by deletes performed
under mattn/go-sqlite3, where the constraint the DDL declared was not enforced.
libSQL turns foreign keys on by default and mattn did not, so the rows predate
the driver swap. The insurance is load-bearing on the only database that
matters, and nothing tested it.

The new case rebuilds the pre-0034 shape with the child foreign keys absent,
which is what that era's schema behaved like, and seeds three orphaned layers
and two orphaned references beside live ones. It asserts the orphans are gone,
the live rows survive attached to the right key, nothing lands keyless, and
foreign_key_check is clean afterwards.

Verified against the defect: with the child manifest_key made nullable and the
joins turned into LEFT JOINs, all five orphans survive and the test fails on
both counts. Recorded honestly, the two guards are redundant with each other —
LEFT JOIN alone still drops them, because INSERT OR IGNORE swallows the NOT NULL
violation. Only removing both carries an orphan forward, and such a row counts,
selects, and joins to no manifest ever again.

Confirmed on a copy of the production database: layers 18803 -> 18643 and
manifest_references 2175 -> 2153, exactly the rows the join excludes, with
manifests unchanged at 3864.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rmjvU2gSRL9wFnmqVsWaF
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 8c9a85826d appview: stop leasing the billing tier refresh, and let it be cancelled
6a7ddb8 moved RefreshHoldTiers under a lease with the comment "one-shot; the
lease is released when it returns". It never returns: past the startup retries
it sits on a 30-minute ticker forever. Three consequences, all observed on a
two-instance run against one database:

The billing-tiers lease is never released on a clean stop, so it survives to
its TTL and the replacement instance waits a full minute for a worker the old
one is no longer running. Worse, the goroutine stays in the shutdown WaitGroup,
so "Timed out waiting for leased workers to stop" now fires on EVERY clean
shutdown. The other four leases released in 12ms and the warning fired anyway.
A warning that is always present cannot report the case it exists for, which is
the jetstream lease genuinely failing to release.

And the lease was the wrong tool regardless. The commit justified it as
"RefreshHoldTiers writes tier state derived from Stripe" that instances would
race on. It writes holdTierCache, a per-process map, from read-only ListTiers
calls; there is no shared state anywhere in the path. Electing one refresher
means every other instance keeps an empty cache forever, so
aggregateHoldFeatures reports "no hold data" on all but one — a regression that
only appears at the scale the lease was added to support. This is the hold
health worker's situation exactly, and that one was deliberately left unleased
in the same commit.

So it runs on every instance again, with a context. The retry backoff was
time.Sleep for up to 45s total against an unreachable hold; it and the ticker
now select on ctx.Done, and ListTiers gets the context instead of
context.Background. LeaseBillingTiers is gone rather than left as a dead name.

Verified: with the backoff restored to time.Sleep the new test fails on its own
2s deadline rather than hanging, which is how a shutdown regression here should
present.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rmjvU2gSRL9wFnmqVsWaF
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 f0b28c04f5 db: cover the DID guard on the batched tag delete
TestDeleteTagsNotInListScopedToDID passes a nil keep list, which takes the
early return and deletes with `DELETE FROM tags WHERE did = ?`. So the scoping
it proves is the shortcut's, not the one inside the chunk loop that f186760
rewrote, and the batched path's `did = ?` had no test at all. Removing it in a
scratch worktree left every existing test green.

The new case gives a second user the same repository:tag pairs and passes a
non-empty keep list, so the chunk loop runs and its delete set collides with
the other user's rows. Verified against the defect: with the guard replaced by
a no-op predicate, 104 of the other user's 105 tags are destroyed.

Two users owning the same repository:tag is the ordinary case rather than a
contrived one, and the blast radius is one user's tag sync silently deleting
another's rows for every name they share.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rmjvU2gSRL9wFnmqVsWaF
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 18c77ace28 test/e2e: drive the billing Drive list against the Stripe sandbox
Three scripts covering what only a browser can see, run end to end against the
sandbox with a real checkout and a real portal cancellation.

batch13-billing-drive.mjs runs the same account either side of one config
change -- whether its default hold appears in server.managed_holds. Managed:
the billing tab offers real tiers, checkout 302s to Stripe, the portal is
reachable. Self-hosted: checkout 403s, the portal still 302s, and the advisor
answers managed_hold_required rather than upgrade_required, which matters
because telling a paying subscriber to "upgrade" would sell them a tier they
already hold.

batch13-portal-cancel.mjs walks into Stripe's portal instead of asserting the
redirect, because the batch card calls a subscriber who cannot cancel the worst
outcome here and a 302 does not prove a cancel control exists at the far end.

batch13-webhook-downgrade.mjs creates three webhooks under an allowance of ten,
then reads the page back after the downgrade.

Every one of these is invisible on a hold owner's account: GetSubscriptionInfo
returns a synthetic "Captain" tier before any Stripe lookup, and
GetWebhookLimits / HasAIAdvisor / GetSupporterBadge bypass on the same first
line. The first run used the shared e2e profile, which still had the owner
signed in, and reported a clean pass built entirely on that bypass. The scripts
now assert the page does not render "Captain", and take a separate profile.

Two traps worth keeping: a bare button[type=submit] matches the nav's hidden
logout button before the form's own submit, and hx-confirm here renders a
custom modal whose backdrop swallows clicks -- strip the attribute rather than
trying to dismiss a dialog that never fires.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 fa6473a896 appview/holdclient: cover the tier fan-out itself, and its failure path
The existing tests covered updateCrewTierWithRetry and UpdateCrewTierOnHold.
UpdateCrewTierOnAllHolds -- the function the Stripe webhook actually calls, and
whose error decides whether a paid upgrade is retried or dropped -- had none.

Three cases: the joined error names every failing hold and not the one that
succeeded; a hold that accepts and never answers does not starve the holds
after it (mutation-verified by making the fan-out serial, which leaves the
healthy hold contacted zero times); and a context deadline aborts the retry
loop rather than running to tierUpdateMaxAttempts.

That last one records a real mismatch rather than an intent. Three attempts at
a 5s client timeout need ~15s, and the webhook allows the whole fan-out 10s, so
under a hang the budget funds two attempts and never three -- confirmed against
a blackholed hold on the dev stack, which failed at exactly 10.0s with a bare
context error rather than the "after N attempts" wrapper. If either constant or
the deadline moves, that test is where the arithmetic gets re-checked.

Also covers the other half in pkg/billing: a fan-out failure has to reach
Stripe as a 5xx and leave stripe_processed_events empty. A hold that is briefly
down otherwise costs the customer their tier permanently -- the same shape of
loss as the customer-lookup hole, one layer further out.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 4dd473bbf1 hold/pds: cover HandleUpdateCrewTier, which had no test
The hold end of the billing fan-out had no test at all -- only the
ErrCrewMemberNotFound sentinel was covered. Its answer decides whether the
Stripe webhook records an event as processed or retries it, so each status it
can return means something different upstream and is covered separately: the
applied path (asserting the stored crew record actually changed, not just the
response body), not-crew as a successful no-op, the 403 on a body userDid that
disagrees with the signed subject, an empty body userDid falling back to the
token subject, 401 unsigned, 400 with no tiers configured, and rank clamping.

Each was mutation-verified. One of them corrected the test's own comment:
removing the 403 guard does not let a body retarget a grant, because every step
after it keys off the token's sub claim and req.UserDID is read nowhere else.
The guard makes a disagreeing body loud rather than silently ignored, and the
stored-tier assertion is the regression guard for the day something reaches for
that unsigned field when it needs "which user".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 2d30f6abb7 test/stripe-integration: give the suite a real database
The suite built its Manager with a nil database, which switches off every
`m.db != nil` branch in HandleWebhook: the idempotency check, the per-customer
ordering guard, and the processed-event record. It ran against real Stripe and
exercised none of the code those guards live in, so it read as far broader
coverage than it was.

It now opens a libsql file under t.TempDir (":memory:" is per-connection, so
the pool's second connection would see no tables), and two new tests cover the
branches that were dead: a redelivery is skipped, and a stale out-of-order
event is ignored. Both assert that the second delivery did not reach a handler
rather than counting rows -- RecordStripeEvent is an idempotent upsert, so
deleting either guard leaves the table identical. Both were mutation-verified
against the guard they cover.

Two fixes fall out of turning the database on:

buildEventPayload never set `created`, which unmarshals as 0. Harmless with no
database; with one, the ordering guard reads every later event for a customer
as older than what it already applied, so the second event silently becomes a
no-op. It is stamped now, with buildEventPayloadAt for the ordering test.

TestHandleWebhookAllSubscribedEvents was failing on this branch and nothing
caught it, because stripe-integration-test is not part of `make test`. It
posted events for the literal "cus_fake" and expected "No user DID found" --
which was true only while a failed customer.Get collapsed to an empty DID.
Since that became a retryable error, the fixture reached the error branch
instead. It now uses a real sandbox customer carrying no user_did, so the
assertion tests the branch it names.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 3263156067 billing: key entitlements on the Stripe product, not the price
Tier resolution matched a subscription's price ID against the configured
stripe_price_monthly/stripe_price_yearly, which inverts what a price change
is supposed to do. Stripe prices are immutable, so changing what a tier costs
means creating a new price, and Stripe never migrates existing subscribers off
the old one. Updating the config to the new price IDs therefore un-tiers
precisely the subscribers a price change is meant to leave alone.

They did not even drop cleanly to free. An unresolved tier logs a warning,
returns nil, and the event is recorded in stripe_processed_events -- so Stripe
answers 200, never redelivers, and a later dashboard Resend is swallowed by the
idempotency check. Reproduced against the sandbox: a subscription on a price
the config does not list granted nothing, and the event could not be replayed
afterwards.

A tier has one product and many prices over its life, so the product is the
durable key for an entitlement. Tiers gain a stripe_product field, and
resolution tries the product first, falling back to the price IDs so configs
without it keep working unchanged. Checkout still keys on price -- that
direction has to name a specific price to charge.

Verified live: a subscription on a price created outside the config, under the
Pro product, resolved to tierName=Pro tierRank=2 and landed on the hold. The
same shape with an unknown product produced the silent no-op before.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 4b9d4bcbeb billing: retry a failed customer lookup instead of dropping the subscription
getCustomerDID returned "" for a FAILED customer.Get exactly as it does for a
customer carrying no user_did. handleSubscriptionChange read that as "not our
customer" and returned nil, so HandleWebhook recorded the event as processed
and answered 200. Stripe never redelivered. A transient Stripe API error
therefore dropped a paid upgrade permanently — the precise "paid but never
received tier" hole 12c55ed was written to close, left open one level down.

It now returns (string, error) so the two cases are distinguishable, and only
the subscription path propagates it. The invoice-failed and dispute handlers
use the DID for logging alone, so a lookup failure there is not worth failing a
webhook over and they ignore it deliberately.

Also fixed: handleSubscriptionChange dereferenced sub.Customer.ID four lines
after an ordering guard that explicitly checks sub.Customer != nil. Confirmed a
real panic, not a theoretical one — the new test panics against the old code.

Five tests, all mutation-verified, and all of them new ground: pkg/billing had
one test file and test/stripe-integration builds its manager with a nil
database, so every `m.db != nil` branch — which is all of the idempotency and
ordering work — was dead there. These use a real database.

The idempotency and ordering tests assert on whether Stripe was CALLED again,
not on row counts. That distinction matters: RecordStripeEvent is an idempotent
upsert, so deleting either guard outright leaves the table looking identical
and a row-count assertion passes. Counting API calls is the only thing that
separates "short-circuited" from "re-applied". My first draft got this wrong
and passed against both mutations.

Makefile: `make test` never ran any of this. The only -tags billing in the file
was a build line, so gate_test.go and checkout_gate_test.go had never executed
in the default target or in CI, and `make lint` never linted the package
either. Both now do; both are clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 894dd243da hold/admin: cover the top-users panel 7d9de7c fixed
7d9de7c moved handle resolution below the sort-and-truncate, so a hold with
~500 crew stopped making ~500 serial identity lookups to render ten rows. It
shipped without a test, and the regression is a two-line move.

The property pinned here is the LOOKUP COUNT, not the wall clock. Timing would
pass or fail on how fast the machine is; the count fails precisely when
resolution moves back above the truncation. Mutation-verified: restoring the
old shape produces 60 lookups for 10 rendered rows against a 50-user hold, and
the failure message names the cause.

The second test covers the 3s resolve deadline the same commit added — a
stalled lookup must degrade to a bare DID rather than consume the reverse proxy
budget, which is what left the client hanging up mid-render before.

resolveHandle becomes a package var, since counting lookups is the only way to
observe either property from outside.

Two things worth knowing for the next test in this package. AdminUI.pds is a
concrete *pds.HoldPDS, so this needed a real one: NewHoldPDS with a file-backed
path (":memory:" is per-connection in libsql and disables the records index
QuotasByDID reads), then Bootstrap, or the first record write fails with
"cannot serialize undefined cid". And BatchCreateLayerRecords writes only to the
CAR store — the records index is fed from the repo event stream, which is not
running in a test, so BackfillRecordsIndex has to be called explicitly or the
quota query returns nothing and the count assertion passes vacuously at zero.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 f4d0c8bf05 auth: verify the hold before caching a captain record on the third path
6758996 started verifying captain records against the publishing DID's
atcr_hold service before caching them, because any account can write an
io.atcr.hold.captain record into its own repo and only a real hold advertises
that service. It covered the two Jetstream writers -- the processor and the
batch backfill -- and left RemoteHoldAuthorizer.GetCaptainRecord alone.

That third path is reachable. A user's sailor profile decides which hold their
content routes to, blob authorization calls CheckReadAccess/CheckWriteAccess
with that DID, and both go through GetCaptainRecord. ResolveHoldURL falls back
to the DID's #atproto_pds endpoint when there is no #atcr_hold service, so a
user who points defaultHold at their own DID serves themselves a captain record
of their own writing -- and it was cached.

The row is the problem, not the fetch. GetAvailableHolds offers every
hold_captain_records row with allow_all_crew=1 to every user's hold picker, so
one unverified row puts an arbitrary DID in front of everyone as a place to
store blobs. GetAccessibleHoldDIDs reads the same table to scope visibility.

Gate the cache write only, not the authorization decision. Failing closed here
would turn a PLC resolution blip into a rejected push, and the freshly fetched
record is no less trustworthy than it was before this commit -- it just must not
become durable. This matches the processor's "skip rather than fail" handling,
where periodic backfill retries an unresolvable DID later.

hasHoldService becomes a package var so the negative case is testable at all:
the real implementation trusts any did:web in test mode, which is the shape
every test here uses. The three new tests are mutation-verified -- removing the
gate caches a row for both a non-hold DID and an unresolvable one, while the
inverse test keeps the gate from degrading into "never cache", which would cost
an XRPC round trip on every authorization while still looking like a pass.

Note in passing: TestFetchCaptainRecordFromXRPC discards its result
(`_ = record; _ = err`) and asserts nothing. Left alone here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 5112425673 appview: make the footer Bluesky link configurable
The link was hardcoded before fa34da0 too — it pointed at
bsky.app/profile/atcr.io. That commit was right to switch to a DID, since
handles change and a stale handle link breaks silently, but the DID went into
components/footer.html, a shared template. Every self-hoster's footer therefore
links to the project's Bluesky account.

Now ui.bluesky_profile, following source_url in the same footer exactly:
config field with a default, plumbed through UIDependencies and PageData, and
guarded with {{ with }} so an unset value omits the link rather than rendering
something wrong. It takes a handle or a DID; the comment says to prefer a DID
for the reason fa34da0 changed it.

Defaulting to the project account matches source_url's logic — both name the
upstream project rather than the operator — and self-hosters who want their own
or none set one line.

The aria-label switched to $.ClientShortName: `with` rebinds the dot, so the
label would otherwise have silently rendered empty.

Two things worth recording:

  * `{{ with }}` hides the link on an empty value, but you cannot get an empty
    value from the environment. Viper runs with AllowEmptyEnv(false), so an
    empty env var reads as unset and the default wins. Only "" in YAML works.
    That applies to every string field in this config, not just this one, and
    the comment now says so.
  * config-appview.example.yaml must NOT be regenerated with `config init`,
    despite what the checklist in CLAUDE.md says. The file is hand-curated well
    past the defaults, and regenerating replaces real Stripe price IDs with
    price_xxx placeholders and blanks registry_domains, managed_holds, theme
    and the tier names. Added by hand instead.

Verified live both ways: the configured value renders, and `bluesky_profile: ""`
in YAML drops the link while leaving the Source link intact.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:26 -05:00
Evan JarrettandClaude Opus 5 34b4516aa7 test/e2e: correct the claim that headed Chromium cannot launch here
It can, and normally in well under a second. Two launches hung for the full
180s handshake timeout under heavy concurrent docker and test load, and I wrote
that up as "headed is impossible from an agent shell" and moved everything to
headless. That was wrong, and wrong in a way that would have quietly degraded
every future browser check.

The display is reachable: DISPLAY=:0, XAUTHORITY set to the mutter XWayland
cookie, both the Wayland socket and /tmp/.X11-unix/X0 present, xdpyinfo happy.
The README now says to check xdpyinfo and retry rather than conclude anything,
and batch10-anonpull.mjs defaults to headed again.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 17e25a4df2 appview: guard the tag-listing paging that gates a shared-record delete
The delete path decides whether an io.atcr.manifest record is still wanted by
enumerating the DID's tag records. That enumeration is load-bearing in a way a
tag listing usually is not: records for every one of a DID's repositories share
one collection, so a single page is a per-account budget rather than a per-repo
one. Past it a live tag falls off the end, the digest reads as unreferenced,
and the shared record is deleted out from under a repository nobody touched —
along with its layers on the hold.

Two cases, both mutation-verified:

  * the tag that keeps the digest alive sits on page 3. Stopping after the
    first page deletes the record. cleanupUntaggedManifest has carried this
    hazard in a comment since 1b91768 with nothing asserting it.
  * the listing never terminates. Concluding "unreferenced" from an incomplete
    read is the dangerous answer, so this must error rather than guess; making
    it return what it found so far deletes the record.

Both assert on whether a deleteRecord for the manifest collection was issued,
not on the returned error, because the error is incidental and the deletion is
the thing that cannot be undone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 b17ebb69a5 test: cover the nested-repo tag rkey on the delete paths
c035f50 fixed a hand-built tag rkey in DeleteManifestHandler and shipped with
no test. The hazard is not specific to that handler: io.atcr.tag rkeys come
from RepositoryTagToRKey, which encodes "/" as "~", so any code building one by
hand targets a record that does not exist — and deleteRecord being idempotent
makes that a silent no-op. The local view looks right and the tag returns on
the next backfill.

The by-digest path now builds tag rkeys too (594d73b), so it could reintroduce
exactly this bug. TestManifestDelete_NestedRepoTagRKey pins it there: push to
stream/cache, delete by digest, and assert the tag is no longer listed.
Listing is what catches a survivor — TagStore.All reads the records back from
the PDS and filters by repository, so a stale one is still reported.

Mutation-verified by hand-building the rkey as "repo:tag": the nested test
fails with the tag still listed, and TestManifestDelete passes unchanged. That
second half is the point — every existing delete test uses a flat repository
name, and a flat name cannot reproduce this bug at all.

batch11-nested-rkey.mjs drives the same property through the UI handler that
c035f50 actually fixed, asserting against the PDS record rather than the page,
since the page looks correct either way until a backfill runs. It needs an
interactive appview login in the Playwright profile and is not yet run; the
session that exists belongs to a different browser profile. Two instrument
notes are baked in: probe /settings rather than the repo page to detect a
session, because /r/ renders for anonymous visitors and can never report a
missing one, and use maxRedirects:0, because RequireAuth 302s and a followed
redirect surfaces as a confusing 405 on DELETE /login.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 0b212a527f appview: guard the two UI delete paths against cross-repo destruction
Same defect as the OCI path, two more places. The io.atcr.manifest record is
keyed by digest alone, so one record backs every repository of a DID holding
identical content, and both of these deleted it without asking whether another
repository still wants it — then purged the layers on the hold.

DeleteManifestHandler already removes this repository's tags before deleting
the record, so a tag remaining at that point can only belong to another
repository. It now checks IsManifestTaggedAnyRepo there and keeps the shared
record when one does, reporting sharedRecordKept so the caller can tell the
difference between "deleted" and "deliberately left alone".

DeleteUntaggedManifestsHandler is the subtler one. Its digest list comes from
GetAllUntaggedManifestDigests, whose tag join is scoped to one repository
(m.repository = t.repository), so a digest tagged only in a DIFFERENT
repository is reported as untagged and swept. The query is a reasonable
per-repository view and a dangerous delete list; the guard goes at the delete,
not in the query, matching how DeleteTagHandler already works. Skips are
counted separately from failures, because a skip is the guard working and
folding it into "failed" would make a correct run look broken.

Both fail closed. Leaving a manifest behind is recoverable; deleting one
another repository is still serving is not.

The new db test pins both halves of the interaction: that the query really does
report a cross-repo-tagged digest as untagged, so a change there is noticed,
and that IsManifestTaggedAnyRepo answers DID-wide, which is the thing actually
standing between the sweep and another repo's live image.

DeleteTagHandler needed no change — 2580dcd already routed it through
ShouldCascadeDeleteManifest, which is where the correct policy was written down.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 701c866723 appview: stop an OCI manifest delete destroying another repo's image
The io.atcr.manifest record is keyed by digest alone (digestToRKey), so one
record backs every repository of a DID holding identical content.
ManifestStore.Delete removed it unconditionally and then called
purgeDeletedManifest, which asks the hold to drop the layer records and free
the blobs. Deleting through one repository therefore stripped the manifest out
from under every other repository sharing that digest and took their bytes with
it — reachable from the public OCI API with `crane delete`, and not recoverable.

Reproduced end to end before fixing: push identical content to shared-a and
shared-b, delete shared-a by digest, and shared-b:v1 answers 404.

The codebase already had the right guard and the right policy written down.
2580dcd added cleanupUntaggedManifest for exactly this hazard, and its comment
says it plainly — the check is "deliberately not filtered to rctx.Repository"
because "a tag in any of them keeps it alive". TagStore.Untag routes through it.
The by-digest path never did.

Delete now makes one pass over the DID's tag records, which answers both
questions at once: which tags in THIS repository point at the digest, and
whether any other repository still does. This repository's tags are removed
either way, because a DELETE scoped to a repository has to stop that repository
serving the image; the shared record and the hold purge only happen when
nothing else tags it. Enumeration failure is returned as an error rather than
swallowed, so the caller fails closed — leaving a manifest behind is
recoverable, deleting a live one is not, and the page budget exists for the
same reason cleanupUntaggedManifest has one.

Covered twice on purpose. The integration test proves the user-visible property
(repo B still pulls). The unit test asserts the thing an end-to-end pull can
only infer: that no deleteRecord for the manifest collection is issued at all.
Both fail against the pre-fix code.

TestManifestStore_Delete needed updating rather than fixing: its fake server
asserted every request was a deleteRecord, which the new tag-listing call
breaks. It now serves an empty tag list and additionally asserts the manifest
delete still happens, so the unshared path stays pinned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 3dcb4b5c02 test/e2e: document the batch-10 traps
Five things that each produced a confident wrong answer during val/10-anonpull,
so the next batch does not rediscover them: the repo page route, tags living in
a <select>, headed Chromium not launching from an agent shell, logged-out
checks needing their own browser profile, and the dev hold defaulting to
public:false with a propagation delay after the flip.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 43bf79c71f test/integration: make every pull client actually read a blob
Two of the three matrix clients never fetched one. orasClient.Pull stopped at
repo.Resolve (a HEAD on the manifest) and regclient's stopped at ManifestHead,
so their pull rows — including anonymous_pull_denied, stranger_pull and
crew_read_only_pull on the private-hold matrix — were passing without ever
exercising blob authorization. They passed on the manifest denial alone.

That is invisible from the outside because it produces the right verdicts for
the wrong reason. The full suite is still green after the fix, so no
authorization bug was hiding behind it; what was hiding was the coverage.

craneClient.Pull was already correct: 5aa13ab added its layer-materialization
loop precisely because crane is lazy and ATCR serves manifests from the user's
PDS, where they are world-readable by design. The oras comment still carried
the pre-5aa13ab rationale — "We don't need to fetch blobs; that mirrors
crane.Pull followed by .Digest(), which is also manifest-only" — which that
commit had already invalidated. Both clients now match crane, and the stale
comment is gone.

TestPullClientsReadBlobs guards all three against regressing to manifest-only.
It runs each client against a registry that serves the manifest happily and
403s the layer, and asserts both that a blob was requested and that the refusal
surfaces as an error. Written first and run before the fix, where it passed for
crane and failed for oras and regclient — which is how the gap was found.

This also closes the hole the plan flagged for craneClient.Pull alone: nothing
protected that loop, and deleting it left the entire suite green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 00e2897c30 test/e2e: drive the anonymous repo page, public and private
The API half of val/10-anonpull is covered by the auth matrix. This covers what
Go tests structurally cannot see: whether a logged-out repo page renders, 500s,
or comes back as a blank panel.

Public hold, logged out: 200, all four tags render, and the digest on the page
matches what the registry serves for the selected tag. That last check compares
against whichever tag is selected rather than a hardcoded one, because the page
renders the selected tag's digest and a hardcoded comparison silently fails
whenever the default changes.

Three instrument bugs are baked into the script as comments, because each of
them produced a confident wrong answer first:

  * The repo page is /r/{handle}/*, not /{handle}/{repo}. The latter is a 404
    "Lost at Sea" page, which reads exactly like a denial if you don't check.
  * Tags are <option>s in a <select>. Scraping a,td,span finds nothing and
    reports "no tags" on a page that is rendering them correctly.
  * Logged-out checks use a throwaway persistent profile. A plain
    chromium.launch() does not complete its handshake here, and clearing the
    shared profile's cookies would cost an interactive re-login.

Private hold, logged out: the plan expects a denial. It is not what happens —
the page returns 200 with every tag listed while /v2/ refuses the same repo
with 401 for the same caller. Recorded as a documented divergence rather than
asserted as a failure: the manifest records are world-readable in the user's
PDS by design, so nothing secret is exposed, and /r/ has used OptionalAuth
since before validate-base with the page never consulting captain.Public. The
two read paths simply disagree, and that predates this range.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 ae1d7ba626 auth: test NarrowToPullOnly, the function that gates the anonymous path
NarrowToPullOnly had no test. IsPullOnlyScope has a thorough one, but it only
answers yes/no — NarrowToPullOnly rewrites the access list, so what it emits is
what gets signed, and anonymous tokens skip the authgate entirely. Nothing
downstream re-authorizes what this function decides to hand out.

The load-bearing property is the allowlist: "pull" is the only action that can
survive. Beyond the per-case assertions, every case re-checks that no other
action reached the output, so a new case cannot accidentally assert its way
past the property the function exists to hold.

The wildcard cases are the point. A wildcard action means "any action" to
distribution's actionSet.contains, so expanding "*" into "pull" is the single
rewrite that would turn a wildcard request into a grant. Mutation-verified:

  * treat "*" as pull            -> the three wildcard cases fail
  * stop narrowing the actions   -> the four narrowing cases fail
  * trim the action slice in place -> DoesNotMutateInput fails

That last one initially did NOT fail, and the fixture is why. The input had
"pull" first, so an in-place trim writing "pull" into index 0 changed nothing
observable and the test passed against the exact defect it was written for.
"pull" is now deliberately not first, with a comment saying so, because the
ordering is the whole instrument here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 915d9adcb2 test/e2e: cover the /auth/token surface, and correct 9d4ad84's provenance
batch09-token.sh drives the request shapes against a running stack, which is
where the interesting part of b25aee3 lives: handler_test.go proves each shape
in-process, but it cannot show which form a real client picks, and that is the
whole reason the commit exists.

What driving real clients turned up, now encoded in the script's comments so a
re-run re-checks it:

  * Docker 29.7.2 and skopeo 1.22.2 use the GET form even holding a credential
    helper secret, and take two token requests for a pull with no 401 retry —
    so neither exercises the POST path at all.
  * containerd 2.3.3 does POST, and gets 200. That is the client b25aee3 was
    written for, and the only one here that would have eaten the old 405.
  * There is no anonymous branch in the handler at this branch; anonymous GET
    is a 401. The anonymous path arrives with val/10-anonpull, so the plan's
    "anonymous pull is GET-only" note describes a later batch.

seed-legacy-devices.go reproduces the day-one production devices table for the
08121f3 check: every row legacy, the real device inserted last so it sits at
the end of the rowid-order scan. Measured here at 200 rows: 7.83s first auth,
backfilled, 0.006s second. It is build-tagged `ignore` so it stays out of
go build ./... while remaining go run-able.

The 9d4ad84 comment claimed the reference PDS and tranquil both answer 403
InsufficientScope for a read-only app password. Only the tranquil half is
supported: the observation is issue #26 on pds.sqrl.systems, and the reference
PDS has no read-only app passwords at all, so the branch cannot be reproduced
against one. The comment now records that provenance and the reason a wrong
guess is harmless — the classification only adds a branch, and an unrecognised
error name falls through to the 503 that shipped before it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 85a07d660a appview: give the O(n) bcrypt scan a guard that can actually fail
TestDeviceStore_ValidateDoesNotScanIndexedRows is documented as "the
regression guard for the O(n) bcrypt scan", but it does not observe whether a
scan happened. It asserts that every row is indexed and that an unknown secret
errors, and both hold with or without the fix. Deleting the
`WHERE secret_lookup IS NULL` filter from the fallback query — which is the
defect 08121f3 removed — leaves it green.

That matters more here than elsewhere in the batch. No backfill of the
production table is possible (plaintext is not recoverable from bcrypt), so
all 244 devices are legacy on day one and migrate lazily on first auth. A
regression on this path locks out every existing user while new devices keep
working, which is the failure mode least likely to show up in a smoke test.

The scan is only observable in time, so the new guard makes one comparison
expensive (bcrypt cost 13, ~300ms here) and asserts a deadline. Six seeded
rows cost ~1.8s to scan and ~0ms to skip. Only one hash is generated: the
others are copies with a mutated final byte, which bcrypt still runs the full
key derivation over before rejecting, so setup stays at a single 300ms hash.

Mutation-verified in a worktree by removing the filter: the old guard reports
ok, the new one fails at 1.84s against a 400ms budget.

Both halves are asserted — an unknown secret (the old code's worst case, where
nothing matches and every row is compared) and a known indexed one — because
only the first catches the missing filter and only the second catches the
index being bypassed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 ce01e47ba6 auth: cover the two batch-09 commits that shipped without tests
9d4ad84 (read-only app password -> 403) and e6959e6 (bounded HTTP clients on
the token path) both landed with no test at all. These are the ones a
regression would be silent in: a revert of either leaves every existing test
green.

Each test was mutation-verified against the defect it claims to catch, in a
throwaway worktree, and required to fail:

  * revert ResolveHoldDID to http.DefaultClient  -> SlowHoldIsCutOff fails
  * revert getServiceAuth to http.DefaultClient  -> SlowPDSIsCutOff fails
  * NewSessionValidator back to &http.Client{}   -> ClientsAreBounded fails
  * drop the InsufficientScope classification    -> IsClassified fails
  * drop the handler's errors.Is branch          -> Returns403 fails, and the
    body it returns is the exact retry-inviting 503 UNAVAILABLE the commit
    exists to remove

The slow-path tests wait on an outer deadline rather than on the call itself.
With an unbounded client these calls never return, so a test that simply
awaited the result would hang the suite instead of failing it, and a hung
suite reports nothing.

The client caps are asserted twice on purpose: once as a field value, which
guards the production 10s/15s numbers, and once functionally, which proves the
call site routes through the bounded client rather than merely declaring one.
Neither half catches the other's regression.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 576a6b9e35 scanner: test grype DB freshness, backoff and reload fallback
scanner/internal/scan had no test file at all, which is why fa1dfb0 could be
written and reviewed without anyone being able to state its defect as an
assertion. Six tests now do.

The load path is only reachable from a test through an indirection, so
grype.LoadVulnerabilityDB is now behind a loadVulnDB package var. Everything
worth testing here is what happens when that call returns a stale database or
fails outright, and neither is reachable from a test that has to perform a
real download.

Covered: freshness is taken from the DB's own build timestamp; a fresh DB is
reused without a download; a stale DB inside the retry backoff keeps serving
without one; the backoff expires and the replacement is adopted with its
predecessor closed; a failed reload with a usable provider in hand keeps
scanning and still advances the attempt timestamp; and a cold-start failure is
an error rather than a scan that silently finds nothing.

Verified by mutation. Restoring `vulnDBBuilt = time.Now()` — the original
defect — fails the freshness test with the stale build time in the message.
Removing the serve-the-old-DB fallback fails the outage test.

One honest limit, recorded in the test file. The backoff is tested twice in
the production code, on the read-lock fast path and again under the write
lock, and mutation shows they are redundant for correctness: deleting either
alone leaves the throttle test passing, and only deleting both fails it. The
fast-path copy exists so a stale DB does not push every scan through the
exclusive lock, which is a contention property, not a behavioural one, and no
unit test can assert it without being flaky.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 2984331f0c hold/gc: test the blob sweep, and give the S3 mock object ages
deleteOrphanedBlobs is the only part of GC that removes bytes and it had no
test. It could not have had one: object age decides whether a blob is
deletable, the in-process harness stamps every object with time.Now(), and
MockS3Client's ListObjectsV2 set no LastModified at all. Neither could express
"this blob is nine days old", so both halves of the grace rule went
unexercised — including the half that protects a push still in flight.

MockS3Client gains ObjectTimes, a per-key LastModified consulted by
ListObjectsV2. Keys with no entry list without a timestamp exactly as before,
so existing tests are unaffected. It also gains DeleteObjectError, matching
the error injection the other operations already had.

Four cases, three of which are reasons NOT to delete: an old unreferenced
blob goes; a young unreferenced blob stays; a referenced old blob stays; a
/link object is never treated as a blob. Plus a failure case pinning that one
undeletable object does not abort the walk, and that a blob which never left
storage is not counted as deleted or reported as reclaimed space.

Verified by mutation. Disabling the grace check deletes the young blob;
disabling the referenced check deletes the live one; both fail. Disabling the
/data suffix check changes nothing, because extractDigestFromPath anchors on
/data$ and rejects everything else — so that check is a redundant early-out
rather than a guard, and the test says so rather than implying otherwise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 5f74299bd7 test/e2e: exercise the stale-preview refusal and the real GC sweep
Three scripts, split by what they cost to run.

batch07-stale-preview.mjs stages a preview and waits out the 30-minute
maxPreviewAgeForDelete constant. batch07-stale-click.mjs is the resumable
half: it re-renders whatever preview the hold already holds and clicks delete
on it. GET /admin/api/gc/status re-renders lastPreview WITHOUT touching
lastPreviewAt, so showing an old preview does not reset its age — which is
what makes a failed run cost seconds instead of another 31 minutes.

Result against the dev hold: "preview is 34m0s old (limit 30m0s) — run Scan
again before deleting" rendered through the progress-to-error fragment chain,
with all 387 records still there afterwards. That chain is the point; the
refusal logic itself already has a Go test, but a refusal that renders as
nothing is indistinguishable from "there was nothing to delete".

batch07-sweep.mjs then runs the destructive path for real: 387 records
deleted of 387 staged, orphaned count to zero, referenced blobs unchanged at
15. Safe only against the dev hold on Storj; production is Bunny + UpCloud
and is not reachable from here.

page.on('dialog') did not reliably intercept hx-confirm on this page, and an
unaccepted native dialog blocks every later evaluate() and innerText(), so
the script hangs rather than fails — the worst failure mode for an unattended
check. Both scripts now strip the hx-confirm attribute before clicking. The
confirm is not what is under test.

Two findings worth carrying, neither introduced by this range:

  * deleteOrphanedBlobs is still unexercised. The bucket holds 19 objects,
    of which 8 are past the 7-day blob grace, and none are unreferenced — so
    there is nothing for it to collect. More pushes cannot help: fresh blobs
    are inside the grace window by definition.

  * Storage accounting is derived from layer records, so this sweep moved the
    dashboard from 1.3 GB to 1.1 KB while the bucket held 147 MB throughout.
    It was overstating by ~9x before (records for blobs held by another hold)
    and understates now (referenced blobs with no layer records). Quotas and
    billing read the same number. Belongs to batch 12.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 0071528b8f test/e2e: check the GC preview panel and hand-check its orphan claim
Drives "Scan for Orphans" through the admin panel and asserts the wiring the
Go tests structurally cannot: that the progress fragment swaps into
#gc-results and hands off to the preview fragment, that every advertised stat
renders a value, and that each table's row count agrees with the stat above
it. A GC that decides correctly and renders a blank panel still gets someone
to click delete on the wrong thing.

Then it hand-checks the claim itself, which is the part that matters: for
each distinct manifest behind an orphaned record, resolve the owner's PDS and
ask whether that manifest is really gone.

Two instrument bugs were found writing this, both in the script rather than
the product, and both worth keeping as comments:

  * The three tables overlap on a Digest column, so classifying by "has
    Digest but no RKey" swallowed Missing Records as orphaned blobs and
    reported 3 blobs against a stat of 0. Classification is now by exact
    header set.

  * Asserting the manifest is ABSENT from the PDS is too strong. A manifest
    can be alive and name a different hold, which is exactly what happens
    when defaultHold is repointed and the image re-pushed. Those records are
    legitimately orphaned here. The only state that means GC is staged to
    destroy live data is a manifest that exists AND still names this hold.

Against the dev hold: 387 orphaned records over 68 distinct manifests, 25
sampled — 17 gone, 8 alive but now pointing at the production hold, 0 still
naming this hold. Orphaned blobs 0, referenced 15, and the counts agree with
the tables.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 4542897f08 hold/gc: escape the DID in listRecords/getRecord repo parameters
fetchUserManifestsFromEndpoint, fetchUserTags and fetchUserProfile
interpolated the user DID straight into a query string. A did:web carrying a
port spells that port as a literal %3A, so the PDS received the parameter
decoded back to ":" — a different DID, matching no repo. listRecords then
answers 200 with an empty list and GC reads it as "this user has no
manifests": every blob they own drops out of the referenced set and is
deleted once past the seven-day blob grace.

There is no error and no status code to notice, which is the same soft
failure 95d4f7c fixed one function away in this file. getRecord for
manifests and checkPredecessorAt already escaped; three of the five call
sites did not.

Scope is local testing only. did:plc, which every production user has,
contains nothing that needs escaping, and did:web with a port is not really
valid in atproto — but it is what the dev stack runs on, so GC there sees a
referenced set of zero and considers every blob in the bucket collectable.
That made it impossible to validate the sweep end to end, which is how it
surfaced.

Found by test/integration/gc_test.go, added here: it pushes an image and
asserts GC accounts for every blob the push wrote. The blob grace period is
a package constant, so nothing pushed during a test can age past it and "no
orphans" is vacuous; the load-bearing assertion is referenced == total,
which grace does not touch. Against the unescaped code it reported
referenced=0 of 4.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 891ad01de3 hold/gc: require a predecessor's successor to name this hold
2e55352 taught the scan broadcaster that a successor label is only
interesting when it points at us; the GC copy of the same check was left
accepting any non-empty successor. Confirmed still divergent at the head of
this stack: gc.go was a bare `if captain.Successor != ""` while
scan_broadcaster.go:1473 compares against sb.holdDID.

A hold that retired into some third hold is that hold's predecessor, not
ours, and its manifests are not a reason to keep blobs referenced here. GC
therefore now makes the same comparison the broadcaster does, against
gc.pds.DID().

This is the one change in the batch that makes GC delete more rather than
less, so it is deliberately its own commit and carries a floor. If this hold
cannot say who it is, ourHoldDID() returns "" and the old permissive answer
stands: we cannot conclude a successor is not us, and over-protecting merely
leaks blobs while guessing the other way destroys them. That branch has its
own test, because an empty DID silently turning every predecessor into a
stranger is exactly how this reconciliation would become the next blob-loss
bug.

The inconclusive-on-failure semantics 95d4f7c added are untouched: only the
answers from a hold that actually replied are affected.

Verified by mutation: forcing the comparison back to the permissive form
fails exactly one case, the successor naming a third hold, and leaves the
unknown-own-DID fallback passing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 48eee49ef9 hold/gc: cover checkPredecessorAt and the unresolved-holds reset
95d4f7c split the fetch-and-parse half of the predecessor probe into
checkPredecessorAt precisely so it could be tested against a local server,
but no test ever followed. The paths that had none are the ones that used to
delete another hold's blobs: a 500, a refused connection, a body that is not
JSON, and an envelope wrapping a garbage record all have to report
definitive=false, because only a definitive answer is allowed into the
process-lifetime predecessorCache.

Verified by mutation rather than by passing: flipping the six failure-path
returns in checkPredecessorAt to definitive=true, which is the pre-95d4f7c
semantic, fails all four cases.

Also pins the reset that the predecessorUnresolved field documents but
nothing enforced. Removing the clear at the top of analyzeRecords makes the
test fail, which is the point: without it one outage is permanent, every
later run short-circuits on the stale entry, and GC silently stops
reclaiming anything that hold's manifests touch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 bb45a80d51 hold/pds: cover the scanner-disconnect teardown 05b856b fixed
newTestScanBroadcaster builds the struct with only a database, so nothing in
the suite ever reached handleWriter, handleReader, or the Unsubscribe teardown
they share — which is precisely what 05b856b changed. These give the subscriber
a real WebSocket so the teardown actually runs.

The invariant is the one Unsubscribe documents: a dropped scanner unwinds both
goroutines and each calls Unsubscribe, so everything past the `found` guard must
happen exactly once. Closing `done` twice panics and takes the hold down with
it, and re-running the requeue UPDATE would unassign jobs a replacement scanner
had already claimed.

Three cases: Unsubscribe called twice on the same subscriber, handleWriter
releasing and closing the connection once done is closed, and the real shape of
a scanner vanishing — client closed, both goroutines unwinding into the same
subscriber, plus a late duplicate Unsubscribe after the fact.

Passes -race -count=5.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 8cd59a61f1 appview: stop a UI session outliving the OAuth session behind it
Only one oauth_sessions row is kept per account, so signing in again — on a
second device, or simply a second time — replaces it and leaves every earlier
ui_sessions row pointing at an oauth_session_id that no longer exists. Get
checked only expiry, so those still read back as usable.

Found on a live appview: four ui_sessions rows, three orphaned, and requesting
/settings/user with an orphaned cookie returned 200 with the account's handle
rendered throughout, where an anonymous request gets a 302. The browser looks
signed in while the credential behind it is gone, so every PDS-backed action
fails against a UI insisting the session is fine. It now fails closed and sends
the user back through login.

Get also never checked ownership. oauth_sessions is unique on
(account_did, session_id), so the existence check is scoped by both; matching
session_id alone would let one account's live OAuth session validate another
account's dangling reference. That has its own test.

An empty oauth_session_id stays valid, since Create makes sessions that never
had one, and a test pins that so the check cannot start rejecting them.

TestSessionStore_CreateWithOAuth referenced an OAuth session it never inserted,
which is an orphan by definition, so it now creates the row. Its intent was
that CreateWithOAuth persists the ID; it relied on the orphan behaviour only
incidentally. Its not-found branch used t.Error and then dereferenced the nil
session, so that is now t.Fatal.

Pre-existing at efabb677 rather than introduced by this range.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 0a6f20fa74 test/e2e: prove a browser session survives an OAuth refresh
oauth-refresh-e2e.sh covers the refresh mechanics; this covers the symptom the
range exists to stop — a signed-in browser being thrown out when the access
token rotates underneath it.

The test is only meaningful because ui_sessions is a table carrying an
oauth_session_id rather than an in-memory map, so restarting the appview to
clear the refresher's cache does not by itself log the browser out. Verified
against the row the browser actually uses: one oauth_sessions row, and the
ui_sessions row created by the login points at it. rev advances 1 to 2 while
/settings/user keeps rendering.

Drives the login itself. Given a handle it fills the field and clicks through
consent, which is the whole flow whenever the PDS already has a session. Two
things it must not do, both learned by doing them:

  * Never navigate while waiting for a human. The first version re-issued
    goto() every two seconds and wiped the login form out from under whoever
    was typing into it.
  * Never bail permanently at a password field. Returning there left the flow
    stranded on the Authorize screen once the password had been submitted,
    because nothing was left to click it. It now pauses and resumes.

ATCR_E2E_FRESH clears only the appview's cookies. Clearing all of them takes
the PDS session with it, which turns a handle-and-consent flow into a password
prompt and makes the login impossible to drive unattended.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 30caf43146 test/e2e: drive the OAuth refresh path against a real PDS
client_test.go reproduces the cancellation precisely, but in-process against an
httptest PDS. The failure this range fixes — mass sign-outs — happened against a
real one, through the real refresher and the real oauth_sessions row, so the
unit tests alone are a thinner sign-off than the batch deserves.

Staling the access token in the live row and restarting the appview (the
refresher caches sessions in memory, so editing the DB alone changes nothing)
forces the real refresh path. Four concurrent pulls then advance rev 1 to 3
rather than 1 to 4: the compare-and-swap collapses four racing refreshes into
two rotations, with the losers adopting the winner's token instead of each
burning one. Killing a pull 150ms into that window and retrying still succeeds.

Backs ui.db up first, since a burned refresh token would otherwise leave the
account signed out.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 c9f8b4178c credhelper: stop dev builds nagging about an update to themselves
isNewerVersion split versions on "." and ran each component through
strconv.Atoi, discarding the error and substituting 0. For a git-describe
build the last component is "4-18-g8f70cce", which does not parse, so it
became 0 and every published release compared as newer. Running
v0.1.4-18-g8f70cce printed "Update available: v0.1.4" on every single
invocation, naming a version the binary was already 18 commits past.

Versions are now parsed properly: the "-<commits>-g<sha>" tail is recognised
and kept as a count of commits past the tag, and a version that cannot be
read in full returns false rather than being silently treated as 0. That
second part is the actual root cause — the comparison could not distinguish
"this component is zero" from "I could not read this component".

Ordering for a git-describe build is deliberately not semver, where a
prerelease sorts below its release. Such a build is commits AHEAD of its tag,
so v0.1.4-18-g8f70cce is newer than v0.1.4 and older than v0.1.4-20-gabc1234.

The function had no tests. Both failing cases are pinned along with the
ordinary release comparisons, so the git-describe handling cannot regress the
normal upgrade path.

Pre-existing at efabb677 rather than introduced by this range.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 724e22a978 test/e2e: only reset the DB when moving backward through the stack
Migrations are forward-only and the appview applies whatever is missing on
boot, so moving to the next batch does not need a reset at all — Air rebuilds
into the new code and the live DB migrates in place. Verified moving onto
val/04-oauth: 0031 appeared in schema_migrations on its own, on top of a
level-27 database, with the appview healthy afterwards.

That matters more than it sounds. ui.db holds the OAuth sessions and the
appview's signing keys, so the old wipe-on-every-switch cost an interactive
`docker-credential-atcr login` per batch, which is most of what made the stack
awkward to hand to an agent. Validating in stack order is all forward motion,
so in the normal case there is now no login at all.

A reset is still done when the live DB carries migrations the branch's code has
never heard of, which is what going backward means, and the per-set snapshot is
still banked so that case can restore rather than start empty.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 c1604b9a04 test/e2e: key DB snapshots on the migration set, not the highest version
Batching reorders migrations. val/04-oauth carries e75b2e2 (commit 48 of the
range), whose migration is 0031, while 0028-0030 only arrive in batches 09, 12
and 14. So val/04 holds {..0027, 0031} and val/09 holds {..0027, 0028, 0031}.
Both have a max version of 0031, so keying the snapshot on the max would
restore val/04's database onto val/09 — a database missing 0028's schema while
schema_migrations claims otherwise.

Snapshots are now keyed on a fingerprint of every migration file present, which
distinguishes those two states. The max version is kept in the filename purely
so the directory stays readable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 f8d9ad7fe9 appview: let a registry domain keep its port, and its /v2
DomainRoutingMiddleware normalized the request Host to a bare hostname but
matched server.registry_domains verbatim, so any configured domain carrying a
port could never match. config-appview.example.yaml ships
`registry_domains: [127.0.0.1:5000, atcr.io]`, which means that entry has been
inert since it was written.

It fails closed in the worst way. That same host is also the auto-detected UI
host, and `host == uiHost` was evaluated first, so /v2/* was answered with
"registry API is not available on this domain, use 127.0.0.1:5000" — naming the
exact host the client had just used. The registry API is unreachable on the dev
stack, and any single-host deployment hits the same wall: listing a host in
registry_domains does nothing if it is also the UI host.

Both sides are now normalized through hostWithoutPort, and a registry domain
takes /v2/* even when it doubles as the UI host, which is a legitimate
single-domain deployment. Everything else is unchanged: a UI-only host still
refuses /v2/, registry domains still redirect non-/v2 traffic to the UI, and
/auth/token and /auth/device/* are still served directly so a cross-host 307
cannot strip the Authorization header.

hostWithoutPort uses net.SplitHostPort instead of the previous LastIndex(":")
scan, which mangled bracketed IPv6 literals into "[::1" and could never match
the "::1" that url.URL.Hostname() yields for the UI host.

The middleware had no tests at all. The two failing cases are pinned first, and
the four pre-existing behaviours are pinned alongside them so the reorder
cannot quietly widen what /v2/ is served on.

Pre-existing at efabb677 rather than introduced by this range, but it blocks
every registry-facing batch in the stack, so it lands at the base.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 50e77ac7a4 test/e2e: snapshot the appview DB per migration level
Wiping ui.db on every switch also wipes the OAuth sessions and the appview's
oauth_p256/jwt_rsa keys, so each batch cost an interactive
`docker-credential-atcr login`. There are only five distinct migration levels
across the stack (27 for batches 00-08, 28 for 09-11, 29 for 12-13, 32 for 14,
34 for 15), so the DB is snapshotted per level and restored instead of
re-migrated. One login now serves every batch sharing a level.

Also skip the compose pin from val/01 onward: a7c7db6 lands there, so the
branch's own compose already has the shared netns and pinning would drag in
later batches' changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 5aefa85048 test/e2e: cover the admin job wiring ab4a4eb changed
jobs_test.go covers the job framework thoroughly, but nothing covers the
wiring: whether the kickoff handler renders the progress fragment into the
right hx-target, and whether the loop actually outlives the request it was
started from. Both are what ab4a4eb changed, and both are invisible to Go
tests — a typo in an hx-target or a fragment that renders blank passes every
assertion we have.

The load-bearing check drives crew import rather than the tier remap. A
one-member remap completes in under a second, so closing the tab "mid-run"
proves nothing; import does a PDS write plus a network PLC lookup per entry,
which leaves a real window to close the browser and watch the job keep going.
It is caught mid-flight at a progress tick with no admin page open.

Seeded members are created on the local-only dev hold and removed in a
finally block. README records the environment traps found while building
this: 127.0.0.1 vs localhost, in-memory sessions dying on every hold rebuild,
UA/IP pinning that makes curl log you out, and the forward-only appview
migrations that require a per-batch DB reset.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00