refactor(seams): add pluggable extension points across backend, CLI, and UI

[ren-jentic]

Summary

Introduces backward-compatible seams so downstream consumers can inject alternate implementations and mount extra components without editing core code. When no consumer wiring is supplied, behavior is identical to today.

• Backend
  • Neutral Broker protocol + transport-neutral value objects in shared/broker (so the broker surface and any consumer depend on the protocol, not the concrete pipeline).
  • DI AppContainer for the app factories (inject a Broker, mount extra routers/installers after the built-in surfaces). The async worker honors an injected broker_factory too, so injected brokers reach both the sync and async paths.
  • Registrable config sub-models (register_config, with collision guards) and telemetry events (register_telemetry_event) without editing the core schema/enum.
  • Ordered MigrationTarget registry (register_target) as the single source of truth for migration sequencing, with reversible up/down.
• Testing: ships jentic_one.testing compliance bases (isinstance + inspect.signature checks) so implementations can prove they honor the seam contracts.
• CLI: exportable pkg/core container + root builder; internal/cmd -> pkg/core stays one-directional so alternate binaries can compose their own command tree.
• UI: renames the src alias @/ -> @oss-internal/ across the SPA and exposes the route/handler registries as append-only composition points.
• Arch tests: sources ORM-convention facts from jentic-one-rules with a committed vendored fallback, so a standalone clone still self-enforces.
• Docs: adds docs/development/extending-jentic-one.md — a unified composition guide for downstream integrators (build an AppContainer; register_config / register_target / register_telemetry_event; subclass the jentic_one.testing compliance bases).

⚠️ Breaking Change: unknown top-level config keys now fail loudly

AppConfig now sets model_config = ConfigDict(extra="forbid").

Behavior change: an unrecognized top-level config key — one that is neither a known core field nor a registered extension section — now causes a loud failure at startup (a ConfigError) instead of being silently ignored.

This is defensively correct: it ensures extensions are formally registered via register_config (registered sections are extracted by load_config() before validation, so they are accepted), and it surfaces misconfiguration instead of dropping it. Downstream users must be warned: any config with legacy or typo'd top-level keys that was previously tolerated will now fail on boot until the offending key is removed or migrated to a registered extension section.

Test plan

• Backend unit tests for the new extension seams:
  • tests/unit/test_config_extensions.py — register_config, registered_config_models, AppConfig.extension(), collision guards (core fields + reserved extensions), and load_config extraction / extra="forbid" rejection of unregistered keys.
  • tests/unit/shared/telemetry/test_event_registry.py — register_telemetry_event + resolve_wire_name (wire-name validation, built-in enum/map collision rejection, precedence, idempotency/conflict).
  • tests/unit/broker/test_injected_broker_seam.py — an injected Broker is honored by both the sync path (execute.py selection) and the async path (PipelineExecutor via run_execution), plus AppContainer wiring onto app.state.broker.
• make test-fast — unit + arch.
• ruff check + ruff format --check clean on touched files.
• CI: lint + typecheck + score + UI lint/tests.
• Note: test_origin_propagation[admin/services/auth_service.py] fails on this branch and on main — pre-existing, unrelated (that file is untouched here).

Merge

Squash + merge.

Made with Cursor

[jentic-harness]

[Orchestrator] Session started — ai-review

[jentic-harness]

[ReviewAgent] Reviewed this seams refactor end-to-end (backend broker/config/telemetry/migrations, CLI container, UI alias rename, arch-facts vendoring). The design is clean, backward-compat is well-reasoned, and the docstrings are excellent. The UI portion is a mechanical @/→@oss-internal/ rename that checks out. A few substantive items before merge:

1. (Correctness / PR-description mismatch) An injected Broker via AppContainer reaches only the sync path, not the async worker.
The PR states: "The async worker honors an injected broker_factory too, so injected brokers reach both the sync and async paths." But AppContainer exposes only broker: Broker | None (an instance), which the factories stash on app.state.broker. The sync router reads app.state.broker ✅. The async worker, however, reads app.state.broker_factory (broker/web/app.py:164) — and nothing bridges container.broker → app.state.broker_factory. AppContainer has no broker_factory field, and no code in shared/web/ ever sets that attribute. So a downstream consumer using the documented container seam gets sync-only injection; the worker silently falls back to default_broker. This is also semantically unavoidable as written: the worker needs a Callable[[UpstreamRunner], Broker] (per-runner), which a single injected instance can't satisfy. Either add a broker_factory field to AppContainer and wire it onto app.state, or correct the PR description / docstrings to state the container's broker is sync-path-only.

2. (Test coverage) The new extension points are largely untested.
The entire value of this PR is the seams, yet the only new seam with direct tests is MigrationTarget/register_target (+ the compliance harness self-test). There are no tests for: AppContainer.extra_routers/extra_installers/broker injection through create_surface_app/create_combined_app; register_config/registered_config_models/AppConfig.extension() (incl. the collision guards); register_telemetry_event/resolve_wire_name (incl. wire-name validation + collision rejection). Given item #1 slipped through precisely because there's no injection test, please add coverage that mounts an app with a populated container and asserts an injected broker is actually invoked on both paths.

3. (Behavior change, not called out) AppConfig now sets extra=\"forbid\".
main had no such constraint. Any existing deployment carrying an unrecognized top-level config key (that isn't a registered extension) will now fail at load_config() instead of silently ignoring it. This is defensible (catches typos/misconfig loudly) and extensions are popped before validation, so the mechanism is sound — but it's a breaking change to config parsing that isn't flagged in the PR body or a migration note. Please confirm no shipped example/deploy configs carry stray keys and note the change.

4. (Non-blocking, worth a comment) Injected sync broker bypasses per-request runner selection.
In execute.py:573-574, when app.state.broker is set, the per-request runner (chosen via RunnerRegistry for the scheme, wrapped with circuit-breaker/bulkhead/deadline) is discarded in favor of the injected instance. The # it owns its own transport comment covers intent, but it means an injected broker opts out of the built-in resilience stack — a non-obvious tradeoff worth stating explicitly in the AppContainer.broker docstring.

5. (Docs) No consumer-facing composition guide.
Docstrings are thorough, but there's no docs/ entry tying the seams together for a downstream integrator (build an AppContainer, register_config, register_target, register_telemetry_event, subclass the jentic_one.testing compliance bases). Per CLAUDE.md's documentation expectation for non-obvious patterns, a short "extending jentic-one" doc would pay off — especially given the CLI/UI/backend seams are meant to be used together.

Nothing here is a security regression — the migration FK-rollback ordering, per-schema version tables, and the SSRF/egress path are untouched or improved. Items #1–#3 are the ones I'd want resolved before squash-merge.

[ren-jentic]

This looks solid and executes perfectly on the 'Dependency Overlay with Strict Seams' directive from the architecture spec (jentic-one-internal#704).

Key takeaways:

• AppContainer in the Go CLI (cli/pkg/core/container.go) successfully models the bi-directional boundary without pulling internal/ outward.
• Moving to DB_TARGETS with register_target instead of a static dict in Alembic resolves the Postgres schema collision danger natively.
• The jentic_one.testing.compliance harnesses are an elegant fix for Python's structural typing gaps (isinstance not validating method signatures).
• The neutral Broker interface decouples the execution layer properly.

A thorough follow-through on the open-core strategy.

[jentic-harness]

[Orchestrator] Session started — ai-review

[jentic-harness]

[ReviewAgent] Reviewed the seam/extension-point refactor across backend, CLI, UI, and docs. The design is coherent and the bulk of the diff (UI `@/` → `@oss-internal/` alias rename, ~300 files) is mechanical and safe. The config/telemetry/migration registries are clean, well-guarded (collision + idempotency checks), and integrated through single chokepoints (`resolve_wire_name`, `DB_TARGETS`). No secrets introduced; `init-schemas.sql` adds only commented example grants, and `rules-clone.sh` fails soft. Below are asks before merge.

1. (Correctness + docs mismatch — blocking) The injected `AppContainer.broker` never reaches the async worker path, contradicting the PR's headline claim.

The PR description, `docs/development/extending-jentic-one.md`, and the test docstring all state an injected `Broker` is honored by both the sync router and the async worker ("one pipeline, two callers"). But the wiring only closes the sync half:

• `AppContainer` exposes a `broker` instance → the factories set `app.state.broker` (`app_factory.py:383-384`, `432-433`).
• Sync path (`broker/web/routers/execute.py:572-574`) reads `app.state.broker` → ✓ honored.
• Async path (`broker/web/app.py:164-167`) reads `app.state.broker_factory` (a callable) — but nothing anywhere in `src/` ever sets `app.state.broker_factory` (grep confirms only readers, no writers). `AppContainer` has no `broker_factory` field, and neither app factory derives one from `container.broker`.

Net effect: an integrator who follows the docs and injects `AppContainer(broker=MyBroker())` gets it on the sync path, while the async worker silently falls back to `default_broker` (the built-in `DefaultBroker`). That is exactly the "async path that drifted from the sync one" the executor docstring warns against.

`test_async_path_invokes_injected_broker` passes only because it constructs `PipelineExecutor(registry, broker_factory=...)` directly, bypassing `AppContainer` → `app.state` → the broker lifespan. So it proves the plumbing below the gap works but never exercises the container→worker bridge that the docs promise.

Please either (a) have the app factories set `app.state.broker_factory = lambda _runner: container.broker` when `container.broker is not None` (an injected broker owns its transport, so ignoring the runner is correct), or (b) add a `broker_factory` field to `AppContainer` that the factories propagate — and add a test that drives the async path through an `AppContainer`, not around it. Also reconcile the docs claim with whatever is chosen.

2. (Minor inconsistency) `create_surface_app` silently drops the `extra_routers` prefix.

`RouterSpec` is documented as `(router, prefix, tags)`. `create_combined_app` honors it (`root.include_router(router, prefix=prefix, ...)`, `app_factory.py:468-470`), but `create_surface_app` binds it to `_extra_prefix` and drops it (`app_factory.py:385`), mounting the router at root. An integrator passing a prefix to a surface app loses it with no error. Please align the two (apply the prefix in both, or document why surface apps ignore it).

3. (Escalation — `needs-human`) Per `REVIEWS.md` this PR trips two explicit human-review triggers: it changes module boundaries (`shared/`, `broker/`) and modifies `.harness/setup.sh` (agent setup). Adding `needs-human` and deferring final approval to a maintainer. The `.harness` change itself (fetch internal rules, fail-soft, export `JENTIC_RULES_DIR`) looks correct.

Non-blocking observations (no action required): `search_mode` moving `Literal["lexical"]` → `str` defers validation from config-load to first-query in `resolve_strategy` — a documented, acceptable tradeoff. The `extra="forbid"` breaking change is clearly called out in both the PR body and the docs. Widening `TelemetryEvent.name` to `... | str` is consistent with `sink.record` and the `str(event.name)` serialization site.

[jentic-harness]

[Orchestrator] Session started — ai-review

[jentic-harness]

[ReviewAgent] Reviewed the seam refactor end-to-end (backend DI/config/telemetry/migrations, CLI, UI alias rename, arch-rules vendoring). This is a well-executed, backward-compatible change: the arch boundary holds (shared/ never imports broker/ — verified), value objects were centralized in shared/broker/execution.py with re-exports so existing import sites are untouched, search_mode loosening is safe because resolve_strategy still validates per-dialect at resolve time, SSRF validation stays at the web edge before the broker call (injected brokers can't bypass it), and the 67 new/affected unit+arch tests pass with ruff clean on touched files. The extra="forbid" breaking change is documented in both the PR body and docs/development/extending-jentic-one.md. Nice work on the vendored-facts OSS-safety guard.

One substantive point plus minor notes:

1. The injected-Broker seam can't reach the surface that actually runs a broker. broker/web/app.py::create_app(ctx) takes no container, and __main__._build_app calls mod.create_app(ctx). Because the broker must run as the sole surface (_build_app raises on broker + others), the broker is never part of create_combined_app — which means:

   • the root.state.broker / broker_factory wiring added to create_combined_app is inert (no broker router is mounted there, and its combined worker starts with _start_worker(ctx) and no executor), and
   • the shipped standalone broker binary has no path to inject a Broker via AppContainer without a downstream fully re-implementing broker/web/app.py's assembly (broker_lifespan, admission gate, auth, error handlers, registry resolver).

   The new tests prove create_surface_app/create_combined_app honor a container, and that PipelineExecutor(broker_factory=...) works — but not that a downstream can actually inject a broker into the real broker deployment through the documented AppContainer path. Please either thread container through broker/web/app.py::create_app (and __main__) so the seam is reachable for the sole surface that uses it, or, if the intended model is "downstream calls create_surface_app with the broker's own routers/lifespan directly," document that explicitly in extending-jentic-one.md and drop the inert combined-app broker wiring to avoid the misleading impression it takes effect.

2. Minor — TelemetryEvent.name and TelemetrySink.record were widened to TelemetryEventName | str. Confirmed the only consumer (telemetry/client.py) uses str(event.name), so this is safe; flagging only so future .value-style access on name is avoided.

3. Minor — AppContainer.extra_routers mount in create_surface_app ignores the per-router prefix (uses _prefix throw-away), while create_combined_app applies it. That asymmetry is defensible (surfaces mount at root) but worth a one-line comment so an integrator isn't surprised a prefix is silently dropped in surface mode.

None of these block correctness — default behavior is unchanged and verified. #1 is the one worth resolving before merge since it's the gap between the headline capability and what's wired.

[jentic-harness]

[Orchestrator] Session started — ai-review

[jentic-harness]

[ReviewAgent] Reviewed the seams work — a well-structured, backward-compatible, and unusually well-documented PR. The 394-file count is dominated by the mechanical UI @/ → @oss-internal/ rename, which I verified is complete and consistent (tsconfig/vite/vitest/eslint aliases all updated; no real @/ import statements remain, only comment prose). The substantive backend/CLI seams are clean: full arch suite passes (256) including module-boundary enforcement, and the new unit tests pass (67). A few asks before merge:

1. Streaming silently bypasses the injected Broker — behavior/doc mismatch (most important). The Broker protocol exposes only execute(), which returns a fully-materialized ExecutionOutcome. But the sync streaming path (broker/web/routers/execute.py::_handle_streaming) calls open_streaming_response(runner, ...) directly against the runner — it never reads app.state.broker/broker_factory. So an injected Broker reaches the buffered-sync and async paths, but not streaming requests. The docs (docs/development/extending-jentic-one.md) and the Broker docstring state the injected broker owns transport/resilience and is honored by "both callers … the sync and async paths, not just one of them" — which reads as full data-plane coverage. A downstream injecting a broker for transport routing, egress policy, or response redaction would have streaming traffic escape it with no warning. This is a security-relevant gap in the seam's stated contract. Please either (a) explicitly document that streaming is not routed through the injected Broker (and why — execute() can't model a lazy body), or (b) extend the seam to cover it. At minimum the docs should not imply total coverage.

2. The sync-path test doesn't actually exercise _handle. test_sync_path_selection_prefers_injected_broker re-implements the injected if injected is not None else broker_factory(runner) selection inline (the test even says "Mirrors broker/web/routers/execute.py"). Because it's a copy of the production expression rather than a call into _handle, it won't catch drift if the real selection logic changes. The async-path test is genuinely end-to-end through run_execution — nice. Consider making the sync assertion drive the real handler (or a thin extracted selector function) so it guards against regressions rather than restating them.

3. search_mode validation moved from load-time to resolve-time — confirm this is intended. Widening Literal["lexical"] → str means an invalid/typo'd search_mode (e.g. "lexcial") now passes config validation and only fails later at resolve_strategy (via SearchUnsupportedError) on first search, rather than loudly at boot. This is slightly at odds with the PR's broader "fail loudly at startup" theme (extra="forbid"). It still fails loudly eventually and the message is good, so this is a judgment call — just flagging the deferred-failure tradeoff in case boot-time rejection was intended.

Non-blocking: the extra="forbid" breaking change is clearly called out in the PR body and docs — good. Nice touch on the vendored-facts OSS-safety guard and the FK-safe reversed downgrade ordering.

[Manuel-Jentic]

Alternative worth considering: keep the public repo on plain @, push the rename into enterprise

@ren-jentic — the @/ → @oss-internal/ rename is mechanically correct and I verified it works. But I want to float an alternative, because renaming the public SPA's own alias to @oss-internal bakes the private-tier vocabulary into every import line of the public repo (335 files reading @oss-internal/ — a name that only makes sense if you know about the private enterprise repo, which OSS users don't have).

I tested this empirically. The key fact is that Vite/Rollup has one flat, global alias table for the combined build, so exactly one of the two repos must give up @ — but it doesn't have to be OSS.

Test results (reproducible script below)

1) OSS '@' + host also claims '@' (naive keep-@)           BUILD-FAILS (collision)
2) OSS '@oss-internal' + host keeps '@' (CURRENT PR)       PASS
3) OSS neutral '@app' + host keeps '@'                     PASS
4) OSS '@' + enterprise uses '@ent' for self, '@'→OSS      PASS   ← "keep public as @"

The "keep public as @" option (variant 4), stress-tested

OSS stays exactly as it is today (@ → src, zero churn). The enterprise build is the one that disambiguates:

// enterprise vite.config.ts (private repo)
resolve: { alias: {
  '@oss': resolve('../jentic-one/ui/src'),  // consumer-facing OSS mount
  '@ent': resolve(__dirname, 'src'),        // enterprise's own code
  '@':    resolve('../jentic-one/ui/src'),  // OSS's own '@/' imports resolve to OSS
}}

I hardened this against the cases that actually bite in a real SPA, and all pass:

• Deep transitive OSS chains (foo → deep/a/b/leaf → shared/marker, all @/) resolve to OSS.
• Shared singleton is NOT duplicated across the boundary: enterprise importing an OSS store both via a re-export and directly via @oss/shared/store gets the same module instance (sameSingleton=true, mutable count accumulates across all call sites — no double-instantiation).
• Standalone public OSS build (plain @ → src, unchanged) still resolves the deep chain — the public repo needs no change at all.

The trade-off is symmetric — someone must cede @:

• Current PR: OSS cedes @ (renames to @oss-internal) so enterprise keeps @. Cost: churn + private-tier naming in the public tree.
• Variant 4: enterprise cedes @ (uses @ent/ for itself, maps @ → OSS) so OSS stays pristine @. Cost: the enterprise repo has an @ → OSS mapping (mildly unusual, but it lives in the private repo where the split is understood).

My argument for variant 4: a cost that exists only because of the private split should be paid by the private repo, not stamped across every public import line. If you'd prefer OSS to own a distinct prefix regardless, I'd at least suggest a neutral name (@app/ / @ui/) over @oss-internal — variant 3 proves it's behaviorally identical without leaking the tier vocabulary.

Not blocking — the current approach works. Just think variant 4 (or a neutral prefix) keeps the OSS repo cleaner for the thousands who only ever see the public side. What do you think?

Reproducible test (bash, needs node + npx vite)

# Builds a tiny OSS package + enterprise consumer and executes the bundle,
# asserting OSS internals resolve to OSS (no cross-binding) across alias schemes.
# Full script: https://github.com/jentic (attach on request) — core loop:
#   OSS: shared/marker.ts, modules/foo/index.ts (imports via a switchable prefix),
#        deep/a/b/leaf.ts (transitive), shared/store.ts (mutable singleton)
#   ENT: main.ts imports @oss/modules/foo + @oss/shared/store + @ent/... ,
#        builds with the alias block under test, then node executes dist/out.js.
# PASS = output shows OSS_MARKER, deep leaf resolved to OSS, sameSingleton=true.

[jentic-harness]

[Orchestrator] @ren-jentic — Review-thrash limit reached after 4 non-approval rounds. The agent and reviewer can't converge — needs a human decision on what to do next. Post /harness-reset-thrash to clear the gate, or take over manually.

[ren-jentic]

/harness-reset-thrash

[jentic-harness]

[Orchestrator] Thrash counter cleared (scope: just the review-thrash gate on this PR — no agent restart, no review undone). Re-add ai-review or ai-review-and-trigger-implement to trigger a new review pass.

[jentic-harness]

[Orchestrator] Session started — ai-review

[jentic-harness]

[Orchestrator] Session started — ai-review

[jentic-harness]

[Orchestrator] @ren-jentic — Run failed: .harness/setup.sh failed with exit 127; see /var/lib/harness/runs/threads/jentic-jentic-one/pr-562/runs/d91e7afd-bc6e-4f68-95f7-9ece397327b0/logs/setup.log

[jentic-harness]

[Orchestrator] Session started — ai-review

[jentic-harness]

[Orchestrator] @ren-jentic — Run failed: .harness/setup.sh failed with exit 2; see /var/lib/harness/runs/threads/jentic-jentic-one/pr-562/runs/daffb69b-3485-4cdf-a7ab-dbd203f412d8/logs/setup.log

[jentic-harness]

[Orchestrator] Session started — ai-review

[jentic-harness]

[ReviewAgent] Approved — this is a well-executed, genuinely backward-compatible refactor. The seam design is clean and the "no consumer wiring → identical behavior" claim holds up under inspection. Verified locally:

Correctness

• 257 arch tests + 435 broker unit tests + 69 new seam/config/telemetry/migration/compliance tests all pass; mypy clean on the new modules.
• No dangling references after the renames: default_pipeline→default_broker, pipeline=→broker=, and open_streaming_response now routes through DefaultBroker.execute_streaming. All prod + test call sites updated.
• Value-object extraction to shared/broker with re-exports from the old modules keeps every existing import working; module-boundary arch test still green (shared/broker depending on fastapi is arch-neutral by design).
• TelemetryEvent.name widened to TelemetryEventName | str; the one consumer (client.py) uses str(event.name), so plain-string wire names are safe.

Breaking change (extra="forbid") — correctly handled: both shipped configs (config/local.yaml, config/local-sqlite.yaml) load cleanly; registered extension sections are extracted before core validation and round-trip via AppConfig.extension(); unregistered top-level keys are rejected loudly. The change is clearly called out in the PR body and the docs.

Security

• The injected-Broker seam does not bypass the SSRF guard: validate_upstream_url() runs in the router before _resolve_broker() on both the buffered and streaming paths. The only thing an injected broker opts out of is the built-in resilience stack, which is explicitly documented in the Broker protocol docstring and the extending guide.
• register_config / register_telemetry_event / register_target all have collision guards (core-field shadowing, reserved extensions key, wire-name regex, conflicting re-register) with test coverage.
• Rules-facts vendoring: the private clone lands in a gitignored .rules/ path and the loader is read-only, so private rule content can't leak into the public repo; standalone clones self-enforce against the committed vendored subset.

Docs — docs/development/extending-jentic-one.md is a solid, accurate composition guide covering all seams, the wiring order, and the breaking change.

Non-blocking nits (optional):

1. src/jentic_one/shared/jobs/protocols.py:86 — the UpstreamExecutor docstring still reads run_execution(pipeline=default_pipeline(runner)); the kwarg is now broker= (run_execution(broker=default_broker(runner))). Stale reference only.
2. migrations/run.py: with --direction down and no --target, the default -1 is applied per database across the loop, so a bare --db a --db b down-migrates one step in each. That's a reasonable default but worth a one-line note in scripts/migrate.sh help so nobody expects a single global step.

[jentic-harness]

[Orchestrator] @ren-jentic — Agent review concluded approved. Ready for human review.

[Manuel-Jentic]

Small hardening ask: BaseSearchStrategyComplianceTest should assert the strategy actually resolves, not just that dialect is a string

The inspect.signature checks in jentic_one.testing.compliance are a real upgrade over bare runtime_checkable — nice. One gap I hit while reviewing the enterprise overlay against this PR:

test_has_required_attrs only asserts isinstance(instance.dialect, str). That passes for any string, including a wrong one. Concretely: the enterprise strategies declared dialect = "postgresql", but PostgresBackend.dialect_name returns "postgres", so resolve_strategy((dialect_name, mode)) never finds them and raises SearchUnsupportedError at runtime — while the compliance suite stays green. A downstream implementer gets a passing test and a broken search path.

Suggestion: add a compliance check that the registered (dialect, name) is actually resolvable — e.g. assert strategy_cls.dialect is one of the known backend dialect_name values, or (stronger) that resolve_strategy returns the strategy for a backend whose dialect_name == strategy_cls.dialect. That turns the most likely real-world drift (a plausible-but-wrong dialect string) from a silent prod failure into a caught test failure — which is the whole point of the compliance bases.

Not blocking; the signature checks already cover the harder case. This just closes the one class of drift the current attr check waves through.