Skip to content

fix: match recorded tool approvals for Windows paths - #2939

Merged
dungdong-aws merged 1 commit into
Amazon-Q-Developer:feature/OHIfrom
dungdong-aws:fix/fsread-approval-paths
Oct 16, 2026
Merged

dungdong-aws merged 1 commit into
Amazon-Q-Developer:feature/OHIfrom
dungdong-aws:fix/fsread-approval-paths

Conversation

@dungdong-aws

@dungdong-aws dungdong-aws commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Agentic mode asks for permission again for every single file it reads, without limit. The reporter of aws/aws-toolkit-visual-studio#617 clicked Allow "more than 500 times" for one change, on files that were inside his project.

Root cause

ChatSessionService.addApprovedPath stores approved paths with forward slashes (C:/project/file.ts). isPathApproved tried to normalize the incoming path the same way, but used /\\\\/g, which only matches a doubled backslash. A normal Windows path (C:\project\file.ts) passed through unchanged, so it never matched the stored form, either exactly or through the parent-folder check. Every approval was recorded and then never recognised, and the user was asked again on each call.

The other half of the original report, approvals never being recorded for fsRead (it sends paths: [...], but the controller recorded only input.path || input.cwd), is already fixed by #2940 (471a7f4e2, on main and feature/OHI). That fix was a side effect rather than a targeted one: #2940 caches approvals from the targets the acceptance check resolved, and that check covers every entry of a multi-path read. An earlier revision of this PR added a getApprovalPathsFromInput helper for the recording gap; it was dropped once #2940 landed. This PR fixes the matching side.

Changes

tools/toolShared.ts (production change, +43/−6):

  • normalizePathSeparators: converts every \ to /, matching how addApprovedPath stores paths.
  • isSamePath: treats an approval for the file itself as a match. isParentFolder is a strict ancestor check and reports equal paths as unrelated. Case is ignored on Windows and macOS, the same precedent isParentFolder follows, and a trailing separator does not change identity.
  • Removed the unused rootDir computation.

Why this does not widen approval

  • Approvals stay keyed per tool name, so a read approval does not grant writes.
  • Only identity and ancestor relationships match; a sibling that shares a name prefix (C:/proj vs C:/project) still asks.
  • Case-insensitive comparison applies only on platforms whose default filesystems are case-insensitive.

Tests / validation

New tests:

  • toolShared.test.ts: Windows path vs the stored separator form; file inside an approved Windows directory; trailing separator on an approved file; case handling per platform; prefix-sharing sibling still not approved.
  • fsRead.test.ts: a multi-path read approved once does not ask again for those paths, while an unapproved sibling still does.
  • agenticChatController.test.ts: every checked path of a multi-path read is recorded after approval.

Ran locally on Linux: tsc --noEmit clean; agenticChat controller, errors, file tools, executeBash, tilde-path and toolShared suites — 417 passing, 4 pending. Three of the new isPathApproved tests fail against the current base and pass with this change. Windows CI is the platform validation for the Windows-specific cases; no IDE session was run.

Release target

Base feature/OHI. Draft — not deployed anywhere.

@dungdong-aws

Copy link
Copy Markdown
Contributor Author

Pushed a second commit. The description above covers only the first half of the defect — here is the rest, and why it is in the same PR.

A second defect in the same mechanism: on Windows a recorded approval never matched

ChatSessionService.addApprovedPath stores a path with its separators normalized to /. isPathApproved normalized the incoming path with /\\\\/g — a regex that matches two literal backslashes, so an ordinary Windows path passes through untouched. C:/project/file.ts (stored) therefore never compared equal to C:\project\file.ts (checked), and the fallback workspaceUtils.isParentFolder is a strict ancestor check that reports two equal paths as unrelated (path.relative returns '').

Effect: on Windows, an approval for a file never matched that file again — for every tool, not just reads. That is consistent with the reporter being on Windows and re-prompted for writes and shell commands as well.

CI proved it rather than argument. With only the plural-paths fix, the new fsRead regression test passed on Linux and failed on Windows (Test (Windows), run 38642934926): AssertionError: The same read should not be asked about twice. Fixing the plural-paths bug alone would have left the reporter's platform exactly as broken.

Changes in the second commit

  • isPathApproved normalizes separators the way the recording side does, and compares identity separately from ancestry (isSamePath): case is ignored only on win32/darwin — the precedent isParentFolder already follows — and a trailing separator does not change identity.
  • Drops a dead rootDir local that was computed and never read.
  • New isPathApproved tests: Windows separators, approved Windows directory containing a nested file, trailing separator, case sensitivity asserted both ways per platform, and a sibling path sharing a prefix (/p/file.js approved must not approve /p/file.js.bak).

Production total for the PR is now 77 added lines across toolShared.ts and agenticChatController.ts; the rest is tests.

Validation

prettier --check and eslint clean locally (only pre-existing import/no-nodejs-modules warnings). Compile and unit suites were not run locally — the host could not enforce a memory cap from the automation context — so CI is the first full validation. No IDE session, so no runtime reproduction. One unrelated pre-existing failure (should return completion items for CloudFormation, YAML) appears in the Windows test job and is untouched by this change.

@codecov-commenter

codecov-commenter commented Oct 14, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...rc/language-server/agenticChat/tools/toolShared.ts 97.29% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

isPathApproved normalized separators with a pattern that only matched
doubled backslashes, so Windows paths never matched approvals recorded
for a parent folder or for the same path in a different separator form.
Normalize separators and compare paths consistently, and add tests for
multi-path reads and Windows-style paths.
@dungdong-aws
dungdong-aws force-pushed the fix/fsread-approval-paths branch from d2b6f91 to 6cfd24b Compare October 16, 2026 17:01
@dungdong-aws dungdong-aws changed the title fix: remember file-read approvals for every approved path fix: match recorded tool approvals for Windows paths Oct 16, 2026
@dungdong-aws
dungdong-aws marked this pull request as ready for review October 16, 2026 17:15
@dungdong-aws
dungdong-aws requested a review from a team as a code owner October 16, 2026 17:15

@ashishrp-aws ashishrp-aws left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI-assisted review at 6cfd24b.

Approving. The root cause checks out. addApprovedPath stores filePath.replace(/\\/g, '/'), but isPathApproved normalized with /\\\\/g, which only matches a doubled backslash, so the exact-file lookup never hit on Windows. Directory approvals already happened to work, because path.win32.normalize inside isParentFolder converts /. That fits the report: per-file approvals, which are what fsRead records, were the ones repeatedly re-asked. isSamePath is needed because isParentFolder returns false for equal paths (relative !== ''). Using a single \→/ helper on both sides removes the mismatch at its source. The tests cover the reported case, a nested directory, the per-platform case rule and the prefix-sharing sibling (file.js vs file.js.bak). CI is green on Windows and Linux.

Non-blocking:

  • Small widening on macOS. The exact match used to be case-sensitive everywhere, and now isSamePath lowercases on darwin. An approval for File.js now also covers file.js, which can be a different file on a case-sensitive APFS volume. It's the same precedent isParentFolder already follows for directories, so this is acceptable, but say so in the PR description.
  • Root edge case. replace(/[\\/]+$/, '') turns an approved / into '' and C:/ into C:. Nothing sensible should approve a filesystem root, but a guard (|| sep) would keep isSamePath('/', '') from returning true.
  • Interaction with the pre-existing hard-link gap. Approved paths return before the multiply-linked check in requiresPathAcceptance. On Windows, file-level approvals now actually match, so that gap is newly reachable there. It's not introduced by this PR, but it's worth tracking alongside #2907's follow-ups.
  • Nit. toolPaths.has(filePath) || toolPaths.has(normalizedFilePath) is now redundant with the loop's isSamePath. Keeping it as a fast path is fine.

@dungdong-aws
dungdong-aws merged commit a4941a4 into Amazon-Q-Developer:feature/OHI Oct 16, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants