Commit Graph
516 Commits
Author SHA1 Message Date
Evan JarrettandClaude Opus 5 9d8bd513da appview: render the install scripts from config instead of shipping ATCR's
seamark.dev's /install and /settings/devices told users to pipe
seamark.dev/static/install.sh into bash. That file was the unmodified ATCR
script: it announced itself as the "ATCR Credential Helper Installer",
installed docker-credential-atcr, and finished by telling the user to configure
credHelpers for atcr.io, the wrong registry for that deployment. Anyone
following the documented setup ended up pointed at another service. The
templates hardcoded docker-credential-atcr, "atcr" and ~/.atcr/device.json
alongside a correctly themed {{ .RegistryURL }}.

The scripts are now rendered from config by a handler, rather than forked per
brand. A theme overlay was the alternative and was worse: it needed a full copy
of both install.sh and install.ps1 per brand, four scripts to keep in sync, and
the operator asked for these values to come from config.

credential_helper.name is the single knob. Docker resolves a credHelpers value
x by exec'ing docker-credential-x, so the credHelpers value, the binary suffix
and the config directory are genuinely one word, not three that can drift. It
is validated against a strict pattern because it is interpolated into a shell
script.

install.sh renders byte-identical to the deleted static file under the atcr
default, so existing installs are unaffected. install.ps1 differs by one line,
where a stale usage comment named a path the script is not served at.

Two behaviour changes worth noting: these two URLs drop from a one-year
Cache-Control to five minutes, since the body now depends on deployment config;
and credential_helper.tangled_repo becomes a real overridable default. It was
previously assigned over unconditionally and read by nothing, while the shipped
script used a different URL form.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:38:10 -05:00
Evan JarrettandClaude Opus 5 2743445e65 appview: fix two /auth/token defects, wrong login host and dropped scopes
Both are pre-existing and were found while working on finding 27.

sendAuthError built its "docker login <host>" line from r.Host. /auth/token is
served on the UI domain as well as on every registry domain, and the
WWW-Authenticate realm points at the UI domain's copy, so a client following
the realm was told to run "docker login seamark.dev" - the one host that
deliberately refuses /v2/* with an OCI UNSUPPORTED error pointing at
seamark.cr. It now uses the service resolved for the token, which is the
registry domain, falling back to the deployment's primary rather than to
r.Host. The single-domain case still prints a host that serves /v2/, and an
unconfigured service supplied by the client cannot steer it.

Separately, the scope parameter was read with .Get, taking the first value
only. The Docker token spec allows scope to be repeated, so a client asking for
two repositories was issued a token covering one and got a 401 on the other.
Both wire forms are now flattened, on the GET query string and on the OAuth2
POST body, which had the same defect via PostFormValue.

Empty and whitespace-only values are dropped. Exact duplicate scope strings
collapse, but two entries naming the same repository with different actions are
left alone: merging them would union the action sets, and every gate downstream
is written only to narrow.

More entries now reach the anonymous gate added in af7522b, which is the
intended effect. Its per-entry verdict is unchanged: public entries survive,
private ones are dropped, and an all-private request still gets the challenge.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:37:56 -05:00
Evan JarrettandClaude Opus 5 84872580a3 appview: show a chart's deprecation where people actually look
digest.html rendered a Deprecated chip, but the shared helm-metadata partial
never read .Deprecated, so the repo page carried no deprecation signal at all.
The data was already on the struct; this was a display omission.

The notice leads the metadata card, above the description: deprecation decides
whether you use the chart at all, so it has to be read before the prose that
sells it. It uses the alert/alert-warning callout the codebase already uses for
this kind of thing, including in the adjacent helm-digest-content partial,
rather than a bare badge. A lone small badge reads as a stray tag once it is
outside the digest page's row of status chips, and leaves no room to say why it
matters.

The digest page now shows deprecation twice, deliberately: its header chip is
the scannable signal above the fold, and this carries the explanation further
down. Removing the chip would push the only signal below the install command,
which is the burial this fix is meant to undo.

Note the repo page's Overview tab, which is where most people land, still shows
nothing: it renders the README and never touches chart metadata. Only the Chart
tab gains the notice here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:37:56 -05:00
Evan JarrettandClaude Opus 5 0490278fb8 appview: don't report a failed README guess as an error
Four of the twelve home-page repos showed "We couldn't load the README, it may
be rate-limited or private". The URL in those cases was not configured by
anyone: it was derived from org.opencontainers.image.source, a label images
inherit from their base image, so the raw URL named an unrelated project and
404ed. A 404 on a URL the appview guessed is an expected outcome the owner
cannot act on.

The failure flag is now set only when the owner actually pointed us at the URL,
via the io.atcr.readme annotation. A derived URL that fails renders as if there
were no README. Both paths keep their debug log, now carrying an "explicit"
field so the two cases stay distinguishable.

Render failures are suppressed for derived URLs too. The panel's copy and its
"Edit README" action address an owner who configured a source; on a derived URL
there is no configured source, and content that failed to render is very likely
another project's README anyway.

This is the alarming half of the finding. Rendering the wrong project's README
when the fetch succeeds is the larger half and is untouched: there is no
reliable way to detect an inherited label, since the only signal is
org.opencontainers.image.base.name, which is not always set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:37:38 -05:00
Evan JarrettandClaude Opus 5 44a17cbcdc appview: stop the upgrade banner inventing an improvement across arch mismatch
On a public repo, to anonymous visitors, the digest banner read "2 fixes, 12
Critical / 27 High / 26 Medium vulns, -22 layers, -53.6 MB" while the same page
showed 244 vulnerabilities and Layers (22), and its own "View diff" link landed
on "Layers 22 -> 22, every layer Unchanged".

Platform matching only ran when both sides were manifest lists. The comment
after that block said the mismatched case would "fall through and show a basic
banner without layer/vuln details", but no such branch was ever written and
nothing guarded the fallthrough, so execution continued into the layer and vuln
computation with the unresolved originals still in place. The newer side was
the multi-arch index, which carries no layers of its own and is not scanned, so
all 22 layers of the other side read as removed and the vuln delta was computed
against an absent scan.

Returns 204 for either mismatch direction, as the no-common-platform path
already does.

The promised "basic banner" is not implementable as the function stands, which
is presumably why it never appeared: the template renders only NewerTag,
DiffURL and Summary, and DiffSummary is nothing but deltas, so a delta-less
banner collapses to the tag name and would be suppressed by the existing
"nothing meaningful changed" guard anyway. The comment is replaced with one
that says what is actually true.

Showing a real banner here would mean resolving the index to the child matching
the single-arch side's platform, and that side's os/arch is not in the appview
DB at all: Platforms is populated only for manifest lists and the manifests
table has no os/arch columns. It would need either a config fetch from the hold
at render time or denormalising os/arch during ingest. Not attempted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:37:38 -05:00
Evan JarrettandClaude Opus 5 39919cc832 appview: stop webhooks reaching private addresses
The URL check accepted http:// while telling the user "must be https", and
guarded no addresses at all. POST /api/webhooks with http://127.0.0.1:9/hook
returned 200 and created the webhook, so both scheduled deliveries and the
synchronous Test button would dial arbitrary destinations from the appview
host, on demand, for any authenticated user. Loopback, link-local (including
the cloud metadata endpoint at 169.254.169.254) and RFC1918 were all reachable.

Enforces https, and refuses non-public destinations.

The load-bearing half is the dial-time check, not the creation-time one. An
attacker controls their own DNS, so a hostname that resolves publicly when the
webhook is created can resolve to loopback when it is delivered, and a
creation-time check cannot see a redirect either. The guard is therefore a
net.Dialer Control hook on the delivery client, which inspects the resolved
address on every connection attempt. Transport.Proxy is explicitly nil:
honouring HTTP(S)_PROXY would route around the Control hook and hand the
bypass straight back. Redirects are re-validated per hop and capped at 3.

The creation-time check stays so the user gets an immediate, comprehensible
error instead of a silent delivery failure later.

IPv4-mapped IPv6 is unmapped before every check, so ::ffff:127.0.0.1 and
friends hit the IPv4 rules. Ranges with no net.IP helper are listed explicitly:
CGNAT, NAT64, ::/96, TEST-NET and reserved space.

Both outbound paths are covered, since the scheduled dispatcher and the Test
button both funnel through attemptDelivery. The dispatcher's other client is
deliberately left unguarded: it fetches quota stats from holds, which
legitimately live on private addresses, and those URLs are not user-supplied.

Note this removes the ability to point a webhook at a localhost receiver in
local development. There is deliberately no environment-variable escape hatch,
since a security toggle read from the environment is the same bypass wearing a
nicer coat.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:37:19 -05:00
Evan JarrettandClaude Opus 5 dfd604b106 hold/scanner: stop one undispatchable job freezing all scanning
Vulnerability scanning produced nothing across the whole deployment for
nine days, from 2026-08-25 01:20:48 until a scanner restart on 2026-09-03.
The scanner was connected and idle, the hold's discovery pass kept
reporting unscannedFound=15 every four hours, and no scan_jobs row was
created in that entire window.

hasActiveJobs counted pending, assigned and processing rows globally with
no age bound, and waitForCapacity spins while it is true. dispatchLoop
calls it before popping any candidate, so a single pending row that never
reached a terminal state reported "busy" forever: discovery kept pushing
candidates into unscannedQueue and nothing ever popped them. That is why
the symptom was an empty queue rather than a growing one.

Nothing papered over it because push-triggered enqueue only fires for
owner or a tier with scan_on_push, which in production means pro alone.
All 210 manifests pushed to this hold in that window came from free,
supporter, or accounts with no crew row, so the frozen proactive loop was
the only source of jobs.

Nor could it recover on its own. Only Enqueue and drainPendingJobs
dispatch a pending row, and drainPendingJobs runs only when a scanner
newly connects; reDispatchTimedOut considered assigned rows only. The
hold had been up since Aug 14 and the scanner since Aug 21 on the same
websocket, so the drain path had not run since the row appeared.

So bound the capacity gate to pending rows younger than pendingStaleAfter,
give reDispatchTimedOut a pending reclaim, and check RowsAffected on the
assign UPDATE now that two dispatchers can race for a row. waitForCapacity
warns and names the blocking jobs after ten minutes without capacity,
because the failure mode above was completely silent.

Two adjacent fixes for the same outage. The scanner never called
InitLogger, so log_level and log_shipper were dead config and an idle
scanner was mute, which is what made nine days invisible. And skipReason
now also skips a job whose layers contain nothing tar-shaped: the job that
wedged this queue was an in-toto attestation whose config mediaType is an
ordinary image config, so the existing config-type check missed it and
buildOCILayout would have handed Syft an empty image.

The regression tests were verified against the old logic first: three of
them fail on it and pass on the fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPWkeCKcbtGoyXyyeMhSps
2026-09-02 21:12:13 -05:00
Evan JarrettandClaude Opus 5 af7522b154 appview: stop /auth/token granting anonymous pull the registry will refuse
An unauthenticated token request for a repo on a private hold came back with a
signed token granting pull, and /v2/ then 401ed that exact token. The
authorization server and the resource server disagreed about the same request.

The old behaviour was deliberate — "minting a pull-only token is not a grant",
with the hold owning the decision via captain.Public — but a token spec expects
the server to issue the subset it will authorize, so granting pull and then
refusing it is the wrong shape.

Adds an optional AnonymousAuthorizer, kept separate from Authorizer because the
anonymous path has no DID and no auth method (three of Authorize's four
arguments are meaningless) and because it must drop whole entries rather than
narrow actions in place, where entries can belong to different owners. Denied
entries are dropped; if nothing granting survives, the caller gets the standard
401 challenge rather than a token with an empty access list, so docker prompts
for credentials instead of proceeding to a second 401.

The scope-less /v2/ ping and the actionless entry NarrowToPullOnly preserves on
purpose both bypass the gate entirely — no identity resolution, no hold lookup —
since anonymous discovery depends on them.

Fails open on any lookup error, matching the /v2/ check, which states the
reason: the hold is the enforcing authority and a transient failure must not
break anonymous pulls of public images. /v2/ still enforces; this is a
correctness and UX fix, not a security fix, and nothing was exposed.

Also closes the successor asymmetry documented under finding 3: /v2/ applies a
single-hop migration redirect before checking read access, so judging the
pre-migration identity here would have reintroduced the disagreement this gate
removes. It was one extra local read of hold_captain_records. Tests pin both
directions and prove the chain is not followed past one hop.

Costs one directory-cached identity resolution plus two local SQL reads per
granting entry, and no call to the hold.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:08:00 -05:00
Evan JarrettandClaude Opus 5 4caeb25031 appview: rebuild the JS bundle so the committed asset matches src
Picks up the two JS changes in this batch: the crane destination argument in
updatePullCommand (app.js) and syncDiffMenu (repository.js).

The bundle is a committed build artifact, and nothing in the dev loop keeps it
current on its own. Air's pre_cmd is go generate and its cmd is a Go build; it
never invokes esbuild. Bundling happens in npm run js:watch, a separate
process. So a src-only change leaves the committed bundle stale until someone
runs npm run js:build by hand, which is how cbd0c5f came about.

Without this both fixes are invisible in a served page, and the crane one is
half-live in the worst way: first paint carries the destination because that
comes from the Go template helper, while the dropdown re-render does not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:03:29 -05:00
Evan JarrettandClaude Opus 5 85f312f953 appview: refuse /v2/_catalog with UNSUPPORTED, and send Allow on every 405
/v2/_catalog answered a bare request with 200 {"repositories":[]} but 400ed
any n, because buildDistributionConfig leaves Catalog.MaxEntries at 0 and the
library rejects n > max. crane catalog sends n=1000, so it always failed. The
same endpoint both worked and rejected a legal parameter.

Setting MaxEntries was the obvious fix and is the wrong one. This registry has
no global catalog and will not grow one: repositories live in per-user ATProto
PDS namespaces, and buildStorageConfig hands the library a placeholder
inmemory driver, so its enumeration is empty by construction. An empty 200
asserts that this registry contains no repositories, which is false and
silently so. UNSUPPORTED says the true thing, and matches the vocabulary
DomainRoutingMiddleware already uses to refuse /v2/* on the UI domain.

There is no conformance cost: _catalog is not in the OCI distribution spec at
all. It is a Docker Registry HTTP API V2 extension, and the spec places
repository discovery out of scope. Docker Hub and GHCR both refuse it outright
and Quay returns an unconditional empty list; none of them 400s a legal n.

The path had three distinct behaviours, not two, and all three now collapse to
one: GET varied by parameter, HEAD was answered by gorilla's MethodHandler
with a bare 405, and the trailing-slash form 301-redirected because
distribution sets StrictSlash(true). Both path forms are registered, and chi
prefers a static pattern over the /v2/* wildcard regardless of declaration
order (verified against the pinned chi version, and pinned by a test that
fails if a request reaches the distribution stand-in).

Also adds the Allow header that RFC 9110 requires on any 405 — a MUST in both
15.5.6 and 10.2.1, not a SHOULD. The value is empty, which 10.2.1 defines as
"the resource allows no methods": true for the catalog, and true for /v2/* on
the UI domain, so the pre-existing gap in DomainRoutingMiddleware is closed
too. Naming a method there would advertise something that does not work.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:03:20 -05:00
Evan JarrettandClaude Opus 5 8bc6d65e1e appview: emit crane's mandatory destination in the pull command switcher
The client switcher built every command as "<client> pull <ref>". That is
valid for docker, podman, buildah and nerdctl, but crane requires a
destination:

  $ crane pull seamark.cr/user/bench8x1:v2
  Error: requires at least 2 arg(s), only received 1

So selecting crane handed the user a command that cannot run. pullPrefix is a
prefix-only helper, which is precisely why it could not express this; add a
matching pullPostfix that returns " <image>.tar" for crane and "" for
everything else, including "none" (image reference only), which must get
neither prefix nor postfix.

Both render paths change together, since fixing one leaves the bug visible in
the other: the Go template helper paints first, and updatePullCommand in
app.js re-renders when the dropdown changes.

Repository names may contain slashes, so only the last path segment is used —
otherwise the destination would name a subdirectory that does not exist. A
name ending in "/" yields no destination at all rather than a bare ".tar",
on the grounds that a visibly wrong-arity command beats silently writing a
hidden file. That input is not reachable through the real repo-name path.

The test asserts the whole command string rather than just the postfix, so it
covers the prefix/postfix interaction and the "none" case where both vanish.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:03:01 -05:00
Evan JarrettandClaude Opus 5 3158460298 appview: hide the current tag from the repository Diff dropdown
The Diff menu listed every tag including the one being viewed, and clicking
that entry did nothing: diffToTag returns early on to === currentTag, with no
navigation, no toast, no feedback.

Fixed in JS rather than in the template, which is the part that is easy to
get wrong. #diff-dropdown sits outside #tag-content, and the tag selector
block is marked "stays in DOM, never swapped" — so a {{ range }} filter would
be correct on first paint and stale after the first htmx tag swap, omitting
the originally loaded tag and re-including the newly current one. Same dead
entry, harder to see.

syncDiffMenu() reads the live value from #tag-selector and is called from
initTabs(), which already runs on load and again from the htmx:afterSettle
handler for #tag-content, so it stays correct across swaps.

Hidden rather than disabled: a disabled row still takes space and still reads
as an item to a screen reader, and "diff against the tag you are already on"
is meaningless rather than temporarily unavailable. Uses style.display to
match filterTags() in the same file, since daisyUI's .menu li rules outrank
Tailwind's .hidden.

The template guards the dropdown with {{ if gt (len .AllTags) 1 }} and tag
names are unique per repo (tags PK is did+repository+tag), so exactly one
entry is ever hidden and the menu can never end up empty. The early return in
diffToTag stays as a backstop.

Pre-existing, not a deploy regression. The baseline missed it because the
test repo had one tag, so the check skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:02:50 -05:00
Evan JarrettandClaude Opus 5 1631898005 appview: give every Repository() error an OCI code instead of 500 {}
A user with an unresolvable defaultHold (did:web:localhost%3A8080) got HTTP
500 with a body of literally {} on every request in their namespace, and
crane retried it three times because 500 is retryable.

The cause is in the distribution library. handlers/app.go:755 switches on the
error type returned by Repository() with cases for ErrRepositoryUnknown,
ErrRepositoryNameInvalid and errcode.Error, and no default. A bare fmt.Errorf
matches none of them, so context.Errors stays empty; ServeJSON then finds no
ErrorCoder, leaves sc == 0, falls through to 500, and Errors.MarshalJSON
renders the nil slice as {} via omitempty.

So every error leaving Repository() must be coded. Four were not:

  hold URL unresolvable  -> 404 NAME_UNKNOWN when errors.Is
                            atproto.ErrHoldDIDPermanent, else 503 UNAVAILABLE
  no hold DID configured -> 500 UNKNOWN, but with a body and a log line
  invalid image name     -> 400 NAME_INVALID
  name missing an owner  -> 400 NAME_INVALID

The permanent/transient split is the point: a DNS blip must stay retryable,
but a did:web that can never resolve must not be retried at all. Stored user
data that cannot resolve is a 4xx condition, not a server fault, and the
NAME_UNKNOWN message now names the hold so the owner can fix their profile.
The hold DID is already world-readable in their sailor profile record.

Tests assert through a helper that replays distribution's exact type switch,
so an uncoded error still surfaces as 500 {} and the assertions bind to the
real behaviour rather than to the constructors.

Does not address the logrus line at app.go:757, which fires unconditionally
before the type switch and cannot be avoided by any returned error type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:02:40 -05:00
Evan JarrettandClaude Opus 5 2ee5a35525 appview: stop the schema-drift check reporting phantom index differences
reportSchemaDrift logged 76 differences on every boot, all false positives:
38 indexes each reported twice, once as missing from the database and once
as present but undeclared. The only difference was a single space before the
column list.

The two sides of the comparison are built differently. referenceSnapshot()
applies schema.sql to a local in-memory libsql, which stores the CREATE text
verbatim as "ON t(col)". Production is an embedded replica syncing to Bunny,
whose parser re-emits normalized DDL as "ON t (col)". describeIndexes
collapsed whitespace runs but could not normalize a space that exists on one
side only, and SchemaDrift compares by exact string.

Normalize whitespace adjacent to ( ) and , so both spellings converge. Space
after ) is deliberately left alone, and the space before DESC is untouched,
so column order and direction still have to match.

This mattered because the check exists to catch a migration recorded but not
executed (0004 was, which is why 0009 exists). At 76 phantom findings, real
drift would have been one line among 77, under a hint telling the operator to
write a corrective migration that is not needed. The first boot after this
lands is the first honest reading of that warning.

The existing tests all passed because they run local-only, which is exactly
how this shipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-02 21:02:28 -05:00
Evan JarrettandClaude Opus 5 a63b839613 appview: gzip UI and static responses, leaving /v2 alone
Go's net/http compresses nothing by default and the appview is served by a
raw Go server behind a load balancer that does not compress either, so
everything went out uncompressed: style.css at 180 KB, bundle.min.js at
105 KB, and the homepage HTML at 95 KB. Lighthouse put style.css alone at
1,768 ms of blocked first paint, and mobile Performance measured 77 against
98 on desktop, entirely on paint metrics (TBT 10ms, CLS 0).

klauspost/compress is already a direct dependency and ships gzhttp, so this
costs no new one. Brotli would save roughly 11 KB more across the three
largest assets in exchange for a runtime dependency, which is not worth it.

The /v2 skip is the part that needs care. The OCI registry API is mounted on
the same chi router as the UI (server.go:561), and container layers are
already gzipped tarballs, so compressing that path burns CPU for no gain.
The content-type allowlist would catch most of it, but /v2/* also serves
application/json for tag listings and errors, so the path check keeps the
registry out of the compression path entirely. The predicate matches the one
the domain-routing middleware already uses.

Measured against a local build, gzip vs identity:

    /                   14,526 ->  4,298   71%
    /css/style.css     183,990 -> 31,596   83%
    /js/bundle.min.js  107,781 -> 32,648   70%
    /icons.svg          26,360 ->  8,403   69%
    total              332,657 -> 76,945   77%

Verified end to end against a running binary: UI and static responses carry
Content-Encoding: gzip with Vary: Accept-Encoding, /v2/ and /v2/*/tags/list
carry neither, and woff2 stays untouched. Tests cover both directions plus
the under-1 KB and no-Accept-Encoding cases.

HTTP/2 is the remaining half and cannot be fixed here: the load balancer
terminates TLS and negotiates no ALPN at all, which pins every request to
HTTP/1.1. That is an LB setting.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0124r73LT4qoFE82TqwH2Gu9
2026-09-02 20:21:33 -05:00
Evan JarrettandClaude Opus 5 cbd0c5f05c appview: rebuild the JS bundle so the committed asset matches src
The tracked bundle predated a219df9, so it carried neither the
alert-error match in testWebhook nor plainTextReason's 400 branch. Any
deploy that copied the committed asset without regenerating would have
shipped the old JS and findings 13, 15 and 17 would have looked unfixed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPWkeCKcbtGoyXyyeMhSps
2026-09-02 19:55:04 -05:00
Evan JarrettandClaude Opus 5 27fab41c1c appview: make the webhook cap agree between creation and delivery
The UI and the dispatcher asked the same question and got different
answers. getWebhookLimits short-circuited on a disabled billing manager and
returned unlimited without consulting it, while server.go hands
BillingManager.GetWebhookLimits straight to NewDispatcher, bypassing that
short-circuit entirely. With billing compiled out the stub answers
non-captains with (1, false).

So a non-captain saw "N / unlimited webhooks configured", could create as
many as they liked, and only the oldest was ever delivered. allTriggers was
false on that same path, so even the surviving one was restricted to
FreeTriggerMask; a webhook set to a scan trigger fired nothing at all, with
no message anywhere and only an INFO line server-side.

Route both paths through the manager so they cannot drift. The intended
non-billing policy is one webhook with TriggerFirst | TriggerPush |
TriggerQuota, which is what the stub already returned and what the shipped
config's Free tier specifies (max_webhooks: 1, webhook_all_triggers: false),
so enabling billing leaves free users exactly where they were and only
unlocks upward. That also removes a downgrade cliff: nobody can accumulate
webhooks under a phantom unlimited and lose them when billing turns on.

Drop the dead webhookLimits{Max: 1}, overwritten on the following line, and
give a nil manager the same policy rather than a third answer.

Latent, not live: production has zero webhooks configured today.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
2026-09-02 12:45:15 -05:00
Evan JarrettandClaude Opus 5 62aea5f3f0 credhelper: print the verification URL that carries the code
The interactive prompt named codeResp.VerificationURI while openBrowser was
handed verificationURL, the one with ?user_code= appended. Pressing Enter
therefore always worked, which is why this went unnoticed; copying the
printed URL instead landed on /device with no code.

That matters more than it looks, because the branch tests the wrong thing.
isTerminal(os.Stdin) asks whether stdin is a TTY, not whether a browser
exists, so an SSH session on a headless box takes the headed path and is
told to press Enter to open a browser it does not have. The non-interactive
branch, which already printed the full URL, is only reached by piping stdin.
Printing the code-carrying URL in both branches makes that mismatch moot
rather than requiring a smarter predicate.

Also give /device without a code the styled device-error page instead of
bare text/plain, matching the expired-code path beside it, and stop
renderError panicking when Templates is nil.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
2026-09-02 12:45:04 -05:00
Evan JarrettandClaude Opus 5 a219df9545 appview/js: fix webhook test result, 400 toasts, and Layers tab init
Three unrelated client-side defects found while baselining production.

The webhook Test button always reported success. renderAlert writes no
status code, so both outcomes are HTTP 200 and the result lives in the
markup, which partials/alert.html emits as "alert alert-error". testWebhook
looked for class="error", which that string does not contain, and resp.ok
is always true, so the failure branch was unreachable. A webhook pointed at
a dead URL was reported as delivered. Match alert-error instead.

Avatar upload rejections lost the server's reason. The htmx:responseError
handler maps status codes to fixed strings and had no 400 case, so
"File too large (max 3MB)" and "Invalid file type" both surfaced as
"Something went wrong". Surface the body when it is short plain text; a
rendered error page or a long trace is not toast material.

The repository page's Layers tab was never initialised. initLayersTables
runs from DOMContentLoaded, when the panel is still a spinner, and from
htmx:afterSettle, which htmx.process() does not emit. So empty-layer hiding
and no-history run collapsing never ran there, and the checkbox claimed
rows were hidden while all of them were on screen. Export it and call it
from the tab controller. It now also re-seeds checkboxes within the loaded
scope, which fixes the digest page contradicting itself: the stored
preference was honoured for the rows and ignored for the control.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
2026-09-02 12:45:04 -05:00
Evan JarrettandClaude Opus 5 1253ca15ec appview: report a never-scanned image as unscanned, not as a failure
digest_content.go branched on Error == "never-scanned" to pick the
"not scanned yet" copy, but nothing anywhere produced that string: both
FetchVulnDetails and FetchSbomDetails returned the human sentence
"No scan record found" for a missing record. So vulnReason and sbomReason
could never be "not-scanned", the friendly branches in vulns-section.html
and sbom-section.html were dead code, and every unscanned image fell
through to fetch-failed.

Free-tier accounts have scan_on_push off, so this was every image they
push, told "Scan data couldn't be loaded... try refreshing in a minute"
about something that had never been scanned and never would be by
refreshing. The digest page showed the raw internal string instead.

Replace the prose sentinel with a NotScanned bool the 404 path actually
sets, and give other non-200 statuses a distinct message so a 500 from the
hold stops being indistinguishable from an absent record. The detail
templates branch on it before Error, so nothing leaks the internal value.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
2026-09-02 12:44:48 -05:00
Evan JarrettandClaude Opus 5 27ce122db0 auth: stop logging an unresolvable hold DID at ERROR
A hold DID that can never resolve is a property of stored user data, not a
fault on our side. The value comes from a user's own sailor profile
defaultHold, so any account can choose the appview's ERROR volume, and
nothing is cached on the failure path, so it re-logs on every request for
that user.

On production this was not a rounding error: two accounts pointing at
did:web:localhost%3A8080 produced 2956 of 2958 ERROR lines over seven days,
99.9%. The genuine rate underneath was about two a day, which made
level=ERROR useless as a signal or an alert threshold.

Classify at the resolution boundary instead of string-matching prose.
ErrHoldDIDPermanent marks a malformed identifier or a missing DID document;
those log at DEBUG while everything an operator could act on stays at ERROR.
didWebHostUnusable is conservative on purpose: it only claims the cases we
are sure about (percent-encoded ports, bare IPs, localhost), so an
unfamiliar failure stays loud rather than being quietly swallowed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
2026-09-02 12:44:48 -05:00
Evan JarrettandClaude Opus 5 a4aedbd2c8 gc: retry transient PDS failures instead of pinning a user's storage
Both paginated walks bailed on the first error, and the caller treats a
failed walk as "assume everything is referenced", so one blip skipped that
DID's storage for the whole run.

Measured before changing anything: every DID GC had classified unreachable
but healthy failed only one or two runs out of six, and replaying the exact
same listRecords calls afterwards returned 200 in 45-680 ms with no rate
limiting. Ordinary blips on small self-hosted PDSes, amplified into a
full-DID skip.

Share one listRecordsPage helper between fetchUserTags and
fetchUserManifestsFromEndpoint. The retry decision splits deliberately:
timeouts, connection reset, 5xx and 429 get another attempt, while DNS
failure, TLS failure, connection refused and any 4xx do not. Those are
stable facts about an endpoint, and retrying them would only slow the run
and keep a dead PDS looking alive longer. Unrecognised errors stay
permanent, so an unfamiliar failure degrades to today's behaviour rather
than hammering.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
2026-09-02 12:44:33 -05:00
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 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 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 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 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 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 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 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 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 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