Runner augmentation: register agents + engines via a3t/augment#45
Runner augmentation: register agents + engines via a3t/augment#45matt-wright86 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces a shared ChangesDataRegistries refactor and Runner augmentation
Sequence Diagram(s)sequenceDiagram
participant Example as examples/lvl3 index.ts
participant Runner
participant Augment as augment.ts
participant Registry as DataRegistry
Example->>Runner: new Runner()
Example->>Runner: registerTaskAgent(agentDefinition)
Runner->>Augment: invoke prototype method
Augment->>Registry: ensure("agentDefinition")
Augment->>Registry: register AgentDefinition
Augment->>Runner: register task agent via createTaskAgent
Compact metadata
🐰 A registry once eager, now ensures on call, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
orchestrator-v2/packages/interfaces-task/src/types.ts (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
keyof DataRegistries & stringdoesn't actually restrict keys — it collapses to plainstring.Because
DataRegistries extends Record<string, unknown>, TypeScript'skeyofoperator folds every explicitly-augmented property name into the index signature's key type. Per the TS handbook/release notes, when a type has a string index signature,keyofproducesstringregardless of the concrete literal members that have been merged in.Consequences:
DataRegistryGuards<T extends keyof DataRegistries & string>accepts any string forT, not just keys actually declared via module augmentation (e.g.,taskAgent,engine). A typo likeensure("egnine", guard)(Line 35-38) type-checks fine.- When the key literal isn't independently pinned by inference at the call site,
Pick<DataRegistries, T>silently resolves the value tounknowninstead of erroring or preserving the guard's asserted type — this also affects thecreateDataRegistry<T extends keyof DataRegistries & string>overload inrunner/src/data-registry.ts(Lines 16-18 there), which relies on this exact constraint to selectPick<DataRegistries, T>.This defeats the compile-time safety the shared
DataRegistriescontract is meant to provide — precisely the guaranteeregisterTaskAgent/registerEnginerely on.📚 TypeScript reference
Per TS 2.9 release notes: "If X contains a string index signature, keyof X is a union of string, number, and the literal types representing symbol-like properties, otherwise" it uses the literal property names.
Worth confirming intent — if this is an accepted trade-off for extensibility, no action needed; otherwise consider a design that preserves literal-key enforcement (e.g., not extending
Record<string, unknown>directly, or a separate compile-time-checked key registry).Also applies to: 35-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orchestrator-v2/packages/interfaces-task/src/types.ts` around lines 3 - 9, `DataRegistries`’s `Record<string, unknown>` base makes `keyof DataRegistries & string` widen to plain `string`, so invalid registry keys slip through. Update the shared typing in `types.ts` and the dependent `createDataRegistry`/guard signatures so keys are constrained to the actual augmented registry names, not any string; keep the existing `DataRegistries`, `DataRegistryGuards`, and `Pick<DataRegistries, T>` call sites working with literal-key inference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@orchestrator-v2/packages/interfaces-task/src/types.ts`:
- Around line 3-9: `DataRegistries`’s `Record<string, unknown>` base makes
`keyof DataRegistries & string` widen to plain `string`, so invalid registry
keys slip through. Update the shared typing in `types.ts` and the dependent
`createDataRegistry`/guard signatures so keys are constrained to the actual
augmented registry names, not any string; keep the existing `DataRegistries`,
`DataRegistryGuards`, and `Pick<DataRegistries, T>` call sites working with
literal-key inference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ffdf1149-9ab5-45d3-89e1-afa2096e092a
⛔ Files ignored due to path filters (1)
orchestrator-v2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
orchestrator-v2/examples/lvl3/package.jsonorchestrator-v2/examples/lvl3/src/index.spec.tsorchestrator-v2/examples/lvl3/src/index.tsorchestrator-v2/examples/lvl3/tsconfig.jsonorchestrator-v2/packages/agent-3-task/package.jsonorchestrator-v2/packages/agent-3-task/src/augment.tsorchestrator-v2/packages/agent-3-task/src/create-task-agent.tsorchestrator-v2/packages/agent-3-task/src/enroll-task-agent.tsorchestrator-v2/packages/agent-3-task/src/index.tsorchestrator-v2/packages/agent-3-task/src/run-task-agent.spec.tsorchestrator-v2/packages/agent-3-task/src/run-task-agent.tsorchestrator-v2/packages/agent-3-task/tsconfig.jsonorchestrator-v2/packages/agent-3-task/vite.config.tsorchestrator-v2/packages/engine/package.jsonorchestrator-v2/packages/interfaces-task/src/index.tsorchestrator-v2/packages/interfaces-task/src/types.tsorchestrator-v2/packages/runner/src/data-registry.spec.tsorchestrator-v2/packages/runner/src/data-registry.tsorchestrator-v2/packages/runner/src/runner.tsorchestrator-v2/packages/runner/src/types.tsorchestrator-v2/pnpm-workspace.yamlorchestrator-v2/tsconfig.json
💤 Files with no reviewable changes (1)
- orchestrator-v2/packages/agent-3-task/src/enroll-task-agent.ts
Rebuilds the agent-3-task Runner augmentation on top of the work-item unification (#44). registerTaskAgent/registerEngine on Runner.prototype now target registerWorkItemHandler, and a bare new Runner() lazily creates its guarded data registry via a new ensure(type, guard) instead of throwing "Unknown data type". - ensure(type, guard) on MutableDataRegistry (+ shared ensureRegistry helper) - ./augment export + a runner dependency on agent-3-task - regression test for bare-Runner registration Co-Authored-By: Eric Siebeneich <eric.siebeneich@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
matt-wright86
force-pushed
the
feat/runner-augmentation
branch
from
e44e8df to
6b441ef
Compare
1 participant
Summary
A
Runneraugmentation so a barenew Runner()can register task agents and engines with a single import.import "@bifrost-ai/agent-3-task/augment"teaches theRunnerclassregisterTaskAgent()/registerEngine()via declaration merging +Runner.prototype. Includes the deferred-configRunner, theagent-3-taskaugment, a runnablelvl3example, and the fix that makes it work end-to-end.Motivation
Registering a task agent meant hand-wiring a data registry, guards, an engine, and a free
enrollTaskAgentcall. This collapses it to:The
Runnercore stays domain-neutral — each domain package opts in to its own methods through a side-effect/augmentimport, so the orchestrator never learns about "task agents".Type of change
RunnerRunnerregistration threwUnknown data type/simplifypass; removed a debug log + a duplicate fileRunnerregistration regression testChecklist
go/npxwere not usedmake lint/make test/make build— N/A:orchestrator-v2uses its ownvp/oxlint toolchainvp run -r buildclean ·vp run -r testgreen (41/41)mainWhat changed
agent-3-task/augment—registerTaskAgent/registerEngineonRunner.prototype, plus theDataRegistriesmodule augmentation for type-safe registries.Runner—new Runner()with no args (identity/url resolved atstart());createTaskAgentreplacesenrollTaskAgent.ensure(type, guard)on the mutable data registry — the fix. A barenew Runner()starts with an empty data registry, soregister*calledget("agentDefinition"|"engine")and threwUnknown data type.ensurelazily creates the guarded registry on first use, so theisEngine/isAgentDefinitionvalidation is preserved.lvl3example + a regression test that drives a bare-Runnerregistration (also closes the "example has no tests" CI gate).Notes for reviewers
taskAgentDataGuardsis now unused. The augment threads guards individually, so the bundle has no consumers — revive it via a registryseed(bundle)method, or drop it. Left as a design call.refactor/orchestrator-v2-hardening([Experiment] orchestrator-v2 hardening: telemetry, resilience, capability routing #43) ininterfaces-taskandrunner/runner.ts— whichever lands first, the other rebases.ExecutionStatsits neutral home (out ofinterfaces-task-source).