refactor(harness): registry-driven server seeding loop (PR 1.7)

[PattaraS]

Related issue

N/A — part of the modular native-harness registry workstream (designs/harness-modular-registry-proposal.md).

Summary

PR 1.7 of the modular native-harness registry refactor: collapses the server's built-in
native-agent seeding loop onto the NativeHarnessProvider seam. It's a disjoint hub
(depends on 1.1, not the runner PRs).

• omnigent/server/app.py: the 11 hand-written _ensure_default_<x>_agent helpers + their
  11 _build_<x>_native_bundle partners collapse into two registry-driven functions —
  _build_native_bundle(provider) (resolves provider.materialize_agent_spec via the seam)
  and _ensure_default_native_agents(...) (loops NATIVE_CODING_AGENTS). Net −455/+146.
• The per-harness model arg variance (codex requires a keyword-only model; kiro/opencode
  default it; the rest take just tmpdir) is bridged by one inspect.signature check, so the
  produced bundles are byte-identical to the pre-loop builders.
• debby / polly / _ensure_extra_builtin_agents stay hand-written (shipped-example and
  env-driven paths — not _materialize_*).
• The 11 per-harness built-in agent-name constants move out of server/app.py (where the
  collapsed loop no longer needs them) into a shared, public block in
  omnigent/native_coding_agents.py — CLAUDE_NATIVE_AGENT_NAME … KIMI_NATIVE_AGENT_NAME,
  each = <registry row>.agent_name. Seeding and tests reference a single named
  source of truth instead of magic-string literals. (See the follow-up commit note below.)

Redeploy safety (the core invariant)

builtin_agent_id(name) is sha256("builtin:<name>")[:32] — a pure hash of the agent name.
Persisted conversation.agent_id rows point at these ids, so both the seeded ids and the
bundle bytes must stay byte-identical or a redeploy orphans live sessions. Verified
empirically: _build_native_bundle(provider) produces byte-identical tarballs (sha256 match)
to today's named builders for claude/codex/pi/kiro/opencode/qwen — across all three model-arg
variants — and the seeded ids are frozen in a new test.

ELI5

Eleven copy-pasted "build this agent's starter bundle and register it" pairs in the server
startup became one loop over the harness registry. The agent ids are hashes of their names, and
the names didn't change, so every existing session still points at the right agent after a
redeploy — pinned by a test that freezes the expected ids.

Test Plan

uv run pytest tests/server/test_builtin_bundles.py tests/server/test_app.py \
  tests/test_harness_plugins.py -q
uv run pytest tests/server/ -q
uv run pre-commit run --all-files

• test_builtin_bundles.py: the native _BUILDERS cases now drive the generic
  _build_native_bundle(provider) (claude/codex/pi/kiro — a spread of the model-arg variants),
  keeping the valid-tarball + reproducibility assertions; debby/polly keep their named-builder
  cases.
• New pins: test_native_seed_ids_are_byte_stable freezes the 11 expected 32-char ids;
  test_native_seed_loop_covers_every_native_agent asserts the loop resolves a provider for
  every NATIVE_CODING_AGENTS entry (a dropped harness raises instead of silently unseeding).

Demo

N/A — internal server-startup refactor, no user-facing or UI change.

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

Behavior-preserving refactor. Beyond the updated/added unit tests, I verified byte-stability
directly: captured sha256 of every named builder's output on main, then confirmed
_build_native_bundle(provider) reproduces each hash exactly on this branch (claude, codex,
pi, kiro, opencode, qwen — covering bare-tmpdir, required-model, and defaulted-model
signatures). The two stale references to deleted function names in e2e comments/messages
(test_host_codex_native_e2e.py, e2e_ui/conftest.py) were updated; both were text-only, not
calls.

Follow-up (ef98b092) — shared agent-name constants. An earlier revision of this PR deleted
the 11 _<X>_NATIVE_AGENT_NAME constants outright (the seeding loop uses agent.agent_name),
which pushed callers that need one specific built-in onto magic-string literals
("claude-native-ui", "qwen-native-ui", …) in the tests. That was an overcorrection; they're
now restored as public constants in omnigent/native_coding_agents.py (the module that
already indexes the registry rows), and every caller — test_app.py,
test_scheduled_tasks_routes.py, and the two host e2e tests (which previously each redefined
their own local literal) — imports the shared symbol. Net: one registry-derived source of truth,
no magic strings.

---

Workstream progress (designs/harness-modular-registry-proposal.md, ~14 PRs):
Phase 1 — 1.1 (#3239), 1.2 (#3244), 1.3 (#3314), 1.5a (#3495), 1.5b-i (#3500), 1.5b-ii
(#3501), 1.5c (#3543) landed; 1.6 (#3568) in review; 1.7 is this PR. Remaining: 1.8
(enumerations); Phase 2 — 2.1–2.4. 1.7 is a disjoint hub — independent of the 1.6 review.
(Also flips the stale 1.5c ledger row to landed.)

[Copilot]

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

[PattaraS]

/review

[omnigent-ci]

Polly AI Review

Summary

Clean, behavior-preserving refactor. It collapses 11 near-identical _ensure_default_<x>_agent / _build_<x>_native_bundle pairs into a single registry-driven loop (_ensure_default_native_agents + _build_native_bundle), while correctly leaving the shipped-example (debby/polly) and env-driven paths hand-written. The redeploy-safety invariant is real and well-guarded. No blocking issues found. No visual demonstration needed (internal server-startup refactor, no user-visible surface).

I independently verified the central claim: for all 11 native agents (claude, codex, pi, opencode, cursor, kiro, goose, hermes, antigravity, qwen, kimi) the registry-driven _build_native_bundle(provider) produces a byte-identical (sha256-matching) tarball to each corresponding named builder — across all three model-arg variants (bare tmpdir, required keyword-only model, defaulted model). I also confirmed the 11 frozen ids in _EXPECTED_BUILTIN_AGENT_IDS match builtin_agent_id(name) exactly, and that resolve_hook + the inspect.signature bridge dispatch each _materialize_<key>_agent_spec correctly.

Blocking issues

None.

Security vulnerabilities

None. No new attack surface — this is startup seeding that runs the same materialize/bundle/tar logic as before. No lockfile or dependency-extra changes in this diff.

Non-blocking notes

• inspect.signature bridge is name-based, not contract-based. _build_native_bundle passes model=None iff a parameter literally named model exists. This is correct for all current harnesses (verified), but it silently assumes every future _materialize_* either takes only tmpdir or an optional/keyword model. If a future harness adds a different required kwarg, _build_native_bundle will raise at seed time rather than route it. That's an acceptable fail-loud contract, but worth a one-line comment so the next author knows the bridge only understands the model axis. test_native_seed_loop_covers_every_native_agent guards provider presence but not signature compatibility.

• Test scope broadened silently. test_ensure_default_antigravity_agent_seeds_card and test_ensure_default_native_agents_seeds_qwen_card now call _ensure_default_native_agents, which seeds all 11 agents rather than the one under test. Functionally fine (idempotent, and the assertions still target the specific agent), just slightly heavier and less targeted than the names imply.

• Error message coverage. The two raise paths (no materialize_agent_spec hook in _build_native_bundle, and no provider row to seed from in the loop) are not exercised by a test. test_native_seed_loop_covers_every_native_agent asserts the happy path (no missing providers) but nothing pins the raise-on-missing behavior the docstring advertises as the contract. Low value to add, but the contract is currently untested.

Overall: solid refactor with strong empirical backing and good regression pins (the byte-stable id table is exactly the right guard for the redeploy invariant). Ready to merge from a correctness standpoint.

---

_{Automated review by Polly · workflow run}

[Copilot]

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

[Copilot]

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

[PattaraS]

Thanks Polly — no blocking issues, and the byte-identical verification across all 11 agents matches what I checked locally. On the three non-blocking notes:

1. inspect.signature bridge is name-based — good call; added a comment at the call site (315d895) spelling out that the bridge understands only the model axis and that a future materializer with a different required kwarg fails loud at seed time (missing-arg TypeError) rather than routing silently. Kept it fail-loud rather than contract-based since there's exactly one axis today.
2. Test scope broadened — intentional: _ensure_default_native_agents seeds all 11, and the assertions still target the specific agent, so it's a superset that stays correct (and idempotent). I also added test_ensure_default_native_agents_seeds_every_native_agent to cover the whole set explicitly.
3. Raise-path coverage — addressed in a follow-up you didn't see (this review ran on 6935fce, before bcd32792): test_ensure_default_native_agents_raises_when_provider_missing and test_build_native_bundle_raises_without_materialize_hook now pin both raise contracts. That follow-up also parametrized the bundle-builder tests over all 11 agents (was a 4-agent sample) to recover the coverage the deleted per-harness helpers used to contribute transitively.

[PattaraS]

On the Coverage status (82% < baseline 83%)

I dug into this with the coverage artifacts rather than guessing — sharing the analysis since the drop looks alarming but is expected arithmetic, not an under-tested change.

Exact numbers (from this run's coverage.xml):

• Overall: 90484/109698 = 82.48%; gate is 82.48 + 0.5 (tolerance) ≥ 83 → fails by 0.02%.
• omnigent/server/app.py: 93.5% covered (621/664 stmts).
• Uncovered lines inside the new seeding functions (_build_native_bundle, _ensure_default_native_agents, lines 545–615): zero. All 43 uncovered lines in the file are pre-existing code I didn't touch (debby/polly builders, request handlers). Both raise-paths are now pinned by tests.

Why the total still dipped ~0.5%: this PR deletes ~314 lines of per-harness seeding boilerplate that were heavily covered (transitively, via the e2e/server-integration suites) and replaces them with ~55 fully-covered lines. Removing above-average-covered code from an 83% base mechanically lowers the ratio — the numerator drops by ~314 while the denominator drops by ~330. It's a "lost coverage by deleting well-tested code" effect, not new untested code.

What I already added (commit bcd32792): parametrized the bundle-builder tests over all 11 native agents (was a 4-agent sample) so every _materialize_* path is covered directly rather than only via e2e, plus the two raise-path tests. That recovered what it honestly can; the remaining gap is the deleted boilerplate itself.

I'd rather not pad unrelated *_native.py modules just to move the integer back — that's gaming the metric. Coverage isn't a required check here (only Merge Ready + Maintainer Approval gate), so flagging for a maintainer call: merge as-is (the change itself is fully covered), or let me know if you'd like additional real tests somewhere specific.

[Copilot]

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

[PattaraS]

/review

[omnigent-ci]

Polly AI Review

Summary of review

This is a behavior-preserving backend refactor that collapses 11 hand-written _build_<x>_native_bundle / _ensure_default_<x>_agent pairs into a registry-driven _build_native_bundle(provider) + _ensure_default_native_agents(...) loop. I verified the redeploy-safety invariant, the model-arg bridge, and dangling-reference risk against the checked-out source. The change is correct and well-guarded. No visual demonstration is required (internal server-startup refactor, no user-visible surface).

Blocking issues

None.

I specifically checked the highest-risk failure modes and confirmed each is sound:

• Redeploy safety holds. _ensure_builtin_agent keys the row by name and content-addresses the bundle as {agent_id}/{sha256}, with agent_id = builtin_agent_id(name) a pure hash of the name. The loop seeds each agent under agent.agent_name (unchanged), so ids and bundle bytes stay stable. test_native_seed_ids_are_byte_stable freezes all 11 ids as a tripwire — good.
• Model-arg bridge is correct. I confirmed the actual signatures: only _materialize_{codex,kiro,opencode}_agent_spec declare model (all keyword-only with a None default); the other eight take just tmpdir. The "model" in inspect.signature(...).parameters check passes model=None exactly where the old named builders did, and tmpdir is passed positionally to all — so produced bytes match the pre-loop builders.
• No dangling references. The removed server_app._<X>_NATIVE_AGENT_NAME constants and deleted builder/ensure functions are no longer referenced by non-test code. The untouched e2e tests (test_host_claude_native_e2e.py, test_host_opencode_native_e2e.py, test_host_codex_native_e2e.py) each define their own local "*-native-ui" constant rather than importing from server_app, so removing the module constants does not break them. test_scheduled_tasks_routes.py was correctly updated to inline the literal name.
• Error path is valid. OmnigentError(message: str) accepts the plain-string messages used in both raise sites, which are covered by test_ensure_default_native_agents_raises_when_provider_missing and test_build_native_bundle_raises_without_materialize_hook.

Security vulnerabilities

None. No new inputs, deserialization, network, or auth surface — this only reorganizes trusted startup seeding of first-party agent specs. No lockfile or dependency-extra changes in the diff.

Non-blocking notes

• Bridge is intentionally single-axis. The model-only bridge means a future materializer requiring a different required kwarg fails loudly at seed time (server startup) with a TypeError. This is documented in the inline comment and is the right trade-off, but worth noting it surfaces as a boot failure rather than a graceful skip — acceptable given _ensure_default_native_agents deliberately favors loud failure over silent unseeding.
• Test coverage is a genuine improvement. _NATIVE_BUILDERS now exercises every native agent (not the prior 3-item sample), and the new test_native_seed_loop_covers_every_native_agent pins the provider-resolution contract. Net effect is broader coverage with less code.

Overall: a well-scoped, well-tested refactor that preserves the byte-stability invariant it targets. Ready to merge from a correctness standpoint.

---

_{Automated review by Polly · workflow run}

[Copilot]

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

[PattaraS]

Follow-up pushed (ef98b092) addressing the built-in agent-name constants.

An earlier revision of this PR deleted the 11 _<X>_NATIVE_AGENT_NAME constants from server/app.py because the collapsed seeding loop iterates NATIVE_CODING_AGENTS and reads agent.agent_name directly. But that pushed callers needing one specific built-in onto magic-string literals ("claude-native-ui", "qwen-native-ui", "antigravity-native-ui") in the tests — an overcorrection.

Now restored as a shared, public block in omnigent/native_coding_agents.py (the module that already indexes the registry rows), each = <row>.agent_name:

• CLAUDE_NATIVE_AGENT_NAME … KIMI_NATIVE_AGENT_NAME — one registry-derived source of truth.
• test_app.py and test_scheduled_tasks_routes.py import them instead of literals.
• The two host e2e tests (test_host_claude_native_e2e.py, test_host_codex_native_e2e.py) previously each defined their own local _CLAUDE/_CODEX_NATIVE_AGENT_NAME = "…" literal — those are folded onto the shared constants too.

The seeding loop stays registry-driven; the constants exist for callers that need a specific built-in by name. 112 server tests pass, e2e files import the new symbol, ruff/pre-commit clean.

[PattaraS]

/review

[omnigent-ci]

Polly AI Review

Blocking issues

None. I verified the core claims against the checked-out main source:

• Byte-identity of bundles holds. The inspect.signature bridge passes model=None iff the materializer declares a model parameter. I confirmed the three signature shapes match the old builders exactly: codex (model: str | None, required kw-only) and kiro/opencode (model: str | None = None) all previously received model=None; the remaining eight (claude, pi, cursor, goose, hermes, antigravity, qwen, kimi) take only tmpdir and are now called bare. Same arguments in, so same bytes out — the reproducibility test backs this per-key.
• Redeploy safety holds. builtin_agent_id is a pure hash of the agent name, NativeCodingAgent.agent_name is unchanged, and the 11 frozen ids are pinned by test_native_seed_ids_are_byte_stable. NATIVE_CODING_AGENTS has all 11 registry rows.
• Failure paths raise loudly, not silently. A missing provider row (_ensure_default_native_agents) and a missing materialize_agent_spec hook (_build_native_bundle) both raise OmnigentError, each covered by a new test. This preserves the "a dropped harness fails instead of silently unseeding" contract.
• No circular import introduced. harness_plugins does not import native_coding_agents, so the new constant block in native_coding_agents.py (importing the 11 *_NATIVE_CODING_AGENT rows) is safe.
• No stale hard references remain to the removed _build_<x>_native_bundle / _ensure_default_<x>_agent functions or the server_app._<X>_NATIVE_AGENT_NAME module constants across the tree (the grep hits are all in main's pre-PR source, which the PR itself rewrites).

Security vulnerabilities

None. No new inputs, deserialization, network, or auth surface — the loop only rearranges startup seeding of already-trusted, in-tree bundle materializers. No lockfile or extras changes.

Non-blocking notes

• tests/e2e/test_host_opencode_native_e2e.py is not in this PR's file set yet still contains the assertion message "_ensure_default_opencode_agent did not run." (line 216) referring to a function this PR deletes. It's a harmless message string (the test keeps its own local _OPENCODE_NATIVE_AGENT_NAME literal, so it won't break), but the sibling claude/codex e2e tests were updated to the new name/import while opencode was missed. Worth folding into this PR's cleanup for consistency.
• Minor asymmetry: the codex and claude e2e tests were migrated to import the new public *_NATIVE_AGENT_NAME constants, but the opencode e2e test still hardcodes the "opencode-native-ui" literal. Not a correctness problem, just an incomplete sweep of the same pattern.

Summary

A tidy, high-value refactor: 11 copy-pasted builder/seeder pairs collapse into one registry-driven loop with a narrow, well-documented signature bridge, net −455/+146. The two invariants that matter — byte-identical bundles and byte-stable name-derived agent ids (so a redeploy never orphans persisted conversation.agent_id rows) — are both preserved and now pinned by explicit tests, and the failure paths raise rather than silently unseed. No blocking or security issues found; the only follow-up is the incomplete migration of the opencode e2e test (a stale message string and a hardcoded literal). Ready to merge once that nit is addressed. No visual demonstration is needed — this is an internal server-startup refactor with no user-visible change.

---

_{Automated review by Polly · workflow run}

[PattaraS]

Thanks Polly — approve confirmed, and good catch on the incomplete sweep.

Fixed in acdb86a0: test_host_opencode_native_e2e.py now imports the shared OPENCODE_NATIVE_AGENT_NAME constant (dropping its local "opencode-native-ui" literal) and its assertion message points at _ensure_default_native_agents instead of the deleted _ensure_default_opencode_agent. That was exactly the asymmetry you flagged — claude/codex were migrated, opencode was missed. All three host e2e tests are now on the shared constant.

The behavior-preservation verification (byte-identical bundles across the three signature shapes, byte-stable name-derived ids, raise-on-missing) matches what I checked locally — appreciate the independent confirmation.

[github-actions]

🏷️ Doc impact: no-doc-update

This is an internal refactor collapsing 11 hand-written native-agent seed helpers into one registry-driven loop (plus symbol/test renames); the set of built-in native agents and their behavior is unchanged, so no user-facing surface changed.

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

[PattaraS]

Correction: my note above said the opencode e2e cleanup landed in acdb86a0 — that commit was pushed after this PR had already squash-merged, so it never reached main. The opencode sweep is instead in a small standalone follow-up: #3656.