Repository navigation
Conversation
aviggiano
left a comment
There was a problem hiding this comment.
Technical verdict: REQUEST_CHANGES for exact head 9d668454dfa9cf99fd86ae29bcf315d2bab0a71a, reviewed against unstable at 81aec3bdff8fa943118d9979a2a68eaa4c678466. This is a COMMENT review of the draft.
[P2] Preserve completion correlation when an existing trusted hook rewrites the command.
In command-invocations.ts:232, both pre- and post-hook processing derive the reservation identity from the event's command text. Codex 0.146.0 runs the pre-hooks against the original input, but a trusted pre-hook's updatedInput.command changes the command subsequently reported by PostToolUse. Consequently, the post-hook lookup at lines 267–269 looks under a different hash and returns before matching the original session/tool-call identity.
I reproduced this with the real pinned Codex CLI, a deterministic local Responses server, the shipped hook CLI, and an existing user hook trusted by its exact hash; no global hook-trust bypass was used:
- A trusted
PreToolUsehook appends the harmless suffix# rewritten-by-trusted-hookusingupdatedInput.command. call-1reserves the original command, executes once, and returnsProcess exited with code 0.- Its
PostToolUsehas the same session andtool_use_id: call-1, but the rewritten command. No completion receipt is written; durable status remainsactive. - A subsequent identical original command with the distinct identity
call-2is denied ascoalesced ... status: active, although the first command has finished. After controller exit the unreleased reservation becomescompletion_unknownand still blocks it.
This breaks the promised admission of intentional sequential invocations in the qualified path and is relevant to the documented preservation of existing user/project hooks. Please durably correlate terminal events to the originally admitted reservation by session/tool-call identity independently of rewritten command text, while retaining conservative orphan handling and completed-call replay protection. Add a real-CLI regression with an already trusted rewriting hook: the later distinct call should run after terminal completion, while replaying the completed original call should stay coalesced.
Validation otherwise passed: all 12 CLI/dependency builds; runtime and CLI test compilation; 30 focused registry, real-Codex, hook-CLI, CLI-contract and adapter-boundary tests; 10 generated Codex/Claude/DeepSeek adapter tests; 4 preamble tests; strict changed-file lint, formatting, docs and diff checks. Additional independent checks admitted exactly one of 24 racing processes in each of five completed generations, kept stale/PID-reused/torn/mismatched records coalesced, and exercised the production sealed CLI launcher.
Scope assessment: the draft correctly says this is partial and does not close #1317. Claude/DeepSeek and direct Kimi/Pi/OpenCode remain unprotected; installed Codex 0.162.0 also explicitly reports an unqualified version and receives no guard. Those documented exclusions should remain explicit; passing the 0.146.0 tests does not establish provider-independent enforcement.
|
Author follow-up to the P2 review, implemented at Terminal completion now finds the originally fsynced reservation by exact session/tool-call identity, independent of rewritten command text or cwd. The reservation itself is the durable correlation record, avoiding a second-index publication/crash window. Ambiguous native identities do not release any reservation. The real Codex 0.146.0 regression now runs both without a rewrite and with an existing exact-hash-trusted rewriting hook, through the shipped hook CLI. Each fresh/resumed run coalesces a live duplicate, records terminal completion despite the rewrite, permits the later distinct invocation, and denies a completed original-call replay. An unrelated untrusted hook remains disabled; no global hook-trust bypass or paid provider request is used. Additional focused tests cover independent-process completion after command/cwd changes, wrong-session identities, ambiguous old-completed/live identities, and torn/corrupt/symlinked/mismatched records. Existing crash/orphan/race protections remain covered. Validation at this head: 14 invocation tests, 20 CLI/contract/boundary tests, 10 generated adapter tests and 4 preamble tests passed; all 12 CLI/dependency builds, runtime/CLI test compilation, strict changed-file lint (against the PR base), formatting, docs and diff checks passed. The build retains its existing dashboard chunk-size warning. Provider/version exclusions and explicit fail-open diagnostics are unchanged. #1317 remains open. The PR remains draft for independent re-review. |
aviggiano
left a comment
There was a problem hiding this comment.
Technical verdict: REQUEST_CHANGES for exact head c19fe8ea961e4eb5f6c823393ad5d5d06bbb2fc1, reviewed against unstable at 81aec3bdff8fa943118d9979a2a68eaa4c678466. This is a COMMENT review of the draft; no branch changes were made.
The earlier P2 completion-correlation issue is fixed. Terminal lookup now uses the original durable reservation's session/tool-call identity, and ambiguous identities do not release reservations. I independently exercised the real pinned Codex 0.146.0 against a deterministic local Responses server using the production sealed trusted CLI launcher and an existing exact-hash-trusted rewriting hook. In both a fresh session and its resumed session, the live duplicate was denied, the original native polling handle completed the rewritten command, a later distinct call in the same session ran, and replaying the completed original call was denied. Four intended workers produced exactly four completed reservations. An unrelated untrusted hook remained disabled, and no global hook-trust bypass or paid provider request was used.
Two CI regressions remain:
-
[P2] Make the campaign Codex stub distinguish version probes from model invocations. The new version probe invokes
codex --versionduring command construction. The existing campaign stub reads stdin and logsstartedfor every invocation, including these probes. A minimal reproduction using that exact compiled stub and onlyconfigureCommandInvocationGuardrecords one fake start followed bystub codex found no output contract in the prompt, before any model execution. The campaign-resume CI job consequently fails the source line 604 assertion with5 !== 15(logical attempts versus recorded starts). The prior PR head has the same failure; the base's campaign gate passed. Handle--versionbefore prompt reading/start logging and rerun the campaign gate, preserving the real attempt-count assertion. -
[P3] Correct the new documentation heading level. Line 19 adds
### Command invocation admissiondirectly below the document's H1. External static analysis fails MD001 (Expected: h2; Actual: h3). Use an H2 and rerun the check. The repository'sdocs:checkand Prettier checks do not catch this rule.
Validation at this head: all 12 CLI/dependency builds and runtime/CLI test compilation passed; 14 invocation tests (including the optional real pinned-CLI qualifications), 20 hook-CLI/CLI-contract/adapter-boundary tests, 10 generated Codex/Claude/DeepSeek adapter tests, and 4 preamble tests passed. Strict changed-file lint, Prettier, docs and diff checks passed. Independent stress admitted exactly one of 24 racing processes in each of five generations; historical completions/replays did not release active successors. Wrong sessions, ambiguous identities, torn/malformed/symlinked/mismatched records, crash with a live orphan, stale ownership and PID-start-time reuse remained conservative. Installed Codex 0.162.0 explicitly stayed inactive.
CI snapshot: Build gates, package/dependency/policy gates, Semgrep and both Socket checks pass. External static analysis and campaign-resume E2E fail as described above. Runtime support/adapter, runtime integration shards 1–4, CLI package tests, and benchmark/typecheck lanes are still in progress.
Scope remains accurately documented as partial: qualified Linux Codex/OpenRouter only; Claude/DeepSeek and direct Kimi/Pi/OpenCode remain unenforced, and unsupported versions/platforms/context or operational failures explicitly fail open. #1317 remains OPEN. Passing the qualified path does not establish provider-independent enforcement.
|
Addressed the re-review in
Validation at this head:
Still draft for independent review. #1317 remains open; this follow-up does not expand the qualified-provider coverage. |
aviggiano
left a comment
There was a problem hiding this comment.
Technical verdict: APPROVE for exact head 2856d5564efe2773b5a77960cfa399bd4b0aeeb0, reviewed against unstable at 81aec3bdff8fa943118d9979a2a68eaa4c678466. This is a COMMENT review of the draft for its documented qualified-provider scope. No branch changes were made.
Both remaining regressions are resolved:
-
The campaign stub handles exact
--versionprobes before reading stdin, logging model starts, or entering the held-node path. The configure-only regression passes, including its positive control that real execution still records start/completion and writes the expected output. I reran exactpnpm --filter @ultrafuzz/cli test:e2e: 2 passed, including the full engine-kill/controller-kill/resume campaign (about 12 minutes). I observed five logical starts and ten separate version probes. The original full campaign test body is byte-identical to the base: attempt counts, completed-node reuse, accounting, report verification and cleanup assertions all remain intact and passed. The exact-head campaign CI job also passes on Node.js 24. -
Command invocation admission is now an H2. All changed Markdown passes
markdownlint-cli@0.49.0with the repository configuration. The exact-head external static-analysis job is green; its logs confirm markdownlint 0.49.0 and successful Markdown linting.
The prior completion-correlation fix remains sound under the real pinned Codex 0.146.0. I reran the deterministic local Responses-server checks and independently exercised the production sealed CLI launcher with an existing exact-hash-trusted rewriting hook. Fresh and resumed sessions each coalesced a live duplicate, completed through the original native polling handle despite rewritten command text, admitted a later distinct call in the same session, and denied replay of the completed original call. Four intended workers produced exactly four completed reservations. All four completed-call replays remained denied through the sealed CLI after controller exit. The unrelated untrusted hook stayed disabled; no global hook-trust bypass or paid provider request was used.
Additional validation passed: all 12 CLI/dependency builds; runtime and CLI test compilation; 14 invocation tests, 20 hook-CLI/CLI-contract/adapter-boundary tests, 10 generated Codex/Claude/DeepSeek adapter tests, and 4 preamble tests; strict changed-file ESLint, Prettier, docs and diff checks. Independent stress admitted exactly one of 24 racing processes in each of five generations. Historical completions/replays did not release active successors; crash/orphan, wrong-session, ambiguous-identity, stale/PID-reuse, torn/malformed/symlinked/mismatched-record protections passed. Installed Codex 0.162.0 explicitly remained inactive.
CI snapshot for this exact head: Build gates, external static analysis, campaign E2E, package/dependency/policy gates, Semgrep and both Socket checks pass. Runtime support/adapter tests, runtime integration shards 1–4, CLI package tests, and benchmark/typecheck lanes are still in progress; the entire CI run is not yet complete.
#1317 remains OPEN. The documented exclusions remain material: Claude/DeepSeek and direct Kimi/Pi/OpenCode are unenforced; unsupported versions/platforms/context and operational failures explicitly fail open. This approval covers the qualified Linux Codex/OpenRouter implementation and does not establish provider-independent enforcement or close that issue.
aviggiano
left a comment
There was a problem hiding this comment.
Follow-up technical verdict: REQUEST_CHANGES for exact head 2856d5564efe2773b5a77960cfa399bd4b0aeeb0. This supersedes my earlier technical approval: the runtime-support CI lane that was still pending has now failed, and I reproduced its failure. This remains a COMMENT review; no branch changes were made.
[P2] Preserve the capability-removal handoff when adding the unsupported-provider diagnostic. The new block at workflow.tsx:2706 sits between Reflect.deleteProperty(unstructuredArgs, "ultrafuzzTaskRuntime") and executionAgent.generate(unstructuredArgs). The existing prompt-authority assertion at generated-workflow-verifier.test.ts:8607 requires those statements to be adjacent. Consequently, the required runtime-support/Bun CI job fails final-report prompt authority is bounded, tamper-evident, and constant-size across large projections: 1,054 supporting tests passed, one failed, and two skipped. The subsequent Bun adapter command is not reached.
Reproduction on the exact head after compiling runtime tests:
node --test --test-name-pattern '^final-report prompt authority is bounded, tamper-evident, and constant-size across large projections$' packages/runtime/dist-test/test/generated-workflow-verifier.test.jsThis exits 1 consistently. The test file is byte-identical to unstable at 81aec3bdff8fa943118d9979a2a68eaa4c678466. Using read-time, in-memory controls without editing the checkout, the same whole test passes with the base workflow template, and passes with only the five-line diagnostic block relocated above the privileged-capability comment/deletion.
Please move the diagnostic before that comment/deletion, retaining capability removal immediately before the adapter call and preserving the existing assertion. Then rerun the named regression and pnpm --filter @ultrafuzz/runtime test:release:supporting. The observed problem is a deterministic required-test regression introduced by this PR. The previously verified campaign/probe, Markdown and real pinned-Codex behavior remains valid.
|
Addressed this follow-up and the additional runtime-shard-1 regression in
Both failures were reproduced before the fixes. The reviewer's read-time base-template and relocated-diagnostic controls passed; both named regressions then passed after fresh compilation. Validation (local Node 22.23.1 / Bun 1.3.14):
New exact-head CI is queued; its Node 24 lanes are not yet verified. The PR remains draft for independent review, and #1317 remains open. Provider qualification and unsupported-provider behavior are unchanged. |
aviggiano
left a comment
There was a problem hiding this comment.
Technical verdict: APPROVE for exact head bc11d7898af6acdc711fa9817f0ae70b1145b3d1, reviewed against unstable at 81aec3bdff8fa943118d9979a2a68eaa4c678466. This is a COMMENT review of the draft for its documented qualified-provider scope. No branch changes were made.
The remaining regressions from the previous head are resolved without weakening their assertions:
- The unchanged unsupported-provider diagnostic now runs before the privileged-capability comment and removal.
Reflect.deleteProperty(unstructuredArgs, "ultrafuzzTaskRuntime")is immediately followed by the adapter call. The existing final-report authority test is byte-identical to the base and passes. - The group-execution-defaults expectation now includes exactly the two intended command-poll/coalescing prompt lines; the literal comparison remains intact. The named regression passes. The earlier campaign version-probe fix and Markdown heading fix remain intact.
I independently reran the real pinned Codex 0.146.0 with a deterministic local Responses server and the production sealed trusted CLI launcher. Fresh and resumed sessions each denied a live duplicate, completed the rewritten command through its original native polling handle, admitted a later distinct call in the same provider session, and denied replay of the completed original call. Four intended workers produced exactly four completed reservations. An unrelated untrusted hook remained disabled, with no global hook-trust bypass or paid provider request.
Other exact-head validation passed: both named regressions; the configure-only campaign probe and its real-execution positive control; 14 invocation tests, 20 hook-CLI/CLI-contract/adapter-boundary tests, 10 generated Codex/Claude/DeepSeek adapter tests, and 4 preamble tests; runtime and CLI test TypeScript checks; strict changed-file ESLint, Prettier, docs and diff checks. Independent stress admitted exactly one of 24 racing processes in each of five completed generations; historical completion/replay did not release an active successor. Stale ownership, PID-start-time reuse, changed native session, mismatched/torn receipts and torn reservations remained coalesced. Installed Codex 0.162.0 explicitly stayed inactive.
Nonblocking hardening note: the generic helper's override recognizer misses short-option-equals forms such as -c=features.hooks=false; a direct helper call with that input reports enabled. The shipped factories do not expose raw extra arguments, and pinned Smithers emits separate -c/value arguments, so this does not block the qualified shipped path. Extend the recognizer before accepting arbitrary caller-supplied CLI arguments.
The entire exact-head CI run is completed successfully, including the aggregate release-gates check. The previously failing runtime-support/Bun lane reports 1,055 Node tests passed, zero failures, two optional real-Codex skips; Bun reports 49 passed, zero failures, one conditional skip. The previously failing runtime shard 1 reports 99 passed and zero failures, including the exact prompt expectation. Runtime shards 2–4, all 198 CLI tests, both campaign E2E tests, package/dependency/policy gates, workspace typecheck, build and external static analysis also pass. Semgrep and both Socket checks are green. The optional real-Codex qualification skipped in CI passed in the local runs described above.
Scope remains partial and explicit: qualified Linux Codex/OpenRouter only. Claude/DeepSeek and direct Kimi/Pi/OpenCode remain unenforced; unsupported versions/platforms/context and operational failures fail open with diagnostics. #1317 remains OPEN. This technical approval covers the qualified implementation and does not establish provider-independent enforcement or close that issue.
| const codexHome = path.join(root, "codex-home"); | ||
| fs.mkdirSync(codexHome); | ||
| fixtureCodexHookSettings(root, codexHome, marker, rewrite); | ||
| const verification = `${JSON.stringify(process.execPath)} -e ${JSON.stringify(`require('node:fs').appendFileSync(${JSON.stringify(counter)}, 'work\\n'); setTimeout(() => console.log('verification-complete'), 6000);`)}`; |
There was a problem hiding this comment.
The real Codex qualification test only runs a command that exits successfully. PostToolUse is the only path that marks a reservation complete, so a change in how Codex reports failed or stopped commands could leave later calls blocked without this test noticing. Add cases for a nonzero exit and a command stopped through its native session, then check the receipt and the next invocation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/runtime/test/command-invocations.test.ts
Line: 654
Comment:
**Failed commands lack coverage**
The real Codex qualification test only runs a command that exits successfully. `PostToolUse` is the only path that marks a reservation complete, so a change in how Codex reports failed or stopped commands could leave later calls blocked without this test noticing. Add cases for a nonzero exit and a command stopped through its native session, then check the receipt and the next invocation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary
Partially addresses #1317 for the shipped Codex/OpenRouter path, including models routed through Codex. This draft intentionally does not close the issue: direct provider paths still need a managed command runner or an equivalent upstream admission contract.
# ultrafuzz-command-key: NAMEcomment. Retain native polling handles and expose durable status/coalescing diagnostics.Safety and scope
No age-based lease admits duplicate work. Dead-controller, torn and corrupt reservations remain completion-unknown; a child can outlive its controller. Status
activeonly establishes that the reserving controller is alive. The guard does not normalize shell syntax or infer execution options absent from the provider hook. It is accidental-duplication protection, not a same-UID security boundary.The shared registry owns admission. Provider CLIs continue owning shell execution; Smithers' post-launch observation callbacks are not used as admission gates. No project-specific command, path or workload is encoded.
Validation
pnpm --filter @ultrafuzz/cli... build— all 12 dependency/workspace builds passed (existing dashboard chunk-size warning only).pnpm docs:check, andgit diff --checkpassed.To run the optional provider qualification after building and compiling the runtime tests, set
ULTRAFUZZ_TEST_CODEXto an installed@openai/codex@0.146.0binary and runnode --test packages/runtime/dist-test/test/command-invocations.test.js.Draft for independent review; not ready for merge.
The reviewed changes appear safe to merge, with a non-blocking gap in command-ending tests.
Fix with agent prompt
Summary
Adds durable command admission for qualified Codex sessions, including models routed through Codex.
Scope acknowledged by
aviggiano: unsupported providers and versions, hidden execution options, operational failures, and completion-unknown reservations are intentional limits. The author explicitly leaves full provider-independent enforcement deferred.Diagram
sequenceDiagram participant Codex participant Hook as command-hook participant Registry participant Shell Codex->>Hook: PreToolUse Hook->>Registry: Exclusively create reservation alt Matching unfinished reservation exists Registry-->>Hook: Existing handle Hook-->>Codex: Deny duplicate and return status guidance else Reservation created Hook-->>Codex: Allow command Codex->>Shell: Execute Shell-->>Codex: Terminal result Codex->>Hook: PostToolUse with original call identity Hook->>Registry: Write matching completion receipt endReviews (1) · Last reviewed commit: "fix(runtime): preserve capability handof..." · Reviewed by Greptile