Skip to content

feat: one object-store settings parser for the Action, gateway and backfill - #1883

Merged
marcusrbrown merged 10 commits into
mainfrom
feat/object-store-config-spine
Oct 20, 2026
Merged

marcusrbrown merged 10 commits into
mainfrom
feat/object-store-config-spine

Conversation

@marcusrbrown

Copy link
Copy Markdown
Collaborator

Problem

The Action, the gateway server and the gateway deny-key backfill each read object-store (S3) settings their own way:

  • The Action validated endpoint and prefix inline.
  • The gateway server checked only that values were non-empty, and cast S3_SSE without checking it.
  • The backfill read only AWS_REGION, S3_BUCKET and S3_PREFIX, so on R2 or MinIO it silently targeted plain AWS.

A KMS key that the effective encryption mode would ignore was dropped silently everywhere.

Change

There is now one runtime parser, parseObjectStoreSettings, in packages/runtime/src/object-store/settings.ts. It trims every value and treats an empty value as unset. It applies the endpoint rules from #1877, plus prefix, SSE mode, KMS and expected-owner validation. Errors name the setting and the rule, never the value.

  • Action: inputs.ts maps the s3-* and aws-region inputs onto the parser. Region stays optional.
  • Gateway: one loader, loadObjectStoreSettings in object-store-config.ts, serves both the server and the backfill.
    • S3_REGION is required and falls back to AWS_REGION.
    • Every setting accepts a _FILE form.
    • New optional settings: S3_ALLOW_INSECURE_ENDPOINT, S3_EXPECTED_BUCKET_OWNER and S3_SSE_KMS_KEY_ID.
  • Secret reading: the readers move to secrets.ts. Only object-store settings treat a blank _FILE as unset and use the same-named env var, because compose always sets S3_REGION_FILE and S3_ENDPOINT_FILE. Every other secret keeps the existing rule, where a blank file is an opt-out.
  • Startup logging: an invalid object-store setting fails startup with gateway config invalid and the rule it broke. Every other startup failure still logs only gateway startup failed.
  • Encryption rule: getEffectiveEncryption moves to a pure object-store/encryption.ts and is no longer exported from @fro-bot/runtime.

Behaviour changes

Surface Change
Action s3-sse-kms-key-id is an error unless the effective mode is aws:kms. On a custom endpoint the default is AES256, so set s3-sse-encryption: aws:kms.
Action The s3-bucket and s3-sse-encryption error text changed. Both now start with the input name.
Action and gateway S3_EXPECTED_BUCKET_OWNER / s3-expected-bucket-owner must be a 12-digit account ID.
Gateway Startup now rejects an invalid S3_ENDPOINT (http or a private address without the flag, or a metadata address), an invalid S3_PREFIX, an unknown S3_SSE, or a KMS key without effective aws:kms.
Gateway Values are trimmed. A whitespace-only S3_PREFIX uses the default fro-bot-state.
Gateway A blank S3 _FILE defers to the env var. Other secrets are unchanged.
Backfill It honours S3_REGION (before AWS_REGION), S3_ENDPOINT, S3_SSE, S3_PREFIX and the other S3 settings. With an endpoint and no S3_SSE, it now derives AES256.
Backfill An empty S3_PREFIX now uses fro-bot-state. Deny keys written at the bucket root by an earlier empty-prefix run are not read.
Backfill Credentials and app ID go through the gateway secret readers. A blank credential file now exits 1, symlinks and files over 4 KB are rejected, and only trailing whitespace is trimmed.

deploy/README.md has a settings table and a rollout checklist:

  • confirm the endpoint passes;
  • confirm the encryption mode the bucket accepts;
  • set S3_REGION=auto for R2/MinIO;
  • stop or quiesce the gateway before running backfill --apply;
  • to retire a static key, remove both the file and any AWS_* env vars.

Known, unchanged

A blank AWS key file on the daemon means no static credentials. The AWS SDK default chain then applies, and that chain still reads AWS_* from the environment. This is how instance and role credentials work, and the behaviour was the same before this change.

Verification

  • bun run check-types, bun run lint and bun run build pass. dist/ is rebuilt.
  • Tests: runtime 1383, Action 2607, workspace-agent 773, harness 439, gateway 5416, evals 173, scripts 688.
  • Revert checks fail as expected for:
    • the region fallback;
    • endpoint validation;
    • the KMS rule;
    • trimming;
    • the owner format;
    • the S3-only blank-file rule (17 tests fail if every secret falls through);
    • the startup log.

fro-bot
fro-bot previously approved these changes Oct 20, 2026

@fro-bot fro-bot left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Verdict: PASS

Blocking issues

None

Non-blocking concerns

  • P2 — Unused region fallback can prevent startup. object-store-config.ts:50 eagerly reads AWS_REGION and its _FILE even when valid S3_REGION wins. An embedded newline in the unused env value, or an unreadable/symlinked fallback file, therefore aborts gateway startup and backfill. Previously the daemon ignored these sources. Resolve S3_REGION first and read the fallback only when needed. Confirmed with the focused probe below.

Missing tests

  • Add precedence coverage with valid S3_REGION and malformed AWS_REGION or rejected AWS_REGION_FILE; assert the canonical region succeeds. Existing tests cover precedence only with valid fallback values. Retain rejection coverage when the fallback is actually needed.

Risk assessment (MED): likelihood of regression, security exposure, or blast radius

Stricter validation intentionally affects gateway startup and backfill destinations. The identified regression requires an invalid unused fallback. No blocking security or public-API defect found. Reviewed source and tests; no suites, builds, lint, or type checks run.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 39370774313
Cache hit
Session ses_ea861834dffeH2B1rL2nbCwI2R

Focused probe tested whether an invalid, unused fallback overrides a valid canonical region. It exited 1 with the embedded-line-break error for AWS_REGION, confirming the regression:

bun -e 'process.env.S3_BUCKET="review-bucket"; process.env.S3_REGION="us-east-1"; process.env.AWS_REGION="unused\nregion"; const {loadObjectStoreSettings}=await import("./packages/gateway/src/object-store-config.ts"); try {console.info(loadObjectStoreSettings().region)} catch(error) {console.error(error instanceof Error ? error.message : String(error)); process.exitCode=1}'

Review synthesis tooling was inaccessible under this run's filesystem permissions; findings were independently checked against source and the focused probe. Inline posting was unavailable through the response-file delivery contract.

@fro-bot fro-bot left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Verdict: PASS

Re-reviewed ad933de. The unused-region-fallback issue is resolved. Added gateway and backfill tests cover ignored malformed fallbacks and rejection when the fallback is needed.

Blocking issues

None

Non-blocking concerns

None

Missing tests

None

Risk assessment (MED): likelihood of regression, security exposure, or blast radius

The overall PR intentionally tightens startup validation and changes backfill configuration handling. The follow-up fix is narrowly scoped, preserves fallback validation, and introduces no identified security or public-API regression.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 39372744761
Cache hit
Session ses_ea861834dffeH2B1rL2nbCwI2R

Inspected the three-file delta from 1143d10 to ad933de, related secret readers, parser behavior, and test isolation. Prior concern and missing coverage are addressed. No tests, builds, lint, or type checks were run.

@marcusrbrown
marcusrbrown merged commit 423fa2f into main Oct 20, 2026
15 checks passed
@marcusrbrown
marcusrbrown deleted the feat/object-store-config-spine branch October 20, 2026 05:07
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