Skip to content

feat(harness): add NativeHarnessProvider seam foundation (PR 1.1) - #3239

Merged
PattaraS merged 2 commits into
mainfrom
harness-seam-1-1-pr
Jul 28, 2026
Merged

feat(harness): add NativeHarnessProvider seam foundation (PR 1.1)#3239
PattaraS merged 2 commits into
mainfrom
harness-seam-1-1-pr

Conversation

@PattaraS

@PattaraS PattaraS commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

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. This is PR 1.1 — the additive foundation that unblocks the rest.

Critical path: 1.1 → 1.2 → 1.5 → 2.2 → 2.3. After 1.1+1.2 land, 1.3–1.8 touch mostly disjoint hubs and can go in parallel.

Related issue

N/A — first implementation PR of the modular native-harness registry (Phase 1, PR 1.1). Tracked in designs/harness-modular-registry-proposal.md.

Summary

First, purely additive step of Phase 1: introduce the behavior side-channel that later PRs will dispatch native harnesses through. No hub is rewired yet, so this changes no runtime behavior — the built-in native harnesses still run exactly as before.

  • Add NativeHarnessProvider (a frozen dataclass of dotted import-path strings for a native harness's lifecycle hooks) and the native_providers field on HarnessContribution, plus native_providers() / native_provider_for_key() accessors.
  • Populate 11 built-in provider rows uniformly from the omnigent.<key>_native module layout: run_<key>_native (CLI + resume launch), _materialize_<key>_agent_spec (agent seeding), and the _auto_create_<key>_terminal builder re-exported from omnigent.runner.native. Hooks that are still runner closures / inline dispatch (interrupt, stop, spawn-env, bridge-dir) stay None until those hubs migrate onto the seam in later PRs.
  • Add omnigent/native_dispatch.py: a lazy resolver over the existing load_object, with resolve / resolve_hook / resolve_hook_for_key, so a hub can resolve a hook instead of branching on key == "<x>". Resolution is intentionally uncached (dispatch runs at most once per resume/launch/seed; importlib.import_module already caches the module), which keeps monkeypatch.setattr("...:run_x", ...) working.

Import hygiene preserved: provider rows hold strings; only the resolver imports the target modules, and only at dispatch time — building the registry never pulls in the runner / CLI / native-harness stack.

The community validator still rejects native metadata; Phase 2 (2.1) flips it to positive validation.

This PR also adds an append-only Implementation progress ledger to the design doc so each PR in the stack records its status without editing the plan tables (which would conflict across the stack on rebase).

Test Plan

python -m pytest tests/test_native_dispatch.py tests/test_harness_plugins.py -q   # 18 passed
python -m ruff check omnigent/harness_plugins.py omnigent/native_dispatch.py tests/  # clean
  • New tests assert the provider rows cover every native agent 1:1, that the required hooks (run_native, auto_create_terminal) are set, and — the key guard — that every populated built-in provider path actually resolves to a callable (imports all 33 target symbols across the 11 harnesses), so a typo'd import path or a renamed run_<x>_native symbol fails here rather than at dispatch time.
  • Resolver tests cover colon/dot import forms, the uncached-resolution contract (a monkeypatched target is reflected), and the unset-hook / unknown-key None paths.

Demo

N/A — no user-facing behavior change (additive data model + resolver only).

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

Unit tests accompany the new code (tests/test_native_dispatch.py, additions to tests/test_harness_plugins.py). Because this PR adds a data model + resolver and wires no dispatch hub, there is no runtime path to exercise end-to-end yet; the behavior-preservation of the built-ins is covered by the existing native suites, which are unchanged. The design-doc ledger is documentation-only.

This pull request and its description were written by Isaac.

Copilot AI review requested due to automatic review settings July 24, 2026 14:31

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/L Pull request size: L label Jul 24, 2026
PattaraS added a commit that referenced this pull request Jul 24, 2026
Add an append-only "Implementation progress" ledger to the modular-registry
proposal so each PR in the stack records its own status without editing the
plan tables (which would conflict across the 1.1→1.2→1.3 stack on every
rebase). Seed it with 1.1 (#3239, in review).

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

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.

@omnigent-ci

omnigent-ci Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None. This is a purely additive change — a new frozen NativeHarnessProvider dataclass, a native_providers field/accessors on the registry, and a lazy string→callable resolver. No hub is rewired, so no runtime path changes. Import hygiene holds: native_dispatch imports only load_object, NativeHarnessProvider, and native_provider_for_key from harness_plugins (no runner/CLI/native-harness stack), and provider rows carry dotted strings resolved only at dispatch time. I confirmed all 33 built-in hook paths (run_<key>_native, _auto_create_<key>_terminal, _materialize_<key>_agent_spec across all 11 harnesses) resolve to real callables, so the honesty guard test is meaningful and passes.

Security vulnerabilities

None. load_object imports a fixed set of internal, code-defined import strings; nothing here is attacker-influenced. No new dependencies, extras, or lockfile changes.

Non-blocking notes

  • PR description contradicts the implemented caching — reconcile before later PRs depend on it. The description says resolution is "intentionally uncached … which keeps monkeypatch.setattr("...:run_x", ...) working" and claims tests cover "the uncached-resolution contract (a monkeypatched target is reflected)." But native_dispatch.resolve does cache per import path via _RESOLVE_CACHE, its docstring says so, and the actual test is test_resolve_is_cached (there is no monkeypatch-reflection test). The behavior is internally consistent (code + docstring agree), so this isn't a bug today, but the description is stale and the stated monkeypatch guarantee is false: once a path is resolved, a later monkeypatch.setattr on that target won't be reflected without reset_resolve_cache_for_tests(). Fix the description, and make sure downstream dispatch-hub tests (PRs 1.3–1.7) either reset the cache or don't rely on the "uncached" contract that no longer exists.

  • resolve re-resolves any target that legitimately resolves to None. The cache check is if cached is None:, so a hook whose resolved object is falsy-None would never cache. Harmless for the current all-callable targets, but a sentinel/in check would be more correct if non-callable values are ever resolved. Minor.

  • native_providers() / native_provider_for_key() do linear scans per call; fine at this scale and matches the existing native_agents() pattern.

Summary

Clean, well-scoped foundational PR: additive data model plus a lazy resolver, no behavior rewiring, good import hygiene, and a strong test that all built-in provider paths resolve to callables (catching typos/renames at test time rather than dispatch time). The one thing worth correcting is the PR description's claim of "intentionally uncached" resolution and a monkeypatch-reflection contract — the code caches and has no such test — so that expectation shouldn't be carried into the dependent PRs. No visual demonstration is needed (backend-only, no user-visible change). Approve once the description/caching mismatch is reconciled.


Automated review by Polly · workflow run

PattaraS added 2 commits July 28, 2026 22:12
First, additive step of Phase 1 of the modular native-harness registry
(designs/harness-modular-registry-proposal.md). Introduces the behavior
side-channel that later PRs will dispatch through; no hub is rewired yet, so
this changes no runtime behavior.

- Add `NativeHarnessProvider` (frozen dataclass of dotted import-path strings
  for a native harness's lifecycle hooks) and the `native_providers` field on
  `HarnessContribution`, plus `native_providers()` / `native_provider_for_key()`
  accessors.
- Populate 11 built-in provider rows uniformly from the `omnigent.<key>_native`
  module layout (`run_<key>_native`, `_materialize_<key>_agent_spec`, and the
  `_auto_create_<key>_terminal` builder re-exported from `omnigent.runner.native`).
  Hooks that are still runner closures / inline dispatch (interrupt, stop,
  spawn-env, bridge-dir) stay None until those hubs migrate onto the seam.
- Add `omnigent/native_dispatch.py`: a lazy, per-path-cached resolver over the
  existing `load_object`, with `resolve` / `resolve_hook` / `resolve_hook_for_key`
  so hubs resolve a hook instead of branching on `key == "<x>"`. Import hygiene
  preserved — provider rows hold strings; only the resolver imports the target
  modules, and only at dispatch time.
- Tests: provider rows cover every native agent 1:1, required hooks are set, and
  every populated built-in path actually resolves to a callable (guards against
  a typo'd path or renamed symbol); resolver colon/dot forms, caching, and
  unset-hook / unknown-key None paths.

The validator still rejects community native metadata (Phase 2 flips it).

Co-authored-by: Isaac
Signed-off-by: Pat Sukprasert <pattara.sk127@gmail.com>
Add an append-only "Implementation progress" ledger to the modular-registry
proposal so each PR in the stack records its own status without editing the
plan tables (which would conflict across the 1.1→1.2→1.3 stack on every
rebase). Seed it with 1.1 (#3239, in review).

Co-authored-by: Isaac
Signed-off-by: Pat Sukprasert <pattara.sk127@gmail.com>
@PattaraS
PattaraS force-pushed the harness-seam-1-1-pr branch from 9e7f5a2 to 6adcaed Compare July 28, 2026 14:13
Copilot AI review requested due to automatic review settings July 28, 2026 14:13

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.

@PattaraS

Copy link
Copy Markdown
Contributor Author

/review

@PattaraS
PattaraS enabled auto-merge (squash) July 28, 2026 14:14
@omnigent-ci

omnigent-ci Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None. The change is purely additive (verified: no hub imports native_dispatch or resolve_hook yet), so no runtime path is rewired and no existing behavior changes. I confirmed the load-bearing claims hold against main:

  • All 11 native *_native.py modules exist and each exports run_<key>_native and _materialize_<key>_agent_spec (checked all 11 keys).
  • omnigent.runner.native (a package) re-exports every _auto_create_<key>_terminal for all 11 keys via its __init__, so the omnigent.runner.native:_auto_create_<key>_terminal paths resolve.
  • load_object handles both module:attr and module.attr forms as the resolver relies on.

The test_builtin_native_provider_paths_resolve guard genuinely imports all 33 populated symbols, so a typo would fail in CI rather than silently at dispatch.

Security vulnerabilities

None. Provider rows are hardcoded string constants (no user/plugin input reaches load_object in this diff), and imports happen lazily only at dispatch time. No new secret handling, deserialization, or network surface.

Non-blocking notes

  • PR description contradicts the code on caching. The description states resolution is "intentionally uncached ... which keeps monkeypatch.setattr("...:run_x", ...) working," but native_dispatch.resolve does cache in _RESOLVE_CACHE, and the module docstring + test_resolve_is_cached assert caching. These can't both be true. The behavior itself is defensible (dispatch runs at most once per resume/launch/seed), but the description's monkeypatch rationale is now false: once a path is resolved, a later monkeypatch.setattr on the target symbol will not be reflected because resolve returns the cached object. Since there are no dispatch-site callers yet, this is latent — but worth reconciling the description/docstring before a later PR builds a test expecting hot-swap behavior. (Tests currently sidestep it by calling reset_resolve_cache_for_tests.)
  • None-valued resolutions re-import every call. resolve uses cached = _RESOLVE_CACHE.get(import_path); if cached is None: reload. If any target symbol ever legitimately resolves to None, it will never be cached and will re-import_module on each call. Not a concern for the current callables-only rows; a import_path not in _RESOLVE_CACHE check would be more robust if the resolver is ever used for non-callable values.
  • Process-global cache has no invalidation outside tests. Fine for the built-in registry, but if Phase 2 community plugins can re-register providers, a stale cache entry could shadow an updated path. Worth keeping in mind when the seam gains dynamic contributors.

Summary

Solid, well-scoped foundational PR. It adds a frozen NativeHarnessProvider data model, an import-light lazy resolver, and a strong resolution guard test, without rewiring any hub — so it carries no runtime-behavior risk and the "additive only" claim checks out. No blocking or security issues. The only real cleanup is reconciling the caching story: the PR description claims uncached-for-monkeypatch while the code caches, and that mismatch should be fixed (in doc or code) before a later PR depends on either behavior.


Automated review by Polly · workflow run

@PattaraS
PattaraS merged commit c41d404 into main Jul 28, 2026
65 checks passed
@PattaraS
PattaraS deleted the harness-seam-1-1-pr branch July 28, 2026 14:31
@github-actions github-actions Bot added the no-doc-update Merged PR does not need a docs update label Jul 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ Doc impact: no-doc-update

Internal refactor adding a lazy provider-hook resolver seam for existing native harnesses (no new harness/provider/policy or changed user-facing config or defaults), so no documentation update is needed.

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>
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/L Pull request size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants