Repository navigation
fix: match recorded tool approvals for Windows paths - #2939
dungdong-aws merged 1 commit into
Conversation
|
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
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 Changes in the second commit
Production total for the PR is now 77 added lines across Validation
|
|
Codecov Report❌ Patch coverage is
📢 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.
d2b6f91 to
6cfd24b
Compare
ashishrp-aws
left a comment
There was a problem hiding this comment.
🤖 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
isSamePathlowercases on darwin. An approval forFile.jsnow also coversfile.js, which can be a different file on a case-sensitive APFS volume. It's the same precedentisParentFolderalready follows for directories, so this is acceptable, but say so in the PR description. - Root edge case.
replace(/[\\/]+$/, '')turns an approved/into''andC:/intoC:. Nothing sensible should approve a filesystem root, but a guard (|| sep) would keepisSamePath('/', '')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'sisSamePath. Keeping it as a fast path is fine.
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.addApprovedPathstores approved paths with forward slashes (C:/project/file.ts).isPathApprovedtried 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 sendspaths: [...], but the controller recorded onlyinput.path || input.cwd), is already fixed by #2940 (471a7f4e2, onmainandfeature/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 agetApprovalPathsFromInputhelper 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 howaddApprovedPathstores paths.isSamePath: treats an approval for the file itself as a match.isParentFolderis a strict ancestor check and reports equal paths as unrelated. Case is ignored on Windows and macOS, the same precedentisParentFolderfollows, and a trailing separator does not change identity.rootDircomputation.Why this does not widen approval
C:/projvsC:/project) still asks.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 --noEmitclean; agenticChat controller, errors, file tools, executeBash, tilde-path and toolShared suites — 417 passing, 4 pending. Three of the newisPathApprovedtests 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.