feat(e001-s002): /plan routing with Contract-Net scoring and HTTP execution#5
feat(e001-s002): /plan routing with Contract-Net scoring and HTTP execution#5Dumidu1212 merged 1 commit intomainfrom
Conversation
…, e2e and unit tests
WalkthroughAdds a planning/execution feature: new Planner with HTTP executor, tracing, and metrics; exposes POST /plan route; updates app/server wiring to inject planner; increments Swagger version; introduces metrics endpoint exports; adds e2e and unit tests; bumps @types/jest dev dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as Fastify App (/plan)
participant P as Planner
participant R as Registry
participant S as Scorer
participant E as HTTP Executor
participant H as HTTP Tool
participant T as TraceStore
participant M as Metrics
C->>A: POST /plan { capability, input, execute, timeout_ms }
A->>P: plan(ctx)
P->>T: record("plan_start", ctx)
P->>R: getTools(capability)
R-->>P: tools[]
loop candidates
P->>S: score(tool, ctx)
S-->>P: score
P->>M: inc planner_bids_total{capability, tool}
end
alt execute == true
loop ranked candidates
P->>T: record("attempt", tool)
P->>E: execute(tool, input, overallAbort)
E->>H: HTTP request (timeout/abort aware)
alt 2xx
H-->>E: response JSON
E-->>P: success {output, latency}
P->>M: inc planner_selection_total{capability, tool}
P->>M: observe planner_execution_latency_ms{tool}
P-->>A: 200 { selected, execution: success, traceId }
else error/timeout
H-->>E: error/timeout
E-->>P: failure/timeout
P->>M: inc planner_fallbacks_total{capability}
P->>T: record("fallback", reason)
end
end
P-->>A: 200 { error: ALL_CANDIDATES_FAILED, traceId }
else execute == false
P-->>A: 200 { selected (plan only), traceId }
end
A-->>C: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/unit/planner.fallback.test.ts (1)
41-44: Assert the actual fallback tool.Right now the test only checks that
selected.toolIdis defined, so it would still pass if the planner kept pickingt1. Asserting the ID is't2'validates that the fallback really occurred.Apply this diff:
- expect(res.selected?.toolId).toBeDefined(); + expect(res.selected?.toolId).toBe('t2');src/routes/plan.ts (2)
15-26: Consider requiring the capability field in the request schema.While the planner handles missing capability gracefully (returning
INPUT_INVALID), requiring it at the schema level would provide faster feedback and clearer API semantics. The current permissive schema allows requests without a capability to reach the planner.If you want to enforce capability at the schema level, apply this diff:
body: { type: 'object', additionalProperties: false, + required: ['capability'], properties: { capability: { type: 'string' },
64-67: Document the HTTP 200 response for application-level failures.The handler always returns HTTP 200, even when the planner reports failure (e.g.,
NO_CANDIDATES,ALL_CANDIDATES_FAILED). This is a valid design choice (application-level errors in the response body rather than HTTP error codes), but it should be documented for API consumers to understand the error handling model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(1 hunks)src/app.ts(1 hunks)src/executors/httpExecutor.ts(1 hunks)src/metrics/metrics.ts(1 hunks)src/planner/planner.ts(1 hunks)src/routes/plan.ts(1 hunks)src/server.ts(2 hunks)src/tracing/traceStore.ts(1 hunks)tests/e2e/plan.e2e.test.ts(1 hunks)tests/unit/planner.fallback.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/app.ts (5)
src/registry/service.ts (1)
IRegistryService(4-4)src/planner/contracts.ts (1)
IPlanner(63-65)src/registry/model.ts (2)
toolSchema(24-71)registrySchema(73-81)src/routes/tools.ts (1)
toolsRoutes(8-48)src/routes/plan.ts (1)
planRoutes(9-69)
src/planner/planner.ts (5)
src/planner/contracts.ts (6)
IPlanner(63-65)IScorer(49-51)IToolExecutor(54-60)PlanContext(10-23)PlanResult(40-46)JsonRecord(7-7)src/registry/service.ts (1)
IRegistryService(4-4)src/tracing/traceStore.ts (1)
TraceStore(16-34)src/executors/httpExecutor.ts (1)
execute(9-59)src/metrics/metrics.ts (3)
plannerBidsTotal(14-18)plannerSelectionTotal(21-25)plannerFallbacksTotal(28-32)
tests/e2e/plan.e2e.test.ts (4)
src/planner/contracts.ts (2)
IToolExecutor(54-60)ExecutionResult(32-37)src/registry/service.ts (1)
IRegistryService(4-4)src/planner/planner.ts (1)
Planner(26-116)src/tracing/traceStore.ts (1)
TraceStore(16-34)
src/server.ts (5)
src/planner/scoring.simple.ts (1)
SimpleScorer(40-54)src/executors/httpExecutor.ts (1)
HttpExecutor(8-60)src/tracing/traceStore.ts (1)
TraceStore(16-34)src/planner/planner.ts (1)
Planner(26-116)src/app.ts (1)
buildApp(14-30)
tests/unit/planner.fallback.test.ts (5)
src/planner/contracts.ts (2)
IToolExecutor(54-60)ExecutionResult(32-37)src/registry/service.ts (1)
IRegistryService(4-4)src/planner/scoring.simple.ts (1)
SimpleScorer(40-54)src/tracing/traceStore.ts (1)
TraceStore(16-34)src/planner/planner.ts (1)
Planner(26-116)
src/executors/httpExecutor.ts (1)
src/planner/contracts.ts (3)
IToolExecutor(54-60)JsonRecord(7-7)ExecutionResult(32-37)
src/routes/plan.ts (1)
src/planner/contracts.ts (2)
IPlanner(63-65)PlanContext(10-23)
🔇 Additional comments (12)
src/server.ts (3)
6-9: LGTM! Clean planner component imports.The imports for
SimpleScorer,Planner,HttpExecutor, andTraceStorealign with the new planning architecture introduced in this PR.
25-28: LGTM! Proper dependency injection setup.The planner is correctly instantiated with all required dependencies (registry, scorer, executor, traces) following the constructor signature from
src/planner/planner.ts.
30-30: LGTM! Updated buildApp call aligns with new signature.The
buildAppcall now correctly passes bothregistryandplanner, matching the updated signature insrc/app.ts.tests/e2e/plan.e2e.test.ts (1)
48-58: LGTM! Well-structured end-to-end test.The test appropriately validates the planning flow: capability matching, candidate scoring, selection logic, and execution status. The assertions cover all key aspects of the
/planendpoint behavior.src/app.ts (4)
7-9: LGTM! Imports align with the new planning feature.The new imports for
planRoutesandIPlannersupport the integration of the planning endpoint into the application.
11-14: LGTM! Clear documentation and updated signature.The comment helpfully describes the function's purpose, and the signature change correctly adds the
plannerdependency alongsideregistry.
20-20: LGTM! Appropriate version bump for new feature.Bumping the Swagger API version from 0.1.0 to 0.2.0 correctly reflects the addition of the
/planendpoint as a new minor feature.
24-26: LGTM! Clean route registration with proper dependency injection.The route registration correctly separates concerns:
toolsRoutesreceives only the registry,planRoutesreceives only the planner, andmetricsRoutesrequires no dependencies. This follows good dependency injection practices.src/metrics/metrics.ts (4)
7-18: LGTM! Well-documented planner bid metrics.The
toolsLoaded,toolLoadErrors, andplannerBidsTotalmetrics are clearly documented and appropriately defined with helpful labels (capability,tool) for tracking scoring activity.
20-32: LGTM! Clear selection and fallback tracking metrics.The
plannerSelectionTotalandplannerFallbacksTotalcounters provide good observability into the planner's decision-making and fallback behavior. The label choices allow tracking by capability and tool.
34-40: LGTM! Appropriate histogram configuration for execution latency.The
plannerExecutionLatencyMshistogram has reasonable buckets (50ms to 6.4s) that cover typical tool execution times. The exponential bucket distribution will provide good resolution for percentile calculations.
42-47: LGTM! All metrics properly registered.All newly defined planner metrics are correctly registered with the registry, ensuring they will be exposed via the
/metricsendpoint.
| plannerFallbacksTotal.labels({ capability }).inc(); | ||
| this.traces.record(traceId, 'fallback', { toolId: tool.id, error: execRes.error, status: execRes.status }); | ||
| // If overall deadline elapsed, break early | ||
| if (overallController.signal.aborted) break; | ||
| } | ||
|
|
||
| // No candidate succeeded | ||
| result.execution = { status: 'failure', error: 'ALL_CANDIDATES_FAILED' }; | ||
| this.traces.record(traceId, 'failure', result.execution); | ||
| return result; |
There was a problem hiding this comment.
Propagate overall timeout instead of ALL_CANDIDATES_FAILED.
When the overall deadline aborts overallController, we break out of the loop but later report {status:'failure', error:'ALL_CANDIDATES_FAILED'}. Clients can’t distinguish a plan-level timeout from actual candidate exhaustion, and traces/metrics misclassify the outcome. Surface the timeout immediately when overallController.signal.aborted so the result (and trace) reflects the real failure mode.
Apply this diff:
plannerFallbacksTotal.labels({ capability }).inc();
this.traces.record(traceId, 'fallback', { toolId: tool.id, error: execRes.error, status: execRes.status });
- // If overall deadline elapsed, break early
- if (overallController.signal.aborted) break;
+ // If the overall deadline elapsed, surface timeout immediately
+ if (overallController.signal.aborted) {
+ const reason = String((overallController.signal as unknown as { reason?: unknown }).reason ?? 'overall-timeout');
+ result.execution = { status: 'timeout', error: reason };
+ this.traces.record(traceId, 'timeout', { reason });
+ return result;
+ }
}
- // No candidate succeeded
- result.execution = { status: 'failure', error: 'ALL_CANDIDATES_FAILED' };
+ // No candidate succeeded before the deadline
+ result.execution = { status: 'failure', error: 'ALL_CANDIDATES_FAILED' };
this.traces.record(traceId, 'failure', result.execution);
return result;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| plannerFallbacksTotal.labels({ capability }).inc(); | |
| this.traces.record(traceId, 'fallback', { toolId: tool.id, error: execRes.error, status: execRes.status }); | |
| // If overall deadline elapsed, break early | |
| if (overallController.signal.aborted) break; | |
| } | |
| // No candidate succeeded | |
| result.execution = { status: 'failure', error: 'ALL_CANDIDATES_FAILED' }; | |
| this.traces.record(traceId, 'failure', result.execution); | |
| return result; | |
| plannerFallbacksTotal.labels({ capability }).inc(); | |
| this.traces.record(traceId, 'fallback', { toolId: tool.id, error: execRes.error, status: execRes.status }); | |
| // If the overall deadline elapsed, surface timeout immediately | |
| if (overallController.signal.aborted) { | |
| const reason = String((overallController.signal as unknown as { reason?: unknown }).reason ?? 'overall-timeout'); | |
| result.execution = { status: 'timeout', error: reason }; | |
| this.traces.record(traceId, 'timeout', { reason }); | |
| return result; | |
| } | |
| } | |
| // No candidate succeeded before the deadline | |
| result.execution = { status: 'failure', error: 'ALL_CANDIDATES_FAILED' }; | |
| this.traces.record(traceId, 'failure', result.execution); | |
| return result; |
| class StubExec implements IToolExecutor { | ||
| async execute(tool: Tool): Promise<ExecutionResult> { | ||
| // Pretend both succeed; latencies differ by name | ||
| const latency = tool.id === 'fast' ? 10 : 50; | ||
| return { status: 'success', latency_ms: latency, output: { id: tool.id } }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the method signature to match IToolExecutor interface.
The execute method signature is missing required parameters. The IToolExecutor interface expects three parameters: tool, input, and overallAbort.
Apply this diff to fix the signature:
class StubExec implements IToolExecutor {
- async execute(tool: Tool): Promise<ExecutionResult> {
+ async execute(tool: Tool, _input: JsonRecord, _overallAbort: AbortSignal): Promise<ExecutionResult> {
// Pretend both succeed; latencies differ by name
const latency = tool.id === 'fast' ? 10 : 50;
return { status: 'success', latency_ms: latency, output: { id: tool.id } };
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class StubExec implements IToolExecutor { | |
| async execute(tool: Tool): Promise<ExecutionResult> { | |
| // Pretend both succeed; latencies differ by name | |
| const latency = tool.id === 'fast' ? 10 : 50; | |
| return { status: 'success', latency_ms: latency, output: { id: tool.id } }; | |
| } | |
| } | |
| class StubExec implements IToolExecutor { | |
| async execute(tool: Tool, _input: JsonRecord, _overallAbort: AbortSignal): Promise<ExecutionResult> { | |
| // Pretend both succeed; latencies differ by name | |
| const latency = tool.id === 'fast' ? 10 : 50; | |
| return { status: 'success', latency_ms: latency, output: { id: tool.id } }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/e2e/plan.e2e.test.ts around lines 23 to 29, the StubExec.execute method
currently declares only (tool: Tool) but must match IToolExecutor and accept
(tool, input, overallAbort); update the signature to include the missing
parameters and their types (e.g., tool: Tool, input: unknown, overallAbort?:
AbortSignal), keep the existing body logic, and if input or overallAbort are
unused, name them with a leading underscore or explicitly ignore them to satisfy
lint/type checks.
1 participant
Implements candidate filtering, scoring, selection, fallback, and execution via HTTP. Adds /plan route, planner metrics, and e2e tests. Swagger updated via route schema.
Summary by CodeRabbit