Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: luvs01/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
macos 2/2 in run 35511768704 was cancelled at the 20-minute ceiling after the isolate running tests/codex-integration/ codex-app-server-processes.test.ts went silent mid-file: shard files had streamed results continuously until then, then nothing for the remaining ~17 minutes while the orphan bun process stayed alive. A per-test --timeout cannot cover this shape: the wedge is the runtime's own wait loop spinning inside spawnSync after the spawned child has already exited, so the ceiling fires inside the spin and no result line is ever written again. A suite making progress emits a line inside every per-test window, so silence several times that window is a wedge, not slow work. macOS has no timeout(1) to borrow the Linux batch bound, so poll the tee'd log for growth and kill Bun once the silence reaches five times the per-test ceiling; the shared crash classifier already reads status 137 as process death, which is what a wedged runner is, and the last ##[group] in the log names the file it stalled inside. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| ( | ||
| last_size=-1 | ||
| silent_for=0 | ||
| while sleep 15; do | ||
| size="$(wc -c < "$suite_log" 2>/dev/null || echo 0)" | ||
| if [ "$size" -le "$last_size" ]; then | ||
| silent_for=$((silent_for + 15)) | ||
| else | ||
| silent_for=0 | ||
| last_size=$size | ||
| fi | ||
| if [ "$silent_for" -ge "${OCX_MACOS_SUITE_SILENCE_SECONDS:-300}" ]; then | ||
| echo "::error::macOS suite produced no output for ${silent_for}s -- a wedged isolate, not slow work (several times the 60s per-test ceiling). Killing Bun; the last ##[group] in this log names the file it stalled inside." | ||
| pkill -KILL -x bun 2>/dev/null || true | ||
| # tee's read ends only when every descendant that inherited the | ||
| # merged stream is dead; a lingering grandchild must not hang the | ||
| # leg a second time, so give EOF a moment, then reap tee itself. | ||
| sleep 5 | ||
| pkill -KILL -f "tee $suite_log" 2>/dev/null || true | ||
| exit 0 | ||
| fi | ||
| done | ||
| ) & | ||
| watchdog_pid=$! |
There was a problem hiding this comment.
🔍 Workflow change requires explicit security review
This watchdog changes GitHub Actions process termination and CI gating behavior. Repository policy requires explicit security review before merge.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — this stays open for a human security review since it changes CI process termination.
While exercising it: the watchdog had a real defect worth noting for that review. kill $watchdog_pid reaps the subshell but orphans its in-flight sleep 15, which kept the step's output pipes open for up to 15s past each suite — the same 15s the macos-serial-lanes harness uses as its internal deadline, which is why that file timed out on every lane after this landed. Fixed in 05fcb7a by detaching the sleeps' inherited fds (</dev/null >/dev/null 2>&1), so an orphaned sleep costs nothing; verified the pipe now closes in ~0.5s vs 15.0s before.
The first bound stopped the quadratic parse but went too far the other way: once an object closed, every later whitespace-only prefix held forever, so a non-wrapper body's trailing whitespace never reached the client while completion kept it — and a close plus whitespace arriving in one chunk previewed nothing at all. The scan already walks every member, so record each top-level member's name and string-valuedness (last occurrence winning, JSON.parse's own duplicate rule) and consult the table once at the close: exactly one string fallback field is the wrapper's value, anything else streams the raw text byte-exact through its trailing whitespace. progressiveFreeform Input never calls JSON.parse on the accumulated buffer anymore — the classification is entirely the scan's own bounded work — and the 'parse' flag on hold is gone with it. A fenced fallback value now holds like a fenced canonical one instead of publishing the stripped body at close; completion is unchanged either way. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
… step's pipes kill on the watchdog subshell reaps the loop but not its in-flight sleep, which is orphaned with the suite's output fds still inherited. Each suite then held the step's pipes open for up to fifteen seconds past its end — the same fifteen seconds the macos-serial-lanes harness uses as its internal deadline, which is what timed the file out on every lane. The sleeps run detached so an orphaned one costs nothing. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
|
Fixed in d9dc208. Rather than carrying classification across deltas, the scan now records each top-level member it already walks — name plus whether its value is a string, last occurrence winning (JSON.parse's duplicate rule) — and This also covers the worse variant the hold introduced: a close plus trailing whitespace arriving in one chunk previewed nothing at all; it now resolves correctly. The regression test is extended per your suggestion — concatenated deltas are asserted equal to One deliberate side effect worth noting: a fenced fallback value now holds like a fenced canonical |
|
Already updated — the description now describes the member-table classification and byte-exact raw streaming, plus the watchdog fd fix. |
E2E verification — freeform tool-call streaming (live proxy + mock upstream)Drove streamed Result: 7/7 strict byte-exact assertions pass — Differential detail (vs baseline b9d430b and intra-PR 8aa3a6c)
Tested via a Devin session. |
…rvice-vulnerability-in-scanner
codex-retained-root-serialization.test.ts' post-approval seam test timed out at the 45s budget when one spawned child never exited (swept as a dangling process) and the other exited non-zero with empty stderr -- neither a repo code change nor a recognized lock-contention signature. Verified locally: 7/7 pass in ~5s on the merge commit. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 participant
Motivation
parse: true, causingprogressiveFreeformInputto callJSON.parseon the full, provider-controlled accumulated argument on every whitespace delta and produce O(n²) CPU work.{}and cause a CPU denial-of-service against the proxy.Description
scanFreeformWrappernow records each top-level member it already walks — decoded name plus whether its value is a string, last occurrence winning (matchingJSON.parse's duplicate-key rule) — in a bounded member table.afterTopLevelCloseresolves a fully-seen closed object from that table: exactly one string-valued fallback field yields{ kind: "input", valueStart }; anything else yieldsraw, so non-wrapper bodies stream byte-exact through trailing whitespace. A clamped close still holds (unseen tail).progressiveFreeformInputno longer callsJSON.parseon the accumulated buffer at all — the quadratic vector is removed by construction, not rate-limited — and theparseflag onholdis gone. Fallback keys are shared viafreeformFallbackKeys(toolName)fromapply-patch-envelope.ts.inputvalue (preview suppressed; completion authoritative), instead of publishing the stripped body at close.run_macos_suitewatchdog subshell orphaned its in-flightsleeponkill, keeping the step's pipes open ~15s past each suite — racing themacos-serial-lanesharness's 15s internal deadline. The sleeps now run with detached fds so an orphan costs nothing.Testing
bun test tests/responses/responses-freeform-wrapper-keys.test.ts— 8 pass, 0 fail (includes byte-exact trailing-whitespace deltas asserted againstunwrapFreeformToolInput, and aJSON.parsespy proving the buffer is never reparsed).bun test tests/responses/responses-custom-tool-repair.test.ts tests/responses/responses-stream-consistency.test.ts,bun test tests/copilot tests/bridge.test.ts— all pass.bun run typecheck,bun run structure:check,bun run privacy:scan— all pass.Codex Task
Link to Devin session: https://app.devin.ai/sessions/987ebed6d47641c8b96b43ade5b1b761
Open in Devin Desktop: https://app.devin.ai/desktop/session/987ebed6d47641c8b96b43ade5b1b761?variant=devin
Requested by: @luvs01