Surface config syntax errors as findings + Codex multi-project trust + IPv6 loopback#47
Merged
Merged
Conversation
…back
Four issues from the latest inspection round, all in the "silent
failure looks clean" category — exactly the failure mode a
trust-focused reviewer can't afford.
1. **Malformed .codex/config.toml returned a clean report.**
`parseToml` failure in `readCodexMcpServers` was swallowed,
returning an empty server map. A hand-edited config containing
risky settings produced `rating: "none"`, `findingCount: 0`.
Surface as a high-severity `scope_trail.codex_config_syntax_error`
and short-circuit the rest of the Codex detector (partial parses
produce noise, not signal).
2. **Malformed .mcp.json / .claude/settings.json crashed the CLI.**
`JSON.parse` in `readJsonObjectWithSource` had no try/catch — a
syntax error escaped, the CLI exited 1 with a raw SyntaxError, and
the report pipeline + `fail-on` semantics were bypassed entirely.
Wrap parsing; return `{ json: {}, text: raw, parseError }`. Each
JSON-reading detector now emits a `*_config_syntax_error` finding
at first contact:
- `scope_trail.mcp_config_syntax_error` (high) — active configs
- `scope_trail.mcp_sample_config_syntax_error` (low) — samples
- `scope_trail.claude_settings_syntax_error` (high)
3. **Codex `[projects.*]` sections collapsed to one bucket.**
`normalizeSection` mapped `projects.<path>` → `projects` and
`normalizeKey` produced `projects.trust_level` for any project,
so Map keys overwrote. A second new trusted project went
undetected when a first trusted project already existed.
Replace with a `readTrustedProjects(root)` helper that iterates
`parseToml(text).projects` and emits one finding per
newly-trusted project path. Subject now includes the path
(`projects.<path>.trust_level`) so reviewers see which project
changed.
4. **IPv6 loopback `[::1]` was treated as remote.**
`new URL('http://[::1]:3000').hostname` returns `[::1]` with
brackets in Node, but the exclusion list only had `::1` without.
Local IPv6 MCP endpoints were getting flagged as remote.
Include `[::1]` alongside `::1`.
Regression tests for all four. Existing
`detects Codex config permission drift` test updated for the new
per-path subject format.
Test count: 54 → 59.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four issues from the latest inspection round, all in the "silent failure looks clean" category — exactly the failure mode a trust-focused reviewer can't afford. Test count goes 54 → 59 with regression coverage for every fix.
Findings addressed
.codex/config.tomlreturnedrating: "none"instead of failing loudlyscope_trail.codex_config_syntax_error(high); short-circuit the rest of the detector.mcp.json/.claude/settings.jsoncrashed the CLI with rawSyntaxError, bypassingfail-onsemanticsJSON.parseinreadJsonObjectWithSource; detectors emitmcp_config_syntax_error(high),mcp_sample_config_syntax_error(low),claude_settings_syntax_error(high)[projects.*]sections collapsed to one bucket — second new trusted project went undetected when one already existedparseToml(text).projectsper-path; subject now includes the project path so reviewers see which one changed[::1]flagged as remote (Node's URL parser returns hostname with brackets, exclusion list had::1without)[::1]alongside::1in the loopback listWhy "silent failure" matters
ScopeTrail is advertised as a trust-focused permission-drift reviewer. The worst outcome isn't a false positive — it's a clean report that hides real drift. P1 and P2 were exactly this shape: a malformed config or a stealth second-trusted-project produced findingCount: 0 while the underlying state got more permissive. This PR converts those into loud failures.
Test plan
npm test— 59/59 passing locally (up from 54)npm run buildcleandetects Codex config permission drifttest updated for the new per-path subject (projects.c:\dev\example.trust_level)Severity calibration
🤖 Generated with Claude Code