add task-specific model routing - #36
Conversation
WalkthroughThe PR adds the ChangesDelegation configuration
Sequence Diagram(s)sequenceDiagram
participant User
participant Relay
participant delegate-config
participant Implementer
User->>Relay: dispatch with --route
Relay->>delegate-config: load global and project configuration
delegate-config-->>Relay: return resolved route settings
Relay->>Implementer: invoke with effective options
Implementer-->>Relay: return execution result
Relay-->>User: write result.json with route metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 9
🧹 Nitpick comments (2)
skills/codex-delegate/scripts/relay.mjs (1)
233-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
readOnlytype checks.loadDelegateConfiglistsreadOnlyinBOOLEAN_CONFIG_FIELDSand already exits when the value is not a boolean. Each relay then repeats the same check after resolution, so the second check can never fail and its error message can never appear.
skills/codex-delegate/scripts/relay.mjs#L233-L235: delete theconfig.readOnlyboolean check.skills/claude-delegate/scripts/relay.mjs#L289-L291: delete theopts.readOnlyboolean check;opts.readOnlyis a boolean from the flag, the validated config value, or thefalsedefault.skills/grok-delegate/scripts/relay.mjs#L242-L244: delete theconfig.readOnlyboolean check.skills/pi-delegate/scripts/relay.mjs#L167-L169: delete theopts.readOnlyboolean check.skills/vibe-delegate/scripts/relay.mjs#L180-L182: delete theconfig.readOnlyboolean check.🤖 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 `@skills/codex-delegate/scripts/relay.mjs` around lines 233 - 235, Remove the redundant readOnly type-validation blocks from the relay entrypoints: skills/codex-delegate/scripts/relay.mjs lines 233-235, skills/claude-delegate/scripts/relay.mjs lines 289-291, skills/grok-delegate/scripts/relay.mjs lines 242-244, skills/pi-delegate/scripts/relay.mjs lines 167-169, and skills/vibe-delegate/scripts/relay.mjs lines 180-182. Keep loadDelegateConfig and the existing readOnly resolution/default behavior unchanged, since those paths already guarantee a boolean.skills/claude-delegate/scripts/relay.mjs (1)
193-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImprove the error message for a numeric
timeoutthat is not a positive integer.Line 193 converts
timeoutto a seconds string only when the value is a positive safe integer. A value such as0,-30, or1.5falls through. Line 197 then reportsclaude field "timeout" must be a string, even though the user supplied a number. Report the actual constraint.♻️ Proposed refactor
- const fieldValue = field === "timeout" && Number.isSafeInteger(rawValue) && rawValue > 0 - ? `${rawValue}s` - : rawValue; + let fieldValue = rawValue; + if (field === "timeout" && typeof rawValue === "number") { + if (!Number.isSafeInteger(rawValue) || rawValue <= 0) { + fail(`invalid delegate config ${candidate.path}: claude field "timeout" must be a positive whole number of seconds or an h/m/s string`); + } + fieldValue = `${rawValue}s`; + }🤖 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 `@skills/claude-delegate/scripts/relay.mjs` around lines 193 - 199, Update the validation around fieldValue and expectedType so numeric timeout values that are not positive safe integers produce an error stating that timeout must be a positive integer, instead of the generic string-type message. Preserve the existing conversion of valid timeout values to seconds strings and the current type validation for other fields.
🤖 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 `@skills/codex-delegate/scripts/relay.mjs`:
- Around line 227-231: Update the sandbox resolution around loadDelegateConfig
so readOnly and sandbox preserve per-field layer precedence: a higher-layer
sandbox value must not be overridden by lower-layer readOnly. Track each field’s
source layer or resolve readOnly into sandbox while processing layers, then
apply the final resolved value in the opts.sandbox fallback block.
In `@skills/cursor-delegate/scripts/relay.mjs`:
- Around line 379-384: The relay.mjs site requires no code change; its sandbox
result field is already correct. Update
skills/cursor-delegate/references/dispatch-and-poll.md lines 61-67 to add
sandbox to the documented result.json field list alongside readOnly and force.
In `@skills/delegate-config/scripts/discover.mjs`:
- Around line 277-299: Update probeAuth so probe failures—including execution
errors, timeouts, JSON.parse errors, and missing successPattern
configuration—return null rather than false. Guard successPattern before calling
test, while preserving true for matching output and false only for a successful
probe that positively determines the CLI is unauthenticated.
- Around line 239-254: Quote the resolved binary path only when launching
through a Windows shell in the tryArgs execution logic of probeVersion,
probeAuth, and probeModels, using needsWindowsShell to select the quoted value
while preserving unquoted paths for direct execution. Verify the Windows smoke
tests for codex, opencode, grok, and pi, including an installation path
containing spaces.
In `@skills/delegate-config/SKILL.md`:
- Around line 147-149: Update the timeout documentation near the version
compatibility note to state that positive integer timeout values are interpreted
as seconds in all configuration layers, including version 2 defaults and routes;
keep the separate version 1 sandbox-to-permissionMode compatibility guidance
unchanged.
In `@skills/grok-delegate/scripts/relay.mjs`:
- Around line 236-240: Update the autonomy resolution logic in relay.mjs so the
higher-priority project route’s sandbox setting takes precedence over a
lower-layer readOnly value, preserving per-field layer precedence rather than
relying on the merged config order. Apply the same correction to the
corresponding autonomy resolution block in codex-delegate’s relay.mjs.
- Line 88: Update the shared sandbox configuration documentation in
skills/delegate-config/SKILL.md and the discover.mjs reference table to list
valid sandbox values separately for grok and codex: grok supports
workspace-write, read-only, and full-access, while codex supports read-only,
workspace-write, and danger-full-access. Make the guidance explicit so users do
not copy implementer-specific values between them.
In `@skills/kimi-delegate/scripts/relay.mjs`:
- Around line 134-141: Update the timeout validation in the relay options flow
before the watchdog calls setTimeout: parse opts.timeout once, reject null,
zero, and negative durations, and reject values above MAX_TIMER_MS =
2_147_483_647. Reuse the same bound and validation behavior as
agy-delegate/scripts/relay.mjs while preserving the existing malformed-duration
failure message.
In `@skills/qoder-delegate/scripts/relay.mjs`:
- Around line 131-155: Resolve configured values before validating opts.model by
moving the non-empty model check after delegateConfig/model assignment, matching
the agy-delegate and kimi-delegate relay behavior. In the permissionMode
fallback, remove the obsolete config.sandbox branch and the redundant readOnly
type guard, relying on loadDelegateConfig validation. Update the unsupported
permission-mode error to identify that the invalid value came from delegate
configuration rather than labeling it as a CLI flag.
---
Nitpick comments:
In `@skills/claude-delegate/scripts/relay.mjs`:
- Around line 193-199: Update the validation around fieldValue and expectedType
so numeric timeout values that are not positive safe integers produce an error
stating that timeout must be a positive integer, instead of the generic
string-type message. Preserve the existing conversion of valid timeout values to
seconds strings and the current type validation for other fields.
In `@skills/codex-delegate/scripts/relay.mjs`:
- Around line 233-235: Remove the redundant readOnly type-validation blocks from
the relay entrypoints: skills/codex-delegate/scripts/relay.mjs lines 233-235,
skills/claude-delegate/scripts/relay.mjs lines 289-291,
skills/grok-delegate/scripts/relay.mjs lines 242-244,
skills/pi-delegate/scripts/relay.mjs lines 167-169, and
skills/vibe-delegate/scripts/relay.mjs lines 180-182. Keep loadDelegateConfig
and the existing readOnly resolution/default behavior unchanged, since those
paths already guarantee a boolean.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63eb1979-0b1c-4ae1-bef4-41429cc2009d
📒 Files selected for processing (26)
AGENTS.mdREADME.mdskills.sh.jsonskills/agy-delegate/references/dispatch-and-poll.mdskills/agy-delegate/scripts/relay.mjsskills/claude-delegate/references/dispatch-and-poll.mdskills/claude-delegate/scripts/relay.mjsskills/codex-delegate/references/dispatch-and-poll.mdskills/codex-delegate/scripts/relay.mjsskills/cursor-delegate/references/dispatch-and-poll.mdskills/cursor-delegate/scripts/relay.mjsskills/delegate-config/SKILL.mdskills/delegate-config/scripts/discover.mjsskills/grok-delegate/references/dispatch-and-poll.mdskills/grok-delegate/scripts/relay.mjsskills/kimi-delegate/references/dispatch-and-poll.mdskills/kimi-delegate/scripts/relay.mjsskills/opencode-delegate/references/dispatch-and-poll.mdskills/opencode-delegate/scripts/relay.mjsskills/pi-delegate/references/dispatch-and-poll.mdskills/pi-delegate/scripts/relay.mjsskills/qoder-delegate/references/dispatch-and-poll.mdskills/qoder-delegate/scripts/relay.mjsskills/vibe-delegate/references/dispatch-and-poll.mdskills/vibe-delegate/scripts/relay.mjstest/relay-smoke.mjs
| if (opts.sandbox === null) { | ||
| if (config.readOnly === true) opts.sandbox = "read-only"; | ||
| else if (config.sandbox !== undefined) opts.sandbox = config.sandbox; | ||
| else opts.sandbox = "workspace-write"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
readOnly from a lower layer overrides sandbox from a higher layer.
loadDelegateConfig merges layers per field, so resolved can hold readOnly from global defaults and sandbox from a project route. Line 228 checks readOnly first. A project route that sets sandbox: "workspace-write" is then ignored when global defaults set readOnly: true. This contradicts the documented precedence in which the project route wins per field.
Track which layer supplied each of the two fields, or resolve readOnly into sandbox inside each layer so that the last layer wins.
🤖 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 `@skills/codex-delegate/scripts/relay.mjs` around lines 227 - 231, Update the
sandbox resolution around loadDelegateConfig so readOnly and sandbox preserve
per-field layer precedence: a higher-layer sandbox value must not be overridden
by lower-layer readOnly. Track each field’s source layer or resolve readOnly
into sandbox while processing layers, then apply the final resolved value in the
opts.sandbox fallback block.
| modelSource: opts.modelSource, | ||
| route: opts.route, | ||
| timeout: opts.timeout, | ||
| readOnly: opts.readOnly, | ||
| force: opts.force && !opts.readOnly, | ||
| sandbox: opts.sandbox, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
sandbox result field is undocumented. makeResultWriter writes sandbox: opts.sandbox into every result.json, but the reference doc's result-field enumeration never mentions sandbox. This breaks the "documented commands match … its relay.mjs" rule for reference docs.
skills/cursor-delegate/scripts/relay.mjs#L379-L384: field already correct; no code change needed — this is the source of the new contract.skills/cursor-delegate/references/dispatch-and-poll.md#L61-L67: addsandboxto the documentedresult.jsonfield list alongsidereadOnlyandforce.
📝 Proposed doc fix
- `workdir`, `route`, `timeout`, `model` (the requested name or `null`), `modelSource`, `resolvedModel` (the model Cursor actually
served, from its init event), `permissionMode` (the mode Cursor reported applying), `readOnly`,
- `force`, `resumed`, `cursorAgentVersion`, `sessionId`, `startedAt`, and `finishedAt`.
+ `force`, `sandbox`, `resumed`, `cursorAgentVersion`, `sessionId`, `startedAt`, and `finishedAt`.📝 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.
| modelSource: opts.modelSource, | |
| route: opts.route, | |
| timeout: opts.timeout, | |
| readOnly: opts.readOnly, | |
| force: opts.force && !opts.readOnly, | |
| sandbox: opts.sandbox, | |
| `result.json` fields: | |
| - `schema`, `tool` (`"cursor-agent"`), `status` (`completed` | `failed` | `timeout` | `aborted` | | |
| `cursor_agent_unavailable`), `exitCode`, and `signal` (`null` unless the child died on a signal). | |
| - `workdir`, `route`, `timeout`, `model` (the requested name or `null`), `modelSource`, `resolvedModel` (the model Cursor actually | |
| served, from its init event), `permissionMode` (the mode Cursor reported applying), `readOnly`, | |
| `force`, `sandbox`, `resumed`, `cursorAgentVersion`, `sessionId`, `startedAt`, and `finishedAt`. |
📍 Affects 2 files
skills/cursor-delegate/scripts/relay.mjs#L379-L384(this comment)skills/cursor-delegate/references/dispatch-and-poll.md#L61-L67
🤖 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 `@skills/cursor-delegate/scripts/relay.mjs` around lines 379 - 384, The
relay.mjs site requires no code change; its sandbox result field is already
correct. Update skills/cursor-delegate/references/dispatch-and-poll.md lines
61-67 to add sandbox to the documented result.json field list alongside readOnly
and force.
Source: Coding guidelines
| function needsWindowsShell(impl, binaryPath) { | ||
| return process.platform === "win32" && | ||
| (impl.shell || /\.(?:cmd|bat)$/i.test(binaryPath)); | ||
| } | ||
|
|
||
| function probeVersion(impl, binaryPath) { | ||
| const useShell = needsWindowsShell(impl, binaryPath); | ||
|
|
||
| const tryArgs = (versionArgs) => { | ||
| try { | ||
| const raw = execFileSync(binaryPath, versionArgs, { | ||
| encoding: "utf8", | ||
| timeout: PROBE_TIMEOUT, | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| shell: useShell, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'discover\.mjs$' . || true
echo "== file outline =="
ast-grep outline skills/delegate-config/scripts/discover.mjs --view compact || true
echo "== relevant lines =="
sed -n '210,340p' skills/delegate-config/scripts/discover.mjs | cat -n -v
echo "== searches for execFileSync/useShell/quoted argv patterns =="
rg -n "execFileSync|shellExpr|quote|shell|needsWindowsShell|probeVersion|probeAuth|probeModels|spawn|shellCommand" skills/delegate-config/scripts/discover.mjs skills -g '*.mjs' | head -200
echo "== diff stat/name =="
git diff --stat || true
git diff -- skills/delegate-config/scripts/discover.mjs | sed -n '1,220p' || trueRepository: amElnagdy/delegate-skills
Length of output: 29524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== implementer shell flags and binary names =="
sed -n '36,150p' skills/delegate-config/scripts/discover.mjs | cat -n -v
echo "== Node child_process docs availability =="
node - <<'JS'
const cpDocs = require('node:child_process');
console.log(JSON.stringify({
spawn: (typeof cpDocs.spawn),
execFileSync: (typeof cpDocs.execFileSync),
execFileSyncArgs: cpDocs.execFileSync.length
}, null, 2));
JS
echo "== Deterministic shell tokenization probe for spaces =="
python3 - <<'PY'
paths = [
r"C:\Program Files\nodejs\codex.cmd",
r"C:\Program Files\Custom CLI\opencode.cmd",
r"C:\Users\User Name\bin\cursor-agent.cmd",
]
for p in paths:
first_space = p.find(' ')
print(f"path={p!r} first_space_at={first_space} unresolved_token={p[:first_space] if first_space != -1 else p}")
PY
echo "== Existing relay quoting helpers for Windows =="
for f in skills/cursor-delegate/scripts/relay.mjs skills/grok-delegate/scripts/relay.mjs skills/sketch-delegate/scripts/relay.mjs 2>/dev/null; do
[ -f "$f" ] || continue
echo "-- $f --"
rg -n "quotePath|win32-quote|quote" "$f" -A 3 -B 3 || true
doneRepository: amElnagdy/delegate-skills
Length of output: 5253
🌐 Web query:
Node child_process spawn shell true string command line argument escaping
💡 Result:
When using child_process.spawn with { shell: true } in Node.js, command-line arguments passed in the args array are not automatically escaped by Node.js [1][2][3]. Instead, Node.js concatenates the command and the arguments into a single string with spaces and passes that string to the shell for execution [1][4][3]. This behavior creates significant security risks: 1. Command Injection: Because the shell interprets the resulting concatenated string, any user-provided input within the args array that contains shell metacharacters (such as &,;, |, $, etc.) can be used to inject and execute arbitrary commands [2][3]. 2. False Sense of Security: Developers often assume that passing arguments in an array provides the same level of safety as the default spawn (shell: false) behavior, where arguments are passed directly to the OS execve call without shell interpretation [3][5]. Enabling shell: true negates this protection entirely [3]. Due to these risks, Node.js has issued deprecations (e.g., DEP0190) for passing an args array when { shell: true } is enabled [5]. Best Practices: * Avoid { shell: true } whenever possible [3]. Use the default shell: false to pass arguments as a separate array; this is the safest method as it bypasses shell parsing and ensures arguments are treated as literal strings [6][3][5]. * If you must use shell: true (for example, to use shell features like pipes, redirections, or globbing), you are responsible for manually sanitizing and escaping all input [4][2]. However, properly escaping shell arguments is complex and error-prone, so it is highly recommended to design your application to avoid passing unsanitized user input to shell commands [7][8][2]. * Consider using alternative approaches if the goal is simply to execute a command that is not a standard executable (e.g., handling.cmd or.bat files on Windows), as these can often be executed directly with proper path handling rather than by spawning a full shell [2].
Citations:
- 1: child_process should individually escape args[] on shell: true nodejs/node#29532
- 2: DEP0190 (passing args to spawn with shell: true) isn't fixable when
stdiooption is required nodejs/node#58763 - 3: https://docs.cli.vulnetix.com/docs/sast-rules/vnx-node-026/
- 4: Disallow args in child_process execFile/spawn when the shell option is true nodejs/node#57143
- 5: https://www.thenodebook.com/child-processes/spawn-exec-fork
- 6: https://stackoverflow.com/questions/1779858/how-do-i-escape-a-string-for-a-shell-command-in-node
- 7: https://nodejs.org/docs/latest-v26.x/api/child_process.html
- 8: https://nodejs.org/docs/latest-v22.x/api/child_process.html
🌐 Web query:
Node child_process execFileSync shell true command string argument escaping
💡 Result:
When using child_process.execFileSync with the shell: true option, Node.js does not automatically escape or sanitize arguments [1][2][3]. Instead, it concatenates the provided command string and the elements of the args array into a single space-separated string, which is then passed directly to the shell for execution [1][2][3]. This behavior introduces significant security risks because it is vulnerable to command injection [2][3]. If any part of the command or arguments contains unsanitized user input, a malicious actor can include shell metacharacters (such as &, |,;, $, etc.) to execute arbitrary commands [4][5][6]. Due to these risks, Node.js documentation explicitly warns against passing unsanitized user input to these functions when shell: true is enabled [4][5]. Furthermore, recent versions of Node.js (beginning with v24.0.0) have deprecated the use of the args parameter when shell: true is enabled (DEP0190), as providing an array gives a false sense of isolation or automatic escaping that does not exist [2][7][3]. To safely execute commands: 1. Prefer setting shell: false (the default), which bypasses the shell and treats the command and arguments as a distinct, uninterpreted sequence [4][6]. 2. If a shell is strictly required, ensure all user-provided strings are rigorously sanitized, or concatenate the full command string yourself, taking full responsibility for correct quoting and escaping relative to the specific target shell [2][3]. 3. For complex command construction, consider using dedicated libraries that handle cross-platform escaping and quoting [7].
Citations:
- 1: child_process should individually escape args[] on shell: true nodejs/node#29532
- 2: Disallow args in child_process execFile/spawn when the shell option is true nodejs/node#57143
- 3: DEP0190 (passing args to spawn with shell: true) isn't fixable when
stdiooption is required nodejs/node#58763 - 4: https://nodejs.org/docs/v22.11.0/api/child_process.html
- 5: https://nodejs.org/api/child_process.html
- 6: https://github.com/nodesecurity/eslint-plugin-security/blob/main/docs/avoid-command-injection-node.md
- 7: https://github.com/ericcornelissen/shescape/blob/main/docs/recipes.md
Quote the resolved binary path when the probe launches through a Windows shell.
needsWindowsShell selects shell: true for .cmd/.bat paths and for implementers marked shell: true. In those probes, execFileSync joins the first argument string with args and passes the result to the shell, so a resolved path containing spaces is split before execution and discovery reports version: null. Apply "\\"${binaryPath}\\"" only for the shell case in probeVersion, probeAuth, and probeModels. Run the Windows smoke test for codex, opencode, grok, and pi, including an install path with spaces.
🤖 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 `@skills/delegate-config/scripts/discover.mjs` around lines 239 - 254, Quote
the resolved binary path only when launching through a Windows shell in the
tryArgs execution logic of probeVersion, probeAuth, and probeModels, using
needsWindowsShell to select the quoted value while preserving unquoted paths for
direct execution. Verify the Windows smoke tests for codex, opencode, grok, and
pi, including an installation path containing spaces.
Source: Coding guidelines
| function probeAuth(impl, binaryPath) { | ||
| if (!impl.authProbe) return null; | ||
|
|
||
| const useShell = needsWindowsShell(impl, binaryPath); | ||
| try { | ||
| const raw = execFileSync(binaryPath, impl.authProbe.args, { | ||
| encoding: "utf8", | ||
| timeout: PROBE_TIMEOUT, | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| shell: useShell, | ||
| }); | ||
| if (impl.authProbe.jsonField) { | ||
| return JSON.parse(raw)[impl.authProbe.jsonField] === true; | ||
| } | ||
| return impl.authProbe.successPattern.test(raw); | ||
| } catch (err) { | ||
| // Some CLIs exit non-zero when not authenticated but still print a | ||
| // message. Check stderr and stdout from the error. | ||
| const combined = `${err.stdout || ""}${err.stderr || ""}`; | ||
| if (combined && impl.authProbe.successPattern?.test(combined)) return true; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report a failed authentication probe as null, not false.
The docstring on lines 274-276 states that the function returns null when the check failed. skills/delegate-config/SKILL.md line 53 also instructs the reader to treat authenticated: null as unknown. The catch block returns false for every failure, including a probe timeout and a JSON.parse error. Discovery then reports an authenticated CLI as unauthenticated.
Line 291 also dereferences impl.authProbe.successPattern without a guard, while line 296 uses optional chaining. A future probe entry with neither jsonField nor successPattern throws a TypeError inside the try, and the catch converts it to false.
🔧 Proposed fix
if (impl.authProbe.jsonField) {
- return JSON.parse(raw)[impl.authProbe.jsonField] === true;
+ try {
+ return JSON.parse(raw)[impl.authProbe.jsonField] === true;
+ } catch {
+ return null; // unparseable output: unknown, not unauthenticated
+ }
}
- return impl.authProbe.successPattern.test(raw);
+ return impl.authProbe.successPattern?.test(raw) ?? null;
} catch (err) {
// Some CLIs exit non-zero when not authenticated but still print a
// message. Check stderr and stdout from the error.
const combined = `${err.stdout || ""}${err.stderr || ""}`;
if (combined && impl.authProbe.successPattern?.test(combined)) return true;
- return false;
+ // A timeout or a spawn failure proves nothing about authentication.
+ if (err.killed || err.code === "ETIMEDOUT" || !combined) return null;
+ return false;
}🤖 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 `@skills/delegate-config/scripts/discover.mjs` around lines 277 - 299, Update
probeAuth so probe failures—including execution errors, timeouts, JSON.parse
errors, and missing successPattern configuration—return null rather than false.
Guard successPattern before calling test, while preserving true for matching
output and false only for a successful probe that positively determines the CLI
is unauthenticated.
| All settings are optional. Use duration strings such as `90s`, `30m`, or `2h`. For version 1 | ||
| compatibility, positive integer timeouts are interpreted as seconds. Qoder's version 1 `sandbox` | ||
| field is read as `permissionMode`; new configuration uses `permissionMode`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the scope of the integer timeout rule.
The text limits integer timeouts to version 1 compatibility. The relays convert a positive integer timeout to seconds in every layer, including version 2 defaults and routes. The smoke tests rely on this: test/relay-smoke.mjs lines 516-522 and 568 put timeout: 5 inside a version 2 route and expect a successful run with timeout recorded as 5s.
📝 Proposed wording
-All settings are optional. Use duration strings such as `90s`, `30m`, or `2h`. For version 1
-compatibility, positive integer timeouts are interpreted as seconds. Qoder's version 1 `sandbox`
-field is read as `permissionMode`; new configuration uses `permissionMode`.
+All settings are optional. Use duration strings such as `90s`, `30m`, or `2h`. A positive integer
+timeout is interpreted as seconds in any version. Qoder's version 1 `sandbox` field is read as
+`permissionMode`; new configuration uses `permissionMode`.📝 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.
| All settings are optional. Use duration strings such as `90s`, `30m`, or `2h`. For version 1 | |
| compatibility, positive integer timeouts are interpreted as seconds. Qoder's version 1 `sandbox` | |
| field is read as `permissionMode`; new configuration uses `permissionMode`. | |
| All settings are optional. Use duration strings such as `90s`, `30m`, or `2h`. A positive integer | |
| timeout is interpreted as seconds in any version. Qoder's version 1 `sandbox` field is read as | |
| `permissionMode`; new configuration uses `permissionMode`. |
🧰 Tools
🪛 SkillSpector (2.4.4)
[warning] 19: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 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 `@skills/delegate-config/SKILL.md` around lines 147 - 149, Update the timeout
documentation near the version compatibility note to state that positive integer
timeout values are interpreted as seconds in all configuration layers, including
version 2 defaults and routes; keep the separate version 1
sandbox-to-permissionMode compatibility guidance unchanged.
|
|
||
| const AUTONOMY_MODES = new Set(["workspace-write", "read-only", "full-access"]); | ||
| const SAFE_ROUTE = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; | ||
| const CONFIG_FIELDS = new Set(["model", "sandbox", "effort", "timeout", "readOnly"]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how the sandbox config field values are documented and proposed per implementer.
set -euo pipefail
fd -t f 'SKILL.md' skills/delegate-config --exec rg -n -C4 'sandbox|readOnly|autonomy|full-access'
fd -t f 'discover.mjs' skills --exec rg -n -C4 'sandbox|autonomy|full-access|danger-full-access'
rg -n --glob '**/relay.mjs' -C2 'AUTONOMY_MODES|SANDBOX_MODES\s*='Repository: amElnagdy/delegate-skills
Length of output: 3128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate files ---\n'
fd -t f 'relay.mjs|SKILL.md|discover.mjs|delegate-config.json' skills/delegate-config skills | sort
printf '\n--- grok relay autonomy/sandbox references ---\n'
fd -t f 'relay.mjs' skills/grok-delegate --exec rg -n -C6 'CONFIG_FIELDS|AUTONOMY_MODES|sandbox|autonomy|readOnly' {}
printf '\n--- codex relay autonomy/sandbox references ---\n'
fd -t f 'relay.mjs' skills/codex-delegate --exec rg -n -C6 'CONFIG_FIELDS|AUTONOMY_MODES|sandbox|autonomy|readOnly' {}
printf '\n--- delegate-config SKILL.md surrounding doc ---\n'
fd -t f 'SKILL.md' skills/delegate-config --exec sed -n '1,240p' {}
printf '\n--- discover.mjs supports block / no sandbox value docs ---\n'
fd -t f 'discover.mjs' skills/delegate-config --exec sed -n '1,190p' {}
printf '\n--- shared config files around sandbox values ---\n'
sed -n '100,220p' skills/delegate-config/delegate-config.json 2>/dev/null || true
rg -n -C4 'workspace-write|danger-full-access|full-access|autonomy|sandbox' skills/delegate-config.json skills/delegate-config 2>/dev/null || trueRepository: amElnagdy/delegate-skills
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate files ---'
fd -t f 'relay.mjs|SKILL.md|discover.mjs|delegate-config.json' skills/delegate-config skills | sort
echo
echo '--- grok relay autonomy/sandbox references ---'
fd -t f 'relay.mjs' skills/grok-delegate --exec rg -n -C6 'CONFIG_FIELDS|AUTONOMY_MODES|sandbox|autonomy|readOnly' {}
echo
echo '--- codex relay autonomy/sandbox references ---'
fd -t f 'relay.mjs' skills/codex-delegate --exec rg -n -C6 'CONFIG_FIELDS|AUTONOMY_MODES|sandbox|autonomy|readOnly' {}
echo
echo '--- delegate-config SKILL.md surrounding doc ---'
fd -t f 'SKILL.md' skills/delegate-config --exec sed -n '1,240p' {}
echo
echo '--- discover.mjs supports block / sandbox value docs ---'
fd -t f 'discover.mjs' skills/delegate-config --exec sed -n '1,190p' {}
echo
echo '--- shared config files around sandbox values ---'
if [ -f skills/delegate-config/delegate-config.json ]; then
sed -n '100,220p' skills/delegate-config/delegate-config.json
fi
rg -n -C4 'workspace-write|danger-full-access|full-access|autonomy|sandbox' skills/delegate-config.json skills/delegate-config 2>/dev/null || trueRepository: amElnagdy/delegate-skills
Length of output: 41106
Document the shared sandbox field values per implementer.
delegate-config docs/config suggest copying keys between implementers, but grok accepts workspace-write, read-only, and full-access, while codex accepts read-only, workspace-write, and danger-full-access. Update skills/delegate-config/SKILL.md and the discover.mjs reference table so users do not use danger-full-access with grok or full-access with codex.
🤖 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 `@skills/grok-delegate/scripts/relay.mjs` at line 88, Update the shared sandbox
configuration documentation in skills/delegate-config/SKILL.md and the
discover.mjs reference table to list valid sandbox values separately for grok
and codex: grok supports workspace-write, read-only, and full-access, while
codex supports read-only, workspace-write, and danger-full-access. Make the
guidance explicit so users do not copy implementer-specific values between them.
| if (opts.autonomy === null) { | ||
| if (config.readOnly === true) opts.autonomy = "read-only"; | ||
| else if (config.sandbox !== undefined) opts.autonomy = config.sandbox; | ||
| else opts.autonomy = "workspace-write"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
readOnly from a lower layer overrides sandbox from a higher layer.
Line 237 checks config.readOnly before config.sandbox. The merged resolved map loses layer provenance, so a project route that sets sandbox cannot override readOnly: true from global defaults. The documented precedence gives the project route priority per field. The same pattern exists in skills/codex-delegate/scripts/relay.mjs.
🤖 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 `@skills/grok-delegate/scripts/relay.mjs` around lines 236 - 240, Update the
autonomy resolution logic in relay.mjs so the higher-priority project route’s
sandbox setting takes precedence over a lower-layer readOnly value, preserving
per-field layer precedence rather than relying on the merged config order. Apply
the same correction to the corresponding autonomy resolution block in
codex-delegate’s relay.mjs.
| if (opts.timeout === null) opts.timeout = DEFAULT_TIMEOUT; | ||
| opts.modelSource = modelFromFlag ? "flag" : delegateConfig.modelSource; | ||
| opts.addDirs = opts.addDirs.map((dir) => resolve(opts.cd, dir)); | ||
| // The watchdog is relay-only (kimi has no timeout flag), so a malformed | ||
| // --timeout must fail loudly here - a silent 30m fallback would be wrong. | ||
| if (parseDuration(opts.timeout) === null) { | ||
| if (opts.timeout !== null && parseDuration(opts.timeout) === null) { | ||
| fail(`--timeout "${opts.timeout}" is not a duration; use h/m/s strings like 30m, 90s, or 1h30m`); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare parseDuration and timeout validation across all relays.
set -euo pipefail
fd -t f 'relay\.mjs$' skills | while IFS= read -r file; do
echo "===== $file"
ast-grep run --pattern $'function parseDuration($$$) { $$$ }' --lang javascript "$file" || true
rg -n 'MAX_TIMER_MS|parseDuration\(opts\.timeout\)|must be greater than zero' "$file" || true
doneRepository: amElnagdy/delegate-skills
Length of output: 11083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== skills/kimi-relay relevant sections"
sed -n '180,215p' skills/kimi-delegate/scripts/relay.mjs
sed -n '120,145p' skills/kimi-delegate/scripts/relay.mjs
sed -n '360,400p' skills/kimi-delegate/scripts/relay.mjs
echo "===== Node timing probe for clamp and edge cases"
node - <<'JS'
for (const value of [0, 1, 2147483647, 2147483648]) {
let fired = false;
const id = setTimeout(() => { fired = true; }, value);
setTimeout(() => {
console.log(value, "fired immediately:", typeof id === "number" && fired);
}, 2);
}
let last;
setTimeout(() => last = setTimeout(() => {}, Infinity), 2147483648);
setTimeout(() => {
console.log("Infinity after clamp value", Number.parseInt(last, 10) === 1);
}, 2);
JS
echo "===== Agent parseDuration zero behavior"
node - <<'JS'
const parseDuration = (duration) => {
const match = /^(?:(\d+)h)?(?:(\d+)m)?(?:(\d+)s)?$/.exec(duration);
if (!match || (!match[1] && !match[2] && !match[3])) return null;
return (Number(match[1] || 0) * 3600 + Number(match[2] || 0) * 60 + Number(match[3] || 0)) * 1000;
};
for (const input of ["0s", "0h0m0s", "0", "3601h"]) console.log(input, parseDuration(input));
JSRepository: amElnagdy/delegate-skills
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== files and sizes"
wc -l skills/kimi-delegate/scripts/relay.mjs
echo "===== parseDuration section"
sed -n '195,212p' skills/kimi-delegate/scripts/relay.mjs
echo "===== timeout validation section"
sed -n '128,143p' skills/kimi-delegate/scripts/relay.mjs
echo "===== call sites"
rg -n "parseDuration\(opts\.timeout\)|setTimeout\(|timeoutMs" skills/kimi-delegate/scripts/relay.mjs
echo "===== small Node probe"
node - <<'JS'
const values = [0, 1, 2147483647, 2147483648];
for (const value of values) {
let fired = false;
const id = setTimeout(() => { fired = true; }, value);
setTimeout(() => {
console.log(String(value), "fired immediately:", typeof id === "number" && fired, "id:", id);
}, 3);
}
JSRepository: amElnagdy/delegate-skills
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== files ===\n'
git ls-files skills/kimi-delegate/scripts/relay.mjs | xargs -r -n1 wc -l
printf '=== parseDuration ===\n'
sed -n '195,212p' skills/kimi-delegate/scripts/relay.mjs
printf '=== timeout handling ===\n'
sed -n '128,143p' skills/kimi-delegate/scripts/relay.mjs
printf '=== call sites ===\n'
rg -n "parseDuration\(opts\.timeout\)|setTimeout\(|timeoutMs" skills/kimi-delegate/scripts/relay.mjsRepository: amElnagdy/delegate-skills
Length of output: 2424
Reject zero and over-limit timeout values before passing them to setTimeout.
parseDuration("0s") returns 0, and any parsed duration above the 32-bit timer limit is clamped to 1. Since route config can set opts.timeout, timeout: "0s" sends the watchdog to setTimeout(..., 0) and creates an immediate failure instead of rejecting the value. Use the same bounds as agy-delegate/scripts/relay.mjs: reject 0/negative values and values above MAX_TIMER_MS = 2_147_483_647.
🤖 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 `@skills/kimi-delegate/scripts/relay.mjs` around lines 134 - 141, Update the
timeout validation in the relay options flow before the watchdog calls
setTimeout: parse opts.timeout once, reject null, zero, and negative durations,
and reject values above MAX_TIMER_MS = 2_147_483_647. Reuse the same bound and
validation behavior as agy-delegate/scripts/relay.mjs while preserving the
existing malformed-duration failure message.
| if (opts.model !== null && (typeof opts.model !== "string" || !opts.model.trim())) { | ||
| fail("--model must be a non-empty string"); | ||
| } | ||
| const modelFromFlag = opts.model !== null; | ||
| const delegateConfig = loadDelegateConfig(opts.cd, opts.route); | ||
| const config = delegateConfig.values; | ||
| if (opts.model === null && config.model !== undefined) opts.model = config.model; | ||
| if (opts.timeout === null && config.timeout !== undefined) opts.timeout = config.timeout; | ||
| if (opts.permissionMode === null) { | ||
| if (config.readOnly === true) opts.permissionMode = "plan"; | ||
| else if (config.permissionMode !== undefined) opts.permissionMode = config.permissionMode; | ||
| else if (config.sandbox !== undefined) opts.permissionMode = config.sandbox; | ||
| else opts.permissionMode = "auto"; | ||
| } | ||
| if (opts.timeout === null) opts.timeout = DEFAULT_TIMEOUT; | ||
| opts.modelSource = modelFromFlag ? "flag" : delegateConfig.modelSource; | ||
| if (config.readOnly !== undefined && typeof config.readOnly !== "boolean") { | ||
| fail("delegate config qodercli.readOnly must be a boolean"); | ||
| } | ||
| if (opts.contextWindow !== null && !/^[1-9]\d*$/.test(opts.contextWindow)) { | ||
| fail("--context-window must be a positive integer"); | ||
| } | ||
| if (!PERMISSION_MODES.has(opts.permissionMode)) { | ||
| if (opts.permissionMode !== null && !PERMISSION_MODES.has(opts.permissionMode)) { | ||
| fail(`unsupported --permission-mode: ${opts.permissionMode}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Resolve the model before validating it, and drop the unreachable config guards.
Three points in this block:
- Line 131 validates
opts.modelbefore line 137 assigns the configured model. A configuredmodel: ""passesloadDelegateConfigas a string, reaches line 137, and thenbuildArgvskips--modelbecause the value is falsy. The run proceeds on the CLI default with no error.skills/agy-delegate/scripts/relay.mjsline 249 andskills/kimi-delegate/scripts/relay.mjsline 142 run this check after configuration resolution. Move the check to keep the behavior identical across relays. - Line 142 is unreachable.
configLayersrenames a version 1sandboxfield topermissionModeand deletessandbox, andCONFIG_FIELDSrejectssandboxin version 2, soconfig.sandboxis alwaysundefined. - Lines 147-149 are also unreachable.
BOOLEAN_CONFIG_FIELDScontainsreadOnly, soloadDelegateConfigalready fails on a non-boolean value with a path-qualified message.
Line 154 reports a configured value as --permission-mode. Name the configuration source in that message so the user can find the wrong value.
♻️ Proposed change
- if (opts.model !== null && (typeof opts.model !== "string" || !opts.model.trim())) {
- fail("--model must be a non-empty string");
- }
const modelFromFlag = opts.model !== null;
const delegateConfig = loadDelegateConfig(opts.cd, opts.route);
const config = delegateConfig.values;
if (opts.model === null && config.model !== undefined) opts.model = config.model;
if (opts.timeout === null && config.timeout !== undefined) opts.timeout = config.timeout;
if (opts.permissionMode === null) {
if (config.readOnly === true) opts.permissionMode = "plan";
else if (config.permissionMode !== undefined) opts.permissionMode = config.permissionMode;
- else if (config.sandbox !== undefined) opts.permissionMode = config.sandbox;
else opts.permissionMode = "auto";
}
if (opts.timeout === null) opts.timeout = DEFAULT_TIMEOUT;
opts.modelSource = modelFromFlag ? "flag" : delegateConfig.modelSource;
- if (config.readOnly !== undefined && typeof config.readOnly !== "boolean") {
- fail("delegate config qodercli.readOnly must be a boolean");
- }
+ if (opts.model !== null && (typeof opts.model !== "string" || !opts.model.trim())) {
+ fail(modelFromFlag ? "--model must be a non-empty string" : "delegate config qodercli model must be a non-empty string");
+ }
if (opts.contextWindow !== null && !/^[1-9]\d*$/.test(opts.contextWindow)) {
fail("--context-window must be a positive integer");
}
if (opts.permissionMode !== null && !PERMISSION_MODES.has(opts.permissionMode)) {
- fail(`unsupported --permission-mode: ${opts.permissionMode}`);
+ fail(`unsupported permission mode: ${opts.permissionMode}`);
}🤖 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 `@skills/qoder-delegate/scripts/relay.mjs` around lines 131 - 155, Resolve
configured values before validating opts.model by moving the non-empty model
check after delegateConfig/model assignment, matching the agy-delegate and
kimi-delegate relay behavior. In the permissionMode fallback, remove the
obsolete config.sandbox branch and the redundant readOnly type guard, relying on
loadDelegateConfig validation. Update the unsupported permission-mode error to
identify that the invalid value came from delegate configuration rather than
labeling it as a CLI flag.
|
I REALLY like the idea. However, I need to think for a bit if adding another skill is the best option here. |
I am using delegate skills in my real enterprise projects and facing an issue with selecting models: I have to tell the main agent which model to use for each delegate task, especially when using spec-kit. This is why I implemented this idea to make life easier. awaiting someone to review and merge this PR. |
I really like the core of this — especially discovery, propose → approve → write, layered config, and keeping the setup skill out of the dispatch path. After thinking it through, I want a slightly different product cut than task-specific model routes inside an already-chosen implementer. What I’m optimizing for: a fleet — e.g. features → Codex, tests → Grok, UI → Kimi — not only “Codex fast vs Codex high-effort.” Model/effort dials still matter, but implementer selection per lane is the headline. Direction I’ll pursue (in a separate PR):
So I’m going to land this as a new PR centered on setup + fleet lanes, rather than reshape this branch. I’ll borrow the setup UX instincts from here (discovery, approve-before-write, layered config). Thanks for pushing on this — it unblocked the right discussion. |
2 participants
What changed
delegate-configutility skill for discovering installed implementers, authentication state, supported settings, and reported modelsfrontend,backend,fast,medium, andcomplex--routesupport, strict config validation, layered precedence, and route metadata across all ten relaysWhy
The previous single-model configuration could not select different models or effort levels for different task types. Configuration was also merged after some relay defaults were applied, which allowed defaults to override explicit intent, and malformed config values could fail later as child-process errors.
This change lets the orchestrator propose a task-specific configuration for user approval, while keeping explicit relay flags authoritative and rejecting invalid configuration before dispatch.
User and developer impact
Users can define reusable model routes and select one with
--route <name>. Project configuration overrides global configuration per field, and every result records the selected route, model source, and effective timeout for review. Existing version 1 configuration remains readable.The configuration flow presents proposed JSON before writing and requires explicit user approval or adjustment.
Validation
node test/relay-smoke.mjsnpx skills add . --listgit diff --checkNative Windows launch smoke was not run in this environment; no new Windows support claim is made.
Summary by CodeRabbit
New Features
Documentation
Tests