fix(status): self-heal a stuck-PROCESSING terminal via a fresh capture-pane read - #5
Closed
klabulan wants to merge 23 commits into
Closed
fix(status): self-heal a stuck-PROCESSING terminal via a fresh capture-pane read#5klabulan wants to merge 23 commits into
klabulan wants to merge 23 commits into
Conversation
…e markers (fixes awslabs#413) (awslabs#430) * fix(tmux): use paste-buffer -p instead of hand-crafted bracketed-paste markers (fixes awslabs#413) tmux >= 3.7 passes pasted buffer content through vis(3) sanitization (hardening against bracket-end injection), converting each raw ESC (0x1b) byte in the buffer into the literal two characters "^[". CAO's send_keys with force_bracketed_paste=True — used for ALL inbox message delivery — hand-crafted the \x1b[200~/\x1b[201~ markers inside the buffer and pasted with -r, so on tmux >= 3.7 every delivered message rendered with visible "^[[200~"/"^[[201~" garbage in the receiving TUI and multi-line messages could mis-submit. Stop hand-crafting the markers: load only the raw message bytes and paste with -p, which makes tmux itself emit genuine 0x1b markers, conditionally on the pane's DECSET 2004 state. -S was rejected as an alternative because it bypasses the vis(3) sanitization entirely and would reopen the injection surface tmux 3.7 closed — worker-authored message bytes could smuggle control sequences into a receiving TUI. Behavior change for panes that never enable DECSET 2004 (e.g. bash during init, or TUIs that don't request bracketed paste): they now receive the raw text with no markers, and multi-line content submits per line. That is standard tmux paste semantics, not a regression — the same as every non-forced send_keys call already behaved. force_bracketed_paste is kept in the backend interface: the herdr backend writes raw bytes directly to the pty (no tmux paste path, no vis(3) sanitization) and still honors it there. Diagnosis and fix direction by @chaogebaba (awslabs#413). * fix(tmux): gate the bracketed-paste strategy on tmux 3.7 vis(3) sanitization Address review: paste-buffer -p only emits markers when the pane enabled DECSET 2004, and some TUIs (e.g. kiro-cli) never do — the awslabs#230 wrap existed precisely for them, so replacing it unconditionally regressed multi-line delivery on tmux < 3.7 (the majority deployment base). Keep both behaviors, selected once per process from `tmux -V`: - tmux < 3.7: legacy hand-crafted \x1b[200~...\x1b[201~ wrap + -r, byte-identical to the pre-awslabs#413 path (buffer bytes pass through paste-buffer unmodified there). - tmux >= 3.7: raw bytes + -p, since vis(3) sanitization renders hand-crafted markers as literal "^[[200~". Non-2004 panes get tmux-sanctioned per-line semantics; -S stays rejected because it bypasses the sanitization. The pane-state probe suggested as an alternative is not implementable: no bracketed-paste pane format exists on the versions that need the fallback (verified empirically on tmux 3.4 — #{bracket_paste_flag} and Unknown/unparseable versions ("tmux master", probe failure) assume the sanitizing behavior: raw + -p never renders garbage on any version, while a wrongly hand-crafted wrap on a sanitizing tmux does. Live-verified on tmux 3.4 via the real TmuxClient: forced delivery of a multi-line message with the "[Assigned by terminal ...]" suffix arrives as one bracketed unit with genuine 0x1b markers and raw LFs — hexdump byte-identical to the pre-awslabs#413 contract. --------- Co-authored-by: anilkmr-a2z <238313826+anilkmr-a2z@users.noreply.github.com>
… api snapshot, native --env) (awslabs#502) * feat(herdr): broadcast pane.updated subscription instead of per-pane * docs(herdr): update stale subscription comments + strengthen broadcast test * feat(herdr): parse nested data.pane from broadcast pane.updated events * fix(herdr): guard _event_loop against null/non-dict pane in event payload * feat(herdr): drop force-reconnect on register — broadcast covers new panes * test(herdr): strengthen register regression guard; drop dead state * feat(herdr): add _fetch_snapshot helper over api snapshot * fix(herdr): harden _fetch_snapshot against timeout + malformed output * feat(herdr): reconcile from single api snapshot, not 3 subprocess calls * fix(herdr): defensive snapshot parsing in _reconcile + non-dict guard * feat(herdr): startup DB cleanup from api snapshot * feat(herdr): allow --env flag in arg sanitizer * test(herdr): pin env-newline rejection to the unsafe-chars reason * feat(herdr): inject env via native --env, remove shell-export path * fix(herdr): CAO identity env wins over operator --env; document strict value policy * feat(herdr): snapshot-backed durable pane_id map * fix(herdr): bound pane_id map staleness so it self-heals after herdr restart * docs(herdr): record R5 (keep polling) + E (env-survival deferred) decisions * fix(herdr): redact --env from errors + don't return stale pane after failed refresh Addresses both review findings on PR awslabs#502. P1 (credential disclosure): operator-forwarded env values are potentially secret, but native --env args (workspace/tab create) landed raw in TerminalBackendError on command failure, timeout, and sanitizer rejection — surfaced to callers via HTTP str(e). Add _redact_env_values() masking every value after --env to KEY=<redacted>, applied to _run_herdr's error display for all commands and to the sanitizer's unsafe-character rejection. P2 (stale-map self-heal defeated): get_pane_id re-checked the durable map after _refresh_pane_id_map() with no freshness re-gate. A failed refresh preserves the old map+timestamp, so an entry already judged older than _PANE_ID_MAP_TTL was returned anyway — routing events to an obsolete/reused pane after a herdr restart. Re-gate on the TTL after refresh; only read the map when the rebuild succeeded, else fall through to the label fallback. Tests: env-value redaction (helper + command-failure + timeout + sanitizer paths), and get_pane_id falling through to label resolution on failed refresh of an expired entry. * fix(herdr): repr snapshot stderr in log + non-vacuous register test Addresses Copilot review comments on PR awslabs#502. - _fetch_snapshot logged result.stderr via an f-string; stderr can echo user-controlled labels/args with embedded newlines/control chars, forging log lines. Switch to %r (repr) escaping, matching the codebase convention. - test_register_while_connected_does_not_touch_socket now patches asyncio.run_coroutine_threadsafe and asserts it is never called — a behavioral, non-vacuous check of the real contract. The _force_reconnect hasattr guard is kept as a secondary regression tripwire (it was added because the writer.close/write assertions alone pass vacuously). Copilot's third comment (stale pane-map after failed refresh) was already fixed in the prior commit's TTL re-gate; no code change needed.
…rking OpenCode workers (awslabs#496) * Fix deferred-init retry loop re-delivering into already-processing OpenCode workers The deferred-init retry loop (awslabs#479) trusts status_monitor.get_status() which returns a cached value updated only by the event-driven pyte screener at rising-edge/quiescence edges. When pyte lags behind reality the cached status stays IDLE even though OpenCode's TUI already shows the esc interrupt footer, causing the loop to re-paste the full task message into a working terminal. Two fixes: 1. Bump opencode_cli paste_submit_delay from 0.3s to 1.0s (matching kiro_cli) to reduce Enter-swallowing in the first place. 2. Add _worker_is_started_direct() which does a live capture-pane and calls provider.get_status() directly, bypassing the cached status, before each resubmit decision. * Address PR review feedback: scope direct probe to OpenCode only, offload to thread, guard get_status exceptions, add unit tests * chore: apply Black formatting to deferred-submit test file * Declare supports_direct_status_probe default on BaseProvider --------- Co-authored-by: Alex Mercer <alex.mercer@digitalriot.co.uk>
…labs#490) docs/api.md, docs/mock-cli-provider.md, docs/tool-restrictions.md, and docs/skills.md listed a stale subset of providers, missing opencode_cli, cursor_cli, and antigravity_cli, and docs/api.md's exit-command table had antigravity_cli wrong (/exit instead of /quit). Cross-checked against the 9 providers registered in providers/manager.py and each provider's own exit_cli() implementation. Fixes awslabs#476
* feat(skills): add agent profile routing * fix: address agent routing review feedback * test: fix rename monkeypatch on Python 3.10
* fix(memory): bound graph lint projection * fix(graph): bound export projection
…wslabs#500) * fix(tmux): skip bracketed-paste wrap when the pane is a bare shell send_keys(force_bracketed_paste=True) -- the delivery path behind every POST /terminals/{id}/input call, used both for ordinary TUI messaging and for a resume/continue command sent to a hibernated terminal -- always wrapped its content in \x1b[200~...\x1b[201~ escape sequences and sent it via paste-buffer -r, regardless of what's actually running in the pane. When the TUI that used to occupy a pane exits (e.g. Claude Code's own `/exit`) leaving a bare POSIX shell behind, a subsequent send_keys call still gets wrapped -- but a bare shell doesn't understand bracketed-paste escapes, so they glue onto the front of the first token, corrupting the command deterministically (`claude --continue` becomes an unresolvable token, not a timing race). Falling back to the existing paste-buffer -p path isn't safe either: -p's own bracket-emitting decision is driven by tmux's per-pane ?2004h tracking, which the same exited TUI can leave "on" without ever sending ?2004l first. Fix: check the pane's live #{pane_current_command} (the same primitive codex.py/kiro_cli.py already trust for their own "has the TUI exited back to a shell?" detection) against a new BRACKETED_PASTE_INCOMPATIBLE_SHELLS set before wrapping. A known shell gets the plain content with no paste flags at all -- never bracketed, never subject to tmux's own stale tracking -- while \n still becomes Enter so a multi-line command still executes line-by-line. Fails closed to the existing wrap-unconditionally behavior on an unresolvable or unrecognized foreground command. Mirrored in HerdrBackend.send_keys, which implements the identical unconditional-wrap behavior over a different transport and already exposes the same get_pane_current_command primitive. Self-ROAST also caught and fixed a real, adjacent gap this change would have made worse: flow_service.py's execute_flow (async) called send_input directly on the event loop, un-offloaded -- the exact hazard class issue awslabs#382 was fixed for elsewhere (api/main.py's POST /terminals/{id}/input already wraps it in asyncio.to_thread). This change adds a small amount of synchronous subprocess work to that same call (the new pane-command probe), so the missed offload is now fixed alongside it rather than left to surface as a separate report. Reproduced directly (not just theorized): `printf '\033[200~claude --version\033[201~\n' | sh` corrupts to `sh: 1: [200~claude: not found`; the same pane with no bracket wrap at all runs clean. * fix(herdr): read foreground process from process-info, not the broken pane-get field Addresses @anilkmr-a2z's CHANGES_REQUESTED must-fixes: - HerdrBackend.get_pane_current_command now calls `herdr pane process-info --pane <id>` and reads foreground_processes[0].name, instead of `herdr pane get`'s foreground_process field, which is null/absent across all pane states on herdr 0.7.5 -- the bracketed-paste guard this PR adds (and codex/kiro_cli's pre-existing shell_baseline TUI-exit detection) never fired on herdr as a result. Added "--pane" to the herdr CLI argument sanitizer's allowed-flags set (the new subcommand needs it). - Added a realistic-payload test that feeds actual process-info JSON through the real subprocess/parsing path (the existing tests all mocked get_pane_current_command itself, which is exactly the layer that would have hidden this field-name bug), plus an empty-foreground-processes fallback test and a sanitizer allowlist test for the new --pane flag. - Corrected the constants.py comment: bash does understand bracketed paste via readline (version/config-dependent, default-on since readline 8.1/bash 5.1) -- it's kept in the incompatible set because CAO can't detect whether it's active in a given pane, not because bash never supports it. - black-formatted test/backends/test_herdr_backend.py (CI Code Quality check was failing on this). * fix(tmux): correct silent rebase merge of shell-detection + tmux>=3.7 sanitization fix Rebasing this branch onto current main hit a merge that git resolved without flagging a conflict but got semantically wrong: main had independently landed the tmux >= 3.7 vis(3) pasted-buffer sanitization fix (issue awslabs#413, _tmux_sanitizes_paste_buffers()) after this branch was cut, and the auto-merge silently reverted send_keys() back to this PR's pre-awslabs#413 two-branch shape, dropping the version-aware -p/manual-wrap distinction entirely. Manually reconstructed the intended 3-way branch: bare-shell detection (this PR) takes priority and skips both the manual wrap and -p on any tmux version, then the tmux < 3.7 legacy wrap applies for a real TUI, then tmux >= 3.7's -p-only delivery applies otherwise -- combining both fixes instead of one silently regressing the other. Added direct interaction coverage (TestSendKeysShellDetectionCrossedWithTmuxVersion) for the two cells this merge actually got wrong: a bare shell on modern tmux, and a real TUI on modern tmux with force_bracketed_paste=True. Pinned two existing shell-detection tests to legacy_tmux, since they specifically assert the pre-awslabs#413 manual-wrap behavior that's now only valid pre-3.7. * chore: trigger CI
) * feat(mcp): add an explicit model override to handoff/assign CAO's handoff/assign MCP tools -- the documented way to spawn a child/worker agent -- had no way to specify which model the worker should use. Model selection only ever came from a static `model` field baked into a named agent profile file; a caller who wanted to pin a specific model for one worker had no lever short of authoring a dedicated profile first. Found live by a downstream consumer (harness-control): an agent asked to spawn a review worker on a specific model, faced with no parameter for it, fell back to reverse-engineering CAO's raw REST API (repeated /openapi.json dumps, a 40-iteration polling loop) and created a terminal via a raw POST that bypassed all of CAO's own bookkeeping -- cascading into unrelated crashes downstream. The gap itself, not just that particular workaround, is what this closes. Adds an optional `model: Optional[str] = None` parameter threaded end to end through both handoff's and assign's distinct server-side paths: - assign: MCP tool -> _create_terminal (HTTP client) -> POST /sessions/{name}/terminals -> terminal_service.create_terminal -> provider_manager.create_provider -> the resolved provider. - handoff: MCP tool -> POST /terminals/run-step (RunStepRequest) -> run_agent_step -> the same terminal_service.create_terminal -> same downstream. At the provider layer, extends the existing per-call `model` kwarg pattern (already used by copilot_cli/opencode_cli/cursor_cli/ antigravity_cli) to the remaining five providers (claude_code, codex, kiro_cli, kimi_cli, hermes), each of which previously read `model` only from a static profile field via its own `--model <name>` CLI flag. `terminal_service.create_terminal` resolves precedence once (explicit override wins over profile.model) before constructing the provider, matching copilot_cli/opencode_cli's existing "trust self._model as already-resolved" shape; the newly-updated providers' own internal profile.model fallback stays load-bearing for the separate provider_manager.get_provider() on-demand-resurrection path (e.g. after a server restart), which never passes `model` at all. Deliberately out of scope: _create_terminal's new-session branch (used only when there's no current CAO_TERMINAL_ID) does not get `model` -- assign fails fast before ever reaching it, and handoff never calls _create_terminal at all (it uses the separate run-step path), so it's unreachable from either MCP tool in practice. Self-ROAST (independent review) caught and fixed one real regression before this ever ran against a real profile: claude_code.py's native_agent branch (a profile that thin-wraps a native Claude Code agent, which owns its own model config by design) originally logged a warning whenever self._model was truthy -- but by the time it reaches the provider, self._model can no longer be distinguished from "this profile's own model field" vs. "a genuine caller override," so the warning fired on an ordinary, previously-silent, schema-legal profile shape with no caller involvement at all. Removed rather than papered over; the underlying behavior (never applying --model on that branch) is unchanged and correct. * fix(mcp): address review findings on awslabs#501 (untested precedence line, kimi no-profile gap, model validation) Addresses @anilkmr-a2z's CHANGES_REQUESTED items plus their follow-up on Copilot's two automated findings: - Added a test that calls the real terminal_service.create_terminal (not mocked at the model seam like every other test in the suite) with an explicit override AND a profile carrying its own model, asserting provider_manager.create_provider receives the override -- the `model=model or (profile.model if profile else None)` line the whole feature hangs on had no test that would catch a revert to the pre-PR `model=profile.model if profile else None`. - kimi_cli.py: hoisted model resolution out of `if self._agent_profile is not None:` so an explicit override applies even with no agent profile, matching codex.py/hermes.py's shape (previously silently dropped in that case -- not reachable through handoff/assign today, but inconsistent with the PR's own "uniform per-call override" premise). - Added request-boundary validation for `model` (RunStepRequest's field_validator, and the /sessions/{name}/terminals query param) per the narrowed guidance in the review thread: classic word-splitting isn't reachable (every provider shlex.joins), but a control character or newline surviving quoting into the launch command is a real delivery hazard this codebase already guards against elsewhere. New MODEL_ID_RE/MODEL_ID_MAX_LEN constants, shared by both entry points, also close the ValueError->404 mismap Copilot flagged (a malformed model now never reaches terminal_service). Full suite: 1850 passed, 15 pre-existing ag_ui failures (reproduce identically on unmodified main), 0 regressions. * fix(mcp): black-format test_kiro_cli_unit.py, cover validate_model's None branch Investigated the Codecov patch-coverage flag on awslabs#501 (29.6%, 38 lines missing) rather than dismissing it as fork-CI noise: - CI's actual "Code Quality" job was failing (not just action_required) -- black flagged test/providers/test_kiro_cli_unit.py, pre-existing drift in the original commit, same root cause Codecov's own coverage upload depends on that job succeeding. Reformatted. - Cross-referenced every line Codecov flagged as missing against this PR's actual diff (not just the raw file percentages, which mix in large pre-existing gaps like the ENABLE_WORKING_DIRECTORY-flagged branches and security-prompt code this PR never touched). Found one genuine gap in new code: RunStepRequest.validate_model's `if v is None: return v` line is unreachable when `model` is omitted (Pydantic v2 does not run field_validators on a field falling back to its default), only via an explicit `"model": null` in the request body. Added a test for that. Every other flagged line in api/main.py/kimi_cli.py/hermes.py/ mcp_server/server.py is pre-existing and unrelated to this PR's diff. Full suite: 1855 passed, 15 pre-existing ag_ui failures (reproduce identically on unmodified main), 0 regressions.
…er leakage (awslabs#508) _get_cleanup_nudge was unmocked in several assign tests, letting it hit a live server during test runs. Patch it to return "" alongside the existing _create_terminal mocks. Fixes awslabs#503
* feat(config): make CAO_HOME_DIR env-overridable CAO's data dir was hardcoded to ~/.aws/cli-agent-orchestrator. Read it from the CAO_HOME_DIR env var (default unchanged), matching the existing CAO_AGENTS_DIR / CAO_GRAPH_EXPORT_ROOT convention, so the whole data tree relocates with a single override. Motivating case: environments that restrict ~/.aws to protect AWS credentials otherwise leave CAO unable to read its own data. Repoint two spots that hardcoded the same path so the override is complete: the settings_service agent-store / agent-context defaults (the profile dirs the handoff read uses) and the cursor_cli CAO_TMP_DIR fallback. Add env-override tests and document CAO_HOME_DIR in docs/configuration.md. * fix(config): harden CAO_HOME_DIR env read and add missing test coverage Address review feedback on PR awslabs#467: - Treat empty/whitespace CAO_HOME_DIR as unset (falls back to default) instead of silently resolving to CWD via Path("") - Call .expanduser().resolve() so tilde values and relative paths are normalised to an absolute path at import time - Reword comment to be provider-neutral (seccomp-bpf as an example, not the only motivating case) - Pass mode=0o700 to the import-time mkdir() calls for TERMINAL_LOG_DIR and FIFO_DIR so secret-bearing dirs are owner-only when relocated out of the ~/.aws permission umbrella - Fix _restore_default_constants fixture to capture and restore the original CAO_HOME_DIR value rather than unconditionally dropping it; avoids breaking sandboxed test runs where the var is legitimately set - Add tests: empty string, whitespace-only, tilde expansion, and owner-only permissions on import-time dirs - Update derived-path assertions to compare against .resolve() since the new read normalises the path - Add behavioral tests for cursor_cli._cao_tmp_dir: fallback follows CAO_HOME_DIR override (reload order mirrors settings_service tests), and CAO_TMP_DIR env var takes precedence over the fallback - Document the ~/.aws/opencode and ~/.kiro exceptions in the CAO_HOME_DIR section (the "moves everything" claim was overstated), add a security note recommending a private directory, note the empty/tilde handling, and register CAO_HOME_DIR in the env-var reference table * fix(config): address round-2 review feedback on CAO_HOME_DIR - Harden base CAO_HOME_DIR itself with mode=0o700 + best-effort chmod for pre-existing directories (mkdir parents=True only applies mode to the leaf; the base dir was born 0o755 under typical umask) - Quote pipe-pane file_path with shlex.quote so paths with spaces or shell metacharacters from CAO_HOME_DIR don't break terminal logging - Add CHANGELOG entry under [Unreleased] ### Added - Add module-state restore (try/finally) to cursor_cli reload test so importlib.reload side effects don't leak into subsequent tests - Fix permission assertion to check no group/other access (& 0o077 == 0) instead of exact 0o700 which is umask-fragile - Add test for pre-existing base dir chmod behaviour - Generalize docs exceptions to include ~/.copilot/agents alongside ~/.kiro/agents (same provider-native pattern) - Fix constants.py comment: seccomp-bpf filters syscalls not paths; AppArmor/mount namespaces is the accurate mechanism example - Update agent_profiles.py docstrings to note paths derive from CAO_HOME_DIR rather than citing the literal default path
….x agy (awslabs#517) The base paste_submit_delay (0.3s) is too short for the current Gemini agy CLI: an Enter sent that soon after a bracketed paste is consumed as a literal newline, so the pasted task is left UNSUBMITTED and the agent sits at 'ready for my first task'. This silently breaks scheduled flows and supervisor assign/handoff on the antigravity provider (the agent boots, acknowledges its role, and never starts). Override paste_submit_delay (1.5s) so the paste settles before the Enter, and paste_enter_count (1) since agy submits on a single Enter (base default of 2 is Claude Code multi-line mode). Scoped to AntigravityCliProvider; no other provider is affected. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…awslabs#513) * feat(ops-mcp): pass launch model and initial message * test: cover launch session validation branches * fix(mcp): preserve initial message for new sessions
…wslabs#397/awslabs#501/awslabs#513) NOT FINISHED -- pushed as-is to avoid losing work (this checkout lives under /tmp, tmpfs, gone on reboot). Cherry-picked/reapplied the two still-needed fork-only fixes (harness-control#215 event-loop-blocking initialize(), awslabs#225 fullscreen-onboarding-upsell hang) onto upstream's current main, which already contains awslabs#397 (pipe-stall watchdog)/awslabs#501/awslabs#513. Deliberately did NOT bring forward harness-control#186 (already upstream as awslabs#446) or awslabs#303's create_sibling_session + its 2 security fixes (superseded by workain/harness-control#461's own safe MCP-server equivalent). Deliberately did NOT broaden WAITING_USER_ANSWER_PATTERN or wait_until_status's accept-set (the other half of awslabs#225's original fix) -- that touches a SHARED regex with its own documented false-positive risk on ordinary agent prose, used by get_status() in ways this rebase hasn't fully audited against upstream's newer code paths. The narrow, essential fix (auto-dismiss the fullscreen upsell by name) is included; the broader generalization is not. Test suite for the touched files passes (test_claude_code_coverage.py, test_claude_code_unit.py, test_provider_init_timeout.py, test_container_wrapped.py, test_startup_prompt_idle_gap.py) -- 174+ tests, 0 failures, in this checkout. NOT yet run against the FULL fork test suite. Full e2e (workain/harness-control#485's own non-negotiable condition) NOT yet done. Pin not yet updated in workain/harness-control's own CI workflows. See workain/harness-control#485 for the full investigation writeup.
…labs#225-onto-upstream graft Closes two of the three gaps the prior WIP commit (20f5166) flagged as not yet done. Full fork test suite: `uv run pytest test/ -q` -- 61 failed, 5314 passed, 36 skipped, 1 xfailed. All 61 failures reproduced identically on plain upstream/main (9a56f01, no awslabs#215/awslabs#225 cherry-picks) -- confirmed by running the exact same failing files against a clean upstream/main checkout side by side (test/api/test_agui_run_endpoint.py, test/services/agui/test_run_plane*.py, test/telemetry/test_otel_init.py, and the remaining agui coverage/ac5-gate files -- 34 + 27 = 61, matching exactly). All are pre-existing AGUI/telemetry environment-dependent failures unrelated to providers/claude_code.py; zero regressions attributable to this graft. Live smoke test (real cao-server from this checkout, real Claude Code OAuth, real tmux, isolated scratch HOME/CAO_HOME_DIR, torn down after): POST /sessions with an explicit model=sonnet query param -- the same code path assign/handoff's new model parameter drives -- returned synchronously at status=idle (not the incident's zombie status=completed/0 messages), the real Claude Code TUI came up ("Sonnet 5 * Claude Max"), and a real message round-trip got a real model-aware reply ("ALIVE - Sonnet 5 (claude-sonnet-5)."). Terminal stayed healthy past the ~60s window the original zombie died within. Still NOT done (the one remaining gap): harness-control's own full Playwright e2e suite against a build pinned to this commit (workain/harness-control#485's own non-negotiable condition) -- not run, no node/npm available in the environment this verification ran in. harness-control's own CI pin is also not yet updated. See workain/harness-control#485 for the full writeup and workain/harness-control#501 for a complementary (independent, not blocking) proposal to detect any zombie that still gets through after this lands.
…ool (awslabs#432) Implements the feature proposed in awslabs#432 (filed by us): a nullable ordered `group` array and free-form `metadata` JSON on TerminalModel, plus a `list_siblings` MCP tool + matching API endpoint for group-prefix-scoped sibling discovery between terminals created outside the supervisor/worker handoff pattern. - `group`/`metadata` columns on TerminalModel (idempotent ALTER TABLE migration), settable at create_session, `group` updatable via a new `PATCH /terminals/{id}/group` (for consumers whose own grouping can change after creation), `metadata` updatable by the running agent itself via a new `update_metadata` MCP tool. - `GET /terminals/{id}/siblings` + `list_siblings` MCP tool: resolves the caller's identity from its own CAO_TERMINAL_ID (never a client-supplied claim, same mechanism send_message/handoff already use), clamps depth to [1, len(caller_group)] server-side, rejects depth=0 at the API boundary (422) rather than reinterpreting it as an unscoped all-terminals query. A terminal with no group participates in no discovery, in either direction. - Regression tests cover the documented edge cases: mismatched group lengths (shorter sibling excluded, longer sibling still matches on shared prefix), depth=0/negative rejection, no-group exclusion both ways, caller excluded from its own results, depth clamping in both directions. harness-control#160
…nal's response, add migration test Independent self-ROAST findings on the awslabs#432 implementation: - create_terminal(group=[], metadata={}) echoed the raw empty containers in its return dict while storing NULL in the DB -- a follow-up get_terminal_metadata()/GET /terminals/{id} on the same row disagreed (group: [] vs group: null). Now normalized consistently on both paths. - The new group/metadata ALTER TABLE migration had no dedicated regression test (only caller_id's pre-existing migration was covered). Added legacy-table and idempotency coverage matching the existing caller_id migration tests' shape. harness-control#160
…ings (awslabs#432) Fixes all 7 findings from PR awslabs#433's automated Copilot review: - UpdateGroupBody.group / UpdateMetadataBody.metadata are now required fields (no default) instead of Optional[...] = None, so an omitted field in a PATCH body is rejected with 422 rather than silently treated the same as an explicit null (which clears the group/metadata). Explicit null/[] still clears as before. - update_terminal_group_endpoint's docstring updated to match the actual (fixed) semantics. - update_group, update_metadata, and list_siblings service calls in the three async endpoint handlers are now wrapped in asyncio.to_thread, consistent with the rest of this file's handlers, so they no longer block the event loop. - list_siblings_by_group_prefix now prefilters with a SQL LIKE prefix match on the JSON-encoded group column before loading/decoding candidate rows in Python, instead of scanning and JSON-decoding every grouped terminal on the server. The exact Python-level comparison is kept as the source of truth after the SQL narrowing. Adds regression tests for the omitted-vs-null 422 rejection (both endpoints) and for the SQL prefilter actually narrowing the query (via a json.loads call-count spy proving non-matching rows are never decoded), plus edge-case tests for LIKE-wildcard-escaping and text-prefix false-positive avoidance. Independently self-ROASTed by a separate adversarial review agent given this touches an accidental-data-clearing risk and a real perf path; full test suite (4068 tests) passes with zero regressions.
…ngs-based prevention
Removes FULLSCREEN_UPSELL_PROMPT_PATTERN and the runtime detect-and-dismiss
code (send_special_key("2")) added for the "Try the new fullscreen renderer?"
onboarding upsell (workain/harness-control#225). Per the operator's own
"fork = staged upstream PRs only" policy: this was permanent, fork-only
divergence with no path upstream (unlike awslabs#215/awslabs#432, it was never a good fit
for CAO's own orchestrator logic -- it's entirely about one CLI's own
first-run UI text).
Replaced with prevention instead of reaction: `_ensure_skip_bypass_prompt_setting`
is renamed to `_ensure_startup_settings` and now also seeds `"tui": "default"`
into `~/.claude/settings.json` (only if the key is absent -- an explicit prior
choice, e.g. `"tui": "fullscreen"`, is never overridden). Claude Code's own
gate for this prompt (found via `strings` on the installed binary) skips
showing it whenever `tui` is already set to anything, so with this seeded
before first launch the prompt has no runtime shape to detect or dismiss in
the first place. `"default"` (not `"fullscreen"`) keeps the classic renderer
this file's own screen-scraping status detection already expects.
Verified:
- Full suite (test/, 5432 collected excl. deselected): 62 failed / 5371
passed vs a same-branch pre-change baseline's 61 failed / 5370 passed --
the one difference (test_fifo_reader.py::test_cold_start_end_to_end_via_real_reader_thread)
reproduced as flaky, passing 3/3 in isolation; confirmed unrelated (this
patch touches zero fifo_reader code). Zero real regressions.
- Direct mechanism check: `_ensure_startup_settings()` against a real,
isolated, from-scratch `~/.claude/settings.json` writes both
`skipDangerousModePermissionPrompt: true` and `tui: "default"` correctly;
an explicit pre-existing `tui` value is left untouched (new unit tests).
- [unverified] Full live launch-to-idle confirmation (does a real session
launch cleanly with no fullscreen prompt, end to end) was attempted against
an isolated cao-server + real Claude Code CLI process but was inconclusive:
this shared host was under severe concurrent load at test time (load
average 10.6-11, <700MB free RAM, dozens of other live sessions from other
agents) and session creation itself timed out generically -- the same
resource-collapse class awslabs#215 already documents, not something this diff's
own scope change plausibly causes (it touches prompt-suppression config
only, not the async/threading path awslabs#215 fixed). Not re-attempted further to
avoid adding more load to an already-critical shared box. The settings-file
mechanism itself (the actual behavior change) is independently verified
above without needing a full live launch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ommand substitution Codex sessions with a non-trivial system prompt (harness-control's own security preamble + skill list + operating instructions commonly total several KB once escaped) could hang indefinitely at launch with no useful error -- CAO's own init-timeout would eventually fire with a generic "Codex initialization timed out" that gave no hint of the real cause. Root cause: _build_codex_command() inlined the entire escaped system prompt directly into the launch command via -c developer_instructions="<text>". That command gets typed into a still-bare shell pane (codex hasn't started yet), which correctly skips bracketed-paste framing since a bare shell doesn't understand it -- but without that framing, a single line beyond the tty's canonical-mode line-length limit (4096 bytes on Linux) is silently truncated by the kernel before the shell ever sees a complete command. Reproduced live in an isolated scratch tmux pane: a real 8.3KB generated command never executed even with an explicit trailing Enter, confirmed via a marker-file test, while the same text passed dash -n/bash -n as a plain script -- ruling out a quoting bug and confirming line length as the cause. Fix: write the escaped value to a CAO-owned temp file and reference it via a shell command substitution ($(cat <file>)), keeping the actual typed/pasted launch line short regardless of prompt length. Mirrors the file-based approach claude_code.py and kimi_cli.py already use for the same purpose, adapted since Codex has no direct "arbitrary absolute path" flag of its own. 6 existing tests updated (they asserted prompt text directly in the command string; it now lives in the temp file). 3 new tests added, including the actual regression test (a 10KB prompt keeps the launch line under 1000 bytes). Full suite: 61 failed/5635 passed, an exact match to this suite's own pre-existing baseline on unpatched main -- zero new failures, none codex-related.
An account with no OpenAI/Codex credentials configured yet reaches Codex's
real "Sign in with ChatGPT / Device Code / API key" welcome menu on launch --
a correctly-rendered, fully-alive screen, nothing actually broken. But
initialize()'s own wait_until_status(..., {IDLE, COMPLETED}, ...) had no way
to ever succeed for it: that menu never becomes IDLE/COMPLETED on its own,
so the 60s init timeout always fired and the terminal was torn down before
an operator had any real chance to open the session and complete login.
Live-reproduced: a real codex session launched successfully (process spawned,
full command parsed correctly, genuine welcome+login screen rendered) but was
torn down by the timeout every time, exactly matching the reported "session
doesn't even start" symptom -- even though the launch itself was fine.
Fix: recognize the login menu in get_status() (LOGIN_MENU_PATTERN, bottom-
anchored with its footer, same shape as the existing V2 trust dialog check)
and classify it WAITING_USER_ANSWER, then widen initialize()'s own target
status set to include WAITING_USER_ANSWER. Unlike the trust/update dialogs
handled by _handle_trust_prompt, this one can't be auto-dismissed -- it
requires a real human to actually complete OAuth or supply a real API key --
so the fix is "let init succeed and keep the terminal alive for a human",
not "auto-dismiss".
3 new tests, including the actual regression test asserting
WAITING_USER_ANSWER is now in initialize()'s target status set. Full suite:
61 failed/5638 passed (net +3 vs the 5635 baseline, matching the 3 new
tests) -- zero new failures.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…accident test_backend_registry_is_clean_at_test_start isn't part of either codex fix in this branch -- it's a pre-existing upstream/main test (fixing an unrelated issue, awslabs#522) that depends on an autouse fixture living in test/conftest.py. This graft branch is 13 commits behind upstream/main and doesn't have that fixture yet, so the test showed up only as merge-conflict-resolution context when cherry-picking the login-menu fix (git's 3-way merge needed it to place the new tests correctly) and deterministically fails here -- reproduced with a real leak from one of the OTHER pre-existing tests in the same class, none of which are part of either codex fix either. Removing it here; it's already present and working correctly in the actual PR branch this was cherry-picked from (fix-codex-long-developer-instructions), which is based on current upstream/main and has the real fixture. Full codex suite: 152 passed, 3 skipped, 0 failed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e-pane read get_status()'s stale-PROCESSING re-check re-derives from the SAME rolling self._buffers[id] the FIFO push pipeline feeds -- once the underlying process goes genuinely idle and stops emitting output, that buffer stops changing too, so re-running detection on it produces the same PROCESSING/UNKNOWN result forever even though the real pane already shows a ready state. Live-reproduced twice in one operator session on a real production box (app.workain.ai): a real chat message queued behind PROCESSING sat undelivered for ~10 minutes until a manual tmux resize (forcing a fresh redraw) unstuck it. No automatic self-healing existed for this case at all. Adds a rate-limited fallback (STALE_PROCESSING_CAPTURE_INTERVAL_S, default 3s) that reads the pane directly via get_backend().get_history() -- a real tmux capture-pane, not the FIFO-fed buffer -- the same reliable source codex.py's _handle_trust_prompt already uses for init-time dialog detection. tmux always holds the correct, current rendered pane state regardless of output volume, so this can see a genuine ready state the stale buffer cannot. Rate-limited (not on every poll) because get_status() is a hot path across the whole fleet and a capture-pane read is a real subprocess call, unlike the existing cheap buffer re-check -- unbounded, it would repeat the "fork storm freezes the server" class of problem run()'s own docstring already documents for status detection in general.
Owner
Author
|
Filed upstream as awslabs#558 (this fork branch itself was too entangled with unrelated fork history to file as-is — cherry-picked just this commit onto clean upstream |
Owner
Author
|
Closing — superseded by awslabs#558, a clean cherry-pick of this fix filed upstream directly (this branch's own diff was too entangled with unrelated fork history to serve as a useful staging record). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
StatusMonitor.get_status()'s existing stale-PROCESSING re-check re-derives from the samerolling
self._buffers[terminal_id]the FIFO push pipeline feeds. Once the underlying processgoes genuinely idle and stops emitting output, that buffer stops changing too — re-running
detection against it produces the same PROCESSING/UNKNOWN result forever, even though the real
pane already shows a ready state.
Live-reproduced twice in one operator session on a real production deployment: a real chat
message queued behind a PROCESSING status sat undelivered for ~10 minutes, only delivered after
a manual
tmux resize-windowforced a fresh redraw. There was no automatic self-healing path forthis at all — a genuinely-idle terminal could get stuck showing PROCESSING indefinitely.
Fix
Adds a rate-limited fallback: when the cheap buffer-based re-check still can't resolve a cached
PROCESSING status, read the pane directly via
get_backend().get_history()— a realtmux capture-pane, not the FIFO-fed buffer — and re-run provider detection against that. This is thesame reliable source
providers/codex.py's_handle_trust_promptalready uses for init-timedialog detection: tmux always holds the correct, current rendered pane state regardless of
output volume, so this can see a genuine ready state the stale buffer cannot.
Rate-limited via
STALE_PROCESSING_CAPTURE_INTERVAL_S(default 3s, per-terminal) becauseget_status()is a hot path (every poll, across the whole fleet) and a capture-pane read is areal subprocess call, unlike the existing cheap buffer re-check — unbounded, it would repeat the
"fork storm freezes the server" class of problem
run()'s own docstring already documents forstatus detection in general.
Testing
TestStaleProcessingCapturePane) covering: self-heals to the real status,correctly stays PROCESSING when the fresh capture-pane read also shows PROCESSING, non-fatal on
a capture-pane read failure, non-fatal when no provider /
get_provider()raises, rate-limiting(two immediate polls only shell out once; retried after the window elapses), and that the
capture-pane fallback is skipped entirely when the existing cheap buffer re-check already
resolves the status (no added overhead in the common case).
AttributeErrorfor thetime-patch tests since the import doesn't exist pre-fix; the resttrivially pass since old code never reaches the new path either way), restored, confirmed all
pass.
test/services/test_status_monitor.pysuite: 36/36 pass (27 pre-existing + 9 new).and telemetry/otel_init — confirmed unrelated: same failures reproduce on a clean checkout of
this branch's base commit before this change).
ruff checkclean on the changed file.