small fixes 2026-08-21: sliderule-public-cors ListBucket grant; injected-credential contract docs

[espg]

Closes #502
Closes #500

Two independent small-fix issues bundled per CLAUDE.md §5.

What / approach

Issue #502 — bucket-level s3:ListBucket on sliderule-public-cors. The execution role in deployment/aws/template.yaml held s3:GetObject/s3:PutObject/s3:DeleteObject on arn:aws:s3:::sliderule-public-cors/* but nothing at the bucket level. Without s3:ListBucket on the bucket, S3 answers a GET on a missing key with 403 AccessDenied rather than 404 NoSuchKey. That distinction is load-bearing on the sidecar read path, which catches the 404 only:

except FileNotFoundError:
    # obstore's NotFoundError subclasses FileNotFoundError (local + s3).
    return None

That return None is what selects on_miss=fallback; a 403 raises obstore's permission error, which does not subclass FileNotFoundError, so it propagates and the read hard-errors where it should have quietly taken the slow route. The fix is one unconditioned statement, mirroring the Source Cooperative ListBucket grant added under #495 for exactly the same 403-vs-404 reason — a GetObject evaluation carries no s3:prefix context key, so a condition on it never matches during a GET and every absent object comes back 403 anyway:

- Effect: Allow
  Action: s3:ListBucket
  Resource: arn:aws:s3:::sliderule-public-cors

Issue #502 explicitly authorizes editing deployment/aws/template.yaml, which satisfies CLAUDE.md §1. Nothing was deployed.

Pinned by a new test in tests/test_lambda_build.py alongside the existing ExecutionRole policy assertions: the grant exists exactly once, its Resource normalizes to the bare bucket ARN, its Effect is Allow, its actions normalize to exactly ["s3:ListBucket"], and it carries no Condition.

Issue #500 — docstrings on open_store / open_object_store. Documentation only, no behavioural change. Three things a caller using injected credentials needs and cannot see from the signature:

1. The x-amz-acl override. External/published writes (_PUBLISHED_BUCKETS and injected-credential external targets) attach x-amz-acl: bucket-owner-full-control via setdefault, so a caller-supplied value wins — honoured as passed, neither merged nor overwritten.
2. Its permission constraint. Any ACL-carrying PUT needs s3:PutObjectAcl on the target, so overriding to a different canned value carries the same requirement rather than a lesser one.
3. Injected credentials are never refreshed (folded in from output_credentials are injected once and never refreshed: role chaining caps writes at 1 hour, failing the tail of a long campaign #498). output_credentials are resolved once at dispatch and embedded in every worker's invoke payload, so a worker inherits the dispatcher's clock: a long run fails in the tail, at write time, after the compute is paid for, concentrated on the slowest shards. The ceiling depends on how the credentials were obtained — sts:AssumeRole from an already-assumed role (SSO included) is role chaining, hard-capped at one hour and not raisable via MaxSessionDuration, while AssumeRoleWithWebIdentity is not chaining and honours MaxSessionDuration up to 12 hours. This does not affect the fleet's published writes, which go out under the ambient execution role that Lambda rotates transparently; the limitation is specific to the injected-credential escape hatch (Support credential injection for output writes (modular external targets, e.g. source.coop) #26).

client_options was also added to open_store's **kwargs Parameters entry — it had never been named there, so the knob was undiscoverable from where callers actually look.

One deliberate divergence from issue #500's text. The issue states that suppressing the ACL header entirely "is not possible through this path (there is no 'send no ACL' value)". That has not been true since c60701d ("fold review: let a caller strip the ACL header and widen the endpoint exclusion", #495), which gave _with_bucket_owner_acl a None sentinel:

headers.setdefault("x-amz-acl", _BUCKET_OWNER_ACL)
if headers["x-amz-acl"] is None:
    del headers["x-amz-acl"]

The docstrings document the code as it is — passing None strips the header — rather than repeating the issue's premise. Documenting a working suppression path as impossible would recreate the exact "only discoverable by reading store.py" failure #500 was filed to end. Called out here because it is a knowing departure from the issue text, not an oversight.

The sentinel is documented scoped, which is the shape the code actually has: it is interpreted only inside _with_bucket_owner_acl, and _s3_object_store routes client_options through that helper only behind

if (
    _external_target(credentials, endpoint_url, bucket)
    and not read_only
    and not kwargs.get("skip_signature")
):

so on our own buckets, read_only=True, skip_signature=True, or any endpoint_url, no ACL is sent to begin with and a None value reaches obstore raw, which rejects it.

Phases

• Phase 1 — issue Execution role lacks bucket-level s3:ListBucket on sliderule-public-cors: a missing sidecar 403s instead of 404ing, defeating on_miss=fallback #502: add the unconditioned s3:ListBucket grant on arn:aws:s3:::sliderule-public-cors in deployment/aws/template.yaml, plus the pinning test in tests/test_lambda_build.py.
• Phase 2 — issue Document the injected-credential contract on open_store/open_object_store: the x-amz-acl override and the no-refresh lifetime #500: document the x-amz-acl override, its s3:PutObjectAcl constraint, and the never-refreshed injected-credential lifetime in the open_store / open_object_store docstrings.

Both phases went through the fresh-context adversarial review loop (CLAUDE.md §2). Round one: four findings, all folded, one commit each, with a reply and a resolve on every thread. Round two: four findings — three folded and resolved (d78b5e1, 71b5be7, 3e94206), and one left standing and unresolved because settling it needs a live-AWS call this run may not make; it is the first item under "Questions for review" below.

Caveat carried over from issue #502

Whether this fires today is unverified: it needs both the sidecar backend reading from sliderule-public-cors and an absent sidecar, and production may default to inline+compiled, making it a benchmark-only arm. The mechanism is confirmed; the blast radius is not. The bucket is also slated for retirement under #499, so this may be fixed and then deleted. It is still a live one-line hazard fix, which is the reasoning the issue itself gives for doing it now rather than waiting on a post-MVP teardown.

How it was tested

• Full suite: uv run pytest -q — 4605 passed, 38 skipped in 408 s.
• uv run pytest -v tests/test_lambda_build.py tests/test_deploy_lambda.py — 60 passed, covering the new grant assertions.
• uv run pytest -q tests/test_lambda_build.py tests/test_store.py — 89 passed after the round-two folds.
• uv run ruff check src tests and uv run ruff format --check src tests — clean on every file this PR touches.

Two pre-existing failures on main, untouched here (CLAUDE.md §4 — flag, don't fix). Both reproduce on a clean origin/main checkout with this branch stashed:

• ruff check: N818 Exception name 'UnknownCapability' should be named with an Error suffix at src/zagg/registry.py:64. The PR lint bot runs --select=E,F,W,I --ignore=E501, which excludes N, so CI does not see it.
• ruff format --check: tests/data/benchmark/README.md would be reformatted (a fenced Python block inside the markdown).

Questions for review

• Is the issue Execution role lacks bucket-level s3:ListBucket on sliderule-public-cors: a missing sidecar 403s instead of 404ing, defeating on_miss=fallback #502 premise still true, or is the grant already covered by the bucket policy? sliderule-public-cors is in-account (deployment/aws/lambda_handler.py:21), and same-account authorization is the union of identity and resource policy. docs/deployment/benchmark-cicd.md section 10 documents a PublicReadList statement granting s3:GetObject + s3:ListBucket to Principal: "*" on both the bare bucket ARN and /*. If that statement is live on the bucket today, the execution role already passes the s3:ListBucket test S3 uses to answer 404 rather than 403, and the hazard described in Execution role lacks bucket-level s3:ListBucket on sliderule-public-cors: a missing sidecar 403s instead of 404ing, defeating on_miss=fallback #502 has never fired. Deciding needs aws s3api get-bucket-policy --bucket sliderule-public-cors, which is a live-AWS call this run is forbidden to make (CLAUDE.md §1). Landing (a) below is the status quo of this PR minus a comment reword; (b) is a scope ruling and not the routine's to take (§6). Full reasoning is in the standing review thread on deployment/aws/template.yaml. Two options:
  • (a) Keep the grant as defence-in-depth and reword the template.yaml comment to claim only what it buys — the role becomes self-sufficient if the public policy is ever tightened or the bucket is recreated without it. In favour of (a): the grant is harmless and correct regardless of the bucket policy's current state, an identity-policy grant survives a change to that policy, and the bucket policy is not ours to rely on.
  • (b) Verify the live policy and close Execution role lacks bucket-level s3:ListBucket on sliderule-public-cors: a missing sidecar 403s instead of 404ing, defeating on_miss=fallback #502 as already-mitigated, dropping the statement and its test.
• Phase 2 documents the None-strips-the-header hatch that In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #495 added, rather than the "not possible through this path" framing in Document the injected-credential contract on open_store/open_object_store: the x-amz-acl override and the no-refresh lifetime #500's text (see the divergence note above). Confirming that documenting the code over the issue is the wanted call — the alternative is to leave the suppression path undocumented, which seems strictly worse.
• Two smaller items were left standing rather than folded, because both widen scope beyond the finding that raised them: (1) documenting that passing client_options at all takes an ambient s3:// call off the _OBJECT_STORE_CACHE fast path (the cache is gated on not kwargs — issue Sidecar read path rebuilds an S3 client + credential chain per granule (missing the #244 store-cache) #287), and (2) applying the same _statement_resources normalization to the source.coop sibling test, which this PR does not otherwise touch. Say the word if either should land here rather than as a follow-up.
• The two pre-existing lint failures above are left alone per §4. Say the word if a follow-up small-fix issue for them would be useful; opening one is a side-effecting action and is not mine to take (§6).

[espg]

🤖 from Claude (review)

Adversarial review of phase 1 (issue #502), fresh read against CLAUDE.md and the issue's acceptance criteria.

The grant itself is correct. Bare bucket ARN, s3:ListBucket only, no Condition — which is the whole point, since a GetObject evaluation carries no s3:prefix context key and a condition would leave every absent object at 403. It is placed directly after the sliderule-public-cors/* object grant, mirroring the OutputBucketName object/bucket pair above, which is the shape issue #502 asked for. Nothing was deployed, and deployment/aws/template.yaml is edited under the explicit authorization in the issue (CLAUDE.md §1).

The test pins the four things that matter — existence, exactly one statement, exact action list normalized through _statement_actions, and the absence of a Condition — with the failure messages naming the mechanism rather than the assertion. _statement_actions is the right helper: the grant is IAM-identical as a scalar or a one-element list, and a rewrite between those shapes must not fail this spuriously.

Two findings, both diff-scoped, neither blocking:

1. tests/test_lambda_build.py — the new test breaks a "the next test" cross-reference in the test above it.
2. deployment/aws/template.yaml — the comment is longer than the terseness bar and duplicates the test's comment.

No missing test coverage beyond that, no new dependencies, no scope creep past what issue #502 authorizes. Phase 2 (issue #500) is not in this diff and was not reviewed.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Adversarial review of phase 2 (issue #500), fresh read against CLAUDE.md and the issue's acceptance criteria.

Docstrings only — confirmed. git diff touches nothing but the two docstrings; no behavioural change, no new dependency, no wire-format or spec surface, so docs/specification.md and the conformance fixtures are correctly untouched (CLAUDE.md §4). All three requested items are present in open_store: the setdefault precedence with a runnable example, the s3:PutObjectAcl constraint, and the never-refreshed lifetime with the role-chaining vs AssumeRoleWithWebIdentity ceiling and the explicit "this does not affect published writes" carve-out.

One substantive divergence from the issue, and the diff is right where the issue is stale. Issue #500 states there is no "send no ACL" value and that suppressing the header entirely "is not possible through this path". That has not been true since c60701d ("fold review: let a caller strip the ACL header and widen the endpoint exclusion", issue #495), which added a None sentinel to _with_bucket_owner_acl:

headers.setdefault("x-amz-acl", _BUCKET_OWNER_ACL)
if headers["x-amz-acl"] is None:
    del headers["x-amz-acl"]

The docstring documents the code rather than the issue text, which is the correct call — documenting a suppression path as impossible while it works is exactly the "discoverable only by reading store.py" failure #500 exists to end. Flagging it because the PR body still repeats the issue's stale framing in both the What/approach section and "Questions for review", so the PR currently contradicts its own diff. The body should be corrected to say the hatch exists and that the issue's premise aged out.

Two diff-scoped findings, neither blocking:

1. src/zagg/store.py — client_options is still absent from open_store's **kwargs Parameters entry, so the knob remains undiscoverable from where callers look.
2. src/zagg/store.py — the open_object_store cross-reference breaks an inline literal across a line wrap.

No concern about the cross-reference approach itself: pointing at open_store rather than duplicating twenty lines is the right terseness call for two functions that share _s3_object_store.

---

Generated by Claude Code

[espg]

🤖 from Claude

Run status, and one convention conflict to flag rather than guess at (CLAUDE.md preamble).

Where this stands. Both phases are complete, all four adversarial-review findings are folded (one commit each, every inline thread replied to and resolved), and the full local suite is green — uv run pytest -q: 4605 passed, 38 skipped. On CI, ruff, check, build, build / build-x86_64, and build / build-arm64 are all green on 43db0f3; test (3.12) and test (3.13) were still running when this run ended.

The conflict. §5 says a PR that gets waiting because every phase is complete should also be marked ready for review, "don't leave it draft, so the state is unambiguous". This run's instructions say explicitly to leave it in draft and not flip it. Left as draft, for two reasons: draft is the reversible choice and the §1/§2 default, and §2 independently gates the flip on CI being green, which is not yet confirmed — the two test jobs had not reported. So under §2 draft is correct right now regardless of how the §5 question is settled. Flagging it because the next run will face the same fork once CI reports: if the intent is that a phase-complete waiting PR should be ready-for-review, gh pr ready on this one is a one-liner (and --undo reverses it).

Labels are implement + waiting. Three items are open under "Questions for review" in the description — the #499 retirement fork on #502, the deliberate divergence from #500's "no send-no-ACL value" premise (the None hatch has existed since c60701d), and two pre-existing lint failures on main left alone per §4.

---

Generated by Claude Code

[espg]

🤖 from Claude

CI is now fully green on 43db0f3 — closing out the one item left open above.

check	result
test (3.12)	success (11:32:12Z)
test (3.13)	success (11:32:12Z)
ruff	success
check	success
build, build / build-x86_64, build / build-arm64	success

Both test jobs were still running when the previous comment went up; they have since passed, so nothing is outstanding on the CI side.

That changes the shape of the §5 conflict flagged above, so restating it precisely: §2's "draft until CI is green" gate is now satisfied, which means it is no longer doing the work of keeping this in draft. The only thing holding the draft state is this run's explicit instruction not to flip it, which runs against §5's "a phase-complete waiting PR should also be marked ready for review, don't leave it draft". Left as draft — an unattended run should not resolve that fork by picking the irreversible-looking side, and per the CLAUDE.md preamble the rule is to flag a conflict rather than guess. gh pr ready flips it whenever that is the wanted answer (--undo reverses).

Everything else is unchanged: both phases complete, four review findings folded, labels implement + waiting, three open items under "Questions for review" in the description.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Reviewed the full diff against CLAUDE.md, issues #502 and #500, and the surrounding code in all three files — the whole ExecutionRole policy, the neighbouring ExecutionRole assertions plus _statement_actions/_statement_resources, and open_store / open_object_store / _with_bucket_owner_acl / _s3_object_store / _external_target / _PUBLISHED_BUCKETS. Four findings. One challenges the premise of #502 rather than the implementation; the other three are precision problems in claims that will outlive this PR.

Conventions: clean, no findings. Six commits, all title-only in the repo's existing style (phase 1 of issue #502, fold review: <short> (issue #N)), no long bodies, no wip/fixup. No 🤖 attribution in the PR description or in any commit message — correct per §3/§6. Closes #502 and Closes #500 both present, each with its own phases-checklist entry per §5's small-fix bundling rule. No new dependencies (§4). No # noqa / # type: ignore / weakened tests. deployment/aws/template.yaml is named explicitly by #502, which satisfies the §1 carve-out, and nothing was deployed. src/zagg/store.py is 472 lines and tests/test_lambda_build.py 998 — both inside §4's limit. Comment density in both new blocks matches the neighbours rather than exceeding them, and the two pre-existing lint failures are correctly flagged rather than fixed. The four earlier review threads are folded and I have not re-litigated any of them.

Phase 1 — issue #502. The statement is in the right policy and the right role, is the right shape, and grants exactly what the issue asks and nothing more. My objection is upstream of the diff: #502 infers "no identity-side ListBucket, therefore 403", but this repo's own docs say sliderule-public-cors is in-account (deployment/aws/lambda_handler.py:21) and carries a PublicReadList bucket policy granting s3:ListBucket to Principal: "*" on the bare bucket ARN (docs/deployment/benchmark-cicd.md section 10). Same-account authorization is the union of identity and resource policy, so the role plausibly already lists this bucket and the 403 hazard may never have existed. That is a real difference from the source.coop sibling, where the bucket is someone else's and the identity grant genuinely is the only door. Details and the two ways to resolve it are on the template.yaml comment. The grant is harmless either way; the assertion of the mechanism, repeated in the template comment and the test comment, is what I would not land unverified.

Phase 2 — issue #500. The deliberate divergence from the issue text is the right call and I confirmed it independently: _with_bucket_owner_acl really does del headers["x-amz-acl"] on a None value, so #500's "there is no send-no-ACL value" is stale and documenting the code over the issue is strictly better than the alternative. The setdefault description is accurate, the key-case parenthetical is accurate (the helper lowercases caller keys, which is what makes the precedence real), the s3:PutObjectAcl constraint is right, the runnable example actually works end to end (credentials=creds makes _external_target true, so the override path really is reached), and the credential-lifetime paragraph matches #500 and the module's existing density.

What is wrong is the scope of the None claim in both docstrings. _with_bucket_owner_acl is only reached behind _external_target(...) and not read_only and not kwargs.get("skip_signature"); everywhere else client_options goes to S3Store verbatim, and the helper's own docstring says obstore rejects a None header value. So "the only way through this path to send no ACL at all" and "None strips the header outright" both promise more than the code delivers — and for open_object_store the failing shape is its dominant one (ambient writes to our own output bucket, which are not external targets). Two words of scoping fixes both.

Bottom line. Phase 2 meets #500's acceptance criteria in substance — all three requested items are documented, in the right place, and client_options is now discoverable from Parameters — but two sentences are subtly over-claimed in a way that would mislead exactly the caller the issue was filed to help, so I would not call it done until they are scoped. Phase 1 satisfies the literal ask in #502 (the statement, unconditioned, plus a pinning test), and the test is a good pin once Effect and the Resource shape are tightened; whether the underlying defect is real is unverified in a stronger sense than the PR's own caveat admits, and that is worth ten seconds of get-bucket-policy before this lands.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

The statement itself is placed correctly and I have no complaint about its shape: ExecutionRole → Policies[0] (logs-and-output-s3) → Statement, sitting immediately after the object-level sliderule-public-cors/* grant, which mirrors how OutputBucketName and us-west-2.opendata.source.coop each pair an object grant with a bare-bucket ListBucket. Bare bucket ARN, single action, no Condition. That is exactly what issue #502 asks for and nothing more.

The premise is what I cannot reconcile. #502 reasons: the role holds no bucket-level ListBucket, therefore an absent key answers 403. That inference only holds if nothing else grants ListBucket to this principal — and for a same-account principal S3 authorizes on the union of the identity policy and the bucket policy. This repo documents both halves of that union for this bucket:

• deployment/aws/lambda_handler.py:21 calls it "the in-account bucket, sliderule-public-cors". So this is not a cross-account request and the union rule applies (cross-account would need both policies, which is the source.coop situation, not this one).
• docs/deployment/benchmark-cicd.md section 10 describes it as "readable + listable from anywhere" and documents the policy the section-2 stack applies when CreateDistBucket=true:

{"Sid": "PublicReadList", "Effect": "Allow", "Principal": "*",
 "Action": ["s3:GetObject", "s3:ListBucket"],
 "Resource": ["arn:aws:s3:::sliderule-public-cors",
              "arn:aws:s3:::sliderule-public-cors/*"]}

Principal: "*" on s3:ListBucket at the bucket ARN covers the execution role. If that policy is live, the role already passes the "caller holds s3:ListBucket" test S3 uses to choose 404 over 403, and the hazard described in #502 cannot have been firing at all. Account-level Block Public Access does not rescue the premise either — RestrictPublicBuckets narrows a public policy to principals within the owning account, and the execution role is one.

This is materially different from the source.coop grant this is modelled on, where the bucket genuinely belongs to someone else and the identity-side grant is load-bearing. Copying that reasoning across buckets is where I think the issue over-reached.

Concretely, before landing: confirm the live policy with aws s3api get-bucket-policy --bucket sliderule-public-cors, then either

1. public ListBucket is live — keep the statement (it is harmless and cheap) but reword this comment to say what it actually buys: the role becomes self-sufficient if the public policy is ever tightened or the bucket is recreated without it. Asserting a hazard the bucket policy already covers is the kind of claim that outlives the PR and misleads the next reader.
2. it is not live — keep the comment exactly as written and say so in the PR body, which closes the "blast radius is not confirmed" caveat rather than carrying it forward.

Flagging the claim, not the grant. An unconditioned ListBucket on a bucket the account owns and already publishes is safe either way, and #499 retires it regardless.

One piece of this is a ruling rather than a change I can ask for: if the public policy is live, whether #502 should be closed as already-mitigated instead of landed is a scope call and not mine to make.

---

Generated by Claude Code

[espg]

🤖 from Claude

Left standing, not folded — deliberately, and this thread is left unresolved.

Both branches of the finding require aws s3api get-bucket-policy --bucket sliderule-public-cors to choose between them, and that is a live-AWS call this run is forbidden to make (CLAUDE.md §1). Without it I cannot tell whether the PublicReadList statement documented in docs/deployment/benchmark-cicd.md section 10 is actually applied to the live bucket, and the union argument is only as good as that premise: the in-account framing in deployment/aws/lambda_handler.py:21 is right, and same-account authorization really is the union of identity and resource policy, so if that statement is live the role already passes the s3:ListBucket test S3 uses to pick 404 over 403 and the hazard in issue #502 was never firing.

The follow-on — close issue #502 as already-mitigated rather than landing the grant — is a scope ruling, not a diff change, so it is not the fold's to take either (§6).

Raised as a question in the PR body under "Questions for review", with the two options:

• (a) keep the grant as defence-in-depth and reword the comment to say what it buys (self-sufficiency if the public policy is tightened or the bucket is recreated without it). In favour of (a): the grant is harmless and correct whatever the bucket policy currently says, and that policy is not ours to depend on — an identity-policy grant survives a change to it, and issue Retire sliderule-public-cors: NASA buckets cannot host public data; migrate the sidecar cache to source.coop #499 retires the bucket regardless.
• (b) verify the live policy and close issue Execution role lacks bucket-level s3:ListBucket on sliderule-public-cors: a missing sidecar 403s instead of 404ing, defeating on_miss=fallback #502 as already-mitigated.

The comment as written stays unchanged for now, since rewording it presumes the answer.

---

Generated by Claude Code