feat(signals): implement execution signals extraction (P4)#2
Conversation
|
Warning Review limit reached
Next review available in: 59 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds request metadata capture and a new ChangesExecution Signals
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handleChat
participant extractSignals
participant ToolResults
Client->>handleChat: Send dialect and request body
handleChat->>extractSignals: Pass ChatRequest with rawBody
extractSignals->>ToolResults: Parse tool calls and results
ToolResults-->>extractSignals: Return execution and failure data
extractSignals-->>Client: Provide derived execution signals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🤖 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.
Inline comments:
In `@src/signals.ts`:
- Around line 144-154: Guard the `tc.name` access in the signal-processing loop
before invoking `.toLowerCase()`. For undefined or otherwise missing tool names,
skip the edit-tool classification while preserving file-path extraction and
filesTouched updates; only call `.toLowerCase()` when a valid name is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8da5dbe-09a1-4025-813f-9a4199c2c0bb
📒 Files selected for processing (6)
openspec/changes/P4-execution-signals/proposal.mdopenspec/changes/P4-execution-signals/tasks.mdsrc/providers/types.tssrc/server.tssrc/signals.test.tssrc/signals.ts
| for (const tc of signals.toolCalls) { | ||
| const path = extractFilePath(tc.arguments); | ||
| if (path) { | ||
| if (!signals.filesTouched.includes(path)) { | ||
| signals.filesTouched.push(path); | ||
| } | ||
| const lowerName = tc.name.toLowerCase(); | ||
| if (editTools.some((t) => lowerName.includes(t))) { | ||
| fileEdits.push(path); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against undefined tc.name before calling .toLowerCase().
Tool call names are pushed from raw.body without validation (line 91: tc.function.name, line 115: part.name). If either is undefined in a malformed request, tc.name.toLowerCase() on line 150 throws a TypeError. Since rawBody is user-controlled input, this is a reachable crash path.
🛡️ Proposed fix
for (const tc of signals.toolCalls) {
const path = extractFilePath(tc.arguments);
if (path) {
if (!signals.filesTouched.includes(path)) {
signals.filesTouched.push(path);
}
- const lowerName = tc.name.toLowerCase();
+ const lowerName = (tc.name ?? "").toLowerCase();
if (editTools.some((t) => lowerName.includes(t))) {
fileEdits.push(path);
}
}
}📝 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.
| for (const tc of signals.toolCalls) { | |
| const path = extractFilePath(tc.arguments); | |
| if (path) { | |
| if (!signals.filesTouched.includes(path)) { | |
| signals.filesTouched.push(path); | |
| } | |
| const lowerName = tc.name.toLowerCase(); | |
| if (editTools.some((t) => lowerName.includes(t))) { | |
| fileEdits.push(path); | |
| } | |
| } | |
| for (const tc of signals.toolCalls) { | |
| const path = extractFilePath(tc.arguments); | |
| if (path) { | |
| if (!signals.filesTouched.includes(path)) { | |
| signals.filesTouched.push(path); | |
| } | |
| const lowerName = (tc.name ?? "").toLowerCase(); | |
| if (editTools.some((t) => lowerName.includes(t))) { | |
| fileEdits.push(path); | |
| } | |
| } |
🤖 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 `@src/signals.ts` around lines 144 - 154, Guard the `tc.name` access in the
signal-processing loop before invoking `.toLowerCase()`. For undefined or
otherwise missing tool names, skip the edit-tool classification while preserving
file-path extraction and filesTouched updates; only call `.toLowerCase()` when a
valid name is present.
Substring includes("fail") flagged benign tokens like "failover" and
"failsafe" as failures. Switch to word-boundary regex and add a
regression test.
pnpm@11.10.0 dropped Node 20 support (needs node:sqlite), breaking the build job at setup. Bump the CI runner to Node 22.
2 participants
Summary
This PR implements P4: Execution Signals as outlined in the Modlane roadmap. It adds a parser that analyzes the conversation history of incoming agent requests (
OpenAIBodyandAnthropicBody) to extract key session metrics and heuristics. These signals are necessary to support classification-first routing (P5) and error-aware escalation (P6).What Was Added/Changed
ChatRequestinterface insrc/providers/types.tsto retaindialect("openai" | "anthropic") and the originalrawBodypayload.src/server.tsto assign these properties in the gateway during parsing.src/signals.ts):extractSignals(req)to parse OpenAI and Anthropic formats.FAIL,compile error,exit status).proposal.mdandtasks.mdunderopenspec/changes/P4-execution-signals/.Testing
Added comprehensive unit tests in
src/signals.test.tsverifying extraction accuracy under both OpenAI and Anthropic dialects.Verified that:
pnpm run typecheckpasses with zero compiler errors.pnpm run buildsucceeds.pnpm testruns and passes all 22 tests (including 4 new signals tests).Summary by CodeRabbit