Skip to content

s3dol v1: Layer B/C, sibling capability stores, compat shim, testing (ADRs 0001–0012) - #20

Merged
thorwhalen merged 4 commits into
masterfrom
v1/core
Aug 12, 2026
Merged

thorwhalen merged 4 commits into
masterfrom
v1/core

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Stacked on #19 (Layer A). Implements the v1 surface per the accepted ADRs. Refs #11, closes #10.

What lands

module contents
base.py Layer B: BucketCollection/Reader/Store + Endpoint* triads + ObjectHandle. The leaf owns the prefix (pushed into ListObjectsV2; _key_of_id raises rather than slicing a non-matching id), key validity checked before the wire, __len__ refuses with guidance, __delitem__ idempotent (strict_delete opts into the probe), __contains__ resolves the body-less HEAD ambiguity with one cached HeadBucket, on_missing_bucket='assume' never probes and 'create' recovers rather than probes, setdefault/pop re-raise ObjectArchived instead of overwriting it
capabilities.py ADR-0011 D2/D3/D4: sibling stores BucketHandles/BucketUrls/BucketInfo + the handles/urls/info accessors (which refuse a wrapped store and name the parallel-wrap remedy), and the free functions sub/prefixes/delete_many/delete_bucket — including the outward key mapping dol does not provide
values.py, writes.py, reads.py ADR-0005: the closed write domain, str rejected on decidability, as_fileobj registered on io.IOBase (never typing.BinaryIO, which never fires), strategies as picklable frozen dataclasses
recipes.py Layer C: s3_store(...) + codec facades, built by composition only — no class statement
store.py the compat shim: v0's exact signature (incl. is_supabase_endpoint=), the three behaviours dependents' passing tests rely on, a corrected full-parse-or-passthrough de-chunker (v0's truncated multi-chunk payloads), and the ADR-0012 D7 migration warning
testing.py shipped in-memory fake + the exported conformance suite. The fake is the real BucketStore with only the _op_* backend seam swapped for a dict — so drift fails the same laws

utility.py is gone (5 of 7 exception classes and 5 of 9 Resp methods were dead); s3dol/tests/util.py stays, because py2store imports from it under suppress(ImportError).

One ADR amendment came out of implementation

ADR-0011 §D3b-amended (recorded in the ADR itself, not only here): the guarded url_for shim cannot serve both of its call paths. dol.content_url hands over a key already mapped through the outer layers — the walk stops at the provider "because that layer applies its own" — while a direct delegated wrapped.url_for(k) hands over the raw outer key. Applying _id_of_key is right for the first and wrong for the second; re-resolving through the chain is right for the second and double-transforms the first (measured: p/x/x/g). Nothing at the callee distinguishes them.

So url_for now applies dol's stated contract and refuses (NotSupported, naming s3dol.urls(store)[k]) once the leaf has ever been wrapped — driven by a sticky flag set when dol probes the leaf, which unlike the wrapped_self weakref survives the wrapper's death, so the refusal is reliable rather than intermittent.

This is the P1 evidence for dol#86/#83: a keyed capability method is unfixable in the general case — not merely fragile, and not fixable by a better resolution primitive, because the ambiguity is in the call, not the resolution. Sibling stores route through __getitem__, which has one key domain and one caller, and stay correct by construction.

Adversarial review

A 4-lens refute panel (ADR conformance / logic / secrets+state / API+test honesty) produced 39 findings; every one that reproduced is fixed in-branch with a regression test:

  • critical — diagnose() echoed v1 credential kwargs verbatim (the denylist of two v0 names let credentials=('AK','SECRET') and client_kwargs={...} straight through). Now an allowlist, with the access key id shown by prefix only.
  • critical — the divergence table both cried wolf (every env-credentials user told their credentials change) and missed the real move (explicit-vs-env compared equal because both provenance labels truncate to AKIA…). Replaced by a pure identity predicate.
  • env-endpoint userinfo leaked through the C2 exception, the AmbiguousResolution warning, Resolution's repr and diagnose — now redacted at capture, so all four paths are fixed at once while the host survives for detection.
  • client_kwargs was an unpoliced credential carrier (bypassing normalisation, the anon check, and repr redaction) — now structurally refused, mirroring ADR-0012 D4's preset allowlist.
  • validate_endpoint_hygiene echoed the URL it refuses — the exact leak it exists to prevent.
  • credentials=(k, s, None) became the truthy token 'None'; signature_version=None silently re-enabled the SigV2 downgrade this whole layer exists to fix; the C2 guard exempted named soft rows (so preset='aws' + a wasabi env endpoint proceeded silently); deny_means_absent swallowed clock-skew 403s into "key absent"; region capture hijacked an explicit AWS_DEFAULT_REGION from any xxxN hostname label; CallableCredentials double-fetched (burning a single-use token); preset error_overrides kinds were unvalidated (a typo detonated inside the error seam); ambiguous HEAD guessed with no resolver bound (now propagates); diagnose() read the developer's real ~/.aws in its "tier 1" tests (now injectable); a module-level importorskip('moto') skipped 60 pure tier-1 tests.

Two more bugs were caught by the new tests themselves rather than the panel: the content_url double-transform above, and the presign expiry clamp not firing through the fake.

Tests

299 tests + doctests, green. CI now runs 3.10 + 3.12 with --doctest-modules, so every documented example executes against the in-memory fake with no endpoint.

Release ordering (ADR-0007 §5)

Dependent pins are open: reeleehq/lacing#26, thorwhalen/reelee#294, cosmograph-org/http_cosmo_prep#2 (not ours to merge). Do not merge this before those land — and note s3dol.diagnose() ships in #19 precisely so each dependent can record what their environment resolves to before anything moves.

https://claude.ai/code/session_01KqivR5Be21Wh7yRdEG4fiY

…/0012)

Additive, non-breaking 0.1.x change: four new modules, no v0 code touched.
This is ADR-0007 §5 step 0 — s3dol.diagnose() ships BEFORE any resolution
behaviour changes, so dependents can record what their environment resolves
to (and see the v0-vs-v1 divergence table) ahead of the v1 release.

- errors.py: the ADR-0004 taxonomy (KeyError family with unmangled __str__,
  auth errors never KeyError), pure (operation, code, status) classification
  incl. the body-less HEAD ambiguity moto cannot produce, per-provider
  override rows, tested redaction, the one translate_s3_errors seam, and the
  ADR-0012 ConfigurationError + S3DolWarning trees.
- presets.py: Preset/Capabilities rows per ADR-0003 amended by ADR-0012 D4
  (host_patterns/params/requires_endpoint/soft/config_kwargs, derived pinned
  gated on verified, mandatory addressing_style with 'auto' illegal,
  structural no-credential rule, endpoint hygiene). 15 doc-sourced rows.
- connection.py: frozen picklable S3Connection (no live objects, per-instance
  build lock, mandatory __getstate__/__setstate__, redacting repr); pure
  resolve(spec, environ, aws_config) with per-field provenance, the two-rung
  endpoint ladder + C1/C2 guards, two-pass preset selection, omit-not-None
  Config semantics; credential-shape normalisation with per-shape rejections;
  explicit SigV4 always (the #10 root cause, fixed at this layer).
- _diagnose.py: never-raising diagnose() with provenance table and the
  ADR-0012 D7 v0-vs-v1 differential (v0_mangle run through v1's own resolver
  twice). Module named _diagnose so s3dol.diagnose is unambiguously the
  function (a diagnose.py submodule would shadow it after first import).

134 new tier-1/2 tests + 12 doctests; full suite green. Refs #11, refs #10
(closed when the S3Store path switches to S3Connection in the v1 PR).

Claude-Session: https://claude.ai/code/session_01KqivR5Be21Wh7yRdEG4fiY
…esting

Implements the v1 surface on top of Layer A (#19), per ADRs 0001-0012.

Layer B (base.py): BucketCollection/Reader/Store and Endpoint* triads +
ObjectHandle. The leaf owns a normalized prefix (pushed into ListObjectsV2,
_key_of_id RAISES rather than slicing a non-matching id), key validity is
checked before the wire, __len__ refuses with guidance, __delitem__ is
idempotent (strict_delete opts into the probe), __contains__ resolves the
body-less HEAD ambiguity with one cached HeadBucket, on_missing_bucket
defaults to 'assume' (no probe, ever) and 'create' recovers rather than
probes, and setdefault/pop re-raise ObjectArchived instead of overwriting it.
Backend access goes through a module-internal _op_* protocol (ADR-0002's
seam), which is what lets the shipped fake BE the real store.

ADR-0011: no key-taking public methods. Keyed capabilities are sibling stores
(BucketHandles/BucketUrls/BucketInfo, derived via s3dol.handles/urls/info,
which refuse a wrapped store and name the parallel-wrap remedy); non-keyed
operations are free functions (sub/prefixes/delete_many/delete_bucket) with
the outward key mapping dol does not provide. A reflective conformance test
(D5) enumerates every public attribute against an explicit inventory.

**ADR-0011 D3b amended** (recorded in the ADR, not just here): the guarded
url_for shim cannot serve both its call paths — dol.content_url hands over a
key already mapped through the outer layers while a direct delegated call
hands over the raw outer key, and applying _id_of_key is right for one and
double-transforms the other (measured: p/x/x/g). It now applies dol's stated
contract and REFUSES once the leaf has been wrapped, via a sticky probe flag
that (unlike the wrapped_self weakref) survives the wrapper's death. This is
the P1 evidence for dol#86/#83: a keyed capability method is unfixable in the
general case, because the ambiguity is in the call, not the resolution.

Also: values.py (the closed write domain, str rejected on decidability,
as_fileobj registered on io.IOBase not typing.BinaryIO), writes/reads
strategies as picklable frozen dataclasses, recipes.py (s3_store + codec
facades built by composition only), store.py compat shim preserving the three
behaviours dependents' passing tests rely on plus a corrected
full-parse-or-passthrough de-chunker and the D7 migration warning, and
s3dol.testing shipping an in-memory fake + the exported conformance suite.

Adversarial review (4 lenses, 39 findings) fixed in-branch: diagnose echoed
v1 credential kwargs verbatim; env-endpoint userinfo leaked through C2
messages, warnings, Resolution repr and diagnose; client_kwargs was an
unpoliced credential carrier; hygiene refusals echoed the URL they refuse;
(k, s, None) became the truthy token 'None'; signature_version=None silently
re-enabled SigV2; the C2 guard exempted named soft rows; deny_means_absent
swallowed clock-skew 403s; the divergence table both cried wolf and missed
the real identity move; region capture hijacked AWS_DEFAULT_REGION;
CallableCredentials double-fetched; preset error_overrides kinds were
unvalidated; ambiguous HEAD guessed with no resolver bound; diagnose read the
developer's real ~/.aws in "tier 1" tests; a module-level importorskip skipped
60 pure tests. Regression tests added for each.

dol floor >= 0.3.63 (ADR-0006 §3 policy). 299 tests + doctests green; CI now
runs 3.10 + 3.12 with --doctest-modules. Refs #11, closes #10.

Claude-Session: https://claude.ai/code/session_01KqivR5Be21Wh7yRdEG4fiY
An exception payload carrying keys is one of the shapes ADR-0011 lists as
inexpressible for a codec boundary (the raise site has no boundary frame) —
surfaced concretely by the dol P1 trial (dol#86). But a free function holds
the store, so the inverse walk IS available here: .succeeded/.failures now
carry the keys the caller passed, not wire keys they never used.

Claude-Session: https://claude.ai/code/session_01KqivR5Be21Wh7yRdEG4fiY
Base automatically changed from v1/layer-a to master August 11, 2026 17:19
@thorwhalen

Copy link
Copy Markdown
Member Author

⛔ Do not merge yet — and the reason is the ADR, not the code

CI is green and the diff is reviewed (4-lens adversarial panel, 39 findings, all reproducible ones fixed with regression tests). What blocks the merge is ADR-0007 §5's release ordering, which is load-bearing rather than ceremonial because merging auto-publishes to PyPI and version numbers burn permanently.

step state
0 — ship s3dol.diagnose() in a non-breaking 0.1.x release ✅ done — #19 merged; publishing now
0b — each dependent runs diagnose() in their real environment and records the answer ⛔ outstanding — needs a human, see below
1 — land the dol fixes, raise the floor ✅ done — floor is dol>=0.3.63 in this PR; the local workarounds died with it
2 — pin s3dol<1 in the dependents 🟡 2 of 3: reeleehq/lacing#26 ✅ merged, thorwhalen/reelee#294 ✅ merged, cosmograph-org/http_cosmo_prep#2 open — not ours to merge
3 — merge v1 ⛔ this PR

What specifically needs to happen first

  1. cosmograph-org/http_cosmo_prep#2 must land. That repo does from s3dol.store import S3Store while not declaring the dependency at all, so today it takes whatever s3dol the environment has. It is also the one known dependent that passes an explicit endpoint_url= for a non-AWS endpoint — precisely the argument v0 silently drops whenever AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY are exported. v1 honours it, which can move where their data goes. ADR-0007 §5 says this explicitly: "Step 2 before step 3 is not optional: a stalled cross-org PR must not be able to strand that repo on a broken line."

  2. Run s3dol.diagnose(**your_S3Store_kwargs) in each dependent's real deployment environment and read the divergence table. That is the entire point of step 0 and it cannot be done from here — it depends on which env vars and AWS config those environments actually have. The table now reports three things it did not when the ADR was written: a credential-identity predicate (not a label comparison, which both cried wolf for every env-credentials user and missed the real explicit-vs-env override), the v0 branch that raises where v1 falls through to the chain, and the settings pass-2 detection newly supplies for a recognised provider endpoint (the announced path→virtual flip of ADR-0012 D4).

  3. Optionally re-enable lacing's tests/test_artifact_store_s3.py (currently excluded with "fail in CI's clean env (s3dol url_for → None)") against this branch — ADR-0008 proposes it as the cross-repo gate, and a disabled test cannot gate anything. The url_for → None cause is fixed here.

One behaviour change reviewers should look at first

BucketReader.url_for refuses once the leaf has been wrapped by a dol key/value wrapper, instead of guessing. See the PR body and the ADR-0011 §D3b amendment: two call paths reach that one signature with different key domains and nothing distinguishes them, so no implementation is correct for both. dol.content_url stays correct for an unwrapped store (the common case) and raises for a wrapped one. The canonical form, s3dol.urls(store)[k], is correct at any wrapper depth and lifetime.

This was trialled against dol's Option G engine and the finding is posted upstream: i2mint/dol#86 (comment)

@thorwhalen thorwhalen mentioned this pull request Aug 11, 2026
9 tasks
master moved when #19 squash-merged (and CI pushed the 0.1.14 version bump),
which made every Layer A file an add/add conflict against this branch. Resolved
by taking this branch's versions throughout: v1/core already contains master's
Layer A **plus** the adversarial-review fixes that were applied here after #19
was opened — so master is strictly behind on those files.

Concretely, the fixes that live only here (and would have been lost by taking
master's side): the diagnose() credential-echo allowlist, endpoint-URL
redaction at capture, the client_kwargs credential refusal, the
signature_version=None refusal, the C2 soft-row scope, the deny_means_absent
clock-skew guard, the credential-identity predicate, and the region-capture
tightening.

Verified after resolution: 300 tests + doctests pass, ruff clean, and both
review-fix markers are present in the merged files.

Claude-Session: https://claude.ai/code/session_01KqivR5Be21Wh7yRdEG4fiY
@thorwhalen
thorwhalen merged commit 794d534 into master Aug 12, 2026
10 checks passed
@thorwhalen
thorwhalen deleted the v1/core branch August 12, 2026 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

url_for produces SigV2 presigned URLs, which modern S3 and every major S3-compatible provider reject

1 participant