Classify unsupported Codex custom tools as invalid-model failures - #57578
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has <=100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
🟡 Changes recommended
The regex does not match the escaped nested JSON in the actual Codex JSONL payload.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends unsupported-model detection for Codex custom-tool API rejections reported in #57572.
Changes:
- Adds a custom-tools rejection signature.
- Adds ordering and object-boundary regression tests.
File summaries
| File | Description |
|---|---|
actions/setup/js/detect_agent_errors.cjs |
Extends model-error classification. |
actions/setup/js/detect_agent_errors.test.cjs |
Tests the new signature. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| // support custom tools. | ||
| const MODEL_NOT_SUPPORTED_PATTERN = | ||
| /(?:The requested model is not supported|invalid model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|unknown model\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?\s+(?:is\s+)?(?:not found|does not exist|not supported|not available|unavailable)|404\b[^\n]*\bModel\s+not\s+found|No model available\b[^\n]*policy enablement)/i; | ||
| /(?:The requested model is not supported|invalid model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|unknown model\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?\s+(?:is\s+)?(?:not found|does not exist|not supported|not available|unavailable)|404\b[^\n]*\bModel\s+not\s+found|No model available\b[^\n]*policy enablement|(?:["']message["']\s*:\s*["']Invalid value:\s*['"`]custom['"`][^{}]{0,300}?["']param["']\s*:\s*["']tools["']|["']param["']\s*:\s*["']tools["'][^{}]{0,300}?["']message["']\s*:\s*["']Invalid value:\s*['"`]custom['"`]))/i; |
There was a problem hiding this comment.
Fixed in b920635. The detector now parses the raw turn.failed JSONL event and unwraps the escaped nested provider JSON structurally; the regression test uses the exact escaped event shape.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new Codex unsupported-model handling is pointed at the right failure mode, but the implementation is still too brittle and the regression coverage does not fully lock down the promised classification behavior.
The blocking themes
- The detector duplicates Codex error parsing as a regex over serialized JSON instead of reusing structural parsing, so harmless provider/SDK serialization changes can silently drop this back into the generic engine-failure path.
- The reordered-fields regression test only checks the regex match, not
detectErrors(...), leaving the actual invalid-model classification path under-tested.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 35.6 AIC · ⌖ 7.47 AIC · ⊞ 21.8K
Comment /review to run again
| // support custom tools. | ||
| const MODEL_NOT_SUPPORTED_PATTERN = | ||
| /(?:The requested model is not supported|invalid model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|unknown model\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?\s+(?:is\s+)?(?:not found|does not exist|not supported|not available|unavailable)|404\b[^\n]*\bModel\s+not\s+found|No model available\b[^\n]*policy enablement)/i; | ||
| /(?:The requested model is not supported|invalid model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|unknown model\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?\s+(?:is\s+)?(?:not found|does not exist|not supported|not available|unavailable)|404\b[^\n]*\bModel\s+not\s+found|No model available\b[^\n]*policy enablement|(?:["']message["']\s*:\s*["']Invalid value:\s*['"`]custom['"`][^{}]{0,300}?["']param["']\s*:\s*["']tools["']|["']param["']\s*:\s*["']tools["'][^{}]{0,300}?["']message["']\s*:\s*["']Invalid value:\s*['"`]custom['"`]))/i; |
There was a problem hiding this comment.
The new detector hard-codes a very narrow JSON text shape for the Codex tools: custom rejection. We already have extractNestedProviderErrorDetails() / isUnsupportedModelToolsError() that parse nested provider payloads structurally, and those tests cover envelopes like metadata.raw; this regex will miss the same error as soon as the SDK emits a slightly different serialization or escapes the JSON differently, which means the workflow falls back to the generic engine-failure path again.
💡 Reuse the structural parser instead of matching serialized JSON text
Rather than baking more JSON-shape assumptions into MODEL_NOT_SUPPORTED_PATTERN, add a helper in detect_agent_errors.cjs that walks parsed JSON objects the same way codex_harness.cjs does and treats message == "Invalid value: 'custom'" plus param == "tools" in the same nested provider object as unsupported-model. That keeps this classifier aligned with the Codex harness, handles field reordering / wrapper changes, and avoids another round of brittle regex maintenance.
function isUnsupportedModelToolsModelError(output) {
// parse each JSON event/object line, unwrap nested payloads, then check
// message === "Invalid value: 'custom'" && param === "tools".
}
modelNotSupportedError:
MODEL_NOT_SUPPORTED_PATTERN.test(logContent) || isUnsupportedModelToolsModelError(logContent)There was a problem hiding this comment.
Fixed in b920635. The serialized-JSON regex was removed and replaced with bounded structural traversal of error, nested message JSON, and metadata.raw payloads.
| "message": "Invalid value: 'custom'" | ||
| } | ||
| }`; | ||
| expect(MODEL_NOT_SUPPORTED_PATTERN.test(errorOutput)).toBe(true); |
There was a problem hiding this comment.
This reordered-payload test only exercises MODEL_NOT_SUPPORTED_PATTERN.test(...), so it can still pass if detectErrors(...) stops wiring that match into modelNotSupportedError. The previous case checks end-to-end classification for the canonical ordering, but this one leaves the exact regression you are adding here partially unguarded.
💡 Assert the detector output, not just the regex
Add the same end-to-end expectation you used in the preceding test so the regression suite fails if classification ever diverges from the regex match for reordered fields.
expect(MODEL_NOT_SUPPORTED_PATTERN.test(errorOutput)).toBe(true);
expect(detectErrors(errorOutput).modelNotSupportedError).toBe(true);That keeps the test focused on the behavior the PR description promises: routing this payload through the invalid-model path.
There was a problem hiding this comment.
Fixed in b920635. The reordered raw payload now asserts detectErrors(...).modelNotSupportedError end to end.
There was a problem hiding this comment.
Review: Classify unsupported Codex custom tools as invalid-model failures
Change type: bug_fix — applied harden + audit modes.
The change is well-scoped and ships three targeted tests covering the happy path, field-order variation, and the cross-object false-positive guard. One non-blocking reliability note posted inline.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 20.3 AIC · ⌖ 14.6 AIC · ⊞ 6.2K
| // driver, typically raised when a subagent/`task` dispatch requests a model that is disabled | ||
| // by the org/repo Copilot policy). Anchored to the "policy enablement" phrase so that the | ||
| // generic "No model available" wording alone does not produce false positives. | ||
| // - Codex API rejects the `custom` tools value with `param: "tools"` for models that do not |
There was a problem hiding this comment.
The [^{}]{0,300}? span limit guards against cross-object matches, but 300 characters may be too tight if the API returns an error object with a long details, description, or localized message field between the two required keys. Consider raising the cap (e.g. {0,1000}) or adding an inline comment explaining why 300 was chosen, so future maintainers understand the deliberate trade-off.
@copilot please address this.
There was a problem hiding this comment.
Obsolete after b920635: the capped regex span was removed in favor of structural JSON parsing.
There was a problem hiding this comment.
- actions/setup/js/detect_agent_errors.cjs:160: shrink: the two field-order branches in the new Codex regex are duplicated. Replace them with a small helper that checks both substrings once instead of a giant alternation.
- actions/setup/js/detect_agent_errors.test.cjs:163: delete: three new tests cover the same Codex error shape with only field-order and object-boundary differences. One happy-path test plus one reordered-field case is enough.
net: -10 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57578 · codex · mai10 · 4.15 AIC · ⌖ 0.38 AIC · ⊞ 13.5K
Comment /ponytail to run again
| // support custom tools. | ||
| const MODEL_NOT_SUPPORTED_PATTERN = | ||
| /(?:The requested model is not supported|invalid model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|unknown model\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?\s+(?:is\s+)?(?:not found|does not exist|not supported|not available|unavailable)|404\b[^\n]*\bModel\s+not\s+found|No model available\b[^\n]*policy enablement)/i; | ||
| /(?:The requested model is not supported|invalid model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|unknown model\s+['"`]?[a-z0-9._:/@-]+['"`]?(?=(?:\s*$|\s*[\n\r.,;:!?)]))|model(?:\s+name)?\s+['"`]?[a-z0-9._:/@-]+['"`]?\s+(?:is\s+)?(?:not found|does not exist|not supported|not available|unavailable)|404\b[^\n]*\bModel\s+not\s+found|No model available\b[^\n]*policy enablement|(?:["']message["']\s*:\s*["']Invalid value:\s*['"`]custom['"`][^{}]{0,300}?["']param["']\s*:\s*["']tools["']|["']param["']\s*:\s*["']tools["'][^{}]{0,300}?["']message["']\s*:\s*["']Invalid value:\s*['"`]custom['"`]))/i; |
There was a problem hiding this comment.
actions/setup/js/detect_agent_errors.cjs:160: shrink: the two field-order branches in the new Codex regex are duplicated. Replace them with a small helper that checks both substrings once instead of a giant alternation.
There was a problem hiding this comment.
Fixed in b920635. The duplicated regex branches were removed and replaced with a structural helper.
| expect(MODEL_NOT_SUPPORTED_PATTERN.test(errorOutput)).toBe(true); | ||
| }); | ||
|
|
||
| it("matches the Codex custom-tools rejection from unsupported models", () => { |
There was a problem hiding this comment.
actions/setup/js/detect_agent_errors.test.cjs:163: delete: three new tests cover the same Codex error shape with only field-order and object-boundary differences. One happy-path test plus one reordered-field case is enough.
There was a problem hiding this comment.
Addressed in b920635. Coverage now focuses on the raw escaped event, reordered fields, and the required same-object negative guard.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor suggestions, no blockers.
📋 Key Themes & Highlights
Key Themes
- Regex boundary conservatism: The
{0,300}cap is functionally correct for the known payload but could silently miss future Codex errors with additional fields. - Test completeness: The field-reorder test only asserts the raw pattern; the end-to-end
detectErrors()pipeline assertion is missing.
Positive Highlights
- ✅ Root cause properly addressed — JSON-object boundary guard (
[^{}]) elegantly prevents cross-object false positives. - ✅ Both field orderings covered by the implementation and tests.
- ✅ Negative test case (separate objects) is the right regression guard.
- ✅ PR description clearly maps the Codex API payload to the classification path.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 32.4 AIC · ⌖ 15.1 AIC · ⊞ 7.6K
Comment /matt to run again
| // driver, typically raised when a subagent/`task` dispatch requests a model that is disabled | ||
| // by the org/repo Copilot policy). Anchored to the "policy enablement" phrase so that the | ||
| // generic "No model available" wording alone does not produce false positives. | ||
| // - Codex API rejects the `custom` tools value with `param: "tools"` for models that do not |
There was a problem hiding this comment.
[/diagnosing-bugs] The {0,300} character budget might silently miss real Codex payloads if the API ever adds more fields between message and param (e.g. a long details string). Consider raising the limit to {0,1000} — false negatives (missed classifications) are the risk here.
💡 Suggested change
// Before: [^{}]{0,300}?
// After: [^{}]{0,1000}?300 chars fits the current known payload (≈100 chars between the two fields) but is fragile if the error format grows. 1000 chars still prevents cross-object matching while being more future-proof.
@copilot please address this.
There was a problem hiding this comment.
Obsolete after b920635: structural parsing replaces the fixed character-budget regex.
| it("matches when the Codex error fields are reordered", () => { | ||
| const errorOutput = `{ | ||
| "error": { | ||
| "param": "tools", |
There was a problem hiding this comment.
[/tdd] The test for reversed field order doesn't call detectErrors() to confirm the full classification pipeline, unlike the first test. For completeness, add expect(detectErrors(errorOutput).modelNotSupportedError).toBe(true) so we verify the end-to-end path (not just the raw pattern).
💡 Suggested addition
it("matches when the Codex error fields are reordered", () => {
const errorOutput = `{ ... }`;
expect(MODEL_NOT_SUPPORTED_PATTERN.test(errorOutput)).toBe(true);
// Add this:
expect(detectErrors(errorOutput).modelNotSupportedError).toBe(true);
});@copilot please address this.
There was a problem hiding this comment.
Fixed in b920635. The reordered event now exercises the full detectErrors(...) classification path.
|
@copilot this PR is ready for the next forward-progress pass. Outstanding review feedback:
Please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot this PR is ready for the next forward-progress pass. Please refresh the branch state if needed, reconcile the remaining blocking reviews with the latest
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Reconciled against |
|
🎉 This pull request is included in a new release. Release: |
Daily Go Test Parallelizer reported a Codex
tools: customAPI rejection as an unexpected engine failure. This change routes that signature through the existing invalid-model failure reporting path.Codex error classification
Invalid value: 'custom'withparam: "tools"asmodel_not_supported_error.Regression coverage
{ "message": "Invalid value: 'custom'", "param": "tools" }Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.