Fix Codex threat-detection config.toml emitting model_provider under [history]#47832
Conversation
Co-authored-by: davidslater <12449447+davidslater@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes Codex external threat detection routing through the AWF API proxy by placing model_provider at the TOML document root.
Changes:
- Moves
model_providerbefore all TOML table headers. - Adds regression coverage for root-level placement.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/threat_detection_external.go |
Corrects Codex detector configuration ordering. |
pkg/workflow/threat_detection_isolation_test.go |
Verifies the selector remains outside [history]. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47832 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (51 additions, threshold 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-tested.
model_provideris moved to the document root before any table header — this is the right TOML fix.- The regression test robustly validates both that
model_providerprecedes[, and that[history]body does not containmodel_provider. - The inline comment clearly documents the ordering constraint for future maintainers.
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 11.5 AIC · ⌖ 4.47 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion.
📋 Key Themes & Highlights
Root Cause & Fix
The bug was precisely diagnosed: TOML assigns bare keys to the most-recently-opened table, so emitting model_provider after [history] silently bound it as history.model_provider, which Codex ignores. Moving it before any table header is the minimal, correct fix. The added code comment documenting the ordering constraint is exactly the right mitigation for future editors.
Test Coverage
The regression test covers the two relevant invariants (key precedes all table headers; [history] body contains no model_provider key). One minor hardening suggestion is left as an inline comment on the boundary-extraction logic.
Positive Highlights
- ✅ Root cause fixed, not just the symptom
- ✅ Regression test added alongside the fix
- ✅ Clear comment documents the TOML scoping constraint
- ✅ No new dependencies introduced; existing assertion style preserved
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 25.5 AIC · ⌖ 4.65 AIC · ⊞ 6.7K
Comment /matt to run again
| // model_provider key. Extract the [history] table body and assert. | ||
| historyIndex := strings.Index(toml, "[history]") | ||
| if historyIndex == -1 { | ||
| t.Fatalf("Expected [history] table in detection config, got:\n%s", toml) |
There was a problem hiding this comment.
[/tdd] The [history] body extraction searches for the next [ character, which could match a bracket inside a quoted string value (e.g. a base_url like `(host/redacted) Low risk today, but fragile as the config evolves.
💡 Suggested hardening
Scan for [ instead to avoid matching brackets inside quoted values:
if nextTable := strings.Index(historyBody[len("[history]"):], "
["); nextTable != -1 {
historyBody = historyBody[:len("[history]")+nextTable]
}The current assertion is correct for the existing config — this is a minor defensive improvement.
@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit. The historyBody is now built by a line-by-line scan that stops as soon as it encounters a line whose trimmed content starts with [, so [ inside quoted values or arrays can't cause premature truncation.
|
hello
|
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
✅ Test Quality Score: 80/100 — Excellent
Analyzed 1 test(s): 1 design, 0 implementation, 0 violation(s).
📊 Metrics (1 test)
| Metric | Value |
|---|---|
| Analyzed | 1 (Go: 1, JS: 0) |
| ✅ Design | 1 (100%) |
| 0 (0%) | |
| Edge/error coverage | 1 (100%) |
| Duplicate clusters | 0 |
| Inflation | YES — 44 test additions / 7 prod additions ≈ 6.3:1 (justified) |
| 🚨 Violations | 0 |
| Test | File | Classification | Issues |
|---|---|---|---|
TestExternalDetectorCodexConfigModelProviderAtRoot |
pkg/workflow/threat_detection_isolation_test.go:402 |
design_test / behavioral_contract | Test inflation (6.3:1) |
⚠️ Flagged Tests (1 — inflation only)
TestExternalDetectorCodexConfigModelProviderAtRoot (threat_detection_isolation_test.go:402) — test inflation (6.3:1). 44 test lines vs 7 production lines exceeds the 2:1 threshold. This is justified: the test encodes a multi-part TOML structural contract (positive root-placement assertion + negative regression guard ensuring [history] never absorbs model_provider) that requires indentation trimming setup and multiple assertions to fully verify the bug fix.
Verdict
✅ Passed. 0% implementation tests (threshold: 30%). No guideline violations.
TestExternalDetectorCodexConfigModelProviderAtRoot is a high-value design test that encodes the exact TOML ordering invariant fixed by this PR. It calls buildExternalDetectorCodexConfig directly, strips emitted indentation, and makes two orthogonal assertions: (1) model_provider must precede any [table] header, and (2) the [history] table body must not contain model_provider. This is the kind of invariant test that would have caught the original regression automatically.
There was a problem hiding this comment.
Review: Non-blocking issues in the regression test
The production fix in threat_detection_external.go is correct — moving model_provider before any TOML table header is the right call and the comment explaining the ordering constraint is helpful.
The regression test has four medium-severity issues that reduce its future reliability without breaking it today.
Issues found (all non-blocking)
- Hardcoded
"openai-proxy"string (line 415) — uses the literal value instead ofcodexOpenAIProxyProviderID; silent false-negative if the constant is renamed. - Magic 10-space indent (line 408) —
strings.TrimPrefixsilently becomes a no-op if the indentation in the production function changes. - Fragile
historyBodyslice (line 430) —strings.Index(..., "[")truncates on the first[in any value (arrays, inline strings), potentially hiding amodel_providerkey. firstTableIndexmatches[in comments (line 419) — should find the first line-starting[to avoid false matches from TOML comments.
None of these cause the test to pass incorrectly today, but together they make the test brittle under future config changes. The suggested fixes in the inline comments are straightforward.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 43.5 AIC · ⌖ 5.04 AIC · ⊞ 5.7K
Comment /review to run again
| // Trim the shared 10-space indentation so the config reads as plain TOML. | ||
| var trimmedLines []string | ||
| for _, line := range strings.Split(config, "\n") { | ||
| trimmedLines = append(trimmedLines, strings.TrimPrefix(line, " ")) |
There was a problem hiding this comment.
Magic indentation string is silently fragile: the 10-space trim prefix is a bare string literal. If the indentation in buildExternalDetectorCodexConfig ever changes (e.g. tabs, different count), strings.TrimPrefix becomes a no-op, all subsequent strings.Index calls operate on still-indented text, and the ordering assertions give wrong results without any obvious failure signal.
💡 Suggested fix
Extract the indent as a named constant or derive it from the actual output:
const codexConfigIndent = " " // 10 spaces, matches buildExternalDetectorCodexConfigOr make the test resilient by using strings.TrimLeft(line, " ") instead of TrimPrefix, so it strips whatever leading whitespace is present regardless of exact count.
There was a problem hiding this comment.
Fixed in the latest commit. The loop now uses strings.TrimLeft(line, " \t") so it strips any leading whitespace regardless of exact count or type, and the magic 10-space literal is gone. Also switched to strings.SplitSeq per the modernize lint rule.
| // model_provider key. Extract the [history] table body and assert. | ||
| historyIndex := strings.Index(toml, "[history]") | ||
| if historyIndex == -1 { | ||
| t.Fatalf("Expected [history] table in detection config, got:\n%s", toml) |
There was a problem hiding this comment.
historyBody slicing can prematurely truncate on [ in values or arrays: strings.Index(historyBody[len("[history]"):], "[") finds the first [ after the header, but TOML values can contain [ (e.g. array values like allowed = ["a", "b"]). This would truncate historyBody before the end of the [history] section, potentially hiding a model_provider key that appears after the array.
💡 Suggested fix
Scan line-by-line and stop at the next line that starts a new table header:
var historyLines []string
inHistory := false
for _, line := range strings.Split(toml, "
") {
if line == "[history]" {
inHistory = true
continue
}
if inHistory {
if strings.HasPrefix(line, "[") {
break
}
historyLines = append(historyLines, line)
}
}
historyBody := strings.Join(historyLines, "
")
if strings.Contains(historyBody, "model_provider") {
t.Errorf("[history] table must NOT contain a model_provider key, got:
%s", historyBody)
}There was a problem hiding this comment.
Fixed in the latest commit. The historyBody extraction now uses a line-by-line scan (via strings.SplitSeq) that stops at the next line beginning with [, so array literals like allowed = ["a", "b"] can't truncate the section prematurely.
|
@copilot please run the Unresolved review feedback:
Failed checks on the latest head: Run: https://github.com/github/gh-aw/actions/runs/30115096773
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ne scanning Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 PR Triage
Rationale: Fixes a critical TOML config ordering bug that causes Codex threat-detection to use the wrong model provider ( Next step: CI green → human review → merge.
|
|
@copilot please run the Unresolved review feedback:
Run: https://github.com/github/gh-aw/actions/runs/30120416243
|
The branch is up to date. All actionable items from the unresolved review (
The 3 outdated threads will appear collapsed in the UI. The 2 non-outdated threads have been replied to; they remain technically unresolved because that requires human action in the GitHub UI. CI must be re-triggered by a maintainer (close/reopen PR, |
|
@copilot refresh wasm golden |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Updated the |
Branch is up to date (merge commit Review threads status:
CI:
Hand-off: A maintainer needs to (1) resolve the 3 outdated review threads in the GitHub UI, and (2) re-trigger CI (close/reopen PR or push) before merge. |
|
🎉 This pull request is included in a new release. Release: |
The Codex external detector generated a
config.tomlwhere the top-levelmodel_provider = "openai-proxy"selector was emitted after the[history]table header. TOML then binds it ashistory.model_provider, which Codex ignores — falling back to the defaultopenaiprovider, bypassing the AWF api-proxy sidecar, and failing detection runs with401 Unauthorized.Changes
buildExternalDetectorCodexConfig(pkg/workflow/threat_detection_external.go): movedmodel_providerto the first line, ahead of any table header, so TOML assigns it to the document root. Added a comment documenting the ordering constraint.threat_detection_isolation_test.go): assertsmodel_providerprecedes any table header and that the[history]table body contains nomodel_providerkey.Before → after:
The agent Codex config path was already correct and is unchanged. Existing string-based assertion style is kept; no new TOML parser dependency introduced.
model_providerunder[history]table (breaks AWF proxy routing) #47831