Creatidy/creatidy-kernel
5
0
Fork 0
Code Issues 9 Pull requests 2 Projects Releases Packages Wiki Activity Actions

[K6] Add bounded Codex app-server Runtime adapter #26

Merged
adrian.tkacz merged 1 commit from issue-8-runtime-codex into develop 2026-09-27 17:35:38 +00:00
Conversation 2 Commits 1 Files changed 5 +929 -3
kilo commented 2026-09-27 16:02:18 +00:00
Member
Copy link

Closes #8 after owner integration; keep issue open during review.

Implements an opt-in Codex app-server stdio transport and Runtime adapter using native thread/start, turn/start, thread/read and turn/interrupt. The trusted controller supplies pinned Attempt input resolution, a durable Operation claim, schema method inventory for the installed binary, and trusted candidate collection. Unknown effects are not redelivered; known accepted handles can be restored and reconciled. Generated-turn identity remains unknown where unattested; turn completion is not acceptance. No live credentials, custom wire protocol, OS isolation claim, or Program scheduler.

Validation: uv run --locked pytest tests/test_codex_runtime.py tests/test_codex_stdio.py (19 passed), make check (509 passed; all gates), make package-check (passed). No dependency/tooling change; audit not applicable.

Risk: the native initialize handshake does not supply a method inventory, so the caller must provide a matching generated-schema inventory; no live Codex integration was exercised.

Closes #8 after owner integration; keep issue open during review. Implements an opt-in Codex app-server stdio transport and Runtime adapter using native thread/start, turn/start, thread/read and turn/interrupt. The trusted controller supplies pinned Attempt input resolution, a durable Operation claim, schema method inventory for the installed binary, and trusted candidate collection. Unknown effects are not redelivered; known accepted handles can be restored and reconciled. Generated-turn identity remains unknown where unattested; turn completion is not acceptance. No live credentials, custom wire protocol, OS isolation claim, or Program scheduler. Validation: `uv run --locked pytest tests/test_codex_runtime.py tests/test_codex_stdio.py` (19 passed), `make check` (509 passed; all gates), `make package-check` (passed). No dependency/tooling change; audit not applicable. Risk: the native initialize handshake does not supply a method inventory, so the caller must provide a matching generated-schema inventory; no live Codex integration was exercised.
feat: add bounded Codex app-server runtime adapter
All checks were successful
ci / check (pull_request) Successful in 31s
Details
f80e61c78f
kilo reviewed 2026-09-27 16:09:52 +00:00
kilo left a comment
Copy link

PR Review: [K6] Add bounded Codex app-server Runtime adapter (#26)

Repository: Creatidy/creatidy-kernel
Branch: issue-8-runtime-codex -> develop
Frozen head: f80e61c78f
Linked issue: #8 ([K6] Runtime port and Codex app-server adapter)

Summary

Adds src/creatidy_kernel/adapters/codex_stdio.py (opt-in JSON-RPC stdio transport for the Codex app-server: pinned --version precheck, initialize/initialized handshake, correlated request/response, bounded buffers, fail-closed on malformed/uncorrelated/server-initiated messages, no retry after uncertain delivery) and src/creatidy_kernel/adapters/codex_runtime.py (Runtime adapter over native thread/start, turn/start, thread/read, turn/interrupt with trusted-controller input resolution, durable-claim authorization, unknown-on-uncertain semantics, requested/resolved/observed identity separation, and terminal-replay protection). README updated honestly, including that the native handshake advertises no method inventory. 5 files, +929/−3.

Intent / Acceptance Evidence

Independent verification at the frozen head (clean local checkout of issue-8-runtime-codex at f80e61c):

  • uv run --locked pytest tests/test_codex_runtime.py tests/test_codex_stdio.py — 19 passed.
  • make check — 509 passed; uv lock --check, ruff format/check, basedpyright (strict), lint-imports, secret scan, git diff --check all passed.
  • make package-check — wheel metadata/license, sdist rebuild, isolated install passed.
  • grep -ri codex src/creatidy_kernel/core/ src/creatidy_kernel/ports/ — no matches; tests/test_boundaries.py passes unchanged.
Acceptance criterion (#8) Evidence Status
Adapter passes the Runtime suite; records requested/resolved/observed identity separately tests/test_codex_runtime.py asserts requested=="model-one", resolved=="model-one", observed is None; suite is Codex-specific (see gaps) Met
Lost replies, duplicate starts, delayed healthy work, terminal replay, missing sessions, cancel races do not duplicate Attempts test_lost_replies_are_uncertain_and_never_duplicate_turns, test_native_start_duplicate_and_terminal_replay, test_quiet_healthy_work_and_cancel_race, test_restart_requires_durable_handle_and_missing_session_stays_unknown Met
Unsupported recovery/identity unknown; transport success with domain rejection stays rejection reconcile returns UNKNOWN (never invented ABSENT); CodexRejected raised on resolved-model mismatch after successful transport; observed forced to None Met
Fake runtime proves core without Codex; no core module imports Codex details FakeRuntime/FakeWorkspace untouched; grep and boundary tests clean Met
Capability/version mismatch fails closed without weakening test_capability_and_version_mismatch_fail_before_delivery asserts connection.calls == []; constructor checks version, required native methods, trust mode, network/mount policy, sandbox/approval policy Met

Architecture / Information Flow

  • Direction preserved: both new modules live in adapters and import only ports/core/sibling adapter (codex_runtime → codex_stdio, same-layer precedent as forgejo → forgejo_transport); lint-imports layers contract passes.
  • The operation journal stays the dispatch authority: the adapter registers the run as uncertain in _by_key before any RPC, so a lost thread/start/turn/start reply permanently blocks redelivery (ExecutionConflict) and reconcile returns UNKNOWN — matching the crash matrix ("effect uncertain regardless of whether bytes were sent") and ADR 0004 recovery semantics.
  • Terminal-state monotonicity enforced: status change after terminal raises; delayed inProgress cannot regress TERMINAL; candidate replay change raises.
  • Identity handled honestly: observed is always None (app-server does not attest generated-turn identity); requests that arrive with a non-None observed are refused; restored runs report resolved=None rather than inventing attestation.
  • Native interfaces only; no custom wire protocol, no Program scheduler, no acceptance semantics.

Blocking Issues

None.

Non-blocking Suggestions

  1. candidate() silently returns None when a turn previously read as failed/interrupted later reads completed (post-terminal status change), while observe() raises ExecutionConflict for the same transition. Consider making the two consistent.
  2. restore() does not invoke the authorize durable-claim callback that start() enforces. If the externally durable accepted receipt is itself the evidence of the prior claim (as the docstring implies), one explicit sentence saying so would prevent future confusion.
  3. _by_key/_by_handle grow without pruning for the process lifetime (same shape as FakeRuntime; acceptable for a controller prototype, worth a note if long-lived processes are planned).
  4. cancel() returns observed_at=0, fresh_until=0; the semantics are conservative (immediately stale) but undocumented relative to observe()'s freshness contract.

Questions for the Author

  • A definitive native domain rejection during start (e.g. resolved-model mismatch, RPC error response) leaves the effect key permanently uncertain in this adapter instance even though the thread may exist unused. Is the intended recovery path strictly "controller records rejection, quarantines, starts a new Attempt" per ADR 0004? If yes, a comment in start() stating that would forestall future "safe retry" refactors.

Security / Privacy Notes

  • Positive: explicit two-element absolute-path command, no shell, start_new_session process-group termination, byte/line limits on every read path, strict JSON-RPC correlation, server-initiated requests refused (approval_policy is never), stderr discarded, no credentials or secrets handled, no new dependencies (dependencies = [] unchanged — audit correctly not applicable).
  • Residual, documented honestly: the transport executes the configured binary with controller authority; trust mode is limited to TRUSTED_DEVELOPMENT and network/mount grants are refused rather than enforced. README states this.

Test / Documentation Gaps

  • No shared/parameterized Runtime conformance suite exists (unlike Forge, where fake and Forgejo run the same suite per docs/architecture/contracts.md). The new tests cover the contract's required conformance cases for Codex specifically; AC1's "the Runtime suite" is met only in that Codex-specific sense.
  • Activity.WAITING is never produced by this adapter (RUNNING/TERMINAL/UNKNOWN only); unmapped native statuses fall through to UNKNOWN, which is safe but undocumented.
  • No live Codex binary integration was exercised (synthetic fixtures only) — matching the PR's stated risk note that the caller must supply a matching generated-schema method inventory.

Follow-Up Candidates

  • Extract a shared Runtime conformance suite parameterized over adapters (durable, distinct from #8; would need duplicate search before creation via /create-follow-up-issue).
  • Document/map additional native turn statuses (e.g. queued/waiting-like states) if a live integration reveals them.

Overall Recommendation

APPROVE (substantive independent verdict for frozen head f80e61c): no blocking defects; scope, boundaries, fail-closed behavior, tests, and honest documentation satisfy issue #8's acceptance criteria.

Identity limitation: this review was submitted by the same shared kilo account that authored the PR, so it is persisted as a COMMENT-state review rather than a formal APPROVED review to avoid self-approval. A formal approving review from a distinct identity is still required for integration evidence.

## PR Review: [K6] Add bounded Codex app-server Runtime adapter (#26) **Repository:** Creatidy/creatidy-kernel **Branch:** issue-8-runtime-codex -> develop **Frozen head:** f80e61c78f1368293acaf38b883ef2d1e91eeb70 **Linked issue:** #8 ([K6] Runtime port and Codex app-server adapter) ### Summary Adds `src/creatidy_kernel/adapters/codex_stdio.py` (opt-in JSON-RPC stdio transport for the Codex app-server: pinned `--version` precheck, `initialize`/`initialized` handshake, correlated request/response, bounded buffers, fail-closed on malformed/uncorrelated/server-initiated messages, no retry after uncertain delivery) and `src/creatidy_kernel/adapters/codex_runtime.py` (Runtime adapter over native `thread/start`, `turn/start`, `thread/read`, `turn/interrupt` with trusted-controller input resolution, durable-claim authorization, unknown-on-uncertain semantics, requested/resolved/observed identity separation, and terminal-replay protection). README updated honestly, including that the native handshake advertises no method inventory. 5 files, +929/−3. ### Intent / Acceptance Evidence Independent verification at the frozen head (clean local checkout of `issue-8-runtime-codex` at `f80e61c`): - `uv run --locked pytest tests/test_codex_runtime.py tests/test_codex_stdio.py` — 19 passed. - `make check` — 509 passed; `uv lock --check`, ruff format/check, basedpyright (strict), lint-imports, secret scan, `git diff --check` all passed. - `make package-check` — wheel metadata/license, sdist rebuild, isolated install passed. - `grep -ri codex src/creatidy_kernel/core/ src/creatidy_kernel/ports/` — no matches; `tests/test_boundaries.py` passes unchanged. | Acceptance criterion (#8) | Evidence | Status | | --- | --- | --- | | Adapter passes the Runtime suite; records requested/resolved/observed identity separately | `tests/test_codex_runtime.py` asserts `requested=="model-one"`, `resolved=="model-one"`, `observed is None`; suite is Codex-specific (see gaps) | Met | | Lost replies, duplicate starts, delayed healthy work, terminal replay, missing sessions, cancel races do not duplicate Attempts | `test_lost_replies_are_uncertain_and_never_duplicate_turns`, `test_native_start_duplicate_and_terminal_replay`, `test_quiet_healthy_work_and_cancel_race`, `test_restart_requires_durable_handle_and_missing_session_stays_unknown` | Met | | Unsupported recovery/identity unknown; transport success with domain rejection stays rejection | `reconcile` returns UNKNOWN (never invented ABSENT); `CodexRejected` raised on resolved-model mismatch after successful transport; `observed` forced to None | Met | | Fake runtime proves core without Codex; no core module imports Codex details | FakeRuntime/FakeWorkspace untouched; grep and boundary tests clean | Met | | Capability/version mismatch fails closed without weakening | `test_capability_and_version_mismatch_fail_before_delivery` asserts `connection.calls == []`; constructor checks version, required native methods, trust mode, network/mount policy, sandbox/approval policy | Met | ### Architecture / Information Flow - Direction preserved: both new modules live in `adapters` and import only `ports`/`core`/sibling adapter (`codex_runtime` → `codex_stdio`, same-layer precedent as `forgejo` → `forgejo_transport`); `lint-imports` layers contract passes. - The operation journal stays the dispatch authority: the adapter registers the run as `uncertain` in `_by_key` **before** any RPC, so a lost `thread/start`/`turn/start` reply permanently blocks redelivery (`ExecutionConflict`) and `reconcile` returns UNKNOWN — matching the crash matrix ("effect uncertain regardless of whether bytes were sent") and ADR 0004 recovery semantics. - Terminal-state monotonicity enforced: status change after terminal raises; delayed `inProgress` cannot regress TERMINAL; candidate replay change raises. - Identity handled honestly: `observed` is always None (app-server does not attest generated-turn identity); requests that arrive with a non-None `observed` are refused; restored runs report `resolved=None` rather than inventing attestation. - Native interfaces only; no custom wire protocol, no Program scheduler, no acceptance semantics. ### Blocking Issues None. ### Non-blocking Suggestions 1. `candidate()` silently returns `None` when a turn previously read as `failed`/`interrupted` later reads `completed` (post-terminal status change), while `observe()` raises `ExecutionConflict` for the same transition. Consider making the two consistent. 2. `restore()` does not invoke the `authorize` durable-claim callback that `start()` enforces. If the externally durable accepted receipt is itself the evidence of the prior claim (as the docstring implies), one explicit sentence saying so would prevent future confusion. 3. `_by_key`/`_by_handle` grow without pruning for the process lifetime (same shape as `FakeRuntime`; acceptable for a controller prototype, worth a note if long-lived processes are planned). 4. `cancel()` returns `observed_at=0, fresh_until=0`; the semantics are conservative (immediately stale) but undocumented relative to `observe()`'s freshness contract. ### Questions for the Author - A definitive native domain rejection during `start` (e.g. resolved-model mismatch, RPC `error` response) leaves the effect key permanently uncertain in this adapter instance even though the thread may exist unused. Is the intended recovery path strictly "controller records rejection, quarantines, starts a new Attempt" per ADR 0004? If yes, a comment in `start()` stating that would forestall future "safe retry" refactors. ### Security / Privacy Notes - Positive: explicit two-element absolute-path command, no shell, `start_new_session` process-group termination, byte/line limits on every read path, strict JSON-RPC correlation, server-initiated requests refused (approval_policy is `never`), stderr discarded, no credentials or secrets handled, no new dependencies (`dependencies = []` unchanged — audit correctly not applicable). - Residual, documented honestly: the transport executes the configured binary with controller authority; trust mode is limited to TRUSTED_DEVELOPMENT and network/mount grants are refused rather than enforced. README states this. ### Test / Documentation Gaps - No shared/parameterized Runtime conformance suite exists (unlike Forge, where fake and Forgejo run the same suite per `docs/architecture/contracts.md`). The new tests cover the contract's required conformance cases for Codex specifically; AC1's "the Runtime suite" is met only in that Codex-specific sense. - `Activity.WAITING` is never produced by this adapter (RUNNING/TERMINAL/UNKNOWN only); unmapped native statuses fall through to UNKNOWN, which is safe but undocumented. - No live Codex binary integration was exercised (synthetic fixtures only) — matching the PR's stated risk note that the caller must supply a matching generated-schema method inventory. ### Follow-Up Candidates - Extract a shared Runtime conformance suite parameterized over adapters (durable, distinct from #8; would need duplicate search before creation via `/create-follow-up-issue`). - Document/map additional native turn statuses (e.g. queued/waiting-like states) if a live integration reveals them. ### Overall Recommendation **APPROVE** (substantive independent verdict for frozen head `f80e61c`): no blocking defects; scope, boundaries, fail-closed behavior, tests, and honest documentation satisfy issue #8's acceptance criteria. Identity limitation: this review was submitted by the same shared `kilo` account that authored the PR, so it is persisted as a `COMMENT`-state review rather than a formal `APPROVED` review to avoid self-approval. A formal approving review from a distinct identity is still required for integration evidence.
adrian.tkacz deleted branch issue-8-runtime-codex 2026-09-27 17:35:38 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
adrian.tkacz
Labels
Clear labels   
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Creatidy/creatidy-kernel!26
No description provided.