Skip to content

refactor(harness): normalize native launcher pass-through args (PR 1.2)#3244

Open
PattaraS wants to merge 2 commits into
harness-seam-1-1-prfrom
harness-seam-1-2-pr
Open

refactor(harness): normalize native launcher pass-through args (PR 1.2)#3244
PattaraS wants to merge 2 commits into
harness-seam-1-1-prfrom
harness-seam-1-2-pr

Conversation

@PattaraS

Copy link
Copy Markdown
Contributor

Stacked on #3239 (PR 1.1). Base branch is harness-seam-1-1-pr, so this diff shows only 1.2's changes. Merge #3239 first; GitHub will retarget this to main automatically.

Workstream progress

Modular native-harness registry (designs/harness-modular-registry-proposal.md). 12 PRs total — Phase 1 (internal seam, core-only): 1.1–1.8; Phase 2 (community + web): 2.1–2.4.

Critical path: 1.1 → 1.2 → 1.5 → 2.2 → 2.3.

Related issue

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

Summary

The 11 run_<x>_native launchers each spelled their pass-through arg differently (claude_args, pi_args, …). The provider seam (1.1) needs one uniform spelling to call them generically. This introduces extra_args as that spelling and keeps <x>_args as a back-compat alias. No behavior change — every native harness launches identically.

  • Add native_terminal.normalize_extra_args(): reconciles extra_args vs the legacy <x>_args alias — extra_args wins, the legacy alias emits a DeprecationWarning (removal targeted for 0.9.0; we're on 0.7.0.dev0, so the deprecate-and-remove-in-different-releases rule holds), neither yields ().
  • Give all 11 run_<x>_native entry points a keyword-only extra_args and make <x>_args an optional deprecated alias, normalizing at the top of each body so the deep internals keep using the existing local variable unchanged.
  • Migrate the internal callers (resume_dispatch ×10, chat resume-redirect ×6, cli_native ×11) to extra_args so nothing in core trips the new warning; the alias exists purely for external back-compat. (Exploration found zero external callers, so the alias is belt-and-suspenders.)

Test Plan

python -m pytest tests/test_native_terminal.py tests/test_resume_dispatch.py \
  tests/cli/test_cli.py tests/cli/test_chat.py -q          # 378 passed
python -m ruff check omnigent/ tests/  # clean
  • New unit tests cover the four normalize_extra_args branches (extra-only, legacy-only+warns, both→extra-wins+warns, neither→()).
  • Internal-caller tests updated to assert the extra_args spelling. Existing native tests that still call <x>_args= now double as back-compat coverage for the alias.
  • With default warning filters the full native + hub suite is green; the handful of unrelated reds on this machine are pre-existing gateway-env artifacts, byte-identical to the clean tree.

Demo

N/A — no user-facing behavior change (keyword-arg normalization).

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

The <x>_args alias is a soft deprecation, not a hard break today — but I checked the Breaking change box because the parameter is on a one-release deprecation clock (removal in 0.9.0). Tests cover both spellings; the existing native suites (unchanged) cover launch behavior.

Changelog

The run_<x>_native launchers accept a uniform extra_args; the per-harness <x>_args keyword is deprecated and will be removed in 0.9.0.

This pull request and its description were written by Isaac.

PattaraS added 2 commits July 24, 2026 22:18
The 11 run_<x>_native launchers each spelled their pass-through arg
differently (claude_args, pi_args, ...). The provider seam needs one uniform
spelling to call them generically. Introduce extra_args as that spelling and
keep <x>_args as a back-compat alias.

- Add native_terminal.normalize_extra_args(): reconciles extra_args vs the
  legacy <x>_args alias — extra_args wins, the legacy alias emits a
  DeprecationWarning (removal targeted for 0.9.0), neither yields ().
- Give all 11 run_<x>_native entry points a keyword-only extra_args and make
  <x>_args an optional deprecated alias, normalizing at the top of each body
  so the deep internals keep using the existing local variable unchanged.
- Migrate the internal callers (resume_dispatch ×10, chat resume-redirect ×6,
  cli_native ×11) to extra_args so nothing in core trips the new warning; the
  alias exists purely for external back-compat.
- Tests: unit-cover the four normalize_extra_args branches. Existing native
  tests that still call <x>_args= now double as back-compat coverage.

No behavior change: with default warning filters the full native + hub suite
is green (verified the failure set is byte-identical to the clean tree; the
handful of red tests are pre-existing gateway-env artifacts unrelated to this
change).

Co-authored-by: Isaac
Signed-off-by: Pat Sukprasert <pattara.sk127@gmail.com>
Co-authored-by: Isaac
Signed-off-by: Pat Sukprasert <pattara.sk127@gmail.com>
@github-actions github-actions Bot added the size/L Pull request size: L label Jul 24, 2026
@omnigent-ci

omnigent-ci Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None. The refactor is behavior-preserving. normalize_extra_args collapses the two spellings correctly across all four branches, the DeprecationWarning fires only when a legacy <x>_args alias is actually supplied, and all internal entry-point callers (resume_dispatch ×10, chat resume-redirect ×6, cli_native ×11) were migrated to extra_args, so nothing in core trips the warning. stacklevel=3 correctly attributes the warning to the external caller through the caller → run_<x>_native → normalize_extra_args chain.

Security vulnerabilities

None. Pure keyword-argument plumbing; no changes to argument parsing, subprocess construction, or trust boundaries. No dependency or lockfile changes.

Non-blocking notes

  • Contract loosened for claude/codex. claude_args and codex_args were previously required (no default); both extra_args and the legacy alias now default to None, so a caller can omit both and silently get (). Harmless and arguably desirable, but it is a slightly wider contract than before — worth being aware of since a caller that used to fail loudly on a missing arg now succeeds with empty args.
  • Empty-tuple legacy alias still warns. Passing an explicit <x>_args=() (not None) emits the DeprecationWarning even though it carries no args. This is technically correct (the deprecated parameter was used), just a minor sharp edge for any caller that passed an explicit empty legacy tuple.
  • Warning message could name the winner. In the both-set case extra_args wins and the legacy value is dropped, but the message only says the alias is "deprecated." Adding "legacy value ignored; extra_args takes precedence" would make the runtime behavior self-evident. Cosmetic.
  • run_opencode_native is # pragma: no cover, so its normalize call isn't exercised by an integration test — but normalize_extra_args itself has direct unit coverage of all four branches, so the logic is covered.

Summary

A well-scoped, low-risk refactor that unifies the 11 native launchers on a single extra_args spelling while keeping each <x>_args as a back-compat alias on a documented one-release deprecation clock (removal in 0.9.0, from 0.7.0.dev0 — satisfies the deprecate-in-a-different-release rule). The normalization helper is correct, well-tested across all branches, and every internal caller was migrated so no core path emits the new warning. No visual demo is needed (no user-visible behavior change). Ready to merge once the base PR (#3239) lands.


Automated review by Polly · workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Pull request size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant