Skip to content

refactor(harness): registry-driven native launch dispatch — scaffolding + uniform arms (PR 1.5b-i) - #3500

Merged
PattaraS merged 1 commit into
mainfrom
harness-seam-1-5b-i-pr
Jul 29, 2026
Merged

refactor(harness): registry-driven native launch dispatch — scaffolding + uniform arms (PR 1.5b-i)#3500
PattaraS merged 1 commit into
mainfrom
harness-seam-1-5b-i-pr

Conversation

@PattaraS

Copy link
Copy Markdown
Contributor

Workstream progress

Modular native-harness registry (designs/harness-modular-registry-proposal.md). ~14 PRs total — Phase 1 (internal seam): 1.1–1.4, 1.5a/b/c, 1.6, 1.7, 1.8; Phase 2: 2.1–2.4.

1.5b is split so the low-risk bulk (this PR) lands separately from the behavior-preservation-critical special arms (1.5b-ii, stacked on this).

Related issue

N/A — Phase 1, PR 1.5b-i of the modular native-harness registry.

Summary

Wires the provider auto_create_terminal field (declared since 1.1, never dispatched) and collapses the 8 uniform native launch arms in runner/app.py onto it. Behavior-preserving. Net −256 lines in app.py.

  • orchestration.py — add:
    • NativeLaunchContext: one flat frozen dataclass of the inputs the 11 builders may need (incl. claude's 3 closures), each adapter reads only its subset.
    • PreLaunchResult (skip / force_recreate / needs_terminal) — consumed by the shell; the special arms in 1.5b-ii produce it.
    • 11 thin _launch_<x>(ctx) adapters that unpack the context and call the unchanged _auto_create_<x>_terminal builder with that harness's exact kwarg subset (Option A — keeps the 21 direct-call builder tests intact).
    • _launch_native_terminal(harness, ctx, *, ensure_locks, pre_launch=None, resolve_agent_spec=None) — the shared shell: lock → existence-check → pending/error events → resolve_hook(provider, "auto_create_terminal")(ctx). agent_spec resolves lazily inside the create block via resolve_agent_spec, preserving each arm's error semantics (pi unwrapped; cursor/opencode/kimi swallow OmnigentError; the rest none).
  • harness_plugins.py — repoint auto_create_terminal to omnigent.runner.native:_launch_<key>.
  • app.py — the 8 uniform arms (pi, cursor, kiro, opencode, goose, hermes, qwen, kimi) each become one _launch_native_terminal(...) call, picking the per-harness lock dict (kept app-scope so session cleanup can pop by name).
  • qwen's launch-error label is now "Qwen Code" (uniform display_name) vs the former lowercase "qwen" — cosmetic; no test asserted the literal.

Deferred to 1.5b-ii: the 3 special arms (claude/codex/antigravity, which carry pre-call rebuild/transfer/needs-checks) and the turn-path opencode cold-boot — they still call the direct builders here.

Test Plan

python -m pytest tests/test_harness_plugins.py tests/test_native_dispatch.py \
  tests/runner/test_app_sessions_native_workflow_init.py -q   # 73 passed
python -m pytest tests/runner/test_app_sessions_native_terminals_autocreate.py -q   # 48 passed
python -m ruff check omnigent/ tests/ -q   # clean
  • New unit tests: each adapter forwards exactly its builder's kwarg subset; the shell's branches (create / existing-terminal short-circuit / force_recreate teardown / skip+needs_terminal → False / start-error event on failure / lazy-spec-resolved-only-on-create / non-native → None).
  • Behavior-preservation: the workflow_init HTTP suite drives the real launch path for the uniform arms and stays green; direct-call terminals_autocreate (21 sites) unaffected.
  • The 8 converted harnesses' events_lifecycle tests pass (9 passed). The 6 failing tests in that file are all codex (app-server/gateway-env artifacts) and fail identically on clean main — codex's arm is untouched in this PR.

Demo

N/A — backend refactor, behavior-preserving. Launch path covered by the unchanged workflow_init HTTP suite.

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

New unit coverage accompanies the scaffolding (adapter kwarg subsets + shell branches). Behavior-preservation across the runner launch path is covered by the existing native workflow-init / terminals-autocreate suites, which stay green.

Changelog

qwen-native's terminal-start error label now reads "Qwen Code" (consistent with the other native harnesses) instead of "qwen".

This pull request and its description were written by Isaac.

…ng + uniform arms (PR 1.5b-i)

First half of the runner launch seam. Wires the provider's auto_create_terminal
field (declared since 1.1, never dispatched) and collapses the 8 uniform
create-session launch arms in runner/app.py onto it. Behavior-preserving.

- orchestration: add NativeLaunchContext (flat dataclass of the inputs the 11
  builders may need, incl. claude's closures), PreLaunchResult (skip /
  force_recreate / needs_terminal for the special arms in 1.5b-ii), 11 thin
  _launch_<x>(ctx) adapters that unpack the context and call the unchanged
  _auto_create_<x>_terminal builder with that harness's exact kwarg subset, and
  the shared shell _launch_native_terminal(harness, ctx, *, ensure_locks,
  pre_launch=None, resolve_agent_spec=None). The shell runs the lock /
  existence-check / pending+error-event mechanics every arm shared and resolves
  the adapter via resolve_hook(provider, "auto_create_terminal").
- Option A (adapters, builders unchanged) keeps the 21 direct-call builder tests
  intact. agent_spec is resolved lazily via resolve_agent_spec inside the create
  block, preserving each arm's error semantics (pi unwrapped; cursor/opencode/
  kimi swallow OmnigentError via _resolve_session_agent_spec_or_none; the rest
  pass no resolver).
- harness_plugins: repoint auto_create_terminal to omnigent.runner.native:_launch_<key>.
- app.py: the 8 uniform arms (pi, cursor, kiro, opencode, goose, hermes, qwen,
  kimi) become one _launch_native_terminal call each, picking the per-harness
  lock dict (kept app-scope so session cleanup can pop by name). Net -256 lines.
- qwen's launch-error label is now "Qwen Code" (uniform display_name) vs the
  former lowercase "qwen" — cosmetic; no test asserted the literal.

Deferred to 1.5b-ii: the 3 special arms (claude/codex/antigravity) and the
turn-path opencode cold-boot, which still use the direct builders.

Tests: unit-cover each adapter's kwarg subset and the shell's branches
(create / existing-skip / force-recreate teardown / skip+needs_terminal /
start-error event / lazy-spec-only-on-create / non-native None). The workflow-
init HTTP suite exercises the real launch path for the uniform arms and stays
green. Pre-existing codex gateway-env failures in events_lifecycle are unchanged
(verified identical on clean main; codex arm untouched here).

Co-authored-by: Isaac
Signed-off-by: Pat Sukprasert <pattara.sk127@gmail.com>
Copilot AI review requested due to automatic review settings July 29, 2026 10:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the size/XL Pull request size: XL label Jul 29, 2026
@omnigent-ci

omnigent-ci Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None. This is a behavior-preserving refactor, and the preservation holds up under verification:

  • The registry-driven terminal_name / display_name values match the original hardcoded strings exactly for all 8 uniform arms (piPi, cursorCursor, kiroKiro, opencodeOpenCode, gooseGoose, hermesHermes, kimiKimi), so the terminal-existence lookups (registry.get(session_id, agent.terminal_name, "main")) and start-error labels are unchanged. The one deliberate change — qwen's start-error label going from "qwen" to "Qwen Code" — is confirmed cosmetic; no test asserts the old literal.
  • The auto_create_terminal hook resolves for every key, and the lazy resolve_agent_spec semantics are faithfully reproduced: pi surfaces resolution errors as terminal-start errors (no wrapper), while cursor/opencode/kimi route through _resolve_session_agent_spec_or_none (swallows OmnigentError), and kiro/goose/hermes/qwen pass no resolver — matching the pre-refactor arms one-for-one.
  • The new unit suite plus the workflow_init HTTP suite (55 tests) and test_harness_plugins.py all pass locally, and ruff check is clean (no dead imports left behind — the _auto_create_<x>_terminal symbols are still referenced by the interrupt/stop/cleanup paths).

Security vulnerabilities

None. No new trust boundaries, deserialization, network calls, or credential handling. No lockfile or dependency-extra changes in the diff.

Non-blocking notes

  • Hook repointed for the deferred special arms. harness_plugins.py repoints auto_create_terminal to _launch_<key> for all keys, including claude/codex/antigravity, but those three app.py arms still call the direct _auto_create_<x>_terminal builders (deferred to 1.5b-ii). The _launch_claude/_launch_codex/_launch_antigravity adapters exist and resolve correctly, so this is a harmless declared-but-not-yet-dispatched seam — just worth confirming 1.5b-ii actually routes those arms through _launch_native_terminal, since nothing exercises the resolved hook for them today.
  • force_recreate teardown granularity. The shared shell calls registry.cleanup_conversation(session_id), which tears down the whole conversation's terminals, not just the target harness's. This mirrors the existing claude rebuild arm (which does the same), so it's preservation-correct — but the PreLaunchResult.force_recreate docstring frames it as "tear down an existing terminal," which slightly understates the session-wide scope. Minor doc nit for the 1.5b-ii author to keep in mind.
  • _launch_claude's assert ctx.server_client is not None. Fine given the claude arm always binds a client, but an assert is stripped under -O; a future non-claude caller reaching this adapter without a client would get an opaque None deref rather than the assertion. Not reachable today.

Summary

A clean, well-scoped refactor that collapses 8 near-identical launch arms onto a single registry-driven shell for a net −256 lines in app.py, with a thin adapter per harness that keeps the underlying builders (and their 21 direct-call tests) untouched. Behavior preservation is real and verified: terminal names, display labels, lazy-spec resolution, and per-harness error semantics all match the originals, and the full relevant test surface is green. The lazy-resolver design and the PreLaunchResult seam for the deferred special arms are a sensible split. Ready to merge pending the human's call.


Automated review by Polly · workflow run

@PattaraS
PattaraS enabled auto-merge (squash) July 29, 2026 13:14
@PattaraS
PattaraS merged commit 202cf66 into main Jul 29, 2026
70 of 72 checks passed
@PattaraS
PattaraS deleted the harness-seam-1-5b-i-pr branch July 29, 2026 13:25
@github-actions github-actions Bot added the no-doc-update Merged PR does not need a docs update label Jul 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ Doc impact: no-doc-update

Internal refactor consolidating per-harness native terminal launch arms into a shared _launch_native_terminal helper plus adapters, with tests and an internal design-doc status tweak — no harness added/removed, no config or user-facing behavior changed.

Auto-classified on merge. Set the label manually before merging to override. · run

appletechie added a commit to appletechie/omnigent that referenced this pull request Jul 29, 2026
Four conflicts. Three are our copy vs upstream's now-merged copy of the
same change (omnigent-ai#3431, merged 2026-07-29) — took upstream wholesale, since the
maintainer's version is stricter: the helper is public
(harness_owns_its_credential) and gains an early
`if canonical in _HARNESS_FAMILY: return False`.

The fourth, runner/app.py, needed a real port rather than a side:

  ours:     _build_acp_spawn_env(spec, ..., override_harness=acp_override)
  upstream: _build_acp_spawn_env(effective_spec, cwd=cwd, workdir=workdir)

Upstream introduced `effective_spec`, which bakes in the per-session model
override. Taking our side verbatim would have silently dropped that
override for every ACP session, so this keeps upstream's spec AND our
acp:<slug> threading (omnigent-ai#3057).

The openai-agents picker label survives the merge untouched in both
sources despite upstream refactoring harness_plugins.py three times
(omnigent-ai#3239, omnigent-ai#3495, omnigent-ai#3500) — different regions, no conflict.

Signed-off-by: apeltekci <andrew@peltekci.com>
PattaraS added a commit that referenced this pull request Jul 30, 2026
…ch arms (PR 1.5c) (#3543)

Collapse the terminal-ensure / attach path in create_session_terminal —
11 hardcoded `if terminal_name == "<x>" and session_key == "main"` arms —
behind a single generic `_ensure_native_terminal(...)` shell dispatched
through the NativeHarnessProvider seam. The attach-path sibling of the 1.5b
launch shell (#3500/#3501); reuses the `_launch_<x>` adapters and
NativeLaunchContext. codex/antigravity supply an ownership predicate; codex
supplies a `finalize` for its one-shot policy notice — both run under the
per-session ensure lock, matching the inline arms.

- New shell in runner/native/orchestration.py (view-based existence check,
  returns JSONResponse: 200 / 500 / 409), exported from runner/native.
- app.py: 11 arms (~450 lines) -> one collect-then-dispatch block.
- Repoint the HTTP attach-path claude/codex auto_create monkeypatch targets
  to the orchestration module (the seam resolves the adapter there).
- 8 new unit tests for the shell.
- Doc: add 1.5c ledger row; flip stale 1.5b-i/ii rows to landed.

Behavior-preserving: qwen error label -> "Qwen Code" (display_name, as 1.5b-i);
antigravity now wires ensure_comment_relay via the base ctx (the landed
_launch_antigravity adapter already passed it).

Co-authored-by: Isaac

Signed-off-by: Pat Sukprasert <pattara.sk127@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-doc-update Merged PR does not need a docs update size/XL Pull request size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants