Publish Lambda zips to Source Cooperative (issue #497 phases 1-2)

[espg]

Closes #497. Refs #495 (the fleet's half of the same identity model), #499 (retirement of the dist bucket), #496 (the ACL / PutObjectAcl pairing).

What this does

Phases 1 and 2 of issue #497: the our-side, git-tracked half of moving the Lambda deployment artifacts to Source Cooperative. Everything here is inert until Source Cooperative grants arn:aws:iam::742127912612:role/zagg-lambda-release write access at englacial/* (issue #497 question (4)), and nothing goes live on merge — the IAM change takes effect only when an account admin re-applies the benchmark_cicd.yaml stack, and the distribute job stays skipped until the repo variables are set. No live-AWS command was run.

Phase 2 landed partially, for an environment reason, not a design one — see "What did not land" below.

Phase 1 — grant the existing ReleaseRole write access to Source Cooperative

deployment/aws/benchmark_cicd.yaml. No new role: zagg-lambda-release already exists with a stable explicit name (RoleName: !Ref ReleaseRoleName) and exactly the right trust domain (GitHub Actions OIDC, sts:AssumeRoleWithWebIdentity), so it needed S3 permissions, not an identity. It stays separate from the fleet's zagg-lambda-execution because the trust domains genuinely differ — a CI run cannot write datacubes and a person cannot publish releases.

Two statements, mirroring the shape and the reasoning of the ExecutionRole published-destination grant in deployment/aws/template.yaml (lines ~246-309):

- Sid: PublishToSourceCoop
  Effect: Allow
  Action: [s3:GetObject, s3:PutObject, s3:PutObjectAcl,
           s3:AbortMultipartUpload, s3:ListMultipartUploadParts]
  Resource:
    - arn:aws:s3:::us-west-2.opendata.source.coop/englacial/zagg/lambda/*
    - arn:aws:s3:::us-west-2.opendata.source.coop/englacial/zagg/README.md
    - arn:aws:s3:::us-west-2.opendata.source.coop/englacial/zagg/LICENSE
- Sid: ListSourceCoopBucket
  Effect: Allow
  Action: s3:ListBucket
  Resource: arn:aws:s3:::us-west-2.opendata.source.coop

• The two repo-root keys are the ones deployment/aws/publish_mirror.sh also writes, from its MIRROR_REPO_PREFIX="${MIRROR_PREFIX%/lambda}" block — enumerated as exact keys rather than an englacial/zagg/* wildcard, so the grant cannot reach demo/ (the fleet's, issue In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #495) or benchmarks/.
• englacial/zagg/benchmarks/* is deliberately NOT granted — that is question (1) of issue Publish Lambda zips to source.coop: external users cannot stand up their own zagg fleet #497 and is unresolved. See "Questions for review".
• s3:PutObjectAcl travels with s3:PutObject: a write carrying x-amz-acl: bucket-owner-full-control is evaluated against s3:PutObjectAcl on both PutObject and CreateMultipartUpload, so the two halves are only correct together (issue In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #496). Granted on this destination only — the header is not sent to the dist bucket, whose objects we already own. Not every writer sends it yet: distribute_zips.sh does; publish_mirror.sh sends no ACL on any of its five PUTs and is the only writer of the two repo-root keys, so those two are granted but unreachable under this role until that changes. Adversarial review caught this; it is question (8) below, not fixed here, because publish_mirror.sh is a deployment/aws/ script issue Publish Lambda zips to source.coop: external users cannot stand up their own zagg fleet #497 does not name (CLAUDE.md §1).
• The multipart pair covers the failure path PutObject does not (the CLI's own abort of an in-flight layer upload would otherwise 403 and leak parts billed to Source Cooperative). s3:ListBucketMultipartUploads is deliberately not granted: it is bucket-wide, s3:prefix cannot constrain it, Source Cooperative's bucket policy does not grant it, so holding it our side would be denied cross-account anyway (espg, 2026-08-20).
• s3:ListBucket is unconditioned by intent. An s3:prefix condition makes absent objects 403 instead of 404, and distribute_zips.sh seeds versions.json only on a genuine miss and treats every other read failure as fatal — so a conditioned grant would fail every release rather than corrupt the index.
• s3:DeleteObject is deliberately absent (unlike the fleet's grant): a re-published minor overwrites via PutObject, and the mirror never prunes.

The comment block around these two statements is deliberately short: the ACL, multipart and unconditioned-ListBucket rationale is argued once in template.yaml's ExecutionRole grant and cross-referenced here, with only what differs on this role stated locally (adversarial review, b3b849e).

Also recorded, per the issue's "worth recording" note: a comment on DistBucket saying the live bucket's Block Public Access flags are deliberate, not drift (NASA buckets have no clearance to host public data), so the BlockPublicPolicy: false / RestrictPublicBuckets: false properties below it no longer read as though public access were intended. Comment only — no property changed.

Phase 2 — retarget the distribution script to source.coop

Approach taken, and why: retarget .github/scripts/distribute_zips.sh with a --prefix, rather than folding in publish_mirror.sh's layout. It is the smaller and more reviewable of the two, because the read side already speaks this dialect — deployment/aws/stand_up.sh has carried DIST_PREFIX with dist_key() / dist_root() helpers since issue #174:

DIST_PREFIX="${DIST_PREFIX:-}"                       # keys: [PREFIX/]MINOR/ZIP
dist_key()   { local p="$DIST_PREFIX"; [ -n "$p" ] && p="$p/"; echo "${p}${1}/${2}"; }
dist_root()  { local p="$DIST_PREFIX"; [ -n "$p" ] && p="$p/"; echo "${p}${1}"; }

So --prefix englacial/zagg/lambda produces byte-for-byte the layout publish_mirror.sh writes and stand_up.sh already reads, and the write side and read side finally match. Folding in publish_mirror.sh instead would have dragged along its per-minor README.md generation and repo-root metadata block — more surface, no new capability.

• bucket-owner-full-control on the PUTs (issue In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #495 phase 1), on all six uploads: the four zips, SHA256SUMS, and versions.json. Never on the read.
• The ACL is keyed on the destination bucket, mirroring zagg.store._PUBLISHED_BUCKETS, not sent unconditionally — the release role holds s3:PutObjectAcl on the mirror only, so an unconditional --acl would 403 against the in-account dist bucket. Pinned by test_the_scripts_published_bucket_list_matches_the_fleets.
• A published bucket without a --prefix is refused, in the script (d6336ee, adversarial review): its root is another organization's namespace and outside the grant, and the workflow-side vars.LAMBDA_DIST_PREFIX != '' gate lives in the half that could not be pushed. The invariant now holds in a landed artifact, with the workflow gate as defence in depth.
• Only a genuinely absent index seeds versions.json (ca634f1, adversarial review). The read used to swallow every failure into the seed branch and the next statement re-PUT the seed over the good index — a throttle, a 5xx, an expired session or a typo'd --prefix would silently drop every published minor. Now a miss seeds, anything else is fatal, and the CLI's own message survives instead of 2>/dev/null.
• versions.json moves to PREFIX/versions.json, not the bucket root — the mirror bucket's root is another organization's namespace.
• The change is backward compatible: no --prefix and a non-published bucket reproduce today's behaviour exactly.

What did not land, and why

.github/workflows/publish.yml is not in this branch. The workflow half was written and tested locally, then reverted: this run cannot push it. Both routes are refused:

! [remote rejected] claude/497-source-coop-publish -> claude/497-source-coop-publish
  (refusing to allow an OAuth App to create or update workflow
   `.github/workflows/publish.yml` without `workflow` scope)

...and the REST contents API returns the same 403. This is an environment restriction on the run, not a judgement about the change. Question (5) below carries the exact patch so it can be applied in one pass.

Consequence for tests: the two tests originally written to pin the workflow literals were replaced with tests that pin the coupling — deploy-prod gates on everything distribute gates on, and carries every LAMBDA_DIST_* variable distribute uses. Adversarial review then showed the coupling pair pinned the declaration of each variable and not its use, so applying question (5)'s patch minus the --layer-key line left both green while publish-layer-version pointed at an un-prefixed key. Strengthened in 99f5551: a destination variable bound in deploy-prod's env must be referenced by its command, and the variable distribute passes to --prefix must appear in deploy-prod's --layer-key argument specifically. Verified by re-running that exact mutation (fails) and the complete patch (passes) against a working-tree copy of the workflow that was reverted and never staged. Both assertions hold on the tree as it stands.

Phases

• Phase 1 — ReleaseRole grant in deployment/aws/benchmark_cicd.yaml + tests (a36e7e3)
• Phase 2 — distribute_zips.sh --prefix + conditional ACL, + tests (f6ff6f2, 551746d)
• Phase 2, workflow half — publish.yml wiring. Blocked on workflow push scope; patch in question (5).
• Phase 3 — backfill 0.9 → 0.48, publish versions.json. Out of scope: an operator action against live AWS (CLAUDE.md §1). Question (2).
• Phase 4 — default template.yaml's ArtifactBucket/LayerS3Key/FunctionS3Key at the public location. Out of scope: depends on phase 3 having populated the mirror. Question (3).
• Phase 5 — rule the dist bucket's disposition. Out of scope: an espg decision, and issue Retire sliderule-public-cors: NASA buckets cannot host public data; migrate the sidecar cache to source.coop #499 already rules that bucket post-MVP. Question (4).

Adversarial self-review ran after each phase. First pass: two diff-scoped findings folded (f2f7a9d, 91b2037), one left standing (question (6)). Second pass, folded one commit per finding — the in-script prefix guard (d6336ee), the coupling tests' use-not-declaration pin (99f5551), the narrowed index read (ca634f1), the present-tense header and docstring (4e78f33), the untrue "every write carries the ACL" sentence (7785845), the duplicated grant rationale (b3b849e), and the hard-coded role triple (2bb227c). One finding left standing: question (8). The review's own disclosure about how it ran is on its thread and repeated under question (7).

How it was tested

Phase 1 — assertions in tests/test_deploy_lambda.py, which is where benchmark_cicd.yaml already has coverage (it loads the template as CICD for the issue #341 ARN-drift guards) — extended rather than a new module. They mirror test_execution_role_is_the_published_identity in tests/test_lambda_build.py, reusing the same action/ARN normalization so an IAM-identical rewrite cannot fail them spuriously:

• test_release_role_reaches_exactly_the_mirror_keys — the granted ARNs are derived from publish_mirror.sh's own MIRROR_BUCKET/MIRROR_PREFIX shell defaults, not restated, so retargeting the mirror without moving the grant fails here.
• test_published_grant_carries_the_acl_and_multipart_halves — the exact five actions: the PutObject/PutObjectAcl pairing, the multipart pair, and the absence of DeleteObject.
• test_put_object_acl_is_granted_on_the_published_destination_only — sweeps every AWS::IAM::Role the template provisions, derived via _cicd_roles() rather than a typed triple, so a role added later is swept too (2bb227c). Mutation-checked: a fourth role carrying a stray PutObjectAcl on benchmarks/* fails this and the benchmarks guard below.
• test_published_list_bucket_is_unconditional — s3:ListBucket only, no Condition, no ListBucketMultipartUploads.
• test_release_role_does_not_pre_empt_the_benchmarks_prefix — no statement on any provisioned role reaches benchmarks/, englacial/*, or englacial/zagg/*.

Phase 2 — tests/test_distribute_zips.py, extended (it already runs the real script against a stub aws on PATH, no network): the prefixed layout, the index living under the prefix, the ACL on all six uploads and on none of the reads, the no-ACL path for a bucket we own, the script/fleet published-bucket list agreeing, the refusal of a published bucket with no prefix, the refusal to reseed the index on a read failure that is not a miss, and the two publish.yml coupling guards.

uv run pytest -v tests/test_distribute_zips.py tests/test_deploy_lambda.py tests/test_lambda_build.py → 76 passed. Full suite uv run pytest → 4617 passed, 38 skipped. bash -n clean on distribute_zips.sh (the repo wires no shellcheck). uv run ruff check tests clean; both touched test modules ruff format --check clean.

Two pre-existing failures, not touched — both reproduce on origin/main at 8dbb2fe and are unrelated to this change, so they are flagged rather than fixed (CLAUDE.md §4):

• ruff check src tests → N818 Exception name 'UnknownCapability' should be named with an Error suffix at src/zagg/registry.py:64.
• ruff format --check src tests → tests/data/benchmark/README.md:176 would be reformatted (a fenced Python block inside a fixture README).

Questions for review

1. Does englacial/zagg/benchmarks/* join the grant? (Issue Publish Lambda zips to source.coop: external users cannot stand up their own zagg fleet #497 question (1).) The grant deliberately stops at lambda/* + the two repo-root keys. (a) leave it out until zagg-bench-88s/*, zagg-bench-conus/*, zagg-examples/* are ruled on together with issue Retire sliderule-public-cors: NASA buckets cannot host public data; migrate the sidecar cache to source.coop #499's sidecar destination — the current state; (b) add arn:aws:s3:::us-west-2.opendata.source.coop/englacial/zagg/benchmarks/* to PublishToSourceCoop now, accepting an unused grant until something writes there; (c) rule that they lapse, and the question closes permanently. Recommendation: (a).

2. Phase 3 (backfill 0.9 → 0.48) is an operator action. It runs publish_mirror.sh against live AWS, which CLAUDE.md §1 forbids this run from doing. (a) run ./deployment/aws/publish_mirror.sh MINOR --run RUN_ID per minor by hand once the grant lands; (b) add a one-shot workflow_dispatch backfill job in a follow-up PR so the release role does it under OIDC — no long-lived credentials, but a new workflow job to review; (c) skip the backfill and publish forward from the next release only, accepting the cliff. Recommendation: (b) as a follow-up, since it reuses the grant this PR adds. Note: either route needs question (8) settled first — publish_mirror.sh sends no owner ACL today, so under zagg-lambda-release it 403s on its first zip.

3. Phase 4 has a live-AWS unknown this PR cannot settle. publish-layer-version reads the layer zip with the caller's credentials from a same-region bucket. us-west-2.opendata.source.coop is in-region and the release role now holds s3:GetObject on the prefix, so the CI path should work — but an external user's standup holds no such grant and must read the zip anonymously at publish-layer-version time. (a) confirm against live AWS that an unsigned / foreign-principal publish-layer-version from that bucket succeeds, before defaulting ArtifactBucket; (b) have stand_up.sh download the zip and re-upload to the user's own STAGING_BUCKET (it already has that parameter), trading a hop for certainty; (c) defer phase 4 entirely. Recommendation: (a) first, falling back to (b).

4. Phase 5 (dist bucket disposition) is deliberately untouched. This PR only adds the comment recording why it 403s. Confirm phase 5 is fully subsumed by issue Retire sliderule-public-cors: NASA buckets cannot host public data; migrate the sidecar cache to source.coop #499 and this PR should not carry it.

5. The publish.yml patch this run could not push. Apply as-is, or say how you'd rather have it. Three edits:

   • distribute — add vars.LAMBDA_DIST_PREFIX != '' to the if:, and --prefix "${{ vars.LAMBDA_DIST_PREFIX }}" to the distribute_zips.sh invocation. The prefix is gated, not defaulted: an unset prefix would aim the release at the mirror bucket's root, which is another organization's namespace and denied. (Since d6336ee the script also refuses that itself, so a missed gate fails loudly instead of 403-ing mid-release.)
   • deploy-prod — add the same vars.LAMBDA_DIST_PREFIX != '' gate (it already gates on every var it consumes), add DIST_PREFIX: ${{ vars.LAMBDA_DIST_PREFIX }} to the step env, and change the layer key to --layer-key "${DIST_PREFIX:+$DIST_PREFIX/}${MINOR}/lambda_layer_arm64.zip". All three edits are required together — test_prod_deploy_carries_every_destination_var_distribute_uses now fails if DIST_PREFIX is bound and the --layer-key line is left alone.
   • Then gh variable set LAMBDA_DIST_BUCKET --body "us-west-2.opendata.source.coop" and gh variable set LAMBDA_DIST_PREFIX --body "englacial/zagg/lambda" — only after the Source Cooperative grant lands. Until every variable exists, distribute (and with it deploy-prod) skips and the release still attaches zips to the GitHub Release, which is the behaviour issue Publish Lambda zips to source.coop: external users cannot stand up their own zagg fleet #497 asked to preserve.

   docs/deployment/benchmark-cicd.md §9 already documents the new grant; its §11 gh variable set block was left pointing at sliderule-public-cors on purpose, so the docs do not describe wiring that is not in the tree. It needs the two-line update when the patch lands.

6. A standing grant on a bucket being retired. DistributeZips keeps s3:PutObject/s3:GetObject on ${DistBucketName}/*. Once the variables flip to the mirror, nothing writes there — and issue Retire sliderule-public-cors: NASA buckets cannot host public data; migrate the sidecar cache to source.coop #499 is retiring that bucket. Not removed here: it is a scope change past phases 1-2, and it is still the live release path until the workflow half lands. Flagged so issue Retire sliderule-public-cors: NASA buckets cannot host public data; migrate the sidecar cache to source.coop #499's retirement PR inherits the pointer. (Thread left open on the diff.)

7. The adversarial self-review did not run the way CLAUDE.md §2 specifies. §2 requires a separate subagent with fresh context on an Opus-class model for review, and another for the fold. The first review/fold pass ran in the same context that wrote the diff, because no agent-spawning tool was available then. The second pass did run as separate review and fold subagents, and it found seven further issues in code the first pass had already reviewed and signed off — including one false factual claim in a comment (question (8)) and a test that passed under a mutation it was written to catch. That is the fresh-context rule earning its keep; treat anything only the first pass looked at as reviewed by its own author.

8. Two granted keys nothing can currently write. PublishToSourceCoop grants englacial/zagg/README.md and englacial/zagg/LICENSE. publish_mirror.sh is the only writer of those keys and sends no x-amz-acl: bucket-owner-full-control on any of its five PUTs, so under zagg-lambda-release it would 403 on its first PUT — two of the three granted resources are unreachable in practice, and question (2)'s option (a) does not work as written. The false comment asserting otherwise is corrected (7785845); the reachability is not, because publish_mirror.sh is a deployment/aws/ script issue Publish Lambda zips to source.coop: external users cannot stand up their own zagg fleet #497 does not name (CLAUDE.md §1), which makes it a scope change rather than a fold. (a) authorize adding --acl bucket-owner-full-control to publish_mirror.sh's five PUTs in a follow-up, keyed on $MIRROR_BUCKET the way distribute_zips.sh keys on $BUCKET so a self-hosted override still works; (b) drop the two repo-root keys from the grant until a writer sends the header; (c) accept them as write-once-by-hand under a human's own credentials. Recommendation: (a) — it is the only option that also unblocks question (2), and the mirror script is the natural owner of those keys.

Blocked externally, not on a PR

Issue #497 question (4) says the issue is blocked until Source Cooperative grants both ARNs at englacial/*. That is an external party, not an unmerged PR, so this PR carries waiting, not blocked. Phases 1-2 are our-side git-tracked IAM policy and CI wiring: they land regardless and are inert until that grant exists.

[espg]

🤖 from Claude (review)

Adversarial pass over a36e7e3..551746d against CLAUDE.md and issue #497's acceptance criteria.

Disclosure up front, because it changes how much this review is worth. CLAUDE.md §2 requires this pass to run as a separate subagent with fresh context on an Opus-class model. This run had no agent-spawning tool available, so the review was done in the same context that wrote the diff. That is exactly the blind spot the fresh-context rule exists to remove — treat the findings below as a checklist that was met, not as independent confirmation.

What holds up. The grant is the shape issue #497 asked for: the existing zagg-lambda-release extended rather than a new role, s3:PutObjectAcl travelling with s3:PutObject (issue #496), the multipart pair without s3:ListBucketMultipartUploads, and an unconditioned s3:ListBucket. englacial/zagg/benchmarks/* is correctly left out pending question (1). s3:DeleteObject being dropped relative to the fleet's grant is a real tightening and is justified — the mirror overwrites and never prunes.

The strongest thing in the diff is that test_release_role_reaches_exactly_the_mirror_keys derives the granted ARNs from publish_mirror.sh's own MIRROR_BUCKET/MIRROR_PREFIX shell defaults instead of restating them. That is the drift class that actually bites here.

Findings (3 inline):

1. distribute_zips.sh — the published-bucket list duplicates zagg.store._PUBLISHED_BUCKETS with no guard. Silent, release-time failure mode. Diff-scoped, fold.
2. tests/test_distribute_zips.py:139 — a trailing space in an f-string that reads as significant and isn't. Diff-scoped, fold.
3. benchmark_cicd.yaml:311 — DistributeZips becomes a standing grant on a retired bucket. Not diff-scoped; left standing for review (belongs with issue #499).

Two things a fresh reviewer should push on, which this pass cannot settle:

• The workflow half of phase 2 is not in this branch. .github/workflows/publish.yml was written and tested locally, then reverted: pushing it is refused at both the git layer and the REST layer (refusing to allow an OAuth App to create or update workflow .github/workflows/publish.yml without workflow scope). What landed is the script half — distribute_zips.sh gained --prefix and the conditional ACL, backward-compatibly. The result is that the two publish.yml tests originally written to pin the wiring were replaced with tests that pin the coupling (deploy-prod gates on everything distribute does; it carries every LAMBDA_DIST_* var distribute uses) — those hold before and after the wiring lands. That substitution is defensible, but it is a substitution, and a reviewer should confirm it is the one they want rather than a red CI that says the truth.
• deploy-prod sources its layer from whatever distribute staged. That coupling is easy to miss and is the reason the workflow patch touches deploy-prod at all. Whether publish-layer-version can read the layer zip out of a cross-account Source Cooperative bucket is a live-AWS fact this run cannot check, and it is the same unknown phase 4 rests on.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Once LAMBDA_DIST_BUCKET/LAMBDA_DIST_PREFIX point at the mirror, DistributeZips grants write on a bucket nothing writes to any more — and on a bucket that issue #499 is retiring precisely because it cannot host public data. A standing s3:PutObject on a retired NASA bucket is the kind of leftover that outlives the reason for it.

Not folding this: removing a grant is a scope change beyond issue #497 phases 1-2 and belongs with issue #499's retirement, not here. Flagging it so the retirement PR has the pointer rather than rediscovering it.

---

Generated by Claude Code

[espg]

🤖 from Claude

Left by design, and left open rather than resolved so it stays visible for the retirement decision.

Removing DistributeZips is a scope change past issue #497 phases 1-2, and it would also be premature: the grant is still live until the repo variables are flipped to the mirror, and until phase 2's publish.yml half lands (see the run notes on the PR thread) distribute still writes to sliderule-public-cors. Dropping it now would break the release path that exists today in order to tidy one that does not yet.

Recorded under "Questions for review" so issue #499's retirement PR inherits the pointer instead of rediscovering it.

---

Generated by Claude Code

[espg]

🤖 from Claude

Run notes — phases 1 and 2 are in; waiting applied. Three things need a decision, and two of them are about what this run could not do, so flagging them here rather than only in the body.

1. .github/workflows/publish.yml could not be pushed. Refused at both layers:

! [remote rejected] claude/497-source-coop-publish -> claude/497-source-coop-publish
  (refusing to allow an OAuth App to create or update workflow
   `.github/workflows/publish.yml` without `workflow` scope)

The REST contents API returns the same 403. So phase 2 landed as its script half only: distribute_zips.sh gained --prefix and the destination-keyed bucket-owner-full-control ACL, backward-compatibly. The exact three-part patch for publish.yml (plus the two gh variable set calls, to be run only after the Source Cooperative grant lands) is question (5) in the body.

Because the wiring isn't in the tree, the two tests originally written to pin the workflow literals were replaced with tests that pin the coupling — deploy-prod gates on everything distribute gates on, and carries every LAMBDA_DIST_* variable distribute uses. Those hold before and after, and they guard what actually bites: deploy-prod publish-layer-versions straight out of what distribute staged, so retargeting distribute alone would silently aim it at the old destination. docs/deployment/benchmark-cicd.md §11's gh variable set block was deliberately left pointing at sliderule-public-cors so the docs don't describe wiring that isn't there.

2. The adversarial self-review did not run the way CLAUDE.md §2 specifies. §2 wants a separate subagent with fresh context on an Opus-class model for the review, and another for the fold. No agent-spawning tool was available, so both ran in the same context that wrote the diff. Findings and folds are real (two folded — f2f7a9d, 91b2037; one left standing on its thread), but they are not independent confirmation, which is the whole point of the fresh-context rule. The IAM statement is where a same-context reviewer is least likely to catch its own assumption, so that is the paragraph worth a human eye.

3. Two pre-existing lint failures on main, flagged not fixed (CLAUDE.md §4) — both reproduce at 8dbb2fe: N818 on src/zagg/registry.py:64 (UnknownCapability), and ruff format --check wanting to reformat a fenced Python block in tests/data/benchmark/README.md:176.

Nothing in this branch touches live AWS, and nothing goes live on merge: the IAM change needs an account admin to re-apply the stack, and distribute stays skipped until the repo variables exist.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

Independent adversarial review, fresh context — I did not write this diff. I read CLAUDE.md, issue #497 in full, the three existing threads (so I am not re-raising the two folded findings), and then checked the claims against the scripts rather than the description: publish_mirror.sh, stand_up.sh's DIST_PREFIX/dist_root(), template.yaml's ExecutionRole grant, publish.yml as it stands on main, and zagg.store._PUBLISHED_BUCKETS. I also ran the suite and mutated it.

The IAM statement holds up. I verified rather than took on faith: the two repo-root ARNs are genuinely the keys publish_mirror.sh writes (REPO_DEST="s3://$MIRROR_BUCKET/${MIRROR_PREFIX%/lambda}" → englacial/zagg/{README.md,LICENSE}); DeleteObject is correctly absent (no path in distribute_zips.sh, publish_mirror.sh or stand_up.sh deletes anything at the destination); benchmarks/* is correctly absent per question (1); ListBucketMultipartUploads is correctly absent; the unconditioned ListBucket rationale is sound S3 semantics and matches the sibling grant's. Given question (7)'s disclosure that the self-review ran in the same context that wrote the diff, this was the part I scrutinised hardest, and I did not find an error in the policy itself.

The seam is where the problems are. Seven findings, in descending order of consequence:

1. benchmark_cicd.yaml:328 — the comment asserts something half its writers do not do. "Every write here carries x-amz-acl: bucket-owner-full-control" is false for publish_mirror.sh, which sends no ACL on any of its five PUTs — and which is the only writer of the two repo-root keys this statement grants. It also means question (2)'s recommended backfill route (./publish_mirror.sh MINOR --run RUN_ID) fails on its first PUT, and never writes the versions.json that phase 3 asks for. Flagged for a human ruling, not fixed by direction: publish_mirror.sh is a deployment/aws/ script that issue #497 does not name in its authorization list (§1), even though phase 2's prose mentions it.
2. distribute_zips.sh:39 — the invariant that keeps a release out of another org's bucket root lives only in the file that could not be pushed. Three lines in the script make the half-landed state fail-safe and loud instead of a bare AccessDenied on a tag build.
3. tests/test_distribute_zips.py:238 — the coupling guards do not catch the divergence they were written for. I applied question (5)'s patch minus the --layer-key line: both tests stay green while publish-layer-version points at the un-prefixed key. They pin that the variable is declared in deploy-prod's env, not that it is used.
4. distribute_zips.sh:78 — the index still truncates on any read failure, not only the 403 the IAM comment addresses; 2>/dev/null also deletes the evidence. Plus a note that the 404-vs-403 property is cross-account and the two YAML comments reason about Source Cooperative's policy in opposite directions.
5. distribute_zips.sh:9 — present tense for a destination that has not moved. Same in the test module docstring. The PR body is careful about this distinction; the source comments should be.
6. benchmark_cicd.yaml:312 — §4 terseness. ~45 lines of comment for 16 lines of policy, most of it a near-verbatim second copy of template.yaml's. Six copies of one rationale now exist in the tree; they have already started to diverge.
7. tests/test_deploy_lambda.py:685 — the "sweeps all three roles" guards sweep a typed list, not what the stack provisions.

On the shipped default and backward compatibility: the claim is true. With no --prefix and sliderule-public-cors, BASE collapses to today's string, $ACL stays empty, and the six aws s3 cp invocations are byte-identical to the current ones. A release cut today behaves exactly as before. I confirmed the ACL and bucket-list pins are real by mutation: dropping --acl from a single upload, and adding one bucket to PUBLISHED_BUCKETS, each fail the intended test. Test quality is good — finding 3 is the exception, and it is the one that matters most because it guards the half that could not land.

Convention check: Closes #497 present; commit subjects are title-only and match the log's style; no 🤖 in the description or commits; no new dependency (pyyaml is already a core dep); docs/deployment/benchmark-cicd.md is consistent with the tree, including the deliberate choice to leave §11's gh variable set block pointing at the old bucket; both touched test modules are well under the module-size limit. One small process note, not worth its own thread: §2 asks for one commit per phase and phase 2 landed as two (f6ff6f2, 551746d) — understandable given the revert, and the PR body explains it.

Bottom line — do phases 1-2 meet #497's acceptance criteria given the workflow half could not land?

Phase 1: yes, with finding 1 outstanding. The grant is the right shape, correctly scoped, and correctly refuses to pre-empt question (1). But two of its three resources are unreachable by the only script that writes them until the ACL question is settled, so "phase 1 is done" is true of the policy and not yet true of the capability the policy exists to provide.

Phase 2: partially, and the remainder is not merely the workflow file. The script half is correct and inert, which is the right outcome. What is missing beyond the un-pushable patch is that the safety property phase 2 depends on — a published destination is never written without a prefix — currently exists in no landed artifact: not in the script (finding 2) and not in the tests (finding 3). The PR is candid that the workflow half is absent, but it reads as though the tests compensate, and they do not. I would land findings 2 and 3 before calling phase 2 complete, since both are inside this PR's authorized files and both survive the workflow half's absence.

Nothing here is a merge blocker in the "this will break production" sense — the diff is inert by construction and I could not make a release cut today behave differently. Findings 1 and 3 are the two I would not want discovered at release time on a tag, after PyPI has published.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

"every write here carries x-amz-acl: bucket-owner-full-control" is not true of the script this statement was scoped from. deployment/aws/publish_mirror.sh sends no ACL on any PUT:

aws s3 cp "$z" "$DEST/$(basename "$z")" --region "$MIRROR_REGION"
aws s3 cp "$STAGE/SHA256SUMS" "$DEST/SHA256SUMS" --region "$MIRROR_REGION" --content-type text/plain
aws s3 cp "$STAGE/README.md"  "$DEST/README.md"  --region "$MIRROR_REGION" --content-type text/markdown
...
aws s3 cp "$REPO_TOP/LICENSE" "$REPO_DEST/LICENSE" --region "$MIRROR_REGION" --content-type text/plain
aws s3 cp "$WORK/REPO_README.md" "$REPO_DEST/README.md" --region "$MIRROR_REGION" --content-type text/markdown

Three consequences, and the third is the one that bites:

1. The comment asserts a property of the code that half the writers do not have. distribute_zips.sh carries the header; publish_mirror.sh does not.
2. Lines 361-362 grant englacial/zagg/README.md and englacial/zagg/LICENSE, and publish_mirror.sh is the only writer of those two keys — distribute_zips.sh never writes them. So the two keys this grant enumerates can only be reached by a script that, by this PR's own premise (issue In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #496), gets AccessDenied on its first PUT under this role. The grant is not wrong, but it is currently unusable for two of its three resources.
3. It undercuts question (2)'s recommended backfill. ./publish_mirror.sh MINOR --run RUN_ID under zagg-lambda-release fails on the first zip. (It also never writes versions.json, which issue Publish Lambda zips to source.coop: external users cannot stand up their own zagg fleet #497 phase 3 asks for explicitly — so option (a) as written does not satisfy phase 3 either.)

The historical mirror worked without the header because it ran on source.coop's own retired direct-STS credentials, where no cross-account object handover was involved. Under the OIDC role it is a different code path.

What I would do: either add --acl bucket-owner-full-control to publish_mirror.sh's five PUTs (keyed on $MIRROR_BUCKET the same way distribute_zips.sh keys on $BUCKET, so a self-hosted MIRROR_BUCKET override still works), or narrow the comment to say only distribute_zips.sh carries the header today and record that publish_mirror.sh needs the same treatment before it is run against this destination.

This one needs a human ruling, not a fix from me: publish_mirror.sh is a deployment/aws/ script and issue #497 names benchmark_cicd.yaml, .github/workflows/publish.yml and .github/scripts/distribute_zips.sh — not publish_mirror.sh. CLAUDE.md §1 says infra scripts are only touched when the issue names them. Phase 2's text does mention folding in publish_mirror.sh's layout, so the authorization is arguably there, but it should be said out loud rather than assumed.

---

Generated by Claude Code

[espg]

🤖 from Claude

Verified against the source, and it splits in two. Left open — the half that matters needs a ruling, not a fold.

Corrected (7785845): the false sentence. publish_mirror.sh sends no ACL on any of its five aws s3 cp PUTs, so "every write here carries x-amz-acl: bucket-owner-full-control" was untrue of half the writers. The comment now describes what actually happens:

NOT every writer sends it today: distribute_zips.sh does, for a published destination only; publish_mirror.sh sends no ACL on any of its five PUTs and is the ONLY writer of the two repo-root keys below, so those two are granted but not reachable under this role until that changes.

The same claim was in test_published_grant_carries_the_acl_and_multipart_halves's comment and is corrected there too. (That text was trimmed with the rest of the block in b3b849e; the correction survives it.)

Not fixed: the reachability itself. Adding --acl bucket-owner-full-control to publish_mirror.sh's five PUTs means editing a deployment/aws/ script, and issue #497 names benchmark_cicd.yaml, .github/workflows/publish.yml and .github/scripts/distribute_zips.sh — not that file. CLAUDE.md §1 makes that a scope change, and the fold folds findings rather than widening scope. Two of the three granted resources stay unreachable in practice, and question (2)'s backfill route is undercut, until it is ruled on.

Recorded as question (8) in the PR body with three options: (a) authorize the ACL in publish_mirror.sh as a follow-up (keyed on $MIRROR_BUCKET so a self-hosted override still works), (b) drop the two repo-root keys from the grant until a writer sends the header, (c) accept them as write-once-by-hand. Recommendation is (a) — it is the only one that also unblocks question (2)'s option (a), and the grant already anticipates the keys.

Leaving this thread open rather than resolving it.

---

Generated by Claude Code

[espg]

🤖 from Claude

Follow-up: the same false sentence had also reached docs/deployment/benchmark-cicd.md §9 in this PR's diff ("every write carries x-amz-acl: bucket-owner-full-control"). Corrected in 01ecc21 to say which writer sends the header and that the two repo-root keys are consequently unreachable under this role — so all three copies the PR introduced now describe what the code actually does. The stale "would reseed versions.json" line in the same paragraph is corrected there too, since the index read no longer works that way.

The scope question itself is unchanged and still open above; publish_mirror.sh is untouched.

---

Generated by Claude Code