fix(reply): normalize Windows media paths for dedupe#44228
fix(reply): normalize Windows media paths for dedupe#44228aniruddhaadak80 wants to merge 2 commits intoopenclaw:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Windows-specific reply media dedupe mismatches by normalizing drive-letter paths so that local paths (C:\... / C:/...) and file:///C:/... URLs compare equivalently during dedupe filtering.
Changes:
- Normalize Windows absolute drive-letter local paths by canonicalizing separators before dedupe comparison.
- Normalize
file:///C:/...URLs into the same canonical Windows local-path form during dedupe comparison. - Add a regression test covering
file:///C:/...vs bothC:\...andC:/....
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/auto-reply/reply/reply-payloads.ts | Adds Windows drive-letter normalization in filterMessagingToolMediaDuplicates so equivalent local path / file URL forms dedupe correctly. |
| src/auto-reply/reply/reply-payloads.test.ts | Adds regression coverage for Windows drive-letter file:///C:/... URLs deduping against local path variants. |
Greptile SummaryThis PR fixes a Windows-specific media deduplication bug where Key observations:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/reply-payloads.ts
Line: 117-124
Comment:
**Drive letter case not normalized**
The normalization converts all variants to backslash form (e.g. `C:\path`) but does not lowercase the drive letter. This means `file:///c:/tmp/photo.jpg` (lowercase `c`) normalizes to `c:\tmp\photo.jpg`, which will **not** match a `sentMediaUrls` entry of `C:\tmp\photo.jpg` (uppercase `C`), even though on Windows these refer to the same file.
Windows paths are case-insensitive, so a sent record stored with one casing and a reply payload referencing the other casing will silently fail to dedupe.
Consider folding the drive letter to a consistent case before returning:
```typescript
// non-file:// branch
return WINDOWS_ABSOLUTE_PATH_RE.test(trimmed)
? trimmed[0].toLowerCase() + trimmed.slice(1).replaceAll("/", "\\")
: trimmed;
```
```typescript
// file: branch
if (/^\/[a-zA-Z]:\//.test(decodedPath)) {
const win = decodedPath.slice(1).replaceAll("/", "\\");
return win[0].toLowerCase() + win.slice(1);
}
```
(The corresponding test in `reply-payloads.test.ts` covers only uppercase `C:` on both sides, so the case-mismatch scenario is currently untested.)
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 4e0ca4c |
| return WINDOWS_ABSOLUTE_PATH_RE.test(trimmed) ? trimmed.replaceAll("/", "\\") : trimmed; | ||
| } | ||
| try { | ||
| const parsed = new URL(trimmed); | ||
| if (parsed.protocol === "file:") { | ||
| return decodeURIComponent(parsed.pathname || ""); | ||
| const decodedPath = decodeURIComponent(parsed.pathname || ""); | ||
| if (/^\/[a-zA-Z]:\//.test(decodedPath)) { | ||
| return decodedPath.slice(1).replaceAll("/", "\\"); |
There was a problem hiding this comment.
Drive letter case not normalized
The normalization converts all variants to backslash form (e.g. C:\path) but does not lowercase the drive letter. This means file:///c:/tmp/photo.jpg (lowercase c) normalizes to c:\tmp\photo.jpg, which will not match a sentMediaUrls entry of C:\tmp\photo.jpg (uppercase C), even though on Windows these refer to the same file.
Windows paths are case-insensitive, so a sent record stored with one casing and a reply payload referencing the other casing will silently fail to dedupe.
Consider folding the drive letter to a consistent case before returning:
// non-file:// branch
return WINDOWS_ABSOLUTE_PATH_RE.test(trimmed)
? trimmed[0].toLowerCase() + trimmed.slice(1).replaceAll("/", "\\")
: trimmed;// file: branch
if (/^\/[a-zA-Z]:\//.test(decodedPath)) {
const win = decodedPath.slice(1).replaceAll("/", "\\");
return win[0].toLowerCase() + win.slice(1);
}(The corresponding test in reply-payloads.test.ts covers only uppercase C: on both sides, so the case-mismatch scenario is currently untested.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/reply-payloads.ts
Line: 117-124
Comment:
**Drive letter case not normalized**
The normalization converts all variants to backslash form (e.g. `C:\path`) but does not lowercase the drive letter. This means `file:///c:/tmp/photo.jpg` (lowercase `c`) normalizes to `c:\tmp\photo.jpg`, which will **not** match a `sentMediaUrls` entry of `C:\tmp\photo.jpg` (uppercase `C`), even though on Windows these refer to the same file.
Windows paths are case-insensitive, so a sent record stored with one casing and a reply payload referencing the other casing will silently fail to dedupe.
Consider folding the drive letter to a consistent case before returning:
```typescript
// non-file:// branch
return WINDOWS_ABSOLUTE_PATH_RE.test(trimmed)
? trimmed[0].toLowerCase() + trimmed.slice(1).replaceAll("/", "\\")
: trimmed;
```
```typescript
// file: branch
if (/^\/[a-zA-Z]:\//.test(decodedPath)) {
const win = decodedPath.slice(1).replaceAll("/", "\\");
return win[0].toLowerCase() + win.slice(1);
}
```
(The corresponding test in `reply-payloads.test.ts` covers only uppercase `C:` on both sides, so the case-mismatch scenario is currently untested.)
How can I resolve this? If you propose a fix, please make it concise.
This comment was marked as spam.
This comment was marked as spam.
|
The Windows media-dedupe fix itself looks good, and the regression test is solid.
That said, this PR also includes unrelated changes in:
Those changes don’t appear tied to the reply-media dedupe fix and aren’t reflected in the PR title/summary. The branch history also shows they came in as a separate commit ( I’d suggest trimming this PR to just the reply-media dedupe fix plus the regression test, or moving the config/protocol-model changes into a separate PR so this one stays tightly scoped. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. A high-confidence source-level reproduction is clear: a sent media URL such as Next step before merge Security Review findings
Review detailsBest possible solution: Land a focused current-main fix in Do we have a high-confidence way to reproduce the issue? Yes. A high-confidence source-level reproduction is clear: a sent media URL such as Is this the best way to solve the issue? No. The PR targets the right behavior, but it misses Windows drive-letter case folding, is against the older helper shape, and includes unrelated stale push/protocol changes. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a90be474f441. |
Summary
file:///C:/...URLs as different strings.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
file:///C:/...,C:\..., andC:/...media references as the same local file when deciding whether to strip duplicates.Security Impact (required)
Repro + Verification
Environment
Steps
file:///C:/...URL.Expected
Actual
Evidence
Human Verification (required)
pnpm exec vitest run src/auto-reply/reply/reply-payloads.test.ts.pnpm build:plugin-sdk:dts.Compatibility / Migration
Failure Recovery (if this breaks)