Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Quick104 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a large production storage and artifact-delivery feature, including a new session-independent signed URL for administrator artifacts and changed persistence paths for diagnostics, exports, and avatars. Its authentication/data-access implications, cross-component runtime impact, and unresolved substantive review concerns require human review. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 398332215d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // deployment without a private bucket has nowhere to put avatars, so this | ||
| // returns nil and uploads stay unavailable. | ||
| func NewProfileAvatarStore(stores blobstore.Stores) blobstore.Store { | ||
| return stores.Operational |
There was a problem hiding this comment.
When artwork.storage_backend is local but a private S3 bucket is also configured, blobstore.Open sets Stores.Operational to the local assets store, so this now switches avatar reads and writes to disk. The previous implementation always preferred the configured private bucket, meaning existing upload:profile-avatars/... references become unavailable after this upgrade and new uploads are split into a different backend. Keep the private client preference for avatars, as the function comment and architecture document promise.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed one layer down, in dd5db06 on #1112.
You were right about the regression and about where it came from. blobstore.Open set Stores.Operational to the local assets store on a local backend, ignoring S3Private entirely, so an install with artwork on disk and a private bucket configured would have lost its existing upload:profile-avatars/... references and split new uploads across backends.
A configured private bucket now owns the operational store on either backend; only a local backend with no private bucket shares one root. That also removes the duplicated if deps.S3Private != nil conditional from the diagnostics and artifact wiring, since Stores encodes the preference once. TestOpenLocalStillPrefersAConfiguredPrivateBucket pins it, and this function keeps its previous selection rules exactly.
| operation := humaOp("GET", Prefix+"/admin/jobs/{id}/artifact", "downloadAdminJobArtifact", "admin-tasks", | ||
| "Stream a completed job's artifact through the API host. Authorized by the signed capability in the download URL, not by a session.") |
There was a problem hiding this comment.
This adds a client-visible artifact-download operation and backend-dependent public-link behavior, but a repo-wide search of internal/apiv2 finds no admin-job or catalog-export capability endpoint. public_link_supported is only available after retrieving an individual job, so clients still cannot discover the feature independently before choosing this flow; add a domain capability endpoint describing download and public-link support.
AGENTS.md reference: AGENTS.md:L130-L134
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Added in b2b0a89.
GET /api/v2/admin/jobs/capabilities reports artifact_download and public_links. Both depend on the configured backend rather than the release, so version sniffing cannot derive them, which is exactly the case the capability rule exists for. public_link_supported stays on the job projection for the per-row answer the UI renders against.
Tested across the three backend shapes (S3, local, neither) and for administrator-only access.
| return job, nil | ||
| } | ||
| url, err := h.store.PresignGetURL(ctx, job.ArtifactBucket, job.ArtifactKey, catalogSeedPublishExpiry) | ||
| if errors.Is(err, blobstore.ErrNoPresign) { |
There was a problem hiding this comment.
The new local-storage branch makes catalog export publishing return 409 presign_unsupported, while docs/admin-catalog-api.md still states unconditionally that the publish operation saves and returns a seven-day signed URL; it also omits the new artifact route and job response field. Update that API document so clients are not implementing against behavior the server no longer guarantees.
AGENTS.md reference: AGENTS.md:L138-L138
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b2b0a89.
docs/admin-catalog-api.md now records the 409 from publish and why it is backend-dependent, the artifact route with its capability authorization and its 404/503 split, and the capability endpoint. The operation table gained rows for both new routes.
It also tells clients to read download_url from the job rather than constructing the URL, since an S3-backed server returns a presigned storage URL there and both forms are opaque.
| // artifacts — are written this way. | ||
| func (f *Filesystem) PutStream(ctx context.Context, key string, r io.Reader, _ string) error { | ||
| return f.publish(ctx, key, func(w io.Writer) error { | ||
| _, err := io.Copy(w, r) |
There was a problem hiding this comment.
For a large catalog export or diagnostic bundle, cancellation or the upload deadline can expire after publish performs its initial context check, but this plain io.Copy continues until the entire reader finishes and can still rename the object into place. In particular, the admin-job runner's 30-minute upload timeout is ineffective for a local file reader; make the copy loop observe ctx.Done() so canceled writes stop and the temporary file is discarded.
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b2b0a89.
Correct on the mechanism: a local file reader never blocks, so the runner's 30-minute upload timeout could expire during the copy and the object would still be renamed into place. PutStream now wraps the reader so the copy fails once the context is done, and publish already discards the temporary file on any error, so nothing partial is published.
Two tests: an already-canceled context publishes nothing, and cancellation partway through a 128 KiB body leaves no truncated object behind.
| download, err := reg.deps.AdminJobArtifacts.OpenAdminJobArtifact(r.Context(), id) | ||
| if err != nil || download.Body == nil { | ||
| writeProblem(w, r, NewProblem(TypeNotFound, "Job artifact not found.")) |
There was a problem hiding this comment.
After a capability has verified, a database failure or an unavailable local filesystem from OpenAdminJobArtifact is converted to 404 exactly like an absent artifact. This contradicts the route's declared 503 response and makes a valid administrator download look permanently missing instead of retryable during an outage; classify only missing jobs/artifacts as 404 and surface dependency failures as 503 (or unexpected failures as 500).
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b2b0a89.
Agreed, and the reasoning for the blanket 404 only covers the capability check. Before verification the route must not reveal whether a job exists; after it, the caller has proven it was given a URL for that job, so distinguishing the two leaks nothing and reporting an outage as 404 actively misleads.
OpenAdminJobArtifact now returns handlers.ErrJobArtifactNotFound for a missing job or absent artifact, which stays 404. Anything else answers 503 against the declared response. Pre-verification failures are unchanged. Added TestAdminJobArtifactDownloadReportsStorageOutagesAsUnavailable.
da6175a to
1e0d49d
Compare
Diagnostic bundles, catalog-seed artifacts, and profile avatars all required a
private S3 bucket, so an install without object storage could not produce a
diagnostic bundle or export its catalog.
Add PutStream to the store and blobstore.BucketAPI, a filesystem-backed adapter
for the bucket-shaped API those callers were written against. S3 deployments
keep passing *s3client.Client straight through, so their path is untouched; only
a local backend goes through the adapter, which ignores the bucket argument,
reports "local", and normalizes not-found to each caller's sentinel.
recordingStore now forwards PutStream. It previously wrapped only Put, and on a
local install the first write is often a diagnostic bundle rather than artwork —
that write would have published an object without recording the storage
identity, leaving the configured root editable and every key referencing it
orphaned by the next change.
Presigning is the one thing a filesystem cannot do, which has three
consequences. Diagnostics already streamed through the API host on /api/v2 and
falls through to streaming on /api/v1, so it needed nothing. Job artifacts gain
GET /api/v2/admin/jobs/{id}/artifact, authorized by a signed capability rather
than a session: the presigned URL it replaces authorized itself, and the web
opens download_url in a new tab with no Authorization header, so an
administrator-gated route would have answered 401. The capability has its own
signing domain, so an artwork URL cannot be replayed against it. Seven-day
public links genuinely cannot work without storage-side presigning, so the API
answers 409 and public_link_supported lets the UI hide the action instead of
offering one that always fails.
Verified: full Go suite (one pre-existing macOS-only failure in
TestAdminResourceCapabilitiesAndScope, unrelated), affected web tests, tsc, and
the v2 contract diff — 6 changes, all additive, none breaking. Neither
silo-apple nor silo-android references download_url or any admin job artifact
surface, so no client work is required.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six lint findings from the changed-lines gate: reuse the existing apiv2 constants for int64, application/gzip, Content-Disposition, and Content-Length, name the Content-Disposition filename parameter, and lowercase an error string whose line this branch touched. Four review findings: - Streamed local writes now observe the context. A local reader never blocks on the network, so an upload deadline elapsed unnoticed and the object was still renamed into place; the admin job runner's upload timeout was inert for exactly the large exports it bounds. - The artifact route separates a missing artifact from unreachable storage. Once the capability verifies, the caller has proven it holds a URL for that job, so hiding an outage behind 404 told an authorized administrator their download was permanently gone. Pre-verification failures stay 404. - Add GET /api/v2/admin/jobs/capabilities. Artifact download and public-link support both depend on the configured backend rather than the release, and public_link_supported was only readable after fetching a job. - Document the 409 from publish, the artifact route, and the capability endpoint in docs/admin-catalog-api.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3983322 to
b2b0a89
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| } | ||
| body, err := streamer.GetObjectStream(ctx, job.ArtifactBucket, job.ArtifactKey) | ||
| if err != nil { | ||
| return AdminJobArtifactDownload{}, err |
There was a problem hiding this comment.
🟡 Medium handlers/admin_task_jobs_service.go:59
A missing artifact is returned as a storage error, so the route responds with 503 instead of 404 for completed jobs whose artifact was deleted. GetObjectStream returns blobstore.ErrNotFound, but this function propagates it unchanged; map that error to ErrJobArtifactNotFound before returning.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/api/handlers/admin_task_jobs_service.go around line 59:
A missing artifact is returned as a storage error, so the route responds with `503` instead of `404` for completed jobs whose artifact was deleted. `GetObjectStream` returns `blobstore.ErrNotFound`, but this function propagates it unchanged; map that error to `ErrJobArtifactNotFound` before returning.
| // exactly the large exports it exists to bound. | ||
| func (f *Filesystem) PutStream(ctx context.Context, key string, r io.Reader, _ string) error { | ||
| return f.publish(ctx, key, func(w io.Writer) error { | ||
| _, err := io.Copy(w, &contextReader{ctx: ctx, r: r}) |
There was a problem hiding this comment.
🟠 High blobstore/filesystem.go:129
PutStream does not return when ctx is canceled if r.Read is blocked, so the upload goroutine remains stuck with the temporary file open until the source produces data. contextReader checks the context only before entering r.Read; use a cancellation-aware source or an equivalent mechanism that can interrupt the blocked read.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/blobstore/filesystem.go around line 129:
`PutStream` does not return when `ctx` is canceled if `r.Read` is blocked, so the upload goroutine remains stuck with the temporary file open until the source produces data. `contextReader` checks the context only before entering `r.Read`; use a cancellation-aware source or an equivalent mechanism that can interrupt the blocked read.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2b0a8916e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| body, err := streamer.GetObjectStream(ctx, job.ArtifactBucket, job.ArtifactKey) | ||
| if err != nil { | ||
| return AdminJobArtifactDownload{}, err |
There was a problem hiding this comment.
When a retained job points to an artifact that cleanup or an operator has already removed, the local streamer returns blobstore.ErrNotFound, which is passed through here and converted by the route to 503 rather than the documented 404. Fresh evidence in the updated code is that OpenAdminJobArtifact still returns the storage error unchanged, while the route recognizes only ErrJobArtifactNotFound; normalize the store's missing-object sentinel before returning.
AGENTS.md reference: AGENTS.md:L65-L67
Useful? React with 👍 / 👎.
| if deps.Config != nil && deps.S3Private == nil { | ||
| signer := artworkurl.NewJobArtifactSigner(deps.CurrentConfig().Auth.JWTSecret, adminJobArtifactURLTTL) | ||
| adminJobsHandler.ArtifactSigner = signer | ||
| v2deps.AdminJobArtifacts = adminJobsHandler |
There was a problem hiding this comment.
When the assets backend is S3 but no private bucket is configured, deps.Blobs.Operational and privateStore are nil, yet this condition still installs a non-nil AdminJobArtifacts service. Consequently GET /api/v2/admin/jobs/capabilities reports artifact_download: true even though jobs receive no download URL and the artifact service has no store; gate this wiring on an actual operational store.
AGENTS.md reference: AGENTS.md:L130-L134
Useful? React with 👍 / 👎.
| } else if api := blobstore.NewBucketAPI(deps.Blobs.Operational); api != nil { | ||
| privateStore = api |
There was a problem hiding this comment.
On a local backend, assigning BucketAPI here changes the frozen v1 POST /api/v1/admin/catalog/export-jobs flow from rejecting unavailable storage to accepting and completing the export. However, the v1 job projection only calls PresignGetURL, so local storage leaves download_url empty, and the new streaming artifact route exists only under v2; a v1 client can therefore create an export it cannot retrieve. Avoid enabling this local store for the v1 job flow, while retaining it for v2.
AGENTS.md reference: AGENTS.md:L202-L205
Useful? React with 👍 / 👎.
|
The Go check failed on
Job re-run. I have not touched the test: tightening that deadline is a separate concern from this PR, and I did not find an existing issue for it. |
1 participant
Problem
Related issue: #774 — extends the S3-optional storage work past artwork.
Diagnostic bundles, catalog-seed artifacts, and profile avatars all required a private S3 bucket.
Without one, an install could not produce a diagnostic bundle or export its catalog: the export
endpoint answered 503 and diagnostics reported storage unavailable.
Stacked on #1113, which is stacked on #1112. Review those first.
Approach
Add
PutStreamto the store, andblobstore.BucketAPI, a filesystem-backed adapter for thebucket-shaped API those callers were written against.
S3 deployments keep passing
*s3client.Clientstraight through, so their code path is untouched andcarries no regression surface. Only a local backend goes through the adapter, which accepts and
ignores the bucket argument, reports
"local", and normalizes not-found to each caller's sentinel.Those callers pass the bucket an object was written to so a bucket change does not orphan it; a
filesystem has one location, so the argument is meaningless there.
"local"is recorded intoadmin_jobs.artifact_bucketandclient_diagnostic_reports.blob_bucketand handed back on read.It has to be non-empty because readers treat an empty bucket as storage-unavailable.
recordingStorenow forwardsPutStream. It previously wrapped onlyPut, and on a local installthe first write is often a diagnostic bundle rather than artwork. That write would have published an
object without recording the storage identity, leaving the configured root editable and every key
referencing it orphaned by the next change. This was the most consequential finding of the review
pass and is the one thing in this stack worth reading closely.
Presigning
Only S3 can mint a URL that authorizes itself off this server, which has three consequences.
Diagnostics needed nothing. The v2 route already streams through the API host, and the v1
handler falls through to streaming when presigning fails.
Job artifacts needed a route.
GET /api/v2/admin/jobs/{id}/artifactis additive and streams thebytes. It is authorized by a signed capability rather than a session, which is the part worth
justifying: the presigned S3 URL it replaces authorized itself, and the web UI opens
download_urlwith
window.openin a new tab, which sends noAuthorizationheader. An administrator-gated routewould have answered 401 and the download button would have been broken on every local install. The
capability reuses the existing
artworkurlsigner under its own domain, so an artwork URL cannot bereplayed against it and a capability for one job does not open another's artifact. Every rejection
answers 404, so the route never reveals whether a job exists. The v1 response is left presign-only;
it is on the frozen contract.
Seven-day public links genuinely cannot work. That link is handed to someone outside this server,
which only storage-side presigning provides. The short-lived signed download route is not a
substitute — it is a capability for the administrator who is already here. The API answers 409 with
an explanation instead of the 500 it would previously have produced, and the job projection carries
public_link_supportedso the UI hides the action rather than offering one that always fails.NewProfileAvatarStorecollapses to returning the operational store. The previous three-argumentselection logic is now expressed by
Storesitself, and the rules are unchanged: private S3 ownsavatars when configured, a local backend with no private bucket serves them from the shared root,
and a public artwork bucket is never eligible. Review caught that
Openwas not honoring the firstof those rules on a local backend; that is fixed in #1112.
Review also asked for two additions that are in this PR:
GET /api/v2/admin/jobs/capabilitiesreportsartifact_downloadandpublic_linksso a client candiscover both before fetching a job, and
docs/admin-catalog-api.mdnow documents the 409 frompublish, the artifact route, and the capability endpoint.
Validation
go build ./...,go vet ./...,gofmt -l .clean;tsc --noEmit; affected web tests.go test ./.... One failure,TestAdminResourceCapabilitiesAndScope, is pre-existing andenvironment-specific — it reports
"state":"unsupported"because the development machine is macOSwith no cgroups. Confirmed identical at the merge base in a scratch worktree.
make verify-apiv2-openapi,verify-apiv2-web-types,verify-apiv2-contract,verify-route-inventory,verify-local-paths,verify-migration-ledger.public_link_supportedadded as a required response property on the admin job projections.
golangci-lint run --new-from-merge-baseagainst this PR's base: 0 issues.UploadFilereporting size and failing on a missing source, the presign refusal and capability answer, listings
staying inside their prefix with artwork and subtitle keys also present, the diagnostics local store
including not-found normalization, the artifact route serving a signed capability with no session,
and its rejections, that a storage outage after a verified capability answers 503 rather than 404,
that a canceled streamed write publishes nothing, and the capability endpoint across the three
backend shapes. Two are worth calling out:
recordingStorerecords the identity on a firststreamed write, and artwork URLs are byte-identical after the signer was generalized. The expected
artwork URL in that second test was derived by reimplementing the pre-change signer separately
rather than captured from the new code, so it is not circular.
Exercised end to end against a running server with no S3 configured at all:
catalog-seeds/and gunzipped clean.
Authorizationheader — the browser new-tab case — returning 200. Atampered signature, a missing capability, and another job's ID all returned 404 and none reached
storage.
diagnostics/, downloaded back byte-identical bySHA-256, then deleted from disk.
public_link_supportedwasfalse.recordingStorefix on areal server rather than only in a test.
now that more callers can trigger the first write.
Risks
No schema change and no migration.
An S3 deployment is untouched: same clients, same buckets, same presigned URLs, same keys. The
adapter is only constructed when there is no private bucket.
public_link_supportedis a new required property on the admin job projections. It is additive and/api/v2is not locked, and the contract diff confirms none of the six changes is breaking. Neithersilo-applenorsilo-androidreferencesdownload_urlor any admin job artifact surface — Ichecked both — so no client work is required.
The new route is authorized by signature rather than session, which is deliberate and explained
above, but it is the security-relevant part of this change and deserves the closest review. It is
scoped to a single job ID, carries a 15-minute lifetime matching the presigned URL it replaces, uses
a signing domain distinct from artwork, and answers 404 for every failure mode.
Checklist
AI Disclosure
claude-opus-5[1m](Opus 5, 1M context) for implementation; Fable for the review passcode was written, checking file and line claims, the encrypted-settings set, the prefix-collision
assumption, and whether anything sweeps a whole store root. It confirmed the artwork sweep is
prefix-bounded, that none of the storage settings keys is encrypted, and that the
"local"bucketsentinel reads back safely on every path that persists it. It raised four blocking findings, all
verified against the source and all resolved. Two land in this PR and changed the design: the
originally planned artifact route was modeled on the diagnostics download route and would have been
session-gated, which
window.opencannot satisfy — it became a signed capability; andPutStreamwould have bypassed the identity-recording wrapper. A third finding, that publishing a seven-day
link would return 500 on a local store, is fixed here with a typed 409 and the capability field.
The fourth concerned a default-root change that was dropped in refactor(storage): generalize the artwork store into a blob store #1112. Findings I did not adopt: the
reviewer noted that plugin-supplied provider IDs are not validated against the reserved key
prefixes, which is a pre-existing hazard on the shared public bucket rather than one introduced
here; it is documented in
docs/architecture/blob-storage.mdinstead of fixed.🤖 Generated with Claude Code
Note
Add local filesystem storage for diagnostics, job artifacts, and avatars
BucketAPIadapter in bucketapi.go to expose bucket-shaped object operations over the key-basedStoreinterface, so filesystem-backed storage can serve callers that expect S3-style bucketsPutStreamas a required method on theStoreinterface, implemented by filesystem, S3, and memory backends with context cancellation supportGET /admin/jobs/capabilitiesandGET /admin/jobs/{id}/artifactfor streaming completed job artifacts via signed, session-independent URLs when presigning is unavailable; the signer is generalized in artworkurl.go to support both artwork and job-artifact capability domainspresign_unsupportedwhen the backend cannot presignStore.PutStreamis now required — any out-of-treeStoreimplementations must add this method. TheAdminTaskJobAPI response adds requiredpublic_link_supportedboolean. Profile avatar storage now uses only the operational store, ignoring public artwork storage.Macroscope summarized b2b0a89.