Skip to content

fix: preserve durable worker admission JSON output - #571

Open
andrei-hasna wants to merge 4 commits into
mainfrom
fix/worker-admission-json-truncation
Open

fix: preserve durable worker admission JSON output#571
andrei-hasna wants to merge 4 commits into
mainfrom
fix/worker-admission-json-truncation

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • spool worker-admission child stdout to a private file so large Todos JSON is not truncated by pipe output
  • add a regression for a 131073-byte child response

Validation

  • staged secret scan: clean
  • Blacksmith Testbox run 31584228001: pending

Task: 258b9c89-f16f-495c-a490-2e8c0398b6a1


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #571 @ a886b36 — lens: correctness+security+gates, reviewer Hostus (1 of 1)

Reviewed git log --oneline origin/main..HEAD (exit 0; 1 commit), git diff origin/main...HEAD --stat (exit 0; 3 files, 38 insertions, 2 deletions), the full diff, and surrounding worker-admission source and tests.

Commands and gates:

  • bun install — exit 0; setup only, 521 packages installed. This is not the repository test result.
  • package.json declares no test gate and no typecheck script. No repository test command was invented or run.
  • gh pr view 571 --repo hasna/codewith --json statusCheckRollup with a value-safe projection — exit 0; 28 checks: 18 success, 7 failure, 3 skipped.

Forge checks read, by name and conclusion:

  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu — FAILURE
  • cla — SUCCESS
  • Check for spelling errors — SUCCESS
  • Blob size policy — SUCCESS
  • cargo-deny — SUCCESS
  • build-test — SUCCESS
  • Build Codewith Linux CLI — SUCCESS
  • Detect changed areas — SUCCESS
  • python-sdk — SUCCESS
  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl — FAILURE
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 1/4 — SUCCESS
  • Format / etc — SUCCESS
  • sdks — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 2/4 — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 3/4 — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 4/4 — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main) — SKIPPED
  • cargo shear — SUCCESS
  • Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu — FAILURE
  • Argument comment lint package — SKIPPED
  • Bazel clippy on windows-latest for x86_64-pc-windows-gnullvm — FAILURE
  • Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu — FAILURE
  • Argument comment lint - Linux — FAILURE
  • Verify release build on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
  • Argument comment lint - Windows — SUCCESS
  • CI results (required) — FAILURE
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
  • [code]smith — SKIPPED

Blocking P0/P1 findings:

  • P1 — codex-rs/background-agent/tests/worker_admission.rs:656 does not compile in the required Bazel lanes. The forge log reports error[E0282] and error[E0283] at assert_eq!(output.stderr, Vec::new()) because the empty vector element type is ambiguous. This directly fails Linux Bazel test, clippy, release-build, argument-comment-lint, and the required aggregate at the reviewed head. Remedy: make the expected empty vector type explicit as Vec::<u8>::new() and rerun the affected forge lanes.

No other concrete, reachable P0/P1 correctness or security defect was found in the changed production path. The temporary file is private, remains alive until stdout is read, keeps stderr separate, and is removed on drop.

Non-blocking follow-ups:

  • The repository declares no package-level test gate or typecheck script; the forge checks above are the authoritative gates for this candidate.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

Fixed the attributed blocking finding in commit 8671c33875581d2eba4205ae1501d0b5422fcca5 and pushed it to fix/worker-admission-json-truncation.

The regression test now uses Vec::<u8>::new(), resolving Bazel's E0282/E0283 type-inference failure. Before commit: secrets scan staged --json exited 0 with findingCount: 0; git diff --cached --check exited 0. The affected authoritative forge lanes must rerun on this new head before a focused re-review can return GO.

@andrei-hasna
andrei-hasna force-pushed the fix/worker-admission-json-truncation branch from 8671c33 to 2850564 Compare August 12, 2026 18:05
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #571 @ 2850564 — lens: correctness+security+gates, reviewer unresolved-account011 (1 of 1)

Reviewed the full three-file diff against freshly fetched origin/main 923fd0b, plus the complete worker-admission implementation and its CLI/app-server call sites.

Commands and gates:

  • git log --oneline origin/main..HEAD: exit 0; 3 commits read.
  • git diff origin/main...HEAD --stat: exit 0; 3 files, 40 insertions, 2 deletions.
  • Full diff with surrounding source: exit 0; all changed files read.
  • bun install: exit 0; setup only, not a test gate. It created a migrated untracked bun.lock, which was removed so the reviewed tree remained unchanged.
  • The repository declares no typecheck script.
  • The repository declares no test gate: package.json has no test script. This is a non-blocking finding, not permission to invent a gate.
  • gh pr view 571 --repo hasna/codewith --json statusCheckRollup: exit 0; 28 checks total — 19 success, 6 failure, 3 skipped, 0 pending.

Forge checks read, by name and conclusion:

  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-gnu — FAILURE
  • cargo-deny — FAILURE
  • Build Codewith Linux CLI — FAILURE
  • cla — SUCCESS
  • Check for spelling errors — SUCCESS
  • Blob size policy — SUCCESS
  • build-test — SUCCESS
  • Detect changed areas — SUCCESS
  • python-sdk — SUCCESS
  • Bazel test on ubuntu-24.04 for x86_64-unknown-linux-musl — FAILURE
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 1/4 — SUCCESS
  • Format / etc — SUCCESS
  • sdks — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 2/4 — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 3/4 — FAILURE
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm shard 4/4 — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm (native main) — SKIPPED
  • cargo shear — SUCCESS
  • Argument comment lint package — SKIPPED
  • Bazel clippy on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS
  • Bazel clippy on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
  • Verify release build on ubuntu-24.04 for x86_64-unknown-linux-gnu — SUCCESS
  • Argument comment lint - Linux — SUCCESS
  • Verify release build on windows-latest for x86_64-pc-windows-gnullvm — SUCCESS
  • Argument comment lint - Windows — SUCCESS
  • Bazel test on windows-latest for x86_64-pc-windows-gnullvm — FAILURE
  • CI results (required) — SUCCESS
  • [code]smith — SKIPPED

Blocking P0/P1 findings:

  • P1 correctness / required-gate defect in ProcessWorkerAdmissionCommandRunner::run: configuring file-backed stdout and then calling Tokio Command::output() causes stdout to be captured by the output path instead of written to the temp file. The implementation then ignores output.stdout and reads the empty temp file. Both Linux Bazel lanes reproduce this in the newly added process_runner_preserves_large_stdout_for_worker_admission test: 13 passed, 1 failed. The real worker-admission path would therefore parse empty stdout rather than the dependency JSON.

Named remedy:

  • Spawn the command with file-backed stdout and explicit piped stderr, then await wait_with_output() so stderr remains captured while stdout stays bound to the private temp file.

Non-blocking follow-ups:

  • package.json declares no test script.
  • cargo-deny reports pre-existing RUSTSEC-2026-0257; the Codewith CLI and Windows Bazel failures are external V8 download failures. These are outside this three-file candidate, but remain visible forge failures.

Agent: unresolved-account011
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

Fix for the blocking worker-admission stdout defect is pushed at 953b10b.

Changed ProcessWorkerAdmissionCommandRunner::run to spawn with file-backed stdout and explicit piped stderr, then await wait_with_output(). This preserves the private temp-file stdout path while retaining stderr diagnostics.

Validation:

  • git diff --cached --check: exit 0 before commit.
  • secrets scan staged --json: exit 0, 1 file scanned, 0 findings before commit; exit 0 again before push.
  • just fmt: exit 1 because uv is absent for the unrelated Python formatter lanes; the Rust formatter lane completed. This is a lane-chosen command, not a repository gate.
  • Blacksmith Testbox run 31633278755 on exact head 953b10b, lane-chosen command cd codex-rs && just test -p codex-background-agent: SUCCESS, exit 0, 39 passed / 0 failed. The previously failing process_runner_preserves_large_stdout_for_worker_admission test passed.

The prior NO_GO names the old head 2850564 and is stale for the new candidate. This reviewer authored the remedy and therefore is not issuing a self-review GO for 953b10b.

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