Repository navigation
Task 111 · Add repeatable DeepSeek routing certification - #57
Conversation
Reviewer's GuideThis PR introduces a repeatable DeepSeek routing certification workflow for the PR #52 skill-routing architecture, combining live headless CLI probes for six owner and S4-verifier scenarios with lightweight Vitest source guards that prevent required cases or fail-closed handling from being removed. Sequence diagram for DeepSeek routing certificationsequenceDiagram
participant Harness as verify-deepseek-routing.mjs
participant DeepSeekCLI as DeepSeek headless CLI
participant Vitest as Vitest guard
loop six routing cases
Harness->>DeepSeekCLI: spawnSync npx @deepseek-ai/dsh
DeepSeekCLI-->>Harness: stdout, stderr, exit status
Harness->>Harness: expected.test(output)
alt output matches and status is zero
Harness-->>Harness: log PASS
else probe fails
Harness->>Harness: process.exitCode = 1
Harness-->>Harness: log expected output
end
end
Vitest->>Vitest: readFileSync script source
Vitest->>Vitest: assert required cases and task-verifier
Vitest->>Vitest: assert process.exitCode = 1 and expected handling
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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 |
|
Failed to generate code suggestions for PR |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 37 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/verify-deepseek-routing.mjs" line_range="5-10" />
<code_context>
+ ['copilotkit', 'Fix a CopilotKit generative UI rendering bug. Answer only with the single MDE skill owner.', /\bcopilotkit\b/i],
</code_context>
<issue_to_address>
**issue (bug_risk):** The certification accepts any occurrence of the expected skill name in combined stdout/stderr, rather than verifying the routed owner or verifier decision. When `dsh` echoes the submitted prompt or includes it in diagnostics, five probes pass even if routing fails because those prompts already contain their expected terms.
**Triggers:** When the DeepSeek CLI echoes the input prompt or includes request text in a warning/error.
**Suggested fix:** Parse the structured routing result, or require the expected owner and verifier decision in the answer while rejecting echoed prompt/diagnostic text.
</issue_to_address>
### Comment 2
<location path="scripts/verify-deepseek-routing.mjs" line_range="15-20" />
<code_context>
+
+let failed = 0;
+for (const [name, prompt, expected] of cases) {
+ const result = spawnSync('npx', ['--yes', '@deepseek-ai/dsh', '--profile', 'headless', prompt], {
+ cwd: process.cwd(),
+ encoding: 'utf8',
+ timeout: 120000,
+ env: process.env,
+ });
+ const output = `${result.stdout ?? ''}\n${result.stderr ?? ''}`;
+ const ok = result.status === 0 && expected.test(output);
</code_context>
<issue_to_address>
**issue (broader_impact):** Each certification run resolves `@deepseek-ai/dsh` from the network without a version constraint or lockfile entry, so the same repository commit can execute different CLI versions and produce different certification results.
**Triggers:** When npm resolves a newer package release, or the package registry/cache changes between runs.
**Suggested fix:** Pin the CLI version and manage it as a declared dependency, or invoke a repository-controlled executable with a locked dependency tree.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: scripts/verify-deepseek-routing.mjs:10, scripts/verify-deepseek-routing.mjs:20
There was a problem hiding this comment.
Pull Request Overview
The PR successfully implements the DeepSeek routing certification harness and the fail-closed Vitest guard as required by the acceptance criteria. While the functional requirements are met, there are significant implementation concerns regarding the reliability and performance of the certification script and the quality of the associated test suite.
The current use of spawnSync without shell: true will break the script on Windows environments. Furthermore, the sequential execution of probes results in inefficient CI/CD runtimes. The Vitest suite currently employs 'meta-testing'—asserting on the script's source code strings rather than its behavior—which provides low functional assurance and high maintenance overhead. These issues should be addressed to ensure a robust and cross-platform certification process.
About this PR
- The use of
spawnSync('npx', ...)withoutshell: truewill cause the certification script to fail in Windows environments asnpxis not a standalone binary. - The certification script executes all DeepSeek probes sequentially. With 6 cases each potentially taking up to 120 seconds, this could result in a 12-minute execution time, which is inefficient for the CI/CD pipeline.
Test suggestions
- Verify that the certification script executes and validates the CopilotKit, Gemini, and Mastra skill owners using regex expectations.
- Verify that the certification script validates the 'task-verifier' requirement for Supabase and Stripe S4 scenarios.
- Verify that a failure in any DeepSeek probe sets
process.exitCode = 1to fail the CI/CD pipeline. - Verify that the Vitest suite correctly identifies if a mandatory routing probe is missing from the certification script.
- Implement functional testing using
vi.mockfor child_process to verify the certification logic without relying on static source code analysis.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Implement functional testing using `vi.mock` for child_process to verify the certification logic without relying on static source code analysis.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| const lines = String(output).split(/\r?\n/).map((line) => line.trim()).filter(Boolean); | ||
| const explicit = [...lines].reverse().find((line) => /^final answer:/i.test(line)); | ||
| if (explicit) return explicit.replace(/^final answer:\s*/i, '').trim(); | ||
| const structured = [...lines].reverse().find((line) => /^owner=[a-z0-9-]+\s+verifier=(true|false)$/i.test(line)); |
There was a problem hiding this comment.
🔥 The Roast: The previous fix removed the broad regex but put the answer key in every prompt. This fallback then treats any echoed owner=... verifier=... line as the final answer, so a probe can pass without routing at all.
🩹 The Fix: Keep expected values out of the prompts, accept only a structured answer from an explicit final-answer boundary, and add a regression test that echoes the exact prompt.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| child.stdout.on('data', (chunk) => { output += chunk; }); | ||
| child.stderr.on('data', (chunk) => { output += chunk; }); | ||
| let timedOut = false; | ||
| const timer = setTimeout(() => { timedOut = true; child.kill('SIGTERM'); }, 120000); |
There was a problem hiding this comment.
🔥 The Roast: The two-minute timeout sends SIGTERM and then politely waits forever if the CLI ignores it. That is less a timeout than a strongly worded suggestion.
🩹 The Fix: Schedule a forced SIGKILL after a short grace period, or use a child-process timeout helper that guarantees termination, and cover a non-cooperative child with a test.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { evaluateCase, runCases } from '../../../scripts/verify-deepseek-routing.mjs'; | ||
|
|
||
| describe('DeepSeek routing certification', () => { |
There was a problem hiding this comment.
🔥 The Roast: The guard for “required routing cases” was refactored so hard it disappeared. These tests cover parsing and aggregation, but never assert that cases still contains all six owners and verifier decisions; an empty matrix can still become PASS (0/0).
🩹 The Fix: Assert the complete six-case matrix, including each verifier flag, make runCases reject an empty probe list, and add a zero-case regression test.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Task 111 · DeepSeek Routing Certification
Adds a small, repeatable certification harness for the merged PR #52 skill-routing architecture when used by DeepSeek Harness.
What changed
scripts/verify-deepseek-routing.mjsVerification
npm run floor: passgit diff --check: passRecovery review
The preserved Task 108 recovery branches were reviewed separately. They are intentionally not merged wholesale because they contain stale Supabase counts/config and older skill ownership guidance that conflicts with current main/PR #52.
Summary by Sourcery
Add automated DeepSeek routing certification for representative skill ownership and high-risk verification decisions.
New Features:
Enhancements:
Tests: