fix(client): split the conflated hash in registerEnv, so an inline secret can register (#157)

* fix(client): split the conflated hash in registerEnv, so an inline secret can register

Closes serve-stack KNOWN-ISSUES 20. `env.register()` carrying
`secrets: { KEY: { inline: … } }` threw IntegrityMismatchError against any real
StorageProvider — unconditionally, on both shipped secret stores.

ROOT CAUSE. One value was asked to be two incompatible things. The hash was
computed from the def while its secretRefs still held PLACEHOLDER names, then
staging replaced each placeholder with a store-returned ref that is fresh on
every call (`local-secret://<uuid>`, or a random-suffixed ARN), and the mutated
def was written at a URI carrying the pre-staging hash. `putBlob` re-hashes what
it stores and correctly rejected the mismatch.

  Idempotency key needs to be STABLE across stagings  -> must EXCLUDE the fresh ref
  Content address needs to EQUAL the bytes stored     -> must INCLUDE the fresh ref

FIX. Separate the roles. `idempotencyKey` stays the placeholder-derived hash;
`contentHash` becomes the hash of the bytes actually written, so a `sha256:` URI
describes its own contents again and putBlob's check passes because it is TRUE,
not because it was relaxed. This also aligns the client with the worker, which
already hashes the fetched BYTES (bundle-fetcher.ts:113) — the placeholder hash
was wrong at both ends.

The idempotency key is persisted in the blob, because after the split
`resolveLatest` reports the content address and there is nowhere else to read it
from. A bundle registered before this change carries no key, reads as undefined,
and is treated as not-idempotent: it re-registers once and is stable thereafter.

The UUID stays, deliberately: it keeps the ref opaque in a blob that
content-addressed storage hands to any reader, avoids clobbering a value an
in-flight dispatch is mid-resolve() on, and gives each staging its own TTL.

AND FIXED THE INSTRUMENT. The unit stub read the content hash out of the URI and
never recomputed it, so it accepted a pinned URI whose hash did not match its
bytes — which is exactly how an unconditionally-broken path shipped under a green
suite. The stub now enforces what a real provider enforces. Mutation-verified:
re-introducing the defect turns the PRE-EXISTING env-register tests red, where
before they stayed green.

New tests run against the REAL LocalStorageProvider, in `pnpm -r test` rather
than the Docker-gated lane CI never runs — this defect's survival is attributable
to that lane, so a fix verified only there would be unverified in practice.

NOT VERIFIED: that AwsSecretStore fails and is fixed identically. Its ref is a
random-suffixed ARN so the mechanism is the same by construction, but there are
no AWS credentials in this environment and the claim rests on reading, not
running.

Repo-wide: build, lint, typecheck clean; every package's suite green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs: record issue 20 as fixed, and that the AwsSecretStore half stays unverified

Follows the code fix in this branch.

- KNOWN-ISSUES 20 -> FIXED, naming both halves: the split itself, and the
  correction to the unit stub that hid it. The stub fix is called out as the
  more durable half — it is the instrument, and every test built on it was blind
  to this class of defect.
- The at-a-glance table's "issue 20 is the only outright breakage left" line is
  replaced rather than deleted: nothing is currently known-broken, what remains
  is one open feature request (17's base-tree/patch half) and one unverified
  claim.
- The design spec is marked IMPLEMENTED, keeping its original status visible.

The unverified AwsSecretStore half is stated in all three places rather than
quietly dropped. Its spec §5 AC6 says silence is not acceptable there, and a
fix that reads as covering both stores when only one was exercised is exactly
the overclaim this project's threat model warns against.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: BrettNye

deploy/serve-stack/KNOWN-ISSUES.md

@@ -36,9 +36,11 @@ reader does not have to scroll 2500 lines to find the one thing still broken.
 | **17** | dev shapes pinned to a placeholder image | **PREMISE FALSE** (17a, 17b) — base-tree/patch half still open |
 | **18** | `git()` decodes stdout as UTF-8 | **FIXED** (#145) |
 | **19** | no repository history in the workspace | **WITHDRAWN** (19a) — capabilities *can* carry `.git`, measured |
-| **20** | `env.register()` + inline secret always throws | **OPEN** — root-caused, fix designed, not implemented |
+| **20** | `env.register()` + inline secret always throws | **FIXED** (#157) — and the unit stub that hid it was corrected |
 
-**If you read one line: issue 20 is the only outright breakage left.**
+**Nothing in this file is currently known-broken.** What remains is one open
+feature request (17's base-tree/patch half) and one unverified claim (whether
+`AwsSecretStore` behaves identically under 20's fix — no AWS credentials here).
 
 Two entries — 17 and 19 — were filed on premises that did not survive
 verification. They are kept rather than deleted, with the original text intact
@@ -2527,15 +2529,34 @@ with shallow-depth support, rather than a consumer discovering it works by tryin
 
 ## 20. `env.register()` with an inline secret always throws `IntegrityMismatchError`
 
-**Status: OPEN — root-caused, fix designed, NOT implemented.** The only
-outright-broken item left in this file. Reproduced with a minimal case plus a
-control; the cause is a conflated key at `pangolin-client/src/env-register.ts`,
-where `computeContentHash(def)` serves as BOTH the content address and the
-idempotency key. The content address must INCLUDE the freshly-staged secret ref;
-the idempotency key must EXCLUDE it. Splitting the two is the fix
-([#148](https://github.com/QuarrySystems/pangolin/pull/148) files it and specs
-the split). Keeping the UUID suffix was decided deliberately — for opacity,
-non-clobbering re-stage, and per-staging TTL.
+**Status: FIXED** ([#157](https://github.com/QuarrySystems/pangolin/pull/157);
+filed and spec'd in [#148](https://github.com/QuarrySystems/pangolin/pull/148)).
+The two roles are now separate values in `registerEnv`: `idempotencyKey` remains
+the placeholder-derived hash, and `contentHash` is the hash of the bytes actually
+written — so a `sha256:` URI describes its own contents again and `putBlob`'s
+check passes because it is true, not because it was relaxed. That also aligns the
+client with the worker, which already re-hashes the fetched BYTES
+(`bundle-fetcher.ts:113`); the placeholder hash was wrong at both ends.
+
+The key is persisted in the blob, since after the split `resolveLatest` reports
+the content address and there is nowhere else to read it from. Bundles registered
+before the change carry no key, read as `undefined`, and re-register once.
+
+The UUID stayed, as the design argued: opacity in a blob that content-addressed
+storage hands to any reader, no clobbering of a value an in-flight dispatch is
+mid-`resolve()` on, and a per-staging TTL.
+
+**The instrument was fixed too, and that is the more durable half.** The unit
+stub read the content hash out of the URI and never recomputed it, so it accepted
+a pinned URI whose hash did not match its bytes — which is exactly how an
+unconditionally-broken path shipped under a green suite. It now enforces what a
+real provider enforces. Mutation-verified: re-introducing the defect turns the
+PRE-EXISTING tests red, where before they stayed green.
+
+**Still unverified:** that `AwsSecretStore` fails and is fixed identically. Its
+ref is a random-suffixed ARN so the mechanism is the same by construction, but
+there are no AWS credentials in this environment and the claim rests on reading,
+not running.
 
 **Unconditional, not flaky, and it affects both shipped secret stores.** A
 registration carrying `secrets: { KEY: { inline: … } }` cannot succeed against a

docs/superpowers/specs/2026-08-03-env-register-split-key-design.md

@@ -1,7 +1,7 @@
 ---
 title: Split the Conflated Content Hash in `registerEnv` — Design
 date: 2026-08-03
-status: **DESIGNED — ready for a plan.** Root cause confirmed by minimal reproduction with a control. One half is explicitly NOT verified and is marked inline.
+status: **IMPLEMENTED (2026-08-03, #157).** The split shipped; the AwsSecretStore half remains unverified (no AWS credentials in this environment). **Originally:** DESIGNED — ready for a plan. Root cause confirmed by minimal reproduction with a control. One half is explicitly NOT verified and is marked inline.
 branch: fix/env-register-split-key-spec
 authors: [human:Brett, agent:claude-opus-5]
 severity: high (a documented, shipped API path cannot succeed against real storage)

packages/pangolin-client/src/env-register.ts

@@ -100,10 +100,7 @@ function inlineSecretPlaceholder(envBundleName: string, secretKey: string): stri
  * Idempotent: re-registering with identical inputs returns the existing
  * `EnvRef` without bumping `registeredAt` or writing a new blob.
  */
-export async function registerEnv(
-  client: PangolinClient,
-  opts: RegisterEnvOpts,
-): Promise<EnvRef> {
+export async function registerEnv(client: PangolinClient, opts: RegisterEnvOpts): Promise<EnvRef> {
   // 1. Scan `values:` entries for credential patterns. First match throws
   //    so the caller fixes one finding and re-runs.
   const values = opts.values ?? {};
@@ -142,9 +139,7 @@ export async function registerEnv(
   let store: SecretStore | undefined;
   if (inlineSecretKeys.length > 0) {
     if (!opts.secretStore) {
-      throw new Error(
-        'registerEnv: secretStore is required when the bundle has inline secrets',
-      );
+      throw new Error('registerEnv: secretStore is required when the bundle has inline secrets');
     }
     store = client.secretStores[opts.secretStore];
     if (!store) {
@@ -162,7 +157,19 @@ export async function registerEnv(
     // Undefined for pure-values / ref-only bundles (no staging needed).
     store: store?.name,
   };
-  const contentHash = computeContentHash(def);
+  // The IDEMPOTENCY KEY, not the content address. Hashed from the def while its
+  // secretRefs still hold PLACEHOLDER names, so it is stable across stagings —
+  // which is exactly what an idempotency key needs and exactly what a content
+  // address must not be.
+  //
+  // These were one value until KNOWN-ISSUES 20. They cannot be: staging below
+  // replaces every placeholder with a store-returned ref that is fresh on every
+  // call (`local-secret://<uuid>`, or a random-suffixed ARN), so a hash taken
+  // before staging can never describe the bytes written after it. `putBlob`
+  // re-hashes what it stores and correctly rejected the mismatch, making
+  // `env.register()` with an inline secret fail unconditionally against any real
+  // StorageProvider.
+  const idempotencyKey = computeContentHash(def);
 
   const baseUri = buildPangolinUri({
     namespace: client.namespace,
@@ -176,12 +183,19 @@ export async function registerEnv(
   //    before this check would crash on the second identical call
   //    (ResourceExistsException) or hand back a fresh ref that breaks
   //    hash equality.
+  //    The comparison is against the key PERSISTED IN THE PREVIOUS BLOB, not
+  //    against `latest.contentHash` — since the split, that is the hash of the
+  //    stored bytes (post-staging) and so is not comparable to a key computed
+  //    pre-staging. A bundle registered before this change carries no key and
+  //    reads as `undefined`, which is treated as "not idempotent": it
+  //    re-registers once, re-stages once, and is stable thereafter.
   const latest = await client.storage.resolveLatest(baseUri);
-  if (latest && latest.contentHash === contentHash) {
+  if (latest && (await readIdempotencyKey(client, latest.uri)) === idempotencyKey) {
     return {
       name: opts.name,
       registeredAt: latest.registeredAt,
-      contentHash,
+      // The stored blob's real content address, NOT the idempotency key.
+      contentHash: latest.contentHash,
     };
   }
 
@@ -206,30 +220,60 @@ export async function registerEnv(
     }
   }
 
-  // 5. Write the bundle payload at the pinned URI. The pinned URI uses
-  //    the placeholder-derived contentHash (stable across calls); the
-  //    blob body carries the real opaque refs.
+  // 5. Write the bundle payload at the pinned URI.
+  //
+  //    The payload carries the idempotency key alongside the real opaque refs,
+  //    because the next `registerEnv` call has nowhere else to read it from:
+  //    `resolveLatest` reports the content address, which by construction now
+  //    reflects post-staging bytes.
+  //
+  //    Write the CANONICAL JSON bytes (sorted-key serialization) — not
+  //    `JSON.stringify`. The storage provider recomputes the byte-hash and
+  //    compares against the pinned URI's hash, and the worker's bundle-fetcher
+  //    re-hashes the fetched BYTES the same way, so insertion-order JSON would
+  //    diverge on both sides.
+  const payload = { ...def, idempotencyKey };
+  const bytes = new TextEncoder().encode(canonicalJsonString(payload));
+
+  //    The content address is the hash of the bytes ACTUALLY WRITTEN. A
+  //    `sha256:` URI therefore describes its own contents again, and `putBlob`'s
+  //    check passes because it is true — not because it was relaxed.
+  const contentHash = computeContentHash(bytes);
   const pinnedUri = buildPangolinUri({
     namespace: client.namespace,
     type: 'env',
     name: opts.name,
     contentHash,
   });
-  // Write the CANONICAL JSON bytes (sorted-key serialization) — not
-  // `JSON.stringify(def)`. The storage provider recomputes the byte-hash
-  // and compares against the pinned URI's hash; if we wrote insertion-
-  // order JSON, the byte-hash would diverge from the canonical-object
-  // hash embedded in the URI and `put` would throw IntegrityMismatchError.
-  // The worker's bundle-fetcher re-parses these bytes as JSON and
-  // re-hashes the resulting object via canonical JSON, so the round-trip
-  // remains coherent on both sides.
-  await client.storage.put(
-    pinnedUri,
-    new TextEncoder().encode(canonicalJsonString(def)),
-  );
+  await client.storage.put(pinnedUri, bytes);
 
   // The storage layer is the authority on registeredAt — re-read it.
   const after = await client.storage.resolveLatest(baseUri);
   const registeredAt = after?.registeredAt ?? new Date().toISOString();
   return { name: opts.name, registeredAt, contentHash };
 }
+
+/**
+ * Read the idempotency key persisted in a previously-registered env blob.
+ *
+ * NEVER throws: a missing blob, unreadable bytes, non-JSON content, or a blob
+ * predating this field all yield `undefined`, which the caller treats as "not
+ * idempotent". Failing closed here would turn an unreadable prior registration
+ * into a hard error on a path whose whole job is to skip redundant work.
+ */
+async function readIdempotencyKey(
+  client: PangolinClient,
+  pinnedUri: string,
+): Promise<string | undefined> {
+  try {
+    const bytes = await client.storage.get(pinnedUri);
+    const parsed: unknown = JSON.parse(new TextDecoder().decode(bytes));
+    if (parsed && typeof parsed === 'object') {
+      const key = (parsed as Record<string, unknown>).idempotencyKey;
+      if (typeof key === 'string') return key;
+    }
+    return undefined;
+  } catch {
+    return undefined;
+  }
+}

packages/pangolin-client/test/env-register-integrity.test.ts

@@ -0,0 +1,163 @@
+// registerEnv against a REAL StorageProvider (serve-stack KNOWN-ISSUES 20).
+//
+// Deliberately NOT using the in-memory stub from env-register.test.ts. That stub
+// reads the content hash out of the URI instead of computing it from the bytes
+// (`env-register.test.ts` `put`: `parts[parts.length - 1]`), so it accepts a
+// pinned URI whose hash does not describe its own contents. Every test built on
+// it is blind to this entire class of defect — the instrument was lying, which
+// is why an unconditionally-broken API path shipped.
+//
+// `LocalStorageProvider` performs the same byte re-hash the worker does
+// (`bundle-fetcher.ts:113` hashes the fetched BYTES), so this file exercises the
+// invariant both ends actually enforce.
+//
+// Lane: this is a normal unit test, run by `pnpm -r test`. The defect survived
+// because its only executing check lived in the Docker-gated E2E suite, which CI
+// does not run — a fix verified only there would be unverified in practice.
+
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import { mkdtemp, rm } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { randomUUID } from 'node:crypto';
+import { PangolinClient } from '../src/index.js';
+import { registerEnv } from '../src/env-register.js';
+import { LocalStorageProvider } from '@quarry-systems/pangolin-storage-local';
+import { computeContentHash } from '@quarry-systems/pangolin-core';
+import type { SecretStore } from '@quarry-systems/pangolin-core';
+
+/**
+ * Mimics the shape that breaks hash equality: `LocalSecretStore.stage` returns
+ * `local-secret://<randomUUID>`, and `AwsSecretStore` a random-suffixed ARN.
+ * The randomness is the point — a name-derived ref would hide the defect.
+ */
+function makeCountingStore(): SecretStore & { stageCalls: string[] } {
+  const stageCalls: string[] = [];
+  return {
+    name: 'counting',
+    stageCalls,
+    async stage({ name }: { name: string }) {
+      stageCalls.push(name);
+      return { ref: `local-secret://${randomUUID()}` };
+    },
+    async resolve() {
+      return 'unused';
+    },
+  } as unknown as SecretStore & { stageCalls: string[] };
+}
+
+describe('registerEnv integrity against a real StorageProvider', () => {
+  let root: string;
+  let storage: LocalStorageProvider;
+
+  beforeEach(async () => {
+    root = await mkdtemp(join(tmpdir(), 'env-reg-'));
+    storage = new LocalStorageProvider({ rootDir: root });
+  });
+  afterEach(async () => {
+    await rm(root, { recursive: true, force: true });
+  });
+
+  function makeClient(store: SecretStore): PangolinClient {
+    return new PangolinClient({
+      namespace: 'ns',
+      compute: {},
+      credentials: {},
+      storage,
+      targets: {},
+      secretStores: { default: store },
+    });
+  }
+
+  it('registers a bundle carrying an inline secret without throwing IntegrityMismatchError', async () => {
+    const store = makeCountingStore();
+    const res = await registerEnv(makeClient(store), {
+      name: 'prod',
+      values: { REGION: 'us-west-2' },
+      secrets: { GH_TOKEN: { inline: 'super-secret-value' } },
+      secretStore: 'default',
+    });
+    expect(res.name).toBe('prod');
+    expect(res.contentHash).toMatch(/^sha256:[0-9a-f]{64}$/);
+    // Control: the staging actually happened, so this is a real inline path and
+    // not a bundle that quietly took the no-secrets branch.
+    expect(store.stageCalls).toHaveLength(1);
+  });
+
+  it("the pinned URI's hash equals a re-hash of the stored bytes", async () => {
+    const store = makeCountingStore();
+    const res = await registerEnv(makeClient(store), {
+      name: 'prod',
+      secrets: { GH_TOKEN: { inline: 'super-secret-value' } },
+      secretStore: 'default',
+    });
+    // Asserted directly rather than inferred from the absence of a throw: this
+    // is the invariant putBlob enforces and the worker re-checks.
+    const bytes = await storage.get(`pangolin://ns/env/prod/${res.contentHash}`);
+    expect(computeContentHash(bytes)).toBe(res.contentHash);
+  });
+
+  it('the stored blob carries the real opaque ref, not the placeholder and not the secret value', async () => {
+    const store = makeCountingStore();
+    const res = await registerEnv(makeClient(store), {
+      name: 'prod',
+      secrets: { GH_TOKEN: { inline: 'super-secret-value' } },
+      secretStore: 'default',
+    });
+    const body = new TextDecoder().decode(
+      await storage.get(`pangolin://ns/env/prod/${res.contentHash}`),
+    );
+    // A presence beside the two absences, so the absences are not an empty read.
+    expect(body).toContain('local-secret://');
+    expect(body).not.toContain('super-secret-value');
+    expect(body).not.toContain('pangolin/inline/env-prod/GH_TOKEN');
+  });
+
+  it('registering the same bundle twice stages exactly once and reuses registeredAt', async () => {
+    const store = makeCountingStore();
+    const client = makeClient(store);
+    const opts = {
+      name: 'prod',
+      values: { REGION: 'us-west-2' },
+      secrets: { GH_TOKEN: { inline: 'super-secret-value' } },
+      secretStore: 'default',
+    };
+    const first = await registerEnv(client, { ...opts });
+    const second = await registerEnv(client, { ...opts });
+
+    // The single stage call is the control proving the early return fired,
+    // rather than the test never reaching the idempotent path.
+    expect(store.stageCalls).toHaveLength(1);
+    expect(second.registeredAt).toBe(first.registeredAt);
+  });
+
+  it('registering a bundle whose values changed stages again and yields a different pinned URI', async () => {
+    const store = makeCountingStore();
+    const client = makeClient(store);
+    const first = await registerEnv(client, {
+      name: 'prod',
+      values: { REGION: 'us-west-2' },
+      secrets: { GH_TOKEN: { inline: 'v1' } },
+      secretStore: 'default',
+    });
+    const second = await registerEnv(client, {
+      name: 'prod',
+      values: { REGION: 'eu-west-1' },
+      secrets: { GH_TOKEN: { inline: 'v1' } },
+      secretStore: 'default',
+    });
+    expect(store.stageCalls).toHaveLength(2);
+    expect(second.contentHash).not.toBe(first.contentHash);
+  });
+
+  it('a bundle with no inline secrets still round-trips (regression guard on the unchanged path)', async () => {
+    const store = makeCountingStore();
+    const res = await registerEnv(makeClient(store), {
+      name: 'plain',
+      values: { REGION: 'us-west-2' },
+    });
+    const bytes = await storage.get(`pangolin://ns/env/plain/${res.contentHash}`);
+    expect(computeContentHash(bytes)).toBe(res.contentHash);
+    expect(store.stageCalls).toHaveLength(0);
+  });
+});