Skip to content

fix(config): require the upload service settings - #100

Open
bajtos wants to merge 1 commit into
mainfrom
claude/require-upload-config
Open

bajtos wants to merge 1 commit into
mainfrom
claude/require-upload-config

Conversation

@bajtos

@bajtos bajtos commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

The first run of E2E tests (fil-forge/infra-nodes#154) discovered that Hilt in dev cannot delete tenants because it was configured to use the production revocation service. I decided to make the revocation service config required in #99. Claude flagged that there is a similar footgun related to the configuration of the upload service, so in this PR I am making it required too.


Written by Claude.

Require upload.service_id, upload.service_url and upload.product_id instead of defaulting them to prod's Sprue (did:web:upload.fil-forge.com, https://upload.fil-forge.com) and prod's product DID (did:web:hilt.fil-forge.com).

A stage that forgot these settings would register its tenants and provision its buckets on prod Sprue. #99 fixes the same pattern for the revocation settings, which turned every dev tenant delete into a 500.

  • Remove the three SetDefault calls in pkg/config and the --upload-* flag defaults in cmd/main.go.
  • NewUploadClient rejects an empty service DID, URL or product DID, so hilt serve exits during fx construction with, for example, upload.product_id is required: set HILT_UPLOAD_PRODUCT_ID or --upload-product-id.
  • upload.product_id was optional in the constructor. Tenant provisioning always passes it to Sprue's /customer/add, so a serve without it cannot create tenants, and it is now required too. upload.proofs stays optional.
  • The hilt client commands load config but never build the upload client, so they keep working without these settings.
  • Tests: config.Load leaves the upload config empty when nothing sets it; NewUploadClient errors on each missing field and on an invalid product DID; an fx app with the upload client and empty config fails to construct.
  • README and AGENTS.md mark the three settings required.

Safe to merge once reviewed. infra-central's shared modules/apps, used by dev, staging and prod, has set all three HILT_UPLOAD_* variables since 2026-08-19. Smelt's systems/hilt/compose.yml sets them for the integration stack.

This PR and #99 edit neighbouring lines in pkg/config/config.go SetDefaults and the same "Run locally" line in AGENTS.md. Whichever merges second needs a small conflict resolution.

upload.service_id, upload.service_url and upload.product_id defaulted
to prod's Sprue and product DID. A stage that forgot to set them would
provision its tenants and buckets on prod Sprue, the same failure that
the revocation defaults caused for tenant deletes.

Drop the three defaults and make NewUploadClient refuse empty values,
so `hilt serve` exits at startup naming the missing env var and flag.
The product DID was optional in the constructor, but tenant
provisioning always passes it to /customer/add, so serve cannot work
without it. The `hilt client` commands never build the upload client.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Copilot AI balanced review requested due to automatic review settings October 16, 2026 16:00
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T16:02:27.025839Z e26e73f PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is a small, well-tested, consistently-applied config hardening with no leftover default references and no impact on the non-serve code paths.

0 open findings

What changed in this PR

This PR removes the production Sprue/product DID defaults for the upload service settings (upload.service_id, upload.service_url, upload.product_id) and makes all three required, so a misconfigured stage fails fast at hilt serve startup instead of silently registering tenants and provisioning buckets against prod's Sprue. It mirrors the pattern established for the revocation settings in #99, and additionally promotes upload.product_id from optional to required since tenant provisioning always passes it to Sprue's /customer/add.

Changes:

  • Remove the three upload.* SetDefault calls in pkg/config and the corresponding --upload-* flag defaults in cmd/main.go.
  • NewUploadClient now rejects an empty ServiceID, ServiceURL, or ProductID with an actionable startup error, and product_id is always parsed (no longer conditionally).
  • Add tests for missing/invalid fields, empty-config fx construction failure, and no-default loading; update README and AGENTS.md to mark the three settings required.
File Description
pkg/​fx/​upload.go Adds required-field checks for ServiceID/ServiceURL/ProductID and makes product_id parsing unconditional.
pkg/​config/​config.go Removes the three upload.* defaults and documents them as required.
cmd/​main.go Clears the --upload-* flag defaults and marks them required in help text.
pkg/​fx/​upload_test.go Adds missing-field, invalid-product-DID, and empty-config fx-construction test cases.
pkg/​fx/​rpc_test.go Supplies ProductID so the RPC app still constructs under the new requirement.
pkg/​config/​config_test.go Verifies the upload config stays empty when nothing sets it.
README.md Marks the three upload settings as required with a startup-failure note.
AGENTS.md Updates the "Run locally" command to include the now-required upload flags.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants