Skip to content

Add complete fake adapter 2×2 contract matrix - #205

Merged
yihanzhu merged 12 commits into
mainfrom
codex/portable-adapter-contract-tests
Aug 31, 2026
Merged

Add complete fake adapter 2×2 contract matrix#205
yihanzhu merged 12 commits into
mainfrom
codex/portable-adapter-contract-tests

Conversation

@yihanzhu

@yihanzhu yihanzhu commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add one closed fake-only runner for four producer/forge substitution cells.
  • Resolve four honest profiles, run exact schema-2 stage requests/results through the stable core, and independently verify payload and candidate Git facts.
  • Bind the accepted inventory, four distinct fake entrypoints, unrelated target fixture, negative protocol cases, and restore proof.

Review size

This is one end-to-end adapter-contract concern. Its complete implementation keeps runner, four distinct packages, canonical inventory checks, unrelated fixture, and adversarial matrix together. Splitting them would leave an unprovable partial runner or duplicate the same protocol boundary.

Safety

This is inactive, fake-only, and repo-only. It is not a generic adapter launcher and provides no real-adapter qualification, credential, network or host isolation, authority, approval, remote branch write, release, install, profile activation, or deployment. Core generations, resolver, and stable wrapper are unchanged.

The test alone bootstraps the same official jq 1.6 asset used by existing core/resolver tests, with TLS 1.2+ and a fixed platform SHA-256 before use. The product runner and fake adapters remain network-free.

Targeted proof

Exact head: 7536643bbec4d98bd658ccb4040af8383082f0bf
Exact base: 04d5c166318f118012bc222de50e0130f2dbe91d

  • complete fake producer/forge 2×2: 29/29 focused checks
  • all trust-boundary inputs and executable packages are consumed from validated private snapshots
  • every request payload is rehashed immediately before adapter invocation
  • adapter PID/PGID registration is signal-safe; timeout/signal/descendant cleanup is bounded and fully reaped
  • signal cleanup suppresses cross-platform Bash job-control notices while preserving conventional 129/130/143 exits
  • all schema-2 stage chains, sources, refs, payload links, target/candidate/receipt/oracle Git facts are independently checked
  • ShellCheck 0.11.0, Bash syntax, pin checks, and diff check: clean

Tracks #153

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 31, 2026

Copy link
Copy Markdown

Deploying ystack with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7536643
Status: ✅  Deploy successful!
Preview URL: https://2dbc9c01.fabrica-6yx.pages.dev
Branch Preview URL: https://codex-portable-adapter-contr.fabrica-6yx.pages.dev

View logs

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 188587b
Reviewed-base: 04d5c16
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/205-17425/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The contract runner does not independently bind complete Git references, accepts protocol data beyond the validated response, and its negative matrix bypasses the real validation path. These issues undermine the central guarantees the new matrix is intended to prove.

Full review comments:

  • [P1] Bind complete Git refs to the repository map — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jSSh922R1q/adapter-tests/v1/runner.sh:248-249
    The generated package refs use repository IDs such as repo.assets, while this runner's map uses aa.assets, ab.assets, and so on. Here the runner retains only object_id, then verify_package reads a fixed root, HEAD, and path, so a ref with the right blob ID but the wrong repository, revision, hash algorithm, path, or mode can pass. Resolve and verify the complete Git ref through the matching repository-map entry.

  • [P1] Reject multiple JSON responses from adapters — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jSSh922R1q/adapter-tests/v1/runner.sh:309-310
    When an adapter emits one valid response followed by another canonical JSON value, canonical accepts the stream and --slurpfile parses both, but [0] silently discards everything after the first document. Such output therefore passes despite the closed protocol requiring exactly one response; require the slurped response array to have length one before selecting it.

  • [P1] Reject unlinked response payloads — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jSSh922R1q/adapter-tests/v1/contract.jq:108-112
    When a response contains the expected payload plus an additional unrelated payload, this predicate accepts it and later checks validate only the selected producer.patch or candidate.repository. The extra payload is never linked to a stage-result output or even digest-checked, so the runner can report that all payload links were validated while silently accepting arbitrary additional data. Require unique payload IDs and exact correspondence with the stage-result outputs.

  • [P1] Run negative cases through the normal validation path — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.jSSh922R1q/adapter-tests/v1/runner.sh:437-442
    If the normal response-envelope validation regresses and starts accepting partial, degraded, or relabelled output, these negative cases still pass because they use a separate classifier rather than the producer/forge validation path. The relabelled case is classified solely from the requested mode without inspecting the returned labels. Route each fault through the same response and stage-result checks as a normal adapter invocation, then assert the resulting error.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 3de11d9
Reviewed-base: 04d5c16
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/205-72156/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The matrix can emit passing Git-provenance observations for unresolved source references and can miss target repository mutations. These gaps undermine two core assertions of the new contract runner.

Full review comments:

  • [P2] Verify every resolved Git source against the repository map — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.pIQhwiwTDY/adapter-tests/v1/runner.sh:348-350
    When a resolver returns a profile_source, manifest_source, config, prompt, skill, or tool source with a false commit/object/path but retains the correct value_sha256, these checks still pass because validate-profile-set verifies schema and relationships, while verify_package covers only the selected executable packages. The runner can therefore report successful Git provenance for source references that do not exist in their mapped repositories; resolve and compare every source claim against the mapped commit and blob bytes.

  • [P2] Compare the target worktree and HEAD after adapter runs — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.pIQhwiwTDY/scripts/test/portable-adapter-contracts.test.sh:118-119
    If a fake adapter dirties target/source.txt or moves HEAD to another commit with the same tree, this check still passes because rev-parse HEAD^{tree} ignores working-tree changes and same-tree commit changes. Children can reach the fixture through their HOME path and filesystem isolation is explicitly absent, so the runner should recheck the original commit, index/worktree status, tree, and source object before emitting the target-git assertion.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: d0b370a
Reviewed-base: 04d5c16
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/205-50749/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The adapter runner does not reliably contain timed-out subprocesses or stop after termination signals, and its fixture setup can fail under valid Git hash-format configuration. These are functional reliability issues in the new contract suite.

Full review comments:

  • [P2] Terminate the adapter's entire process group on timeout — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.z8tLj5R1Z5/adapter-tests/v1/runner.sh:298-302
    When an adapter is waiting for a subprocess such as git or sleep at the wall-time limit, these signals terminate only the adapter shell PID. Its child processes remain alive and can keep modifying the candidate directory after E_TIMEOUT is reported and cleanup runs. Launch each adapter in its own process group and terminate/reap that group.

  • [P2] Exit after cleaning up on termination signals — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.z8tLj5R1Z5/adapter-tests/v1/runner.sh:31-33
    When the runner receives HUP, INT, or TERM, this trap removes its temporary directory but does not exit, so execution can resume with deleted state and may continue launching or validating adapters. Use signal-specific handlers that clean up and terminate with a nonzero status; reserve the plain cleanup handler for EXIT.

  • [P2] Force the target fixture to use the expected hash format — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.z8tLj5R1Z5/scripts/test/portable-adapter-contracts.test.sh:73-73
    On a host with init.defaultObjectFormat=sha256 or GIT_DEFAULT_HASH=sha256, this unqualified initialization creates 64-character target IDs, while contract.jq labels fixture.target as SHA-1 and the core schema requires 40-character SHA-1 IDs. The accepted matrix then fails at its first stage-request validation based on caller Git configuration; initialize explicitly with --object-format=sha1 under a clean config or propagate the detected format.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 3eae067
Reviewed-base: 04d5c16
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/205-9672/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The runner can execute bytes other than the verified Git package and can leave adapter descendants running after accepting a response. Both undermine the claimed digest and disposable-process boundaries.

Full review comments:

  • [P2] Execute the verified package snapshot — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Dl8eAFprqD/adapter-tests/v1/runner.sh:517-517
    When the fixture repository is modified concurrently after verify_package returns, this call executes the mutable worktree path rather than the Git blob whose digest was checked. Because packages are not rechecked afterward, unpinned bytes can run and still produce an accepted observation; execute the extracted immutable snapshot for producer, forge, and fault packages instead.

  • [P2] Reap the adapter group after a successful leader exit — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.Dl8eAFprqD/adapter-tests/v1/runner.sh:335-337
    When an adapter forks a background helper and exits successfully, wait "$pid" only reaps the group leader and these lines discard the group identity. The runner then validates the response and removes its temporary directory while the helper remains alive, so require the process group to be empty or terminate it after every run, not only on timeout or signals.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: a4b3ac8
Reviewed-base: 04d5c16
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/205-37723/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The request protocol accepts payload digests without validating them against the payload bytes. Combined with mutable target rereads, this can produce a successful observation bound to the wrong target content.

Review comment:

  • [P2] Verify request payload bytes against their digests — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.iC1dfeTYwW/adapter-tests/v1/contract.jq:108-108
    When the target source changes after initial verification and is restored before the final check, this predicate still accepts the request because payload_ok checks only that sha256 is 64 hex characters, not that it matches data. The runner rereads the mutable source for requests and the oracle while retaining the original fixture_sha, so candidates can be built from uncommitted bytes while the observation reports the original target identity. Recompute every request payload digest or build inputs exclusively from target_source_snapshot before invoking adapters.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: e9dea66
Reviewed-base: 04d5c16
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/205-90149/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The runner snapshots several trust-boundary inputs but continues using their mutable original paths. Concurrent changes can invalidate inventory authorization, repository mapping, and reported profile provenance.

Full review comments:

  • [P1] Use the validated inventory snapshot throughout the run — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.wetjjWAF1I/adapter-tests/v1/runner.sh:83-84
    If the inventory file changes after this copy, later code still reads and hashes the original path for expected errors and inventory_acceptance_ref. A concurrent replacement can therefore make the observation claim authorization for bytes whose pinned digest was never checked. Switch subsequent reads and the final digest to inventory_canonical.

  • [P1] Resolve repositories from the checked mapping snapshot — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.wetjjWAF1I/adapter-tests/v1/runner.sh:113-116
    When repository-map.json changes after its initial shape check, mapped_root continues rereading the live file rather than this canonical snapshot. A concurrent writer can redirect later package and source verification to repositories outside the validated fixture mapping. Use mapping_canonical for the shape check and every lookup.

  • [P2] Hash the same profile bytes that were validated — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.wetjjWAF1I/adapter-tests/v1/runner.sh:664-665
    If a profile is modified after validate-profile-set completes, this hashes the live working-tree file without another repository check, so the observation can report a profile digest for bytes that were never validated or used to build the resolved-profile snapshot. Snapshot the profile before validation and use that snapshot for validation, provenance checks, and this digest.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 2dd595e
Reviewed-base: 04d5c16
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/205-65653/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The adapter runner has a signal-handling race that can leave a launched process group running after the runner exits. This undermines the cleanup and containment behavior introduced by the patch.

Review comment:

  • [P2] Register the child group before signals can run — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.8dbSt3BHUz/adapter-tests/v1/runner.sh:355-358
    If HUP, INT, or TERM arrives after the background process is launched but before ACTIVE_CHILD_GROUP is assigned, signal_exit sees no active child, removes the run directory, and exits while the adapter process group continues running. This can leave the timeout or descendant fixture alive, so signals should be blocked around launch and registration or the trap must otherwise be able to discover the newly launched child atomically.

@yihanzhu

Copy link
Copy Markdown
Owner Author

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 7536643
Reviewed-base: 04d5c16
reviewer: operator-default @ high

Posted verbatim by codex-review.sh (codex exec review --json --base refs/codex-review/205-91461/base in an isolated temp worktree, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The adapter matrix, protocol validation, provenance checks, process cleanup, and regression coverage appear internally consistent. No actionable correctness issue was identified in the diff.

@yihanzhu
yihanzhu merged commit cac3a2e into main Aug 31, 2026
2 checks passed
@yihanzhu
yihanzhu deleted the codex/portable-adapter-contract-tests branch August 31, 2026 20:12
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.

1 participant