fix: read the copilot approvals default, and config files with a bom

[AakashVelusamy]

Two settings a Copilot user can have that made them look locked-down on the permissions page while their agent ran tool calls without asking. Nanda caught the first on a real machine after #285 shipped; the second surfaced while testing the fix for it.

They are different triggers with the same outcome — the user simply isn't there.

What the user has	Before	After
chat.defaultConfiguration: {"approvals": "allowAll"}	no record at all	bypassPermissions
the same, next to any other key	default — identical to a locked-down user	bypassPermissions
any settings.json / mcp.json written with a BOM	no record at all	parsed normally
chat.tools.global.autoApprove: true	bypassPermissions	unchanged
chat.permissions.default: "autoApprove"	bypassPermissions	unchanged

1. The session approvals default

chat.defaultConfiguration was not in SECURITY_RELEVANT_KEYS, and a record is only built when at least one key from that set is present — so a settings.json holding only this setting produced no row, and one holding it alongside anything else produced a row saying default. Reproduced on released main (332cfb3) before changing anything:

allowAll alone            -> None
allowAll + another key    -> mode: default   (raw_settings: only chat.agent.enabled)

It is a real key, checked against the VS Code 1.136 configuration registry rather than the docs:

"chat.defaultConfiguration": { type:"object", additionalProperties:false, properties:{
    mode:      { enum:["interactive","plan","autopilot"], default:"interactive" },
    approvals: { enum:["manual","assisted","allowAll"],   default:"manual" }},
  default: { mode:"interactive", approvals:"manual" } }

and the workbench reads it straight into each new session's auto-approve state:

let t = this._configurationService.getValue("chat.defaultConfiguration"),
    n = eWo(t?.approvals);
if (n) { e.autoApprove = ... n }

The registry also carries a migration from chat.agentSessions.defaultConfiguration, so installs that set this before the rename still hold the old name. Both are read, the way chat.tools.autoApprove is kept as the legacy alias for the global switch.

Deliberately not mapped. approvals: "assisted" still prompts, so calling it a bypass would overstate the posture — it stays default and rides along verbatim in raw_settings. mode: "autopilot" selects the chat mode without touching approvals, so it is not a bypass on its own.

2. Config files with a byte order mark

json.loads rejects a leading BOM outright — its own error says Unexpected UTF-8 BOM (decode using utf-8-sig) — so the parse returned None and the user disappeared.

This is not a broken artifact to be ignored. The editors read such a file happily and preserve the BOM through their own edits (the VS Code bundle carries preserveBOM / addBOM handling), so the configuration stays live, stays honoured, and stays invisible to us until someone rewrites the file by hand. Two ordinary ways it happens on a Windows fleet:

• an MDM or provisioning script using PowerShell's Set-Content -Encoding UTF8, which writes a BOM
• an older Notepad, which defaulted to UTF-8-with-BOM

The strip goes in the shared JSONC entry point, so every reader that routes through it is fixed at once rather than only the file that surfaced this:

Reader	Files no longer skipped
Copilot settings (macOS / Linux / Windows)	settings.json and every profile
Copilot MCP (macOS / Linux / Windows)	mcp.json
Augment	settings.json, MCP config
Copilot CLI	config / settings, MCP config

A BOM character inside a string literal is left untouched.

Verified

Both fixes together on the Windows runner as NT AUTHORITY\SYSTEM, with a settings.json written the way an MDM script writes one — BOM present, and carrying only the approvals key, so it was invisible twice over on main:

head: 7f2befa
devE has BOM  : True
record        : bypassPermissions
settings_path : C:\Users\devE\AppData\Roaming\Code\User\settings.json

Released main was first validated on both runners with four real user accounts each, which is how the gap was confirmed end to end rather than in the extractor alone:

devA  chat.tools.terminal.autoApprove          -> default            allow=[git status]  deny=[rm]
devB  chat.tools.global.autoApprove (profile)  -> bypassPermissions  allow=[npm run]
devC  chat.permissions.default: autoApprove    -> bypassPermissions
devD  chat.defaultConfiguration: allowAll      -> NO RECORD

Eight tests. Six fail on main with the real symptoms — _build_record returning None, and JSONDecodeError: Unexpected UTF-8 BOM — and two are guardrails proving assisted and autopilot are not overstated.

Verify

1. Write {"chat.defaultConfiguration": {"approvals": "allowAll"}} into settings.json, saving it with a BOM (Set-Content -Encoding UTF8 on Windows).
2. Run a discovery scan.
3. The Copilot row reports permission_mode: bypassPermissions. Previously there was no row at all.

🤖 Generated with Claude Code

Confidence Score: 5/5

The PR appears safe to merge; no actionable correctness, security, or repository-rule violations remain.

Summary

• Recognizes allowAll approvals under both current and legacy Copilot default-configuration keys.
• Adds those keys to the security-relevant settings set so configurations containing only that setting are reported.
• Removes a leading UTF-8 BOM at the shared JSONC parsing boundary while preserving BOM characters inside string values.
• Adds focused permission-mode and BOM regression tests.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Copilot or MCP configuration file] --> B[Shared JSONC cleanup]
  B --> C[Remove leading UTF-8 BOM]
  C --> D[Remove comments and trailing commas]
  D --> E[Parse configuration]
  E --> F{Copilot default approvals}
  F -->|allowAll| G[bypassPermissions]
  F -->|manual or assisted| H[default]

Loading

[vigneshsubbiah16]

✅ Security consensus: no issues found. (reviewers: claude, semgrep, gitleaks)

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head f3a85196 · 2026-09-05T18:10Z

[cursor]

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

[vigneshsubbiah16]

✅ Security consensus: no issues found. (reviewers: claude, semgrep, gitleaks)

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 7f2befa9 · 2026-09-06T02:00Z

[AakashVelusamy]

Test battery — Stage 1 + Stage 2 on 7f2befa

PR is open against staging, so this is the pre-merge gate: local first, then both real Azure runners. Nothing ran against a live environment.

Where the risk actually is

The approvals key is confined to one extractor. The BOM strip is not — it sits in _strip_jsonc_comments, which has one definition and 13 call sites across six extractor families. So the battery is weighted there rather than at the key mapping.

Reader reached by the change	Exercised how
Copilot settings (macOS / Linux / Windows)	real CLI on both runners
Copilot MCP (macOS / Linux / Windows)	real CLI on both runners
Augment settings + MCP, Copilot CLI settings + MCP	full local suite (960 selected)

End to end on both runners

A settings.json and an mcp.json planted per user, one pair written with a BOM the way an MDM script writes them (Set-Content -Encoding UTF8), one pair without — so the fix and the no-regression case run side by side through the real CLI reporting to a local capture stub.

user	file encoding	settings →	mcp →
devBOM	BOM	bypassPermissions	bomserver
devPlain	plain UTF-8	bypassPermissions	plainserver

Identical on sentinel-agent-runner as root and poc-win-runner as NT AUTHORITY\SYSTEM. The devBOM settings file carries only chat.defaultConfiguration: {"approvals": "allowAll"}, so it was invisible twice over before this PR — unreadable file and unread key.

Proven to fail without the fix

Not just locally — the same planted files, read by staging's code on the runner:

STAGING mcp.json parse : FAILS -> JSONDecodeError Unexpected UTF-8 BOM (decode using utf-8-sig)
STAGING settings record: None

Locally, six of the eight new guards fail on staging with the real symptoms (_build_record returning None, and the same JSONDecodeError); the other two are guardrails proving assisted and mode: autopilot are not overstated as bypasses.

Suites

Leg	Result
macOS local	63 passed
Linux runner, as root	62 passed, 1 skipped
Windows runner, as SYSTEM	56 passed, 7 skipped
Every suite routing through the shared helper	960 passed; the only reds are test_copilot_cli_discovery, which fails the identical 7 on staging on this machine (a real ~/.copilot is present)
CI	three macOS legs green, Windows legs and Cursor still running; Greptile passed

One thing worth recording

A first Windows run reported no permissions for either user while MCP worked fine. That was the fixture, not the code: I had planted only the github.copilot extension, so the canonical Chat row never existed for those users and permissions had nothing to attach to. Re-run with the real extension pair and both users reported correctly. Worth knowing that on a multi-user host the canonical row is chosen once per scan, so a user whose install produces a different row name gets no permissions block — pre-existing behaviour from #285, unchanged here.

Hygiene

Both runners torn down — synthetic users deleted, planted files and stubs removed, residue checks clean.

[AakashVelusamy]

@greptile-apps review

Re-review request: I overwrote the PR description with a REST PATCH when expanding it to cover the second fix, which removed your appended summary block along with it. That was my mistake, not a change in the diff — the head is unchanged at 7f2befa.

Please re-post the review so the confidence score is on record here.

[vigneshsubbiah16]

✅ Security consensus: no issues found. (reviewers: Claude lead review, Semgrep, Gitleaks)

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head fdb890f4 · 2026-09-08T20:40Z

[greptile-apps]

External tests always skip

These cross-repository tests use author-specific paths under /Users/aakashvelusamy/github/.... The normal test workflow checks out only this repository, and the tests skip when those paths are absent. As a result, CI does not enforce the claimed packaging integration, so a regression in the LaunchDaemon entry point could still pass. Use CI-provisioned fixtures or run these checks in a pipeline that checks out both repositories.

Knowledge Base Used: Tool registry and contracts

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[vigneshsubbiah16]

✅ Security consensus: no issues found. (reviewers: claude-opus-5, semgrep, gitleaks)

---

🤖 consensus review · reviewers: Claude, Semgrep, Gitleaks · head 39bbe6a4 · 2026-09-09T15:08Z

[AakashVelusamy]

Comment-disposition audit — head 39bbe6a

Every review finding on this PR, with where it landed.

Greptile

Finding	Disposition
P2 — External tests always skip (tests/test_nm_pr293_battery.py)	Fixed in 39bbe6a. The file is deleted. It reached /Users/aakashvelusamy/github/{ai-gateway-data,setup} by absolute path, so on CI it hit skipTest and asserted nothing while showing green. The BOM behaviour it covered is guarded by TestByteOrderMark in tests/test_copilot_vscode_permissions.py, which runs everywhere.

Confidence Score: 5/5 — "no actionable correctness, security, or repository-rule violations remain."

Consensus / security review

✅ No issues found on 39bbe6a (reviewers: claude-opus-5, semgrep, gitleaks). Clean on every prior head as well.

Cursor (Bugbot + Security Agent)

Nothing open on this head.

Blast radius checked for this round

_strip_jsonc_comments is the widest surface here — 17 call sites across 6 extractor families. The change is a leading-BOM strip, so a file without a BOM is byte-for-byte unaffected and a file with one becomes parseable where it previously raised. No existing record changes shape; records only appear where none could before.