Code Review

Blocking Issues

None.

Suggestions

1. Use import map instead of bare jsr: specifiers in conformance files: packages/testing/vault_conformance.ts and packages/testing/datastore_conformance.ts import from jsr:@std/[email protected] directly, which requires the // deno-lint-ignore-file no-import-prefix suppression. Since deno.json already maps @std/assert → jsr:@std/[email protected], these files could use import { ... } from "@std/assert" instead, eliminating the lint ignore entirely.

Notes

• All new .ts files include the AGPLv3 copyright header ✓
• No libswamp import boundary violations ✓
• Test files are co-located with source files (e.g., vault_test_context_test.ts next to vault_test_context.ts) ✓
• Compile-time compat tests in src/domain/*/testing_package_compat_test.ts correctly verify structural compatibility with canonical types ✓
• Types follow DDD principles: testing interfaces mirror domain Value Objects/interfaces, conformance suites are stateless service-like utilities, repository fakes abstract persistence properly ✓
• Version bump 0.1.0 → 0.2.0 is appropriate for the scope of new exports ✓
• No security concerns ✓

Adversarial Review

Critical / High

None.

Medium

1. vault_conformance.ts:225-231 — assertStringIncludes on joined keys is a weak assertion. The list conformance check joins all keys with commas and uses assertStringIncludes for membership. This is a substring match, not element-membership. While the UUID-based key prefix makes false positives very unlikely in practice, the correct assertion is assertEquals(keys.includes(testKey1), true, ...). A vault implementation whose list() returns a single key containing a comma and both key names as substrings would pass. Suggested fix: replace assertStringIncludes(keys.join(","), testKey1, ...) with assertEquals(keys.includes(testKey1), true, ...).

2. vault_conformance.ts:199-220 — Conformance suite never re-reads testKey1 after writing testKey2. A broken vault using a single global variable for all keys (last-write-wins regardless of key) would pass every behavioral assertion. The suite puts key1, verifies it, overwrites key1, verifies overwrite, writes key2, verifies key2 — but never re-reads key1 to confirm it wasn't clobbered. Suggested fix: add assertEquals(await provider.get(testKey1), "conformance-value-updated") after the key2 roundtrip.

3. datastore_test_context.ts:89 — Shared lockHeld boolean across all lock instances. A single let lockHeld = false is shared across all locks from the same createDatastoreTestContext(). If a test creates two locks with different keys, acquiring one and releasing the other corrupts the state: lock1.acquire() → lockHeld=true, lock2.release() → lockHeld=false, now isLockHeld() reports false even though lock1 is held. The isLockHeld() helper (line 241) returns this single boolean. Suggested fix: use a Map<string, boolean> keyed by lockKey, or track held state per lock instance.

4. datastore_conformance.ts:291-305 — forceRelease conformance silently skipped when nonce is absent. The if (info && info.nonce) guard means a lock implementation that never populates nonce in its LockInfo will skip the entire forceRelease-with-correct-nonce block. The conformance suite passes without ever verifying the primary forceRelease path. Suggested fix: assert that info.nonce is a non-empty string (making it a de-facto required field for conformance), or restructure to test forceRelease even when nonce is absent.

5. datastore_conformance.ts:315-326 — Wrong-nonce forceRelease doesn't verify lock is still held. After forceRelease("wrong-nonce-value") returns false, the test doesn't call inspect() to confirm the lock remains held. A buggy implementation could return false but release the lock as a side effect. Suggested fix: add const stillHeld = await lock.inspect(); assertExists(stillHeld, "lock must remain held after wrong-nonce forceRelease").

6. report_test_context.ts:190 — getContent returns raw Uint8Array reference without cloning. findByName (line 175), findAllForModel (line 199), and findAllGlobal (line 207) all use structuredClone. But getContent returns match?.content directly. A caller mutating the returned buffer corrupts the internal state. Suggested fix: return Promise.resolve(match?.content ? new Uint8Array(match.content) : null).

7. drivers/testing_package_compat_test.ts:107-113 — Driver compat test does not verify execute method. The ExecutionDriver check verifies type, initialize, and shutdown but omits execute — the most important method on the interface. If the execute signature diverges between canonical and testing types, this test won't catch it. Suggested fix: add const _execute: CanonicalExecutionDriver["execute"] = driver.execute to the check function.

8. drivers/testing_package_compat_test.ts:95-103 — Driver compat test does not verify outputs field on ExecutionResult. The outputs: DriverOutput[] field is omitted from the compat check. Suggested fix: add const _outputs: CanonicalExecutionResult["outputs"] = result.outputs.

9. datastore/testing_package_compat_test.ts:42-53 — Datastore compat test does not verify withLock. The DistributedLock check verifies acquire, release, inspect, forceRelease but omits withLock — a required method on the canonical interface. Suggested fix: add const _withLock = lock.withLock(async () => {}) with appropriate type annotation.

Low

1. vault_conformance.ts:252-264 — Cleanup puts empty string rather than deleting. Since VaultProvider has no delete(), cleanup uses put(key, ""). This is acknowledged in a comment, and keys are namespaced with random prefixes, so this is a known limitation rather than a bug. However, repeated conformance runs against real vault backends (AWS SM, etc.) will accumulate orphaned secrets.

2. report_test_context.ts:169-175 — findByName without version returns first match, not latest. When version is undefined, Array.find returns the first element in insertion order. If the canonical implementation returns the latest version by default, this fake has different semantics. Unlikely to cause issues in practice since test data is hand-seeded.

3. datastore_test_context.ts:137-144 — withLock uses synchronous throw vs. acquire's Promise.reject. When lockAcquireFails is true, acquire() returns Promise.reject(...) (line 119) but withLock does a synchronous throw (line 144). Both are caught by assertRejects, but the inconsistency could matter for callers distinguishing sync/async errors.

4. vault_conformance.ts:77 / datastore_conformance.ts:74 — Type regex makes @ prefix optional but error message says @collective/name. The regex ^@?[a-z]... accepts foo/bar without @. Either the regex should require @ or the error message should document that it's optional.

Verdict

PASS — No critical or high severity issues. The medium findings are real gaps in conformance coverage and compat test completeness, but none would cause data loss or security issues. Most are improvements that would strengthen the testing framework. The assertStringIncludes on joined keys (#1) and the shared lockHeld state (#3) are the most likely to cause real confusion for extension authors, and are worth addressing before or shortly after merge.