In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs

[espg]

Closes #495. Closes #501.

Source Cooperative granted the in-region write path (their Option 3): we hand them an IAM role ARN, they add it to the bucket policy on us-west-2.opendata.source.coop, and the fleet writes natively instead of through the data.source.coop Cloudflare proxy. That retires the egress the CA campaign paid — $31.22 for 346.85 GiB, against $29.00 of Lambda compute for the same run.

Phases

• Phase 3 — the execution role becomes zagg's published identity (revised 2026-08-20 after espg's ruling; the first cut of this phase, an assumable source-coop-upload role with injected credentials, is removed by this commit)
• Phase 1 — hand object ownership to the bucket owner on every external write
• Phase 2 — prove WRITE permission in the mode="ping" preflight (question (3) of the original issue, ruled in scope)
• Phase 4 — the lifecycle touch must not run against a published bucket (added 2026-08-20; time-sensitive, the NEON AOP campaign includes California sites)
• Phase 5 — output.touch: auto | always | never (issue output.touch policy: declare the lifecycle-touch behaviour in the template instead of inferring it from the bucket name #501; the operator declares the policy instead of zagg inferring it from the bucket name)

Each phase got a fresh-context adversarial self-review; findings are folded one commit each or answered on their thread.

Phase 3 (revised) — the execution role becomes the published identity

espg rewrote #495 on 2026-08-20: injecting credentials for source.coop writes is the wrong shape. zagg's data model is communal — one datacube, written and appended by many — and the execution role already embodies that for sliderule-public-cors (whole-bucket Get/Put/Delete, by explicit intent, PR #176). Publishing to Source Cooperative is the same policy pointed at a durable public destination; the divide between the two is store maturity, not identity. So level 3 is two more statements on the execution role, not a subsystem — which also removes the assumable role, the sts:AssumeRole, the injected output_credentials for this path, and with them the one-hour role-chaining ceiling (#498 dissolves for the fleet path; not closing it here).

Injection is not removed: it stays load-bearing for source reads and remains the escape hatch for un-negotiated targets — a collaborator's private bucket, R2/MinIO — where no bucket-policy grant is possible.

The issue authorizes editing template.yaml, stand_up.sh, execution_role.yaml, EXECUTION_ROLE.md, docs/deployment/execution-role.md and docs/deployment/standup.md. No live-AWS command was run; the stack update is an operator action.

(a) The SourceCoopUploadRole and SourceCoopPublisherPrincipal are gone, along with SourceCoopUploadRoleName, the ShouldCreateSourceCoopRole condition and the role's Output.

(b) The execution role gets a stable name via a new ExecutionRoleName parameter, default zagg-lambda-execution (espg's ruling), mirroring how benchmark_cicd.yaml parameterizes ReleaseRoleName / BenchmarkInvokeRoleName. It is a parameter, not a literal, because zagg-backend-test is a second stack from this same template and a fixed name collides on CREATE — so stand_up.sh gained an EXECUTION_ROLE_NAME env var and passes it through, plus a guard (review fold) that refuses a non-default STACK_NAME still carrying the default role name, since that failure is otherwise live-AWS-only. The name matters because Source Cooperative names this ARN in their bucket policy, which makes a CloudFormation-generated name (zagg-backend-ExecutionRole-Bh9A4eBsB1Nq) a contract that a stack replacement would silently break.

Adding RoleName to the already-deployed unnamed role forces a replacement — CFN creates the new role, repoints the five Role: references, then deletes the old one at cleanup. That is an operational event, now documented in standup.md: run it against an idle fleet, because a warm sandbox holds credentials vended by the old role and loses them mid-shard when cleanup lands.

(c) The CreateExecutionRole / ExecutionRoleArn backdoor is deleted — espg's ruling: a hatch for users without iam:CreateRole that never worked and nothing used; the supported posture is "an admin stands up the template". Removed: both parameters, the ShouldCreateRole condition, the Condition: on the resource, all five !If sites plus the Output expression, the header comment, the CREATE_ROLE/ROLE_ARN variables and their validation block in stand_up.sh, and the files deployment/aws/execution_role.yaml, deployment/aws/EXECUTION_ROLE.md, docs/deployment/execution-role.md. Doing (b) and (c) in one commit means one role replacement rather than two.

Three reference sites the issue did not enumerate had to follow, or the docs build breaks and the prose lies: the mkdocs.yml nav entry for the deleted page, the "Execution Role" links in docs/deployment/lambda.md and deployment/LAMBDA_DEPLOYMENT.md, and a mirrors execution_role.yaml aside in docs/deployment/benchmark-cicd.md.

(d) Two source.coop statements on the role's logs-and-output-s3 policy, shaped like the sliderule-public-cors block above them:

• Get/Put/DeleteObject + PutObjectAcl + AbortMultipartUpload + ListMultipartUploadParts on englacial/zagg/demo/*, the prefix the fleet publishes to and the only one this role reaches. englacial/zagg/lambda/* and englacial/zagg/benchmarks/* stay out — CI release role, Publish Lambda zips to source.coop: external users cannot stand up their own zagg fleet #497 — and an englacial/zagg/index/* grant for the sidecar index cache (Virtual index entry point: pluggable chunk-index backends for the read path #160, migrating off the retiring sliderule-public-cors, Retire sliderule-public-cors: NASA buckets cannot host public data; migrate the sidecar cache to source.coop #499) was added and then reverted: that cache is moving to a different bucket under a different org, post-MVP, so the prefix was never the destination. Starting narrow is cheap: Source Cooperative grants englacial/* to both ARNs, so every later change is a reviewed edit to our own git-tracked IAM policy rather than another email. DeleteObject is deliberate (overwrite and manifest cleanup need it) and safe (the bucket is versioned, so a delete leaves a marker). PutObjectAcl is required by S3 for any PUT or CreateMultipartUpload carrying x-amz-acl — without it the canned ACL turns every published PUT into AccessDenied (review fold). The multipart pair covers obstore's in-process abort, which holds the UploadId.
• Unconditioned s3:ListBucket on the bucket, and nothing else. This PR's own earlier review established why the condition has to go: a GetObject evaluation carries no s3:prefix context key, so an s3:prefix condition never matches there and S3 returns 403 instead of 404 for absent keys — which ~9 absence checks in the tree do not handle. ListBucketMultipartUploads was granted here at one point and is now deliberately not (espg, 2026-08-20): uploads leaked by a worker killed at the 900 s ceiling are real, but that action lists in-progress uploads bucket-wide and s3:prefix cannot constrain it, so Source Cooperative's data-upload docs omit it and their bucket policy will not grant it — holding it on our side would be denied cross-account regardless. Their 7-day lifecycle reaper is the mechanism; the comment in the template says so rather than claiming discoverability it does not have.

Phase 1 had to follow, or phase 3 would have silently defeated it (the review's one blocking finding). The canned ACL was keyed on how the credentials arrived — explicit credentials, no endpoint_url — and phase 3 makes publishing ambient, so credentials is None and no header was sent at all: every published object would have landed owned by 742127912612, exactly what Source Cooperative's Step 3 exists to prevent, and silently, because the PUT succeeds. The predicate now keys on the destination as well: a small _PUBLISHED_BUCKETS constant in src/zagg/store.py holding us-west-2.opendata.source.coop, a fixed external fact of the same class as the literal bucket ARNs in the template. Deliberately not "send it on every AWS-endpoint write": the header requires s3:PutObjectAcl on the target, which zagg holds on this bucket alone, so sending it everywhere would 403 every self-hoster's own output bucket and sliderule-public-cors, whose bucket policy is not ours to change. zagg.lifecycle's CopyObject self-touch takes the destination the same way, so a skip run cannot strip ownership either.

The docs were rewritten to match: standup.md no longer documents a CreateExecutionRole=false path and its "external stores don't go through the execution role" note now says the opposite for source.coop; lambda.md's output-credentials section now describes injection as the un-negotiated-target escape hatch.

Phase 1 — the canned ACL

Answering question (1) of #495: automatic derivation, no output.acl knob, and it lives in src/zagg/store.py::_s3_object_store rather than being threaded through _output_store_kwargs. Explicit credentials + no endpoint_url + not read_only ⇒ a write target this account does not own ⇒ the store sends x-amz-acl: bucket-owner-full-control via obstore's client_options={"default_headers": …}.

Reading the code made the store-level seam clearly better than the event-level threading the issue sketched, for one concrete reason: _output_store_kwargs is not the only route to an external write. open_object_store carries the side-channel writes (status envelopes, hive manifests, stats sidecars, leaf sub-maps), and zagg.output.tabular.write_tabular — reached from runner.py::_write_tabular_output on the driver, not a worker — puts the temporal parquet with credentials=output_credentials directly. Both share _s3_object_store, so one seam covers the worker path, the dispatcher path, the side channel, and a notebook calling open_store by hand; threading the header through the two _output_store_kwargs functions would have missed the tabular write entirely. So yes — open_object_store needs the header, and it gets it from the same place.

Folded from the phase-1 review:

• src/zagg/lifecycle.py was a real hole (the review's one HIGH). Its lifecycle self-touch issues a boto3 CopyObject, which creates an object — outside the store seam, with the same output credentials — so on a cross-account target every touched object was re-created owned by our account under the requester's default private ACL, silently clawing back the ownership the writing PUT handed over (and the touch is fail-open, so nothing would have said so). It is reachable worker-side on every skipped unit. The copy now carries the canned ACL, deriving the predicate and the value from zagg.store so the two sites cannot drift.
• Read-only stores no longer send it. read_only is threaded into _s3_object_store (never forwarded to S3Store), so the issue Temporal worker applies source s3_credentials to consumer-owned mask/static reads #223 consumer-input channel — temporal.open_dataset opening somebody else's input bucket with explicit credentials — is excluded. open_object_store has no read-only concept and remains a documented exception (inert: S3 interprets x-amz-acl only on object-creating requests).
• Header keys are lowercased before the merge, so a caller's mixed-case X-Amz-Acl actually wins instead of silently losing to ours inside obstore.
• An escape hatch without a config knob: passing {"x-amz-acl": None} strips the header, for a future external AWS target that must not set one (neither obstore-legal value can express absence).
• The comment now describes the real mechanism: default_headers is applied on the reqwest client after signing, so the header rides outside the signature and is accepted because S3 ignores unsigned non-required x-amz-* on header-auth requests. It rides CreateMultipartUpload, which is the request that sets a multipart object's ACL — the load-bearing one at ~131 MB/shard.

Custom-endpoint targets are excluded: canned ACLs are an AWS-S3 concept R2/MinIO do not implement, and an endpoint-routed AWS target is the retired data.source.coop proxy this path replaces.

Phase 2 — the write probe

mode="ping" ran the read-only validate_manifest, so it proved reachability and store identity, never write permission. The first real write is ensure_manifest in mode="setup", invoked InvocationType="Event" — its 500 never reaches the dispatcher — and per-shard status writes are deliberately fail-open (#327). A read-only-but-valid grant therefore burned the whole fan-out before failing.

That is not hypothetical here: Source Cooperative's Option 3 involves no source.coop-vended credentials at all — our own IAM role writes cross-account through their bucket policy — so there is no interactive credential step where a human would notice a misconfigured grant. It fails silently at the first PUT.

_probe_output_write PUTs a zero-byte object and DELETEs it, two requests per run, s3:// stores only:

• Under <store>.status/probe-<uuid> — the run's own async-result sibling (review fold; it was a .probe prefix of its own at first). Two properties, in order: never inside the store root, because docs/specification.md §5.2 makes the leaf hash set discovery-based, so an object stranded by a denied DELETE would be a key-set difference and a verifier would report an intact leaf as tampered; and never a new grant surface, because the probe is fail-closed and whatever prefix it writes becomes a precondition for the run to start — .status is already required writable by the async invoke/poll transport, so a grant covering <store>/* + <store>.status/* passes the probe exactly when the run's real writes would succeed. The uuid keeps concurrent runs from colliding.
• It rides the same store kwargs as the real writes, so it carries the phase-1 canned ACL — an ACL-rejecting bucket is one of the failures this surfaces early.
• The PUT is fail-closed. _handle_ping still runs RequestResponse and returns 500, tagged "check": "write_probe"; _invoke_lambda_ping turns that tag into a grant-oriented remedy instead of the existing "clear the store root" message, which would send an operator to fix the wrong thing. Credential-shape errors are resolved read-side now (the fold hoisted _output_store_kwargs out of the probe), so a malformed output_credentials block keeps the read-side tag rather than being mislabelled a grant failure, and the message states a denied grant as the likely cause rather than a determination. A pre-In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #495 function omits the tag and the old message still applies.
• The DELETE is fail-open but reported (probe_delete + probe_key in the 200 body, plus a dispatcher warning): s3:DeleteObject is not optional for zagg's real writes (store overwrite, manifest cleanup), so a Put-but-no-Delete grant must not pass silently and strand an object per run.
• Coverage, stated honestly: the PUT proves s3:PutObject, and one small PUT is representative of multipart (CreateMultipartUpload/UploadPart/CompleteMultipartUpload are all authorized by s3:PutObject), but it cannot exercise s3:AbortMultipartUpload/s3:ListMultipartUploadParts, which the phase-3 grant carries deliberately.

Operator documentation lives in docs/deployment/lambda.md (where phase 1's output-credentials guidance is), since the probe is not hive-specific — it runs on every s3:// ping, raster included; docs/hive_layout.md keeps a cross-reference.

Phase 4 — the lifecycle touch skips published buckets

The skip-run touch (#388) refreshes LastModified with a CopyObject self-copy so a bucket expiration rule cannot delete data a run just certified current. On Source Cooperative that is actively harmful, for a reason lifecycle.py's own docstring already named: "a versioned bucket mints a new object version per touch." Their bucket is versioned, so the self-copy does not refresh a timestamp in place — it writes a new full-size version and demotes the old one to noncurrent, where it keeps consuming storage.

A single full-skip run over the CA store adds roughly 332 GB of noncurrent versions, doubling the footprint, on a bucket where AWS pays the storage bill as an Open Data sponsor; every later skip run adds another 332 GB. It is invisible from outside, because ListObjectsV2 reports only current versions. And it buys nothing: the touch exists to defeat an expiration rule, which sliderule-public has (clear_old_objects, 30 days) and an archival published bucket does not.

The guard is bucket in _PUBLISHED_BUCKETS, deliberately not _external_target(...). The two differ and the distinction is load-bearing: _external_target is also true for injected-credential targets, whose lifecycle and versioning configuration we do not know — skipping the touch on one of those could let a collaborator's data expire, which is the exact failure the touch exists to prevent. Only the published set is known not to expire. The code says so, and also says that the set is doing double duty: it enumerates buckets that are both not ours (what the canned ACL keys on) and not expiring (what this skip keys on), two properties that coincide today rather than being the same thing — if they ever diverge, the touch needs its own set.

The skip reads as "not applicable", never as a failure. Skipped paths are counted under a "skipped" key added only when non-zero, and never touch "failed" — so a published run reports touched_objects: 0, touch_failed: 0 in the unit records and status objects, which cannot be read as an error in the run parquet. The key is omitted when zero so every existing caller's dict stays byte-identical, mirroring how the unit records already omit touched_objects entirely when no touch ran. One INFO line per call says why.

One pre-existing test had to be replaced rather than kept: test_ambient_published_target_copies_carry_the_acl pinned that the published self-copy carries the bucket-owner ACL — phase 4 makes that copy not happen at all, so the case is moot. The ACL-on-copy behavior is still pinned for the paths that do still copy (injected-credential external targets).

Follow-up, deliberately not implemented here: the underlying signal — when data was last requested for reprocessing — is worth keeping. It belongs in the run's stats/manifest, where it is one small queryable object rather than 141,367 full-object copies inferred from S3 timestamps. Wants its own issue.

Phase 5 — output.touch, declared rather than inferred (#501)

Phase 4's guard is correct for the two destinations we know about, but the property it actually needs is "does this bucket expire objects?" — which zagg cannot read cross-account (s3:GetLifecycleConfiguration is bucket-owner only) and which no bucket name reliably encodes. The operator knows, so #501 makes it declarable, alongside output.store:

value	behaviour
auto (default)	Phase 4 verbatim — touch unless the destination is in _PUBLISHED_BUCKETS. A pure no-op for every config predating the knob.
always	Touch regardless of destination. For an un-negotiated external target whose expiry rule we know about but whose name says nothing.
never	Never touch, local paths included — "never touch" is a statement about the run, not about S3.

An override layered on top of the inference: phase 4 is auto's implementation, not something redone. Phase 4 stayed its own commit and landed first, so the urgent guard is isolated in history and can be split out if the knob runs into trouble.

One thing that had to follow, and would have been a silent bug otherwise: phase 4's review added a defence-in-depth guard inside _touch_s3_object, the seam that issues CopyObject. That seam was written against _skip_published directly, so a policy-blind seam would have made always a no-op on exactly the destination it exists for — the call site would allow the copy and the seam would refuse it. The seam now resolves the same policy, and a test asserts always reaches the copy rather than merely passing the call-site guard.

The semantic core is not a problem here, and #501 explains why: output is allowlist-shaped — the core takes only OUTPUT_LEAF_SHAPING_KEYS (aoi_mask, windowing, time_source) plus the output.grid keys, with output.pyramid as the recorded precedent. A new output key is therefore excluded by construction: no exclusion contract, no hash-version migration, no dependency on #373. output.touch was deliberately not added to OUTPUT_LEAF_SHAPING_KEYS — that would have been the mistake. Since the property holds only by omission, tests/test_semantics.py::test_packaging_knobs_never_change_hash now pins both non-default values as a regression guard.

Validation lives in _validate_store_layout_keys (reached by both the point-pipeline and raster branches), so touch: nevr fails at submission rather than resolving to the default deep inside a worker's skip path — where the wrong answer is either version churn on a published store or a collaborator's data expiring.

The staged sweep was a hole, found by review and fixed: I had written that "sweep entry points carry no config", which is true of the python -m zagg.sweep --stages CLI but false of the caller that actually fires in a run — runner.py reads config.output["sweep"] and calls stage_sweep_after_run on the next line. So run_finisher ran at its auto default and self-copied {root}/aggregation.yaml on a destination the operator had declared never for. touch_policy now threads stage_sweep_after_run → run_stage_sweep → run_finisher, with the CLI path unchanged at auto.

The other consequential fold: _copy_acl's docstring claimed (correctly, under phase 4) that its bucket argument could no longer change its answer. always makes the bucket arm of _external_target live again — it is the only arm that can fire for an ambient published write — so acting on that docstring and deleting the argument would have silently stripped bucket-owner ownership from every object an always run self-copies to Source Cooperative. The docstring now says so, and the test asserts the ACL on the copy rather than merely that a copy happened.

Testing

Local: ruff check src tests / ruff format --check src tests clean on everything this PR touches, and the full pytest run shows no new failures — the 22 failures in this working copy are pre-existing (test_processing, test_spill*, test_spec_conformance, test_stats_toc, test_read_vlen, test_time_axis) and come from an older mortie in the local venv, not from this branch; test_lambda_build.py::test_function_build_succeeds pip-installs and needs network. CI is the authority on all of those.

New coverage:

• tests/test_store.py::TestBucketOwnerAcl — header present on the external-target path (both open_store and open_object_store) and on an ambient write to the published bucket, absent on ambient writes to our own buckets, on the anonymous skip_signature path, on custom endpoints, and on credentialed read-only stores; caller client_options preserved, mixed-case caller header wins, None strips, caller dict not mutated; plus two unmocked obstore constructions that pin what obstore actually accepts (default_headers["x-amz-acl"] == b"bucket-owner-full-control"), which the mocked tests structurally cannot.
• tests/test_lifecycle.py::TestTouchS3 — (phase 5) all three output.touch values against both a published and a non-published destination, always reaching the copy seam (and carrying the canned ACL there) rather than just the call-site guard, and never covering local paths; six wiring pins — one per policy=/touch_policy= call site, each mutation-checked by deleting its kwarg — so output.touch policy: declare the lifecycle-touch behaviour in the template instead of inferring it from the bucket name #501's actual claim, that the operator's declaration governs the touch, cannot regress silently; plus tests/test_config.py::TestTouchPolicy (default, all three values, rejection of anything else) and the semantic_hash regression guard. The self-copy carries the ACL for an external target and not for an ambient or custom-endpoint one, fail-open unchanged; and (phase 4) a published bucket gets no LIST, no HEAD and no CopyObject while reporting failed: 0, an in-account path still copies, and an injected-credential path to a non-published bucket still copies — the last one pins that the guard is the bucket check and not _external_target.
• tests/test_lambda_handler.py — event → _output_store_kwargs → open_store actually lands the header (and does not for an execution-role event); the probe PUTs+DELETEs under the .status sibling with the run's credentials and ACL-bearing kwargs, keys are unique per run, a denied PUT is a 500 tagged write_probe, a failed DELETE still passes but reports, a malformed credentials block on a non-hive ping is not tagged write_probe, the read-side refusal carries no tag, and a local store is not probed.
• tests/test_hive.py::TestInvokeLambdaPingEvent — the dispatcher's write-probe refusal names the grant, not the store root.
• tests/test_lambda_build.py::TestTemplateEnvironment — the execution role reaches exactly englacial/zagg/demo/* with the full published action set, reaches nothing else under the bucket, and holds unconditioned ListBucket and nothing else at bucket level (with the 404 reason attached); no SourceCoop* resource or parameter survives; ExecutionRoleName is a parameter with the stable default, reachable from stand_up.sh, with CAPABILITY_NAMED_IAM pinned; the backdoor is gone from the template, from stand_up.sh and from disk (including the mkdocs nav, whose dangling entry would break the docs build); every Lambda function — base, -extract, and the Fn::ForEach worker variants, pinned at exactly five — takes its Role straight from the role resource; the role's permissions stay inline-only (ManagedPolicyArns absent, so the assertions can see everything it grants); and the one documented second-stack standup passes EXECUTION_ROLE_NAME.
• tests/test_raster_runner.py — the lambda/local parity test drives the real handler with s3:// paths remapped onto tmp_path; the remap now covers the probe's open_object_store binding too, which is what caught this in CI rather than in production.

Template validation is local-only: it parses through the same CFN loader the suite already uses, and the new parameters/resource/output resolve. No validate-template, no change set, no deploy.

Questions for review

1. All three questions on In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #495 are ruled and implemented — role name zagg-lambda-execution (bare), fleet prefix englacial/zagg/demo/*, and GEDI waits for this change. Nothing here is blocked on a decision.
2. One observation carried rather than acted on: sliderule-public-cors has no bucket-level s3:ListBucket statement at all. The 403-vs-404 argument that forced the unconditioned ListBucket on source.coop applies to it identically, so this looks like a live gap on the staging path — but it is a grant change outside In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #495's scope and @espg's call. Worth its own issue if confirmed; not widened here.
3. Confirm with Source Cooperative that their side grants s3:ListBucket unconditionally. Ours is unconditioned now, but cross-account needs both sides to allow.
4. output_credentials are injected once and never refreshed: role chaining caps writes at 1 hour, failing the tail of a long campaign #498 dissolves for the fleet path — no assumable role means no sts:AssumeRole, no injected credentials for this target, and no one-hour role-chaining ceiling. Left open rather than closed here, since that is @espg's call and injection still exists for un-negotiated targets.
5. Pre-existing, untouched, flagged not fixed: pre-commit run --all-files fails check-yaml on deployment/aws/template.yaml on main too (the hook cannot parse CloudFormation short-form intrinsics); ruff check src tests reports N818 on src/zagg/registry.py:64 with the newer local ruff (0.15.16) while CI's pinned v0.14.10 does not; codespell/mypy have standing hits in files this PR does not touch; and tests/test_lambda_build.py::test_function_build_succeeds pip-installs, so it needs network.

[espg]

🤖 from Claude (review)

HIGH — the s3:prefix condition on ListBucket turns every "object not there yet" GET into a 403, and zagg only catches 404.

              - Effect: Allow
                Action: s3:ListBucket
                Resource: arn:aws:s3:::us-west-2.opendata.source.coop
                Condition:
                  StringLike:
                    s3:prefix: englacial/*

The condition is fine for actual LIST calls — obstore builds the store with prefix= from the output path (src/zagg/store.py:198, parse_s3_path), so obstore.list_with_delimiter always sends prefix=englacial/... and matches. The problem is the other thing s3:ListBucket governs: S3 returns 404 NoSuchKey for a GET on a missing key only if the caller has s3:ListBucket on the bucket; otherwise it returns 403 AccessDenied. A GetObject request carries no s3:prefix context key, so a StringLike on s3:prefix never matches on that evaluation — with this policy the answer is always "no ListBucket" and therefore always 403.

That matters because zagg's "is it written yet?" checks are all narrow 404 catches:

    except (FileNotFoundError, NotFoundError):
        return None

src/zagg/hive.py:813 (manifest read), src/zagg/runner.py:4474 (_fetch_result, the fan-out poll), src/zagg/sweep_lease.py:84, src/zagg/sweep.py:494 / :1015, src/zagg/telemetry.py:1041 / :1058, src/zagg/coverage.py:415, src/zagg/client_transport.py:339. An obstore PermissionDeniedError from a 403 sails straight past all of them, so "the manifest hasn't landed yet" and "the lease is free" become hard errors instead of None — mid-campaign, after compute.

What I'd change: drop the Condition and grant s3:ListBucket on the bucket ARN unconditionally. The containment you lose is close to nil — obstore physically cannot list outside the store prefix, and the cross-account path is gated a second time by Source Cooperative's own bucket policy — while what you gain is the 404 semantics the whole absence-detection layer is built on. If you want to keep the condition, the PR needs a paired change making those call sites treat 403 as absence, plus confirmation that Source Cooperative's side grants ListBucket at all (if theirs is also prefix-conditioned, dropping ours does not rescue the 404 and this becomes a question for them before the grant is finalized).

[espg]

🤖 from Claude

Accepted — fixed in 8cf8b10 (fold review: drop the s3:prefix condition that would 403 absent keys (issue #495)).

The Condition block is gone; s3:ListBucket is now granted unconditionally on arn:aws:s3:::us-west-2.opendata.source.coop. The 404-vs-403 reasoning went into the template comment right above the statement so nobody re-adds the condition on containment grounds:

              # Unconditional by intent -- do NOT re-add an s3:prefix
              # condition here. S3 answers a GET on a missing key with 404
              # NoSuchKey only when the caller holds s3:ListBucket on the
              # bucket; otherwise it answers 403 AccessDenied. A GetObject
              # request carries no s3:prefix context key, so a StringLike on
              # that key never matches during a GET evaluation and every
              # absent object would come back 403. ...

tests/test_lambda_build.py::test_source_coop_upload_role_is_opt_in_and_scoped now pins the absence with the reason attached, replacing the old assert bucket["Condition"] == {...}:

        assert "Condition" not in bucket, (
            "an s3:prefix condition on ListBucket makes absent objects 403 "
            "instead of 404, and zagg's absence checks catch 404 only"
        )

On the residual you raised: dropping our condition is necessary but not sufficient. If Source Cooperative's own bucket policy is itself prefix-conditioned on ListBucket, the cross-account evaluation still fails on their side and the GET still comes back 403 — our grant cannot rescue 404 semantics alone. That needs confirming with them before the grant is finalized; recording it here rather than acting on it, since contacting them is not this PR's to do.

[espg]

🤖 from Claude (review)

MEDIUM — the comment promises cleanability the policy does not grant.

  # operator knobs. Multipart actions matter — at ~131 MB/shard the writes are
  # multipart constantly, and an aborted upload must be cleanable.

s3:AbortMultipartUpload lets you abort an upload whose UploadId you are already holding — i.e. obstore's own in-process cleanup when a put fails inside a live worker. It does not let anyone find the uploads that leaked. Enumerating in-flight multipart uploads is ListMultipartUploads, whose IAM action is s3:ListBucketMultipartUploads on the bucket ARN, and that action is in neither statement.

The leak path is the realistic one here: a worker that hits the 900 s Lambda ceiling or gets recycled mid-upload never reaches obstore's abort, so its parts stay in the bucket, invisible to ListObjects, and billed to Source Cooperative. At CA scale (2,721 o9 shards, ~131 MB each) that is not a rounding error, and there is no way to reconcile it later with the current action list.

Two ways out, either is fine: add s3:ListBucketMultipartUploads to the bucket statement (it is prefix-conditionable the same way ListBucket is, though see my note on the s3:prefix condition), or reword the comment to say what is actually true — that the abort action covers obstore's in-process failure path only, and orphan reconciliation is Source Cooperative's lifecycle rule to own. Right now the comment reads as a guarantee a reviewer would take at face value.

[espg]

🤖 from Claude

Accepted, both halves — fixed in 90c45a0 (fold review: grant ListBucketMultipartUploads and say what abort covers (issue #495)).

The bucket-ARN statement now grants the list action, which is what makes orphan reconciliation possible at all:

              - Effect: Allow
                Action:
                  - s3:ListBucket
                  - s3:ListBucketMultipartUploads
                Resource: arn:aws:s3:::us-west-2.opendata.source.coop

And the resource comment no longer reads as a cleanability guarantee — it now states the three separate facts:

  # multipart constantly — but be precise about what they buy:
  # AbortMultipartUpload only aborts an upload whose UploadId the caller
  # already holds, i.e. obstore's in-process cleanup when a put fails inside a
  # live worker. Discovering uploads that leaked needs
  # ListBucketMultipartUploads on the bucket ARN, granted below. Neither
  # rescues a worker killed at the 900 s ceiling: it never reaches obstore's
  # abort, so its parts survive, invisible to ListObjects and billed to Source
  # Cooperative, until a lifecycle rule on their side reaps them.

The test's bucket-statement assertion was extended to the list form (assert bucket["Action"] == ["s3:ListBucket", "s3:ListBucketMultipartUploads"]), and its multipart comment now draws the same abort-vs-discover distinction. Per the note above, the new grant is unconditional — no s3:prefix condition, for the 404 reason in the neighbouring thread.

[espg]

🤖 from Claude (review)

MEDIUM — "the dispatching identity's ARN" is the ambiguity that makes the stack update fail, and this parameter is the one place an operator will read.

      unchanged; set it to the dispatching identity's ARN to stamp the role.

The obvious way to obtain "the dispatching identity's ARN" is aws sts get-caller-identity, which under SSO returns the session ARN:

arn:aws:sts::742127912612:assumed-role/AWSReservedSSO_Project-Power-User_c29c10db812f6efc/espg

That is not a valid principal in a role trust policy. CloudFormation rejects it with Invalid principal in policy and rolls the stack update back — and this is a stack whose whole point is that the role must exist before Source Cooperative can name it, so a failed update is directly on the critical path described three lines up.

The value that works is the IAM role ARN with the SSO path, exactly as already written in the reviewed draft source_coop_grant/trust-policy.json:

arn:aws:iam::742127912612:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_Project-Power-User_c29c10db812f6efc

I'd name that form in the description (arn:aws:iam::<account>:role/..., not the sts:...:assumed-role/... that get-caller-identity prints). While you're there: #495's question (2) rationale for parameterizing — the SSO hash suffix is regenerated when the permission set is reprovisioned, so a hardcoded ARN silently rots — is the reason a reader needs for why this is a knob at all, and it survived into the PR body but not into the template. The :root half of the rationale did make it in; the other half is the one that explains the parameter's existence.

[espg]

🤖 from Claude

Accepted — fixed in dc05707 (fold review: name the IAM role ARN form and the SSO-hash rationale (issue #495)).

SourceCoopPublisherPrincipal's description now names the working form explicitly and rules out the one get-caller-identity prints:

      unchanged; set it to the dispatching identity's IAM role ARN to stamp
      the role. Under SSO that is
      arn:aws:iam::<account>:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_<permission-set>_<hash>
      — NOT the arn:aws:sts::<account>:assumed-role/... session ARN that
      `aws sts get-caller-identity` prints, which CloudFormation rejects with
      "Invalid principal in policy" and rolls the stack update back. A knob
      rather than a literal in this git-tracked template because that SSO hash
      suffix is regenerated whenever the permission set is reprovisioned, so a
      hardcoded ARN silently rots.

That also folds in the missing half of issue #495's question (2) rationale — the hash-suffix rot is why this is a parameter at all, which previously lived only in the PR body while the :root half made it into the template.

Separately, this review turned up a real instance of the same YAML class of bug elsewhere in the file, fixed in 5bed8bb: Outputs.ExtractFunctionArn.Description was an unquoted plain scalar containing (issue #148)., so YAML truncated it at the # and CloudFormation would have shown ARN of the dedicated extraction Lambda function (issue. Now quoted; verified through the repo's CFN loader.

[espg]

🤖 from Claude (review)

MEDIUM — the assumed-role credentials expire in 1 hour and nothing in the tree refreshes them; a CA-sized campaign outlives them.

The design is "the dispatcher assumes this role and injects short-lived credentials through the existing output_credentials path." Tracing that path, the credentials are genuinely static once injected:

        if credentials:
            opts["access_key_id"] = credentials["accessKeyId"]
            opts["secret_access_key"] = credentials["secretAccessKey"]
            if credentials.get("sessionToken"):
                opts["session_token"] = credentials["sessionToken"]

src/zagg/store.py:216. No credential_provider, so no refresh — the _OBJECT_STORE_CACHE comment at src/zagg/store.py:113 says as much ("a statically-supplied token must NOT be cached (it would freeze on a warm worker)"). They arrive from a JSON file the operator hands the CLI (src/zagg/__main__.py:112), and grep -rn 'assume_role\|AssumeRole' src/ deployment/ finds nothing — the assume is a manual step outside the repo.

Two compounding limits:

• MaxSessionDuration is not set, so the role defaults to 3600 s.
• Because the dispatcher already runs under an assumed SSO role, sts:AssumeRole into this role is role chaining, which STS hard-caps at 1 hour regardless of MaxSessionDuration — so raising the property does not actually buy headroom on the laptop path.

A CA o9 campaign is 2,721 shards; it runs for hours. Workers dispatched past the 60-minute mark will get ExpiredToken at write time, after full aggregation — the same burn-then-fail shape phase 2 of #495 exists to prevent, arriving through a different door.

This is a "Questions for review" item rather than a one-line template fix, and I'd rather it be asked than discovered on the first real run: how does a multi-hour run re-assume? Concrete options — (1) set MaxSessionDuration: 43200 and require the dispatcher to assume from a non-chained identity (an IAM user or an EC2/instance-profile dispatcher) so the 12 h actually applies; (2) keep 1 h and have the dispatcher re-assume + re-inject per fan-out batch, which needs a code change in the output_credentials seam; (3) give the workers the role directly via credential_provider with AssumeRoleWithWebIdentity, which breaks the #26 fail-closed posture and is presumably out. Whichever way it lands, the property's absence here is currently silent.

[espg]

🤖 from Claude

Leaving this one standing by design — no code change, deliberately.

The finding is correct on the mechanics (no credential_provider in the output_credentials path, so the injected credentials are frozen; role chaining from an assumed SSO identity is hard-capped at 1 h by STS regardless of MaxSessionDuration), and correct that a CA o9 campaign at 2,721 shards outruns it. But every way out is a design fork, not a one-line template edit:

1. non-chained dispatcher identity (IAM user / instance-profile) plus MaxSessionDuration: 43200;
2. keep 1 h and re-assume + re-inject per fan-out batch, which is a code change in the output_credentials seam;
3. worker-side credential_provider with AssumeRoleWithWebIdentity, which breaks the Support credential injection for output writes (modular external targets, e.g. source.coop) #26 fail-closed posture.

Per CLAUDE.md §6 that choice is @espg's, not the fold's — picking one here would silently settle a scope question. So it is being carried as a Questions for review item on the PR body rather than resolved, and MaxSessionDuration is deliberately not added to the template (adding it would look like option (1) had been chosen while the chaining cap means it buys nothing on the laptop path). Leaving the thread open so it stays visible.

[espg]

🤖 from Claude (review)

MEDIUM — the comment asserts the capability claim; the test does not.

        # ... so a plain standup
        # creates no named IAM role and needs no extra IAM capability beyond
        # what it already requests.
        standup = (REPO_ROOT / "deployment" / "aws" / "stand_up.sh").read_text()
        assert "SourceCoopPublisherPrincipal" not in standup

That capability claim is newly load-bearing in this PR, and it is the half that isn't pinned. ExecutionRole (template.yaml:222) has no RoleName — CloudFormation generates it — so before this commit template.yaml needed only CAPABILITY_IAM. RoleName: !Ref SourceCoopUploadRoleName (template.yaml:279) is the first property in the file that requires CAPABILITY_NAMED_IAM. It happens to already be there:

    --capabilities CAPABILITY_NAMED_IAM CAPABILITY_AUTO_EXPAND \

deployment/aws/stand_up.sh:203. But nothing stops someone from tightening that line back to CAPABILITY_IAM on the (previously correct) reasoning that the template creates no named roles — and the failure surfaces as a CloudFormation InsufficientCapabilities error against live AWS, which per CLAUDE.md §1 is exactly the class of thing no test run can catch for you.

One line, same file already read:

        assert "CAPABILITY_NAMED_IAM" in standup

Also worth noting the test's scope: stand_up.sh is the only in-repo standup path, so the assertion is true, but an operator deploying through the console or a bare aws cloudformation deploy still has to supply the capability themselves. SourceCoopUploadRoleName's description says so — the test comment's "needs no extra IAM capability" is only true of the stand_up.sh path, and reads broader than it is.

[espg]

🤖 from Claude

Accepted — fixed in 317cf16 (fold review: pin CAPABILITY_NAMED_IAM in the standup path (issue #495)).

The one-line pin is in, and the comment now scopes its claim to the stand_up.sh path instead of reading as a general statement:

        # stand_up.sh creates no named IAM role. ... RoleName on
        # SourceCoopUploadRole is nonetheless the first named-IAM property in
        # the template (ExecutionRole lets CloudFormation generate its name),
        # so pin the capability stand_up.sh passes: tightening it back to
        # CAPABILITY_IAM would fail only against live AWS. The claim is scoped
        # to this path -- an operator deploying via the console or a bare
        # `aws cloudformation deploy` supplies the capability themselves.
        standup = (REPO_ROOT / "deployment" / "aws" / "stand_up.sh").read_text()
        assert "SourceCoopPublisherPrincipal" not in standup
        assert "CAPABILITY_NAMED_IAM" in standup

The old "needs no extra IAM capability beyond what it already requests" wording is gone, since it was the part that read broader than it was true.

[espg]

🤖 from Claude (review)

LOW — the substring the guard greps for is the one spelling the regression is least likely to use.

            assert "source.coop" not in json.dumps(role), (

The thing this test defends is real (#26 fail-closed: the worker role must never gain standing write access), and the dotted form catches the direct case — someone pasting arn:aws:s3:::us-west-2.opendata.source.coop/englacial/* into ExecutionRole's inline policy, the way the sliderule-public-cors grant sits there today.

It does not catch the shape the grant would more plausibly arrive in once this PR's role exists: an attached managed policy. ManagedPolicyArns: ["arn:aws:iam::742127912612:policy/source-coop-write"] contains no source.coop — hyphen, not dot — so json.dumps(role) is clean and the test passes green while the execution role has full write access to the bucket. Same for a !Sub-assembled ARN that splits the literal, though that one is a stretch.

Cheap fix, no restructuring:

            blob = json.dumps(role)
            for needle in ("source.coop", "source-coop", "opendata"):
                assert needle not in blob, (...)

Worth also asserting "ManagedPolicyArns" not in role if the intent is "this role's permissions are fully described inline" — the current inline-only shape is what makes the json.dumps grep sound in the first place, and nothing pins it.

[espg]

🤖 from Claude

Accepted, both parts — fixed in f0037b7 (fold review: widen the fail-closed needles and pin inline-only permissions (issue #495)).

            blob = json.dumps(role)
            assert "ManagedPolicyArns" not in blob, (
                f"{name}'s ExecutionRole attaches a managed policy -- its "
                "permissions are no longer fully described inline, so this "
                "guard can no longer see what it grants (issue #26)"
            )
            for needle in ("source.coop", "source-coop", "opendata"):
                assert needle not in blob, (
                    f"{name}'s ExecutionRole now names {needle} -- issue #495 "
                    "grants that access through an assumed role, not standing "
                    "permissions on the worker role (issue #26)"
                )

The needles cover the dotted host form, the hyphenated form a policy or role name would use (arn:aws:iam::...:policy/source-coop-write was the escape you named), and the bare bucket stem. The ManagedPolicyArns pin is the one that makes the json.dumps grep sound in the first place — inline-only is now an asserted property of both roles rather than an accident. Still runs against both copies (template.yaml's inline ExecutionRole and execution_role.yaml); passes green.

[espg]

🤖 from Claude (review)

Adversarial review of phase 3 (86e88b6, deployment/aws/template.yaml + tests/test_lambda_build.py). Six inline findings posted, ranked: one HIGH (s3:prefix-conditioned ListBucket forces 403-instead-of-404 on absent keys, which every except (FileNotFoundError, NotFoundError) site in the tree mis-handles), four MEDIUM (multipart cleanup actions vs. the comment's claim; the trust-principal ARN form; 1-hour credential expiry on a multi-hour campaign; CAPABILITY_NAMED_IAM unpinned), one LOW (execution-role guard greps the wrong spelling).

What I checked and found clean, so it does not get relitigated later:

• YAML # truncation. Every # the diff adds is inside a folded block scalar (>-) or a double-quoted scalar, so none of it truncates. Parsed and confirmed — SourceCoopUploadRole.Description comes out as "Writes englacial/* on us-west-2.opendata.source.coop in-region (issue #495).", and both new parameter descriptions survive whole.
• The !Not [!Equals [..., ""]] gate behaves as claimed. Default: "" plus aws cloudformation deploy (which falls back to template defaults for parameters absent from --parameter-overrides, not to UsePreviousValue) means an existing stack updates to a strict no-op: condition false, no role, no output, no named IAM resource. stand_up.sh does not pass either new parameter, and grep -rn 'cloudformation deploy' .github/workflows/ finds no CI path that deploys template.yaml at all (benchmark_cicd.yaml is a separate stack), so there is no second deploy surface to drift against.
• Condition on an Output alongside Fn::ForEach in the same section. The AWS::LanguageExtensions transform processes the Fn::ForEach::WorkerVariantArns key independently; a sibling plain output carrying a Condition is untouched by it. No interaction.
• s3:PutObjectAcl is correctly present, not excess. A PUT carrying x-amz-acl: bucket-owner-full-control needs both s3:PutObject and s3:PutObjectAcl; same for the CreateMultipartUpload that opens a multipart write with the canned ACL. UploadPart / CompleteMultipartUpload map to s3:PutObject, so no separate action is missing there.
• The ListBucket prefix condition does match obstore's actual LIST calls. _s3_object_store builds S3Store(..., prefix=parse_s3_path(path)[1]) (src/zagg/store.py:198), so obstore.list_with_delimiter always sends prefix=englacial/…; StringLike englacial/* matches, and delimiter is unconstrained. The condition's problem is elsewhere (see the HIGH finding) — the LIST path itself is fine.
• All three new tests pass against the committed template — I re-ran their assertions standalone (the worktree has no zarr, so pytest can't load conftest.py there). The 2-tuple unpack objects, bucket = ...["Statement"] is a good pin: adding a third statement fails loudly rather than silently.

Two notes that aren't findings:

1. PR body says "Four tests", and then lists three; three landed. Worth correcting so a later reader doesn't go hunting for a dropped test.

2. Your question (3) — confirmed, and it does truncate. Loading the template gives:

   ExtractFunctionArn.Description == "ARN of the dedicated extraction Lambda function (issue"
   

   # mid-plain-scalar starts a YAML comment, exactly as you suspected. My read: fix it here. It is one character of change (quote the scalar) in a file this PR already opens, it is the same failure class the new output deliberately dodges by quoting, and leaving it means the next reader has to rediscover that the unquoted neighbours are the broken ones. If you'd rather keep phase 3 surgically scoped to the role, a small-fix issue is the honest alternative — but silently leaving it after having diagnosed it is the worst of the three.

[espg]

🤖 from Claude (review)

High — the lifecycle self-copy re-creates external objects outside this seam, undoing the ownership this line establishes.

This covers every store built through _s3_object_store, but src/zagg/lifecycle.py never goes through it. _s3_client() (src/zagg/lifecycle.py:224-247) builds a boto3 client straight from the same store_kwargs["credentials"], and _touch_s3_object (src/zagg/lifecycle.py:278-289) issues:

s3.copy_object(Bucket=bucket, Key=key, CopySource={"Bucket": bucket, "Key": key},
               MetadataDirective="REPLACE", StorageClass=storage_class or "STANDARD")

CopyObject is an object-creating request. On a cross-account target every touched object is therefore re-created owned by our account under the requester's default private ACL — the module's own docstring already records the mechanism (src/zagg/lifecycle.py:38-42: "ACL: NOT preserved — CopyObject grants the destination the requester's default private ACL unless x-amz-acl/x-amz-grant-* rides the request"). That is exactly the failure mode #495 exists to fix, and it is strictly worse than the status quo: it strips ownership from objects an earlier PUT correctly handed over.

It is reachable on the source.coop path, worker-side, with the same output credentials: src/zagg/hive.py:1521 and src/zagg/processing/raster.py:1353 (touch_current_unit on every skipped unit) and src/zagg/sweep_stages.py:352 (touch_unit_footprint on the store-root aggregation.yaml). The touch is fail-open and only counts failures, so a wrong ACL here is silent.

I would export the predicate and the value from this module (e.g. _external_target(credentials, endpoint_url) + _BUCKET_OWNER_ACL) and pass ACL=_BUCKET_OWNER_ACL on the copy_object when it holds, with a test pinning it — otherwise "every write to an external target hands over ownership" is not true after this phase.

(Neighbouring, non-blocking: src/zagg/catalog/extract.py:229 is the other raw-boto3 writer (boto3.client("s3").upload_file(...)), but it only ever uses ambient credentials, so it cannot reach an external target today.)

[espg]

🤖 from Claude

Accepted and fixed in eec4e55.

src/zagg/store.py now exports the predicate next to the value:

def _external_target(credentials, endpoint_url) -> bool:
    return bool(credentials) and not endpoint_url

_s3_object_store calls it for its own gate, so the two sites cannot drift. src/zagg/lifecycle.py gained _copy_acl(store_kwargs), which imports both from the store seam and returns _BUCKET_OWNER_ACL on an external target and None otherwise; touch_unit_footprint derives it once per footprint and threads it through _touch_s3_tree into _touch_s3_object, which adds ACL=acl to the copy_object params only when it is set. Fail-open behaviour is untouched — the ACL rides a best-effort request and a rejection still just counts a failure (pinned by test_a_failing_copy_on_an_external_target_stays_fail_open).

The module docstring bullet that recorded this as "Documented, not solved" now says solved for the external case, with the in-account public-read-BY-ACL caveat kept as the part that remains a caveat.

Four tests in tests/test_lifecycle.py::TestTouchS3: external target puts bucket-owner-full-control on every copy in the footprint, ambient in-account puts no ACL key at all, a custom endpoint is excluded exactly as in the store seam, and the fail-open one above.

On the neighbour: src/zagg/catalog/extract.py:229 is left alone deliberately. It builds boto3.client("s3") with no credential arguments, so it is ambient-only and cannot reach an external target today — adding an ACL there would be speculative and would need a credentials channel that does not exist.

[espg]

🤖 from Claude (review)

Medium — read-only stores opened with input credentials get the header too, so the invariant this comment states ("injected credentials exist precisely to write somewhere else", line 259-262) is not true.

zagg.temporal.open_dataset forwards the issue #223 consumer-input channel — input_credentials covering event_mask_uri/static_uris, wired at deployment/aws/lambda_handler.py:1197 and src/zagg/temporal.py:792-795 — into both store routes with explicit credentials and no endpoint:

• src/zagg/temporal.py:614-621 → open_store(uri, read_only=True, credentials=credentials, ...)
• src/zagg/temporal.py:636-643 → open_object_store(f"s3://{bucket}", credentials=credentials, ...) (the NetCDF branch)

Those are pure reads of somebody else's input bucket and now carry x-amz-acl on every GET. Practical blast radius is ~zero — S3 evaluates s3:x-amz-acl on object-creating actions only, so the "inertly" claim holds — but the comment is what a future reader reasons from, and the seam already has the answer in hand: _open_s3_store takes read_only (src/zagg/store.py:190) and just doesn't forward it. Threading it into _s3_object_store and gating on credentials and not endpoint_url and not read_only costs one kwarg and makes the code mean what the comment says. open_object_store has no read-only concept, so the NetCDF branch would need its own opt-out (or stays a documented exception).

Minimum ask: name the read-with-input-credentials case here instead of asserting it cannot occur.

[espg]

🤖 from Claude

Accepted and fixed in ffbcfcd for the route that can tell.

_s3_object_store now takes read_only: bool = False as an explicit parameter (consumed here, never forwarded to S3Store, which has no such option), _open_s3_store passes it through, and the gate is elif _external_target(credentials, endpoint_url) and not read_only:. So open_store(uri, read_only=True, credentials=...) — the zagg.temporal.open_dataset .zarr branch at src/zagg/temporal.py:614-621 — sends nothing.

open_object_store keeps sending it, now as an explicitly documented exception rather than an unstated one. Its docstring names the case: no read_only concept on that route, so a credentialed reader built through it carries the header, notably temporal.open_dataset's NetCDF branch (src/zagg/temporal.py:636-643), a pure GET of an issue #223 consumer-input bucket — inert, because S3 interprets x-amz-acl only on object-creating requests. The _BUCKET_OWNER_ACL comment block and the open_store Notes section now state the same split instead of the "injected credentials exist precisely to write somewhere else" absolute you flagged.

Tests in TestBucketOwnerAcl: test_read_with_input_credentials_sends_no_acl_header (also asserts read_only does not leak into the S3Store kwargs) and test_object_store_reads_are_the_documented_exception. tests/test_temporal.py still passes.

[espg]

🤖 from Claude (review)

Medium — the documented "a caller who set x-amz-acl explicitly wins" holds only for an exactly-lowercase key. obstore lowercases header keys, so setdefault on the raw dict cannot see X-Amz-Acl, and the collision is resolved in obstore after our merge — last insertion wins, which is always ours. Against the installed obstore 0.10.0:

>>> _with_bucket_owner_acl({'default_headers': {'X-Amz-Acl': 'private'}})
{'default_headers': {'X-Amz-Acl': 'private', 'x-amz-acl': 'bucket-owner-full-control'}}
>>> S3Store('b', region='us-west-2', client_options=_).client_options
{'default_headers': {'x-amz-acl': b'bucket-owner-full-control'}}

So a caller who deliberately asked for private silently gets bucket-owner-full-control. One line fixes it, and lowercasing is lossless because obstore does it anyway:

headers = {str(k).lower(): v for k, v in (options.get("default_headers") or {}).items()}

test_caller_client_options_are_preserved (tests/test_store.py:179) passes a lowercase key, so it does not catch this — a mixed-case variant would.

[espg]

🤖 from Claude

Accepted and fixed in 909f1c4. _with_bucket_owner_acl now lowercases the caller's keys before merging:

headers = {str(k).lower(): v for k, v in (options.get("default_headers") or {}).items()}
headers.setdefault("x-amz-acl", _BUCKET_OWNER_ACL)

Lossless, as you note, because obstore lowercases them anyway — and it is what makes the documented "a caller who set x-amz-acl explicitly wins" actually true rather than true only for an exactly-lowercase key. The docstring now says why the lowercasing is load-bearing (otherwise the collision resolves inside obstore, where last insertion wins, which is always ours).

New test_caller_mixed_case_acl_header_still_wins passes {"X-Amz-Acl": "private", "X-Custom": "1"} and asserts the merged headers are {"x-amz-acl": "private", "x-custom": "1"}. It fails against the pre-fix merge.

[espg]

🤖 from Claude (review)

Medium-low — "sent unconditionally" is right for the three Object Ownership modes, but the merge leaves no way to remove the header, which is the case question (1) of #495 flagged when weighing automatic vs. a knob.

setdefault lets a caller change the value, not drop it, and neither obstore-legal value expresses "no ACL": None is rejected (TypeError: argument 'client_options': 'None' is not an instance of 'str', obstore 0.10.0) and "" is a live empty x-amz-acl that S3 rejects rather than an absence. A future external AWS target that must not set an ACL therefore has no route short of editing _with_bucket_owner_acl. If you want to keep the derivation knob-free (I think that is the right call for this PR), the cheap hedge is to let a caller pass x-amz-acl: None and have _with_bucket_owner_acl strip the key before handing the dict to obstore — an escape hatch that needs no config surface — and to say so in that docstring.

Related, on the exclusion comment at line 261-264: the retired data.source.coop proxy was reached through endpointUrl, so this exclusion quietly covers it as well. That is moot under the CLAUDE.md §1 no-egress rule, but both this comment and docs/deployment/lambda.md:167 currently justify the exclusion only by "R2, MinIO do not implement canned ACLs", which leaves a reader to rediscover that an endpoint-routed AWS target is excluded too.

[espg]

🤖 from Claude

Accepted, both halves, in 54335bf.

The escape hatch: a caller passing x-amz-acl: None in default_headers now strips the key entirely before the dict reaches obstore (setdefault fills it, then a None value is deleted), so the removal case has a route that needs no config surface and the derivation stays knob-free. The _with_bucket_owner_acl docstring documents it and records why it is needed — obstore rejects a None header value and "" is a live empty x-amz-acl S3 rejects, so neither obstore-legal value expresses absence. Pinned by test_caller_can_strip_the_header_with_an_explicit_none.

The exclusion prose: the comment in src/zagg/store.py and the paragraph in docs/deployment/lambda.md now both say the endpoint_url exclusion covers two shapes — the S3-compatible stores that do not implement canned ACLs (R2, MinIO), and an endpoint-routed AWS target such as the retired data.source.coop proxy hop, which was reached exactly that way and which this native-write path exists to replace. The docs paragraph also names the x-amz-acl: None opt-out and states that nothing in the Lambda config surface reaches it.

[espg]

🤖 from Claude (review)

Low — every test in this class mocks obstore.store.S3Store, so they pin what zagg passes and never what obstore accepts. client_options / default_headers are obstore API surface that validates only at real construction — S3Store('b', region='us-west-2', client_options={'default_header': {...}}) raises ValueError: Invalid key: default_header. A typo or an obstore rename would leave this whole class green while the fleet writes owner-less objects into someone else's bucket.

One unmocked test closes that, no network needed (verified locally against obstore 0.10.0):

def test_real_obstore_store_carries_the_header(self):
    from zagg.store import _s3_object_store
    s3 = _s3_object_store("s3://external/foo.zarr", credentials=self.CREDS)
    assert s3.client_options["default_headers"]["x-amz-acl"] == b"bucket-owner-full-control"

Note the bytes: obstore normalizes header values, and pinning that is useful documentation in its own right.

Two other cases this class does not cover, both cheap: a read-only store opened with credentials (open_store(..., read_only=True, credentials=...), the zagg.temporal.open_dataset shape — see my note on src/zagg/store.py:71), and a mixed-case caller header (see src/zagg/store.py:298).

[espg]

🤖 from Claude

Accepted and added in 48626b0 — the class now has two deliberately unmocked tests, construction only, no network.

test_a_real_obstore_store_accepts_and_normalizes_the_header builds _s3_object_store("s3://external/foo.zarr", credentials=...) for real and asserts s3.client_options["default_headers"]["x-amz-acl"] == b"bucket-owner-full-control", pinning the bytes normalization as you suggested. Its comment records why it exists: client_options/default_headers validate only at real construction, so a typo or an obstore rename would leave the mocked tests green while the fleet wrote owner-less objects.

test_a_real_obstore_read_only_store_carries_no_header covers the second case from your src/zagg/store.py:71 finding against real obstore: with read_only=True and credentials, client_options is None — nothing at all reaches obstore.

The mixed-case case is covered mocked in 909f1c4 (test_caller_mixed_case_acl_header_still_wins), where the assertion is about our merge rather than obstore's acceptance.

[espg]

🤖 from Claude (review)

Low — "it survives SigV4 signing" is true in outcome but inverted in mechanism, and the comment omits the request that actually carries this run. obstore applies default_headers on the reqwest client, i.e. after object_store signs, so the header rides outside the signature. Probed against a local endpoint with the installed obstore 0.10.0 (12 MiB body, 5 MiB parts):

PUT   /bkt/small.bin                        acl='bucket-owner-full-control'  signed_includes_acl=False
GET   /bkt/small.bin                        acl='bucket-owner-full-control'  signed_includes_acl=False
POST  /bkt/big.bin?uploads=                 acl='bucket-owner-full-control'  signed_includes_acl=False
PUT   /bkt/big.bin?partNumber=1&uploadId=U1 acl='bucket-owner-full-control'  signed_includes_acl=False
POST  /bkt/big.bin?uploadId=U1              acl='bucket-owner-full-control'  signed_includes_acl=False

Two things worth writing down here:

1. x-amz-acl is never in SignedHeaders. It is accepted because S3 ignores unsigned non-required x-amz-* on header-auth requests, not because the signature covers it — and it would not survive a presigned-URL path, which rejects unsigned x-amz-*. "Survives SigV4 signing" reads as though it were signed.
2. The header does ride CreateMultipartUpload (POST ?uploads), which is the request that sets a multipart object's ACL — UploadPart/CompleteMultipartUpload ignore it. At ~131 MB/shard multipart is the normal path (it is why the phase-3 role carries AbortMultipartUpload/ListMultipartUploadParts), so this is the load-bearing half of the verification and the comment currently mentions only PUT/GET.

(2) is the part I would add; (1) is a one-word hedge.

[espg]

🤖 from Claude

Accepted and corrected in b930017. The _BUCKET_OWNER_ACL comment no longer says the header survives SigV4 signing. It now says obstore applies default_headers on the reqwest client, i.e. after object_store signs, so the header rides outside the signature: x-amz-acl never appears in SignedHeaders, and AWS accepts it because S3 ignores unsigned non-required x-amz-* on header-auth requests — with the explicit note that it would not survive a presigned-URL path, which rejects unsigned x-amz-*.

Your point (2) is in as well: the comment now names CreateMultipartUpload (POST ?uploads) alongside the single-shot PUT and says that is the load-bearing half — the create request is what sets a multipart object's ACL, UploadPart/CompleteMultipartUpload ignore it, and at ~131 MB/shard multipart is the normal write path. The GET/LIST inertness sentence is unchanged.

[espg]

🤖 from Claude (review)

Adversarial review of phase 1 only (commit 2c8d135a — src/zagg/store.py, tests/test_store.py, tests/test_lambda_handler.py, docs/deployment/lambda.md). Phase 3 (86e88b61) was reviewed separately and is out of scope here. Six inline findings, ranked:

Sev	Where	Finding
High	src/zagg/store.py:265	src/zagg/lifecycle.py's CopyObject self-touch writes through raw boto3 with the same output credentials, bypassing this seam — it re-creates external objects owned by our account, undoing the handover on every skip run
Medium	src/zagg/store.py:71	read-only stores opened with issue #223 input credentials also get the header; "injected credentials exist precisely to write somewhere else" is not true
Medium	src/zagg/store.py:298	setdefault misses a mixed-case X-Amz-Acl; obstore lowercases and our key wins, so the documented "caller wins" fails
Med-low	src/zagg/store.py:63	no way to remove the header (question (1) of #495): obstore rejects a None value and "" is a live empty ACL
Low	tests/test_store.py:119	the class mocks S3Store throughout, so an client_options/default_headers key rename or typo stays green; one unmocked construction pins it
Low	src/zagg/store.py:68	the SigV4 claim is inverted (the header rides unsigned), and the comment omits CreateMultipartUpload, which is the request that actually sets a multipart object's ACL

What checks out. The derivation itself is correct for the store seam: _s3_object_store is genuinely the single construction point for both open_store and open_object_store, so the Zarr write, the status envelopes (src/zagg/client_transport.py, src/zagg/telemetry.py), hive manifests (src/zagg/hive.py), stats sidecars, the sweep families (src/zagg/sweep*.py) and the tabular temporal object (src/zagg/output/tabular.py:199) all inherit it. The _OBJECT_STORE_CACHE is correctly out of reach (its gate requires not kwargs, and every output path passes at least region), and the anonymous skip_signature branch is correctly excluded. The retry_config deep-copy and virtual_hosted_style_request are untouched by the merge, and obstore 0.10.0 accepts client_options alongside both.

Empirically verified against the installed obstore 0.10.0 (local HTTP endpoint, request headers captured): the header is emitted on PutObject, GetObject, CreateMultipartUpload, every UploadPart, and CompleteMultipartUpload — so the multipart path (the normal one at ~131 MB/shard) is covered — but it never appears in SignedHeaders. obstore also normalizes the key to lowercase and the value to bytes, and rejects a misspelled client_options key at construction (ValueError: Invalid key) — which the mocked tests cannot see.

The nine new tests pass on the worktree (tests/test_store.py::TestBucketOwnerAcl, tests/test_lambda_handler.py::TestOutputStoreAcl) and do pin the claimed invariants for the exact-lowercase case; the gaps are listed above.

[espg]

🤖 from Claude (review)

[medium-high] The probe invents a THIRD sibling prefix, so a prefix-scoped grant now hard-fails a run whose real writes would succeed.

The probe writes to <store>.probe/, which is neither <store>/ (the zarr keys) nor <store>.status/ (the async result channel the run already needs writable — src/zagg/runner.py:1340, src/zagg/runner.py:3555, src/zagg/client_transport.py:123). It is a brand-new grant surface, and because the probe is fail-closed it is now a precondition for the run to start at all.

Both grants in-tree happen to cover it, so nothing breaks today:

• deployment/aws/template.yaml ExecutionRole → arn:aws:s3:::${OutputBucketName}/* (bucket-wide)
• SourceCoopUploadRole / source_coop_grant/s3-policy.json → arn:aws:s3:::us-west-2.opendata.source.coop/englacial/* (org-prefix-wide, and englacial/zagg/d.zarr.probe/… sits under it)

But a grant scoped to the store prefix — arn:aws:s3:::bucket/englacial/zagg/d.zarr/*, the least-privilege shape — false-refuses: the operator gets could not WRITE to … and is told to "fix the grant" for a grant that is in fact correct for every byte the run writes. That is the worst direction for a fail-closed check to be wrong in, and it is indistinguishable from a genuine denial.

Concretely, one of:

1. Probe under <store>.status/ instead — e.g. <store>.status/probe-<uuid>. Same "outside the store root" property (so the issue Warm start lambda and memory freeing are *still* a problem #151/zagg.client v2 transport: Event invoke + status-object future resolver #327 sibling precedent still holds), but it reuses a prefix the invoke/poll transport already requires writable rather than adding a fourth thing an operator has to enumerate.
2. Keep .probe and say so in the operator docs — neither docs/hive_layout.md nor docs/deployment/lambda.md currently tells a grant author that <store>.probe/* must be writable, and the docstring here presents the sibling purely as a safety choice rather than as a new permission requirement.

(For the record: the stated reason for avoiding the store root holds and is stronger than the docstring claims — docs/specification.md §5.2 makes in-root/in-leaf debris a key-set difference in the content hash, not just an untidy key, so a stranded probe object inside a published store would make a verifier report an intact leaf as tampered. The sibling instinct is right; it is the choice of which sibling that has the cost.)

[espg]

🤖 from Claude

Accepted, option (1) — fixed in 7aeb9c0. The probe now writes <store>.status/probe-<uuid> instead of <store>.probe/<uuid>.probe, so it reuses the prefix the async invoke/poll transport already requires writable (src/zagg/runner.py:1340, src/zagg/client_transport.py:123) rather than adding a fourth grant surface. A grant scoped <store>/* + <store>.status/* now passes the probe exactly when the run's real writes would succeed — the false-refusal you identified is gone, and a fail-closed check no longer gates on a prefix nothing else touches.

The docstring was rewritten to state the two properties in order, and it now carries your stronger reason for staying out of the store root rather than the weaker "untidy key" one: docs/specification.md §5.2 makes the leaf hash set discovery-based, so a stranded probe object under a leaf is a key-set difference and a verifier would report an intact leaf as tampered. The uuid rationale now also notes it cannot collide with the transport's own run-<run_id> objects.

Tests updated in the same commit (tests/test_lambda_handler.py): test_ping_probes_write_permission_on_s3_stores pins the prefix as …d.zarr.status and the key as probe-…; the delete-failure test's comment now says why an object outside the store root perturbs no leaf hash. The _invoke_lambda_ping docstring in src/zagg/runner.py carried the same .probe description and was corrected in b889716 (it sits in the paragraph that commit rewords anyway).

This also settles the convention collision you flag on the doc thread: docs/deployment/benchmark-cicd.md:207 documents a manual aws s3 cp probe inside the prefix, which is exactly the placement the docstring argues against. With the automated probe under .status/ the two no longer point in opposite directions, and 57af7eb adds an explicit "not to be confused with" note in docs/deployment/lambda.md naming that manual check.

[espg]

🤖 from Claude (review)

[medium] The DELETE outcome is invisible to the only person who could act on it, so a Put-but-no-Delete grant sails through the preflight this phase exists for.

Swallowing the DELETE is the right call — the PUT is the load-bearing half, agreed. But the result of the DELETE currently escapes into a logger.warning in the function's CloudWatch log group, which the dispatcher never reads, and the 200 body reports "write_probe": probed is not None — i.e. "did we probe", not "did the round trip succeed". So the operator running the dispatch sees a clean 200 in both cases.

That matters because s3:DeleteObject is not optional for zagg's real writes. deployment/aws/template.yaml says so explicitly for the execution role ("Delete covers store overwrite and manifest cleanup"), and phase 1 put s3:DeleteObject in the Source Coop grant for the same reason. A grant with PutObject but no DeleteObject therefore:

• passes the probe silently,
• strands one zero-byte object per run in <store>.probe/, forever, since nothing ever sweeps that prefix,
• and fails later at overwrite/cleanup time — the exact "discovered after the compute" shape this phase is built to eliminate.

Suggest surfacing it without changing the fail-open policy: return the delete outcome in the body (e.g. "probe_delete": False alongside "write_probe": True), and have _invoke_lambda_ping emit a logger.warning naming the stranded key and the missing action when it is false. Two lines, and the signal lands where the operator is actually looking.

Related, worth one line in the docstring rather than a code change: a single small PUT is representative for the multipart path (CreateMultipartUpload/UploadPart/CompleteMultipartUpload are all authorized by s3:PutObject), so no multipart probe is needed — but it cannot catch a missing s3:AbortMultipartUpload / s3:ListMultipartUploadParts, which phase 1 granted deliberately. "Prove WRITE permission" in the summary line overstates the coverage by a little.

[espg]

🤖 from Claude

Accepted — fixed in 3597b8b. The fail-open policy is unchanged (the PUT is still the load-bearing half), but the DELETE outcome now leaves the function:

• _probe_output_write returns (uri, deleted) instead of the bare uri, and _handle_ping's 200 body carries "probe_delete": false alongside "write_probe": true — plus "probe_key", so the stranded object can be named. Both keys are omitted entirely for a non-s3:// store, where there is no round trip to report.
• _invoke_lambda_ping (src/zagg/runner.py) parses the success body once and emits a logger.warning when probe_delete is False, naming the stranded key and s3:DeleteObject, and saying why it matters (store overwrite + manifest cleanup) — i.e. the signal now lands in the dispatching operator's log, not the function's.

Docstring line added as suggested, and the summary softened: the one-liner is now "prove s3:PutObject", with a paragraph saying one small PUT is representative of the multipart path (CreateMultipartUpload/UploadPart/CompleteMultipartUpload all authorized by s3:PutObject, so no multipart probe is needed) but cannot exercise s3:AbortMultipartUpload / s3:ListMultipartUploadParts, which phase 1 granted deliberately — a grant missing those still passes.

Tests: tests/test_lambda_handler.py pins probe_delete is True on the happy path, probe_delete is False + the exact probe_key on the delete-failure path, and the absence of both on a local store; tests/test_hive.py adds test_probe_delete_failure_warns_the_operator (warning names the key and s3:DeleteObject) and test_probe_delete_success_is_silent.

[espg]

🤖 from Claude (review)

[medium] The remedy states a diagnosis the tag cannot support: every probe exception becomes "the PUT was denied; fix the grant".

_handle_ping wraps the whole of _probe_output_write in one except Exception and tags all of it "check": "write_probe". Non-permission failures that land in this exact message:

• ValueError from _output_store_kwargs (output_credentials missing keys: …). This one is genuinely reachable: _output_store_kwargs is called in the read half only inside the if get_store_layout(config) == "hive" branch, so on a raster or flat ping (src/zagg/runner.py:1389, issue Raster lambda path writes the store template from the orchestrator — move it behind the setup invoke #264) a malformed credentials block surfaces for the first time inside the probe — and the operator is sent to rewrite a bucket policy when the actual fix is a typo in the event.
• A bad endpointUrl, a region redirect (PermanentRedirect), a nonexistent bucket, an obstore import failure, or network exhaustion after obstore's retries.

The wording is categorical — "a zero-byte probe PUT was denied", "fix the grant (bucket policy or assumed role)" — so it reads as a determination, not a hypothesis. Two concrete fixes, ideally both:

1. Hoist _output_store_kwargs(event) out of _probe_output_write into _handle_ping's first try and pass the resolved kwargs in, so a credentials-shape error keeps the read-side tag it deserves.
2. Soften this string to "the probe PUT failed" and lead with the grant as the likely cause rather than the stated one — the echoed body.get('error') already carries Access Denied when that is what happened, so the operator loses nothing.

Branch order is correct as written, incidentally: the write-probe body also carries "mode": "ping", so checking check first is load-bearing, and tests/test_hive.py::TestInvokeLambdaPingEvent::test_write_probe_refusal_names_the_grant_not_the_store pins it. Backwards compat is fine too — a pre-#495 function omits the tag and falls through to the older branches. The one skew this does not cover is the reverse (old local zagg, new deployed function), which will show the "clear the store root" remedy on a write denial; probably not worth handling, but worth knowing.

[espg]

🤖 from Claude

Both halves accepted — fixed in b889716.

(1) Hoist. _output_store_kwargs(event) now runs in _handle_ping's first try, unconditionally (the hive branch consumes the same resolved dict), and _probe_output_write(event, store_kwargs) takes it as an argument. A malformed output_credentials block therefore keeps the read-side tag instead of being labelled a grant failure. Your reachability note was the deciding one: _output_store_kwargs was called in the read half only under get_store_layout(config) == "hive", so on a raster/flat ping (issue #264) the probe was the first thing to touch the credentials shape at all. A comment at the hoist says so.

(2) Wording. The dispatcher message now reads "a zero-byte probe PUT failed, most likely a denied grant (bucket policy or assumed role); resolve it before dispatching this run" — the failing request is stated, the cause is offered as likely rather than determined, and the echoed body.get('error') still carries Access Denied when that is what happened. The comment above it now says explicitly that the tag identifies which request failed, not why. The _invoke_lambda_ping docstring bullet was updated to match (and to pick up the .status prefix change and the new probe_delete warning).

Test for the hoist landed with the other test work in 55f1b5d: test_ping_bad_credentials_on_non_hive_is_not_a_grant_failure sends a secretAccessKey-less credentials block on a non-hive s3:// ping and asserts the 500 carries output_credentials missing keys with no check key.

Noted on the two things you flagged as correct-as-written: branch order is unchanged, and the reverse skew (old local zagg, new deployed function) is left alone — an operator on a pre-#495 dispatcher seeing the store-root remedy on a write denial is a strictly better position than before this phase, and handling it would mean the local side reasoning about a tag it does not know.

[espg]

🤖 from Claude (review)

[low-medium] The probe is documented only in the hive doc, but it is not hive-specific — and the repo now carries two contradictory .probe conventions.

Three separate points on the doc placement:

1. Scope. _probe_output_write keys off store_path.startswith("s3://") and nothing else, so it runs on every Lambda ping — including the raster path (src/zagg/runner.py:1389, issue Raster lambda path writes the store template from the orchestrator — move it behind the setup invoke #264) and non-hive layouts. This paragraph sits inside the hive-manifest discussion and reads as a property of the hive lifecycle. Phase 1's operator-facing home was docs/deployment/lambda.md; a new object written next to every S3 store on every run, carrying a new grant requirement (see my note on lambda_handler.py:577), belongs there too — the person who needs this is writing a bucket policy, not reading the hive layout spec.

2. "Two requests per run" is accurate for the probe itself but easy to misread here, where the surrounding text is about the manifest: it is two requests added to the ping, and only for s3:// stores. Worth pinning explicitly since the sentence lands right after a paragraph counting manifest requests.

3. Convention collision. docs/deployment/benchmark-cicd.md:207-208 already documents a manual write probe as s3://BUCKET/PREFIX/.probe — a key inside the prefix — which is precisely the placement this docstring argues against. Two .probe conventions pointing opposite directions is a trap for whoever next reasons about which prefixes need to be writable; worth reconciling or at least cross-referencing.

Separately, docs/specification.md §5.2 already enumerates known store-adjacent debris by name (the issue #327 .zarr.status/ prefix) because it perturbs the content-hash key set. A .probe sibling that can persist next to a published dataset when a DELETE is denied is arguably the same class of contract surface and may deserve a line there — flagging rather than asserting, since it lives outside the store root and so does not change any hash today.

[espg]

🤖 from Claude

Accepted, all three points — fixed in 57af7eb.

(1) Scope/placement. The probe is not hive-specific (it keys off store_path.startswith("s3://") and nothing else, so the raster path runs it too), so the operator-facing description moved to docs/deployment/lambda.md as a new ### Write probe {#write-probe} section, immediately after phase 1's Output Credentials material — where the person writing a bucket policy already is. docs/hive_layout.md keeps a one-sentence cross-reference saying the precheck is a read, that the same preflight runs a write probe, that it is not hive-specific, and where it is documented.

(2) "Two requests" is now pinned in the new section: "Two requests, added to the ping, and only for s3:// stores" — with the local-store case named right there so it cannot be read as two requests per run of the manifest machinery.

(3) Grant requirement, which nothing documented before: the new section states the probe writes <store>.status/probe-<uuid>, that this is the run's async-result sibling which the invoke/poll transport already requires writable, and that a grant covering <store>/* + <store>.status/* therefore passes the probe exactly when the run's real writes would succeed — nothing new to enumerate. It also records why the store root is off limits (spec §5.2 key-set difference), what the probe covers and does not (s3:PutObject, multipart-representative, but not s3:AbortMultipartUpload / s3:ListMultipartUploadParts), and the two outcomes (PUT fail-closed and tagged; DELETE fail-open but reported and warned).

Convention collision: addressed by a "not to be confused with" note in the new section pointing at docs/deployment/benchmark-cicd.md and describing that check for what it is — a human-run aws s3 cp inside the prefix, cleaned up by hand in the same command. Left that file itself untouched: it is another PR's territory in this phase split, and after the prefix move there is no longer a live contradiction, just two differently-scoped checks.

On docs/specification.md: no change needed now. §5.2 already enumerates .zarr.status/ by name as known store-adjacent debris, and after the first finding's fix the probe object lives under exactly that prefix — so it is covered by the existing sentence rather than being a new contract surface. (Had it stayed under .probe/, your point would have stood.)

[espg]

🤖 from Claude (review)

LOW-MEDIUM — skipped counts paths; the two keys it sits beside count objects.

touched and failed are per-OBJECT (a tree contributes one per listed key), but skipped is incremented once per path, and one of those paths is an entire leaf tree. For the standard unit footprint a published skip reports skipped: 4 where a successful touch of the same footprint reports touched: 6. Any consumer that sums the three, or derives "fraction of the footprint handled", gets nonsense — and the docstring at lines 193-200 never says which unit it is in. (The log line does say "path(s)", to its credit; the dict key does not.)

Either fix is fine: name it skipped_paths, or state the unit in the touch_unit_footprint docstring in the same breath as "counted under a skipped key".

Related, in tests/test_lifecycle.py:327: assert counts["skipped"] > 0 is weaker than it needs to be. == 4 would pin the paths-not-objects semantics and catch a guard that skipped the tree while letting the three siblings through by some unrelated route.

[espg]

🤖 from Claude

Accepted — renamed to skipped_paths in 5b4268c1, and the unit is stated explicitly rather than implied. The touch_unit_footprint docstring now says:

That key counts INPUT PATHS, not objects, unlike its two siblings: a tree path is one skip here but would have been one "touched" per listed key, so the three are not summable.

The rename is carried through the record field added for the propagation finding — touch_skipped_paths at the unit seams, in sweep_stages, and in the _identity_counts rollup (151240e2) — so the paths-not-objects semantics is legible in the durable record too.

Tests updated in the same commit: assert counts["skipped_paths"] == 4 (with a comment naming the four inputs and contrasting them against the touched: 6 a successful touch of the same footprint reports), and the negative assertion in the credentials test is now "skipped_paths" not in counts.

[espg]

🤖 from Claude (review)

LOW — the log line repeats three lines of rationale per unit and never names what it skipped.

This fires once per (shard, window) unit — thousands of times on an all-skip CA run — and every emission is character-identical: no bucket, no store path, no shard. The operator question at 3 a.m. is "which target did we decide not to touch, and was that the published one or something new in _PUBLISHED_BUCKETS?", and this line cannot answer it. The reasoning it does carry is already in the module docstring (and belongs in docs/hive_layout.md, separate comment), which is where a reader can afford three lines of it.

Suggest collecting the buckets alongside the count (skipped_buckets: set next to skipped) and logging one short identifying line, e.g. lifecycle touch not applicable for {skipped} path(s) on published bucket(s) {sorted(skipped_buckets)} — archival and versioned, see zagg.lifecycle._skip_published (issue #495 phase 4). INFO is the right level.

[espg]

🤖 from Claude

Accepted — implemented as suggested in 5b4268c1. skipped_buckets: set is collected alongside the count in both loops, and the line is now one identifying sentence:

lifecycle touch not applicable for {skipped} path(s) on published bucket(s)
{sorted(skipped_buckets)} — see zagg.lifecycle._skip_published (issue #495 phase 4)

INFO, as you say. A comment above it records why it identifies rather than explains: it fires once per unit, the rationale now lives in _skip_published and in docs/hive_layout.md (the separate finding, folded in cddddc46), and the bucket is what the operator actually needs to tell the published target from something new in _PUBLISHED_BUCKETS.

[espg]

🤖 from Claude (review)

LOW (defense, non-blocking) — the guard lives at the call sites, not at the seam that issues CopyObject.

Traced and airtight as shipped, for the record: touch_current_unit (line 158) and touch_store_root (line 181) both funnel into touch_unit_footprint, and those three are the entire public surface (__all__, line 399). Every caller in the tree goes through them — src/zagg/hive.py:1521, src/zagg/processing/raster.py:1353, src/zagg/sweep_stages.py:352, src/zagg/runner.py:1149 and :3295. No route reaches _touch_s3_tree / _touch_s3_object around the check, and because the bucket is re-split per path, a footprint whose paths span buckets is decided path by path — correct by construction, though no test pins it.

The fragility is that the invariant is two ifs inside one loop body rather than a property of _touch_s3_object, the one function that actually issues the copy. Both helpers are directly callable and any future S3 entry point has to remember. Given the blast radius the issue quantifies (~332 GB of noncurrent versions per full-skip run, invisible to ListObjectsV2, on a bucket AWS sponsors), a redundant early return at the top of _touch_s3_object — counting nothing, so the path-based accounting above is untouched — buys an unbypassable invariant for one line. Judgment call; leaving it is defensible.

[espg]

🤖 from Claude

Accepted as a belt-and-braces net — added in 5b4268c1, with one constraint: it guards but does not count.

_touch_s3_object — the one function that issues CopyObject; a grep of the tree confirms s3.copy_object appears nowhere else — now opens with if _skip_published(bucket): logger.warning(...); return. So the invariant is a property of the copy seam rather than two ifs in one loop body, and no future entry point can bypass it. The path-based accounting stays at the call sites in touch_unit_footprint: counting here as well would double it, since that loop has already counted the path. WARNING level, as you'd expect for something unreachable in practice — a hit means a caller skipped touch_unit_footprint's guard, and the message says exactly that.

Pinned by tests/test_lifecycle.py::TestTouchS3::test_the_copy_seam_itself_refuses_a_published_bucket, which calls the private seam directly (the point of the test) and asserts no copy, no HEAD, and counts == {"touched": 0, "failed": 0}.

[espg]

🤖 from Claude (review)

Phase 4 adversarial review — 31c2453b, fresh context, scope limited to src/zagg/lifecycle.py + tests/test_lifecycle.py.

Verdict: the guard is correct and I found no route around it. Seven findings, none blocking; the two worth acting on before this lands are the stale narrative doc and the dropped skip count.

What I verified rather than assumed:

• Predicate. _skip_published is bucket in zagg.store._PUBLISHED_BUCKETS and nothing else — not _external_target(...). That is the distinction issue In-region writes to source.coop: execution role as the published identity + bucket-owner-full-control on output PUTs #495 calls load-bearing, and tests/test_lifecycle.py:329 pins it from the failing side: an injected-credential target on a non-published bucket is _external_target-true, and the test asserts it still copies. A guard mis-written as _external_target(...) fails that test.
• No bypass. touch_current_unit and touch_store_root both delegate to touch_unit_footprint; those three are the whole public surface (__all__), and every caller in the tree goes through them (hive.py:1521, processing/raster.py:1353, sweep_stages.py:352, runner.py:1149 / :3295). Both S3 arms of the loop check before any client work, so no LIST and no HEAD are issued either — test_published_target_is_not_touched_at_all asserts exactly that (get_paginator, head_object, copy_object all empty), which is what makes the assertion meaningful against this MagicMock fixture: a guard covering only one of the two loops trips one of those three lists.
• Mixed-bucket footprint. The bucket is re-split per path, so a footprint spanning buckets is decided path by path. Correct by construction; no test pins it, which I do not think is worth adding.
• Nothing lost. _copy_acl is still reachable and still correct for every path that still copies; the injected-credential arm stays pinned at tests/test_lifecycle.py:287 and the store seam at tests/test_store.py:130. Replacing test_ambient_published_target_copies_carry_the_acl was the right call — it asserted a copy that must no longer happen.
• Green locally: ruff check + ruff format --check clean on both files; pytest tests/test_lifecycle.py 28 passed.

Findings, by severity (all inline):

#	site	severity	summary
1	lifecycle.py:20	MEDIUM	docs/hive_layout.md:1065 still says every skipped object gets a fresh LastModified, and still frames versioning as an accepted caveat
2	lifecycle.py:196	MEDIUM	"skipped" is read by no caller, so the parquet/status record for a published run is indistinguishable from a touch that never ran
3	lifecycle.py:51	MEDIUM	the module's ACL bullet still advertises the published-copy path this commit made unreachable
4	lifecycle.py:299	MEDIUM	_copy_acl's "bucket is load-bearing" rationale is invalidated by the guard above it
5	lifecycle.py:228	LOW-MED	skipped counts paths while touched/failed count objects, undocumented; test:327 should assert == 4
6	lifecycle.py:230	LOW	per-unit log line repeats three lines of rationale and names no bucket
7	lifecycle.py:208	LOW	guard at the call sites rather than at _touch_s3_object; correct today, one line from unbypassable

Findings 1 and 2 are the substantive ones: 1 leaves the only user-facing description of the touch asserting the opposite of what ships, and 2 means the "not applicable" outcome exists in a dict key nobody reads and an INFO log nobody queries. 3 and 4 are stale prose in the file under review. 5-7 are judgment calls that a reply can close as by-design.

[espg]

🤖 from Claude (review)

[medium] never does not reach this touch on the one path that has a config — the staged sweep still self-copies the root core.

# sweep entry points carry no config — a sweep is driven by the store's own
# manifest — so a caller that has one passes it

That is true of the CLI entry point (python -m zagg.sweep --stages, sweep.py:1312) but not of the caller that actually fires in a run. runner.py:3376-3381:

if config.output.get("sweep") == "stages":
    from zagg.sweep_stages import stage_sweep_after_run

    stage_sweep_after_run(store_path, leaves, store_kwargs=store_kwargs)

The config is read on the line immediately above, and the same function passes policy=get_touch_policy(config) to touch_store_root forty lines earlier (runner.py:3325). But stage_sweep_after_run (sweep_stages.py:574) and run_stage_sweep (:381) take no touch_policy, so run_finisher runs at its auto default and this touch_unit_footprint issues a CopyObject on {root}/aggregation.yaml.

Failure mode, concretely: output.store: s3://collaborator-archive/... + output.touch: never + output.sweep: "stages". The bucket is not in _PUBLISHED_BUCKETS, so auto applies, and the operator who declared never gets one full-size new version of the aggregation core per staged sweep on a destination they told zagg not to touch. Issue #501 defines never as "never touch. For an archival destination" — this is a hole in exactly that case. (always leaks the other way: a published destination the operator overrode still gets skipped here.)

What I would change: thread touch_policy through stage_sweep_after_run → run_stage_sweep → run_finisher and pass get_touch_policy(config) at runner.py:3380, which costs one kwarg on each of two signatures. If that is deliberately out of scope, the fix is still to correct this comment — as written it asserts an invariant ("entry points carry no config") that the shipped caller falsifies, which is what would keep the next reader from noticing.

[espg]

🤖 from Claude

Fixed in 56057ae8 — the code fix, not the comment-only fallback. You are right that the comment asserted an invariant the shipped caller falsifies: runner.py reads config on the line above and then called stage_sweep_after_run without a policy.

touch_policy now threads stage_sweep_after_run → run_stage_sweep → run_finisher, defaulting to "auto" on each so the python -m zagg.sweep --stages CLI path is byte-identical, and runner.py passes touch_policy=get_touch_policy(config) at the chaining site. The wrong comment is gone; the replacement says what is actually true — the post-run chaining path holds a config, the CLI does not, which is what the auto default is for.

Two tests pin it:

• tests/test_sweep_stage.py::TestChainingAndCli::test_stage_sweep_after_run_honours_the_never_touch_policy — a staged sweep under never reports objects_touched == 0 / touch_skipped_paths == 1, and the default still touches the local core. Deleting touch_policy=touch_policy anywhere along the chain fails it (mutation-checked).
• tests/test_runner.py::TestDeclaredTouchPolicyReachesTheRootTouch::test_the_staged_sweep_chaining_carries_the_policy (in 209d50fb) covers the runner.py end of the wire; deleting that kwarg fails it.

[espg]

🤖 from Claude (review)

[medium] _copy_acl's stated invariant is falsified by always, and the docstring now argues for deleting the argument that keeps always from stripping ownership.

The docstring immediately below this line still says:

`bucket` is the DESTINATION being touched. Since phase 4 it cannot change this function's answer — :func:`_skip_published` returns before_copy_acl is ever called for a bucket in the published set, so only the credentials arm of the predicate can still fire here, and the store kwargs alone would do.

Phase 5 breaks both halves. With policy="always", _touch_applies returns True for a published bucket, so touch_unit_footprint:235 calls _copy_acl(store_kwargs, bucket) with bucket="us-west-2.opendata.source.coop" and the bucket arm of _external_target (store.py:124, bool(credentials) or bucket in _PUBLISHED_BUCKETS) is the one that fires — under the ambient execution role, it is the only arm that can.

The behaviour is right: always on a published bucket does carry bucket-owner-full-control, precisely because the argument was kept. The hazard is that the docstring now tells a maintainer the argument is dead ("the store kwargs alone would do"), and acting on that would silently strip bucket-owner ownership from every object an always run self-copies to Source Cooperative — the exact fail-open, invisible outcome the phase-4 grant round added this ACL to prevent.

What I would change: rewrite that paragraph to say the bucket arm is now LIVE (output.touch: always reaches _copy_acl for a published bucket), and pin it — the new test_policy_always_touches_the_published_destination_too asserts copy_object was called but never inspects the call, so assert client.copy_object.call_args[1]["ACL"] == "bucket-owner-full-control" would turn the most consequential side effect of the new knob into a regression guard for one extra line.

[espg]

🤖 from Claude

Both halves taken, in 211da61c.

The _copy_acl paragraph is rewritten: it now says the bucket arm is LIVE — under output.touch: always a published bucket reaches _copy_acl, and under the ambient execution role (no injected credentials) the bucket arm of _external_target is the only arm that can fire — so deleting the argument would strip bucket-owner-full-control from every object an always run self-copies to Source Cooperative. The docstring names the pinning test rather than arguing the argument is dead.

And the test hardening: test_policy_always_touches_the_published_destination_too now asserts the ACL on the copy, not just that a copy happened —

assert {c.kwargs["ACL"] for c in client.copy_object.call_args_list} == {_BUCKET_OWNER_ACL}

[espg]

🤖 from Claude (review)

[medium] The not-applicable log line (13 lines below, lifecycle.py:249-252) now misreports every skip a policy override caused. Flagging it here because this hunk is where the skip decision stopped being about publication.

logger.info(
    f"lifecycle touch not applicable for {skipped} path(s) on published bucket(s) "
    f"{sorted(skipped_buckets)} — see zagg.lifecycle._skip_published (issue #495 phase 4)"
)

Both halves are now conditionally false:

• policy="never" on a LOCAL store — bucket is None, so skipped_buckets stays empty and the operator reads lifecycle touch not applicable for 4 path(s) on published bucket(s) [] — see zagg.lifecycle._skip_published. It names a bucket set that is empty, on a path that has no bucket, and points at a predicate that was never consulted. test_policy_never_covers_local_paths_too walks straight through this line.
• policy="never" on any non-published S3 bucket — the bucket is real but is not published, so the line asserts it is.

The fix is cheap and the diff already carries the pattern: _touch_s3_object's refusal log was rewritten in this same commit to drop "published" and interpolate policy={policy!r}. This line deserves the same treatment, e.g. f"lifecycle touch not applicable for {skipped} path(s) under policy={policy!r}" + the bucket list only when non-empty, and see zagg.lifecycle._touch_applies as the pointer (which subsumes _skip_published under auto).

This matters more than a normal log nit because commit 5b4268c1 reworked this exact line so it would be the thing an operator reads at 3 a.m. to tell the published target from something new — under an override it now tells them the wrong thing.

[espg]

🤖 from Claude

Reworded in 211da61c, following the pattern the same commit used for the seam's refusal log:

where = f" on bucket(s) {sorted(skipped_buckets)}" if skipped_buckets else ""
logger.info(
    f"lifecycle touch not applicable for {skipped} path(s) under "
    f"policy={policy!r}{where} — see zagg.lifecycle._touch_applies (issue #501)"
)

The bucket list appears only when non-empty (so never on a local store no longer prints []), the policy is interpolated, and the pointer is _touch_applies, which subsumes _skip_published under auto. The surrounding comment and touch_unit_footprint's docstring were corrected the same way — a skip is now either the phase-4 published-bucket call or the operator's own declaration, and the log has to say which.

Pinned by tests/test_lifecycle.py::...::test_not_applicable_log_names_the_policy_not_the_inference: never on a local path, asserting policy='never' is in the line, bucket(s) is not, and _touch_applies is.

[espg]

🤖 from Claude (review)

[medium] Nothing pins that the config's declaration reaches the touch — all five policy=get_touch_policy(config) kwargs could be deleted with the suite green.

The lifecycle tests are excellent at the seam (I mutation-checked two of them: making _touch_s3_object policy-blind fails test_policy_always_touches_the_published_destination_too, and making never exempt local paths fails test_policy_never_covers_local_paths_too — those two are doing real work). But every one of them calls lifecycle.touch_current_unit(..., policy=...) directly. Grepping the suite for the wiring:

$ grep -rn 'output\["touch"\]\|output__touch' tests/
tests/test_config.py:3382, 3395     # get_touch_policy / validation only
tests/test_semantics.py:103, 104    # hash guard only

No test builds a config carrying output.touch and drives it through process_and_write_hive (this line), process_and_write_raster_hive (raster.py:1358), or either touch_store_root caller (runner.py:1151, :3325). So the plumbing — which is the entire claim of issue #501, that the operator's declaration governs the touch — is unprotected: drop the kwarg on any of the five and get_touch_policy still has passing unit tests, _touch_applies still has passing seam tests, and the store gets touched against the operator's instruction with no test failing.

What I would change: one end-to-end pin, reusing the skip-path test that already exists. tests/test_hive_windows.py:1658 asserts skipped["touched_objects"] > 0 and skipped["touch_failed"] == 0 on a real local hive skip run — a sibling of that test with config.output["touch"] = "never" asserting touched_objects == 0 and touch_skipped_paths > 0 covers this line, exercises never-on-local through the actual worker seam, and costs a handful of lines. A runner.py root-touch equivalent would close the other two.

[espg]

🤖 from Claude

Agreed, and this was the most valuable finding — the entire claim of issue #501 was untested. Fixed in 209d50fb, with your suggested shape plus coverage of the other sites. Every wiring site now has at least one test that fails when its policy= kwarg is deleted (each mutation-checked by actually deleting it and re-running):

site	pin
hive.py process_and_write_hive	test_hive_windows.py::TestProcessAndWriteHiveWindowed::test_declared_touch_policy_reaches_the_skip_seam — the sibling of :1658 you suggested: a real local hive skip run with output.touch: "never", asserting touched_objects == 0, touch_skipped_paths > 0, and that every mtime under the node is unmoved
processing/raster.py process_and_write_raster_hive	test_raster_pipeline.py::TestRasterHiveWorker::test_declared_touch_policy_reaches_the_skip_seam
runner.py:3325 (_run_local root touch)	test_runner.py::TestDeclaredTouchPolicyReachesTheRootTouch::test_never_leaves_the_root_objects_alone, with test_absent_still_touches_the_root as the auto-default counterpart
runner.py:1151 (RasterStrategy root touch)	test_raster_runner.py::TestRasterLocalRerun::test_declared_never_leaves_the_whole_store_alone — a real end-to-end agg(...) rerun; it covers the raster worker seam and the root seam in one run
runner.py staged-sweep chaining (new in 56057ae8)	test_runner.py::...::test_the_staged_sweep_chaining_carries_the_policy

The raster seam was cheap after all: TestRasterLocalRerun already had a real local skip rerun to sibling.

[espg]

🤖 from Claude (review)

[low] The validation is real but not total, and the value it does not catch fails SILENTLY toward auto — worth one WARNING rather than a code change.

The claim above this line —

# validated at load so a typo fails at submission rather than silently resolving to the default deep inside a worker's skip path

— holds for the routes I traced: validate_config reaches _validate_store_layout_keys on both the point branch (config.py:531) and the raster branch (:1214, via _validate_raster_config), and load_config (:175) and the two client submission paths (client.py:482, client_transport.py:492) all call validate_config. A YAML touch: nevr submitted normally does fail up front with an actionable message. Good.

What it does not cover is the worker's own funnel: load_config_from_dict (config.py:230-260) constructs the PipelineConfig and returns without validating. So a hand-built or hand-edited invoke event carrying touch: "nevr" reaches get_touch_policy → _touch_applies, where the fall-through

return not (bucket is not None and _skip_published(bucket))

quietly treats every unrecognized string as auto. On the archival non-published destination never exists for, that is the touch running against the operator's stated intent with nothing in the log.

I would not add validation to load_config_from_dict (that is a wider change than this issue, and the same gap already exists for store_layout and product_name). But get_touch_policy returning an unknown value without comment is avoidable: logger.warning there — or in _touch_applies — when the value is not in TOUCH_POLICIES costs one line and converts a silent wrong answer into a greppable one. Everything else about this block reads well: None falls through as absent, a YAML touch: true is rejected, and the message quotes both the legal set and the offending value.

[espg]

🤖 from Claude

Taken as the one-line warning in 211da61c; no validation added to load_config_from_dict (agreed that is wider than this issue, and the same gap stands for store_layout and product_name). get_touch_policy now warns when the declared value is not in TOUCH_POLICIES, naming the offending value and the legal set:

touch = config.output.get("touch", "auto")
if touch not in TOUCH_POLICIES:
    logger.warning(
        f"output.touch={touch!r} is not one of "
        f"{', '.join(repr(t) for t in TOUCH_POLICIES)} — falling through to 'auto' "
        "(the issue #495 phase 4 inference); this config was not validated at "
        "submission (issue #501)"
    )
return touch

Fall-through behaviour is unchanged — _touch_applies still treats an unrecognized value as auto — so a hand-built invoke event carrying touch: "nevr" still fails open, but greppably. Placed at the config-read seam rather than in _touch_applies so it fires once per touch call site instead of once per path (_touch_applies runs per path, thousands of times on an all-skip run). Pinned by tests/test_config.py::...::test_unvalidated_typo_warns_before_falling_through_to_auto.

[espg]

🤖 from Claude (review)

Phase 5 adversarial review — 81f4118 (issue #501). Five inline findings, none blocking the design: 3 medium, 1 medium test-coverage, 1 low. The shape of the change is right — an override layered on the inference rather than a replacement — and the parts I tried hardest to break held. What I checked and found clean:

auto is genuinely a no-op. Traced the default from get_touch_policy (config.output.get("touch", "auto"), and default_config() sets no key) through all three lifecycle entry points to _touch_applies, whose auto arm is not (bucket is not None and _skip_published(bucket)) — literally the phase-4 predicate plus a None guard for local paths. The truth table is what issue #501 asks for:

	auto	always	never
published bucket	skip	touch	skip
our / non-published bucket	touch	touch	skip
local path (bucket=None)	touch	touch	skip

Every caller with a config now consults it — hive.py:1527, raster.py:1358, runner.py:1151, runner.py:3325 — with the one exception in the staged-sweep finding.

always reaches the copy, and the tests prove it. I mutation-checked the seam: reverting _touch_s3_object's guard to phase 4's if _skip_published(bucket): fails test_policy_always_touches_the_published_destination_too, and weakening never to return bucket is None fails test_policy_never_covers_local_paths_too. Those two tests are load-bearing, not decorative — a policy-blind seam would have made always a silent no-op on exactly the destination it exists for, and the suite catches it. never is guarded at both the call site and the seam; _touch_s3_tree forwards policy correctly as the 7th positional.

The semantic-core claim survives inspection, and the guard is stronger than the issue argued. semantic_core (semantics.py:604-608) builds "output" as a hardcoded three-key literal — not an iteration over OUTPUT_LEAF_SHAPING_KEYS — so output.touch is excluded structurally, and I confirmed the tuple is still ("aoi_mask", "windowing", "time_source"). The new _cfg(output__touch=...) entries catch the realistic regression (someone adding a line to that literal). And because tests/test_semantics.py:492 already asserts set(core["output"]) == set(OUTPUT_LEAF_SHAPING_KEYS), adding touch to the tuple alone fails that older test — so the two guards close each other's gap. Worth noting since the new test's comment describes only the allowlist half.

Test hygiene around _CLIENTS is handled correctly. The three tests that install a second mock client inside one test body clear lifecycle._CLIENTS first; the fourth (test_policy_never_covers_local_paths_too) builds no client at all and needs no clear. Without those clears the second half of each test would silently assert against the first mock — I checked, and nothing is passing for that reason.

Docs read accurately. The docs/quickstart.md table matches the implementation on all three values including "local paths included" for never, and the touch_skipped_paths claim holds on every path I traced: hive.py:1541 and raster.py write it into the unit meta, _identity_counts rolls it up conditionally, _add_skipped_paths adds the root touch, and run_finisher records it too — in no case does a policy exclusion land in failed.

The three medium findings are all "the code is right, something that describes it is now wrong" — a stale _copy_acl invariant, a log line that names publication for a decision publication did not make, and a comment in sweep_stages.py whose premise the runner.py caller falsifies. The fourth is the missing end-to-end pin from config to touch. pytest tests/test_lifecycle.py tests/test_config.py tests/test_semantics.py is green (86 passed) on this worktree.

[espg]

🤖 from Claude

Phases 4 and 5 are both in, as separate commits in that order — the urgent guard first (31c2453b), the knob on top (81f41184) — so the NEON-timed piece stays isolated in history and splittable if the knob needs more discussion. This PR now closes #495 and #501.

Phase 4 stops the skip-run touch on published buckets. The touch defeats an expiration rule; an archival bucket has none, and Source Cooperative's is versioned, so the self-copy writes a new full-size version and demotes the old one to noncurrent — ~332 GB per full-skip run over the CA store, invisible to ListObjectsV2, on a bucket AWS sponsors. Guarded on bucket in _PUBLISHED_BUCKETS, deliberately not _external_target(...): that is also true for injected-credential targets whose lifecycle config we do not know, and skipping those could let a collaborator's data expire — the exact failure the touch prevents. The skip is recorded as touch_skipped_paths in the run record, never as a failure.

Phase 5 (#501) makes it declarable — output.touch: auto | always | never beside output.store, where auto is phase 4 verbatim and therefore a no-op for every existing config. output.touch is not in OUTPUT_LEAF_SHAPING_KEYS: output is allowlist-shaped, so a new key is excluded from semantic_hash by construction, and there is now a regression guard because that held only by omission.

Two things review caught that are worth your attention, both of which I had wrong:

The staged sweep was a hole in never. I wrote that sweep entry points carry no config. True of the --stages CLI; false of the caller that fires in a run — runner.py reads config.output["sweep"] and calls stage_sweep_after_run on the next line. So the finisher ran at auto and self-copied {root}/aggregation.yaml on a destination the operator had told zagg not to touch. Now threaded end to end.

always resurrects _copy_acl's bucket argument. Phase 4 made the published arm of that predicate unreachable, and I documented it as such. always makes it live again — and under the ambient execution role it is the only arm that can fire — so a maintainer acting on my docstring and deleting the argument would have silently stripped bucket-owner ownership from every object an always run copies to Source Cooperative. Fixed, and the test now asserts the ACL on the copy rather than just that a copy happened.

Also folded: the durable record now carries the not-applicable count (it was 0/0, indistinguishable from a touch that never ran); docs/hive_layout.md no longer promises the touch runs everywhere; the defence-in-depth guard inside the one function that issues CopyObject is policy-aware, without which always would have been a silent no-op on exactly the destination it exists for; and six wiring pins — one per call site, each mutation-checked by deleting its kwarg — because all five policy=get_touch_policy(config) arguments could previously have been removed with the suite green.

Six review rounds across five phases: 51 findings, every one folded as its own commit or answered on its thread. Still waiting on you for review/merge, the two standing grant items, and #498's disposition.