fix(subagent): sanitize id_prefix to prevent assertSafeJobId rejection (#1319) - #1350
fix(subagent): sanitize id_prefix to prevent assertSafeJobId rejection (#1319)#1350griffinwork40 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d5a0316e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const safePrefix = (options.idPrefix ?? 'subagent').replace(/[^A-Za-z0-9_-]/g, '-'); | ||
| const id = `${safePrefix}-${Date.now()}-${counter}`; |
There was a problem hiding this comment.
When the model supplies a long id_prefix (the agent tool schema has no maxLength), replacing invalid characters does not prevent the generated ID from exceeding assertSafeJobId's 128-character limit; for example, a 128-character prefix produces a roughly 144-character ID. With subagent logging enabled, SubagentLogWriter still throws during fork creation, so the stated failure remains possible. Truncate the sanitized prefix to leave room for the timestamp, separators, and counter.
Useful? React with 👍 / 👎.
griffinwork40
left a comment
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (scheduled sweep), generated by the /review tool — a maintainer will follow up.
Prior Findings (Round 1)
No prior automated rounds on this PR — this is round 1.
Review: PR #1350 — fix(subagent): sanitize id_prefix to prevent assertSafeJobId rejection
Reviewed ref: 7d5a0316e806bc715e3763e9b7a739a84e5f0bfe
Change type: hotfix | Regime: light (2 files, ~38 lines)
CI: ✅ All checks pass (Lint & Build, Test ubuntu/macos, PTY scrollback, Docs build, Publish bundle smoke)
Decision: MERGE (with follow-ups)
Severity arithmetic: 0 critical, 0 high → MERGE (with follow-ups); 3 medium advisory (non-blocking per severity floor), 2 low advisory.
The fix correctly sanitizes the charset path: .replace(/[^A-Za-z0-9_-]/g, '-') aligns exactly with BG_JOB_ID_PATTERN in paths.ts. No critical or high findings — safe to merge.
Advisory Findings (non-blocking — suggest follow-up issues)
MEDIUM — Correctness (advisory, non-blocking)
Overlong idPrefix still triggers assertSafeJobId length rejection
src/agent/subagent/fork-resolution.ts:95–96 · file-state · ref 7d5a0316
The fix sanitizes charset but not length. A model-supplied idPrefix ≥ 114 characters (all valid [A-Za-z0-9_-]) produces an assembled id exceeding BG_JOB_ID_MAX_LEN = 128 (safePrefix + '-' + Date.now() (13 digits) + '-' + counter), triggering the same assertSafeJobId throw the PR claims to fix. In practice, models send short prefixes ("research-agent", "verify-fix"), so this is a theoretical gap rather than a production bug — but it leaves the stated invariant ("prevent assertSafeJobId rejection") incomplete.
const safePrefix = (options.idPrefix ?? 'subagent').replace(/[^A-Za-z0-9_-]/g, '-');
const id = `${safePrefix}-${Date.now()}-${counter}`;
// paths.ts:594: if (jobId.length > BG_JOB_ID_MAX_LEN) throw 'exceeds 128 chars'Follow-up suggestion: const safePrefix = (...).replace(...).slice(0, 100); — leaves 15+ chars for timestamp and counter within the 128-char ceiling.
MEDIUM — Spec Compliance (advisory, non-blocking)
Stated invariant "prevent assertSafeJobId rejection" is only half-met
src/agent/subagent/fork-resolution.ts:95–96 · file-state · ref 7d5a0316
assertSafeJobId has three rejection paths (empty, overlong, bad charset). This PR fixes the charset path but not the length path. The PR title says "prevent assertSafeJobId rejection" — which is only partially achieved.
// paths.ts:591–597 — three distinct rejection conditions:
if (typeof jobId !== 'string' || jobId.length === 0) throw ... // not relevant
if (jobId.length > BG_JOB_ID_MAX_LEN) throw ... // NOT fixed
if (!BG_JOB_ID_PATTERN.test(jobId)) throw ... // fixed by PRSame fix as above addresses this.
MEDIUM — Test Coverage (advisory, non-blocking)
No test for overlong-prefix path
src/agent/subagent/fork-resolution.test.ts · file-state · ref 7d5a0316
No test covers an idPrefix ≥ 114 characters, leaving the length-overflow regression path uncovered. Verified: grep -n '128|MAX_LEN|truncat|slice|length' across the test file returns zero matches.
Follow-up suggestion: Add idPrefix: 'a'.repeat(120) → assert resolveForkInputs(args).id.length <= 128 (or that downstream getSubagentLogPath does not throw).
LOW — Correctness (advisory)
All-disallowed prefix collapses to pure hyphens with no fallback
src/agent/subagent/fork-resolution.ts:95 · file-state · ref 7d5a0316
When id_prefix consists entirely of disallowed characters (e.g. '...'), the sanitized prefix is all hyphens ('---'), producing an id like ----1724843521234-1 — valid per assertSafeJobId but provides zero diagnostic signal. A degenerate-case fallback to 'subagent' would preserve debuggability.
LOW — Test Coverage (advisory)
New tests assert prefix shape but not full-id charset conformance
src/agent/subagent/fork-resolution.test.ts:343,356 · diff-context · ref 7d5a0316
The two new tests assert .toMatch(/^research-agent-/) (prefix shape) but do not assert the full assembled id matches BG_JOB_ID_PATTERN (/^[A-Za-z0-9_-]+$/). Adding expect(id).toMatch(/^[A-Za-z0-9_-]+$/) alongside the prefix assertion would catch regressions that insert forbidden characters after the prefix.
What Was Not Checked
- Citations verified inline against branch HEAD
7d5a0316e806bc715e3763e9b7a739a84e5f0bfe. All findings verified against file state at the reviewed ref. - Stated intent: PR #1350 title + body — spec-compliance assessed.
- Did not run the test suite locally (CI shows all GitHub Actions checks green).
- Security: no surface — pure synchronous
.replace()on a string. The sanitizer actively improves path-traversal safety. - API-compat:
resolveForkInputsis not a public export;ForkResolved.idtype unchanged.
1 participant
Fixes #1319.
Model-supplied
id_prefixvalues containing characters outside[A-Za-z0-9_-](dots, spaces, etc.) causedassertSafeJobIdto throw from the log writer, aborting the entire fork. Now sanitized at the source.Changes
src/agent/subagent/fork-resolution.ts-- sanitizeidPrefixvia.replace(/[^A-Za-z0-9_-]/g, '-')before interpolating into fork idsrc/agent/subagent/fork-resolution.test.ts-- 2 test cases for dot and space sanitizationVerification
pnpm lintclean