Skip to content

fix(automation): pair compacted tool results and record provider 400s as failed runs - #659

Open
rkfshakti wants to merge 3 commits into
andrewyng:mainfrom
rkfshakti:fix/scheduled-run-tool-role-655
Open

rkfshakti wants to merge 3 commits into
andrewyng:mainfrom
rkfshakti:fix/scheduled-run-tool-role-655

Conversation

@rkfshakti

@rkfshakti rkfshakti commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #655.

Problem

Scheduled runs abort with OpenAI 400 (Messages with role 'tool' must be a response to a preceding message with 'tool_calls'), including the first run of a brand-new task. Interactive sessions doing the same work succeed. The runner still records last_status=ok.

This is not a duplicate of #350 (load-side pairing repair) or #395 (compaction token estimates). #350 cannot fire on a fresh scheduled session; the 400 happens on the live provider feed after compaction. No other open PR is linked to #655.

Cause

  1. Compaction's outbound slice can start on a tool row (or on a notice that is stripped next), so the parent tool_calls assistant message is gone and the API rejects the request.
  2. TurnEngine.run() converts provider failures into EventType.ERROR and returns without raising. _run_scheduled_task treated a finished generator as success.
  3. The GUI finalize_manual_run path hardcoded status=ok / last_status=ok from the saved transcript, so a live 400 looked successful too.

Fix

  • Align the compaction boundary so the verbatim tail never starts on a tool row; drop leftover orphan tool messages on the provider feed only (canonical history is unchanged).
  • Record scheduled runs as error when the engine emits EventType.ERROR.
  • finalize_manual_run reads a trailing notice/kind=error from the transcript and stamps error instead of ok. A later successful retry is still ok.

Tests

  • tests/test_compaction.py — orphan-tool tail, rewind to parent assistant, notice-strip orphans
  • tests/test_automation.py — scheduled provider 400 is stored as error; scheduler copies that to last_status; manual finalize does the same
  • tests/test_engine.py — _outbound_messages() does not send a leading role=tool after compaction + notice strip

Made with Cursor

@rkfshakti
rkfshakti force-pushed the fix/scheduled-run-tool-role-655 branch 2 times, most recently from 7aa9bc7 to ca2ce33 Compare September 23, 2026 16:31
@rkfshakti

Copy link
Copy Markdown
Contributor Author

Friendly review ping — scheduled-run 400s now pair compacted tool results and record provider 400s as failed runs so the GUI doesn't show success after a 400 (#655, head ca2ce335, CI green, mergeable-clean, 11+ days quiet). Could someone take a look when convenient?

@rkfshakti
rkfshakti force-pushed the fix/scheduled-run-tool-role-655 branch from ca2ce33 to 2c2cefe Compare October 11, 2026 18:50
@rkfshakti
rkfshakti force-pushed the fix/scheduled-run-tool-role-655 branch from 2c2cefe to b5d0805 Compare October 21, 2026 08:08
@rkfshakti

Copy link
Copy Markdown
Contributor Author

Rebased onto main (7bac27388, OPE-226 merge) — the only conflict was a test-insertion collision in tests/test_engine.py (upstream's #670 audit-correlation tests landed on the same seam this PR's test_outbound_messages_does_not_send_orphan_tool_after_compaction occupies). Both blocks kept; tests/test_engine.py 23/23 and the 54 compaction tests pass locally against the rebased head.

@rkfshakti
rkfshakti force-pushed the fix/scheduled-run-tool-role-655 branch 2 times, most recently from 6bf943a to 4151255 Compare November 1, 2026 09:16
@rkfshakti

Copy link
Copy Markdown
Contributor Author

Refreshed onto current main (62 commits behind → 0) and un-reddened pytest.

The failure was in new upstream code, not this diff: test_background_wait_returns_as_soon_as_the_task_exits[runner] asserted status == "exited" but got "running".

Executor.background_output(wait_seconds=N) breaks its wait loop on the first of "process exited" or "new output". A one-shot command prints its last line and only then exits, so the loop wakes on the output, poll() is still None, and the call returns running for a task that is already done — the caller asked to be told when it finished and still has to poll again. It is timing-dependent, which is why the earlier green run on 6bf943a6b passed and this one did not.

Fixed in 716ecfa: after waking on output with the process not yet reaped, wait up to 250 ms (still inside the caller's deadline) for the exit, so the same call returns exited plus the final line.

Deterministic A/B forcing the interleaving (output visible, exit 50 ms later): running before, exited after. Added test_background_output_reports_exit_that_follows_the_last_output which encodes that interleaving — red on the previous head, green now.

Local verification: tests/test_shell.py 45 passed, full suite 5265 passed, 37 skipped. If you would rather keep this PR to the tool-pairing change only, the shell commit (716ecfa) is self-contained and can be lifted out.

rkfshakti and others added 3 commits November 3, 2026 20:58
…er 400s as failed runs

Scheduled runs hit OpenAI 400 when compaction left a tool message without its tool_calls parent, and engine ERROR events were treated as success so last_status stayed ok.

Co-authored-by: Cursor <cursoragent@cursor.com>

# Conflicts:
#	coworker/server/manager.py
Scheduled runs already recorded EventType.ERROR as last_status=error; the GUI finalize path still hardcoded ok. Read the trailing error notice instead, and cover scheduler last_status plus the engine outbound orphan-tool feed.

Co-authored-by: Cursor <cursoragent@cursor.com>
…ast output

`shell_task_output(wait_seconds=N)` breaks its wait loop on the *first* of exit or
new output. A one-shot command prints its last line and then exits a few
milliseconds later, so waking on that line returned status "running" for a task
that was already finished — the caller asked to be told when it was done and
still had to poll again.

After waking on output with the process not yet reaped, wait up to 250 ms
(still inside the caller's deadline) for the exit so the same call returns
`status="exited"` with the final line.

Deterministic A/B, forcing the interleaving (output visible, exit 50 ms later):
status "running" before, "exited" after. The new regression test encodes that
interleaving and fails on the previous code; `tests/test_shell.py` 45 passed,
full suite 5265 passed / 37 skipped.
@rkfshakti
rkfshakti force-pushed the fix/scheduled-run-tool-role-655 branch from 716ecfa to f52ade8 Compare November 3, 2026 15:28
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.

# Bug Report — Scheduled automation runs fail with API 400 ("tool" role error)

1 participant