fix: redact filesystem watcher previews before transport#332
Conversation
|
@honor2030 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe watcher now redacts sensitive information from file previews before emitting events. A redaction pipeline detects ChangesSensitive Data Redaction in Filesystem Watcher
Sequence DiagramsequenceDiagram
participant Watcher as FilesystemWatcher
participant File as FileReader
participant Redactor as redactSensitivePreview
participant Payload as WatcherPayload
Watcher->>File: read preview text
File-->>Watcher: preview content
Watcher->>Redactor: pass preview content & filepath
Redactor->>Redactor: detect .env paths
Redactor->>Redactor: redact assignment keys and bearer tokens
Redactor-->>Watcher: redacted preview
Watcher->>Payload: include redacted preview in emitted event
Possibly Related PRs
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
integrations/filesystem-watcher/watcher.mjs (1)
56-67: 💤 Low valueOptional: quoted-key assignments (JSON/YAML-quoted) bypass redaction.
The assignment regex requires the key to start with
[A-Za-z_], so previews containing JSON- or YAML-quoted secrets such as"api_key": "sk-..."won't match the sensitive-key branch and will only be redacted if the value happens to trip theBearer …fallback. That is outside the dotenv + bearer scope described in the PR, but worth a follow-up if watched roots may include JSON config files (*.json,*.yaml— both already inTEXT_EXTENSIONS).♻️ Sketch: extend regex to allow optionally quoted keys
- const assignment = line.match(/^(\s*(?:export\s+)?([A-Za-z_][A-Za-z0-9_.-]*)\s*([=:])\s*)(.*)$/); + const assignment = line.match( + /^(\s*(?:export\s+)?["']?([A-Za-z_][A-Za-z0-9_.-]*)["']?\s*([=:])\s*)(.*)$/, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integrations/filesystem-watcher/watcher.mjs` around lines 56 - 67, The assignment regex in redactSensitiveLine doesn't match quoted JSON/YAML keys (e.g. "api_key": "sk-..."), so update the regex in redactSensitiveLine to accept an optional single- or double-quoted key (capture the unquoted key in the same group used for isSensitiveKey) and keep the existing capture groups for the separator and value; ensure you strip quotes when calling isSensitiveKey(assignment[2]) (or adjust the capture group to be the unquoted name) so quoted-key assignments are treated the same as bare keys; leave the Bearer fallback and redactSensitivePreview unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@integrations/filesystem-watcher/watcher.mjs`:
- Around line 56-67: The assignment regex in redactSensitiveLine doesn't match
quoted JSON/YAML keys (e.g. "api_key": "sk-..."), so update the regex in
redactSensitiveLine to accept an optional single- or double-quoted key (capture
the unquoted key in the same group used for isSensitiveKey) and keep the
existing capture groups for the separator and value; ensure you strip quotes
when calling isSensitiveKey(assignment[2]) (or adjust the capture group to be
the unquoted name) so quoted-key assignments are treated the same as bare keys;
leave the Bearer fallback and redactSensitivePreview unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 810f73f5-2b0a-4355-b21a-bf93cfee9a03
📒 Files selected for processing (2)
integrations/filesystem-watcher/watcher.mjstest/fs-watcher.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/fs-watcher.test.ts (1)
148-213: ⚖️ Poor tradeoffConsider aligning test patterns with existing async watcher tests.
The three new redaction tests (lines 148-213) call
w.flush()directly rather than using thew.start()/writeFileSync()/await wait()/w.stop()pattern established by the first five tests (lines 43-146). While directflush()calls allow focused unit testing of the redaction logic without debouncing complexity, mixing patterns within the same test suite reduces consistency and may not exercise the actual watcher code path (file detection → debouncing → flush).If the goal is regression coverage of the full watcher pipeline, consider refactoring these tests to follow the existing async pattern:
Example refactor for dotenv test
it("redacts sensitive dotenv preview values before sending observations", async () => { - writeFileSync( - join(root, ".env"), - [ - "OPENAI_API_KEY=sk-test-secret-value", - "PUBLIC_FLAG=enabled", - "AUTHORIZATION=Bearer live-token-value", - ].join("\n"), - ); const w = new FilesystemWatcher({ roots: [root], baseUrl: "http://localhost:3111", logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, }); + w.start(); + try { + writeFileSync( + join(root, ".env"), + [ + "OPENAI_API_KEY=sk-test-secret-value", + "PUBLIC_FLAG=enabled", + "AUTHORIZATION=Bearer live-token-value", + ].join("\n"), + ); + await wait(800); - await w.flush(root, ".env"); - - expect(captured).toHaveLength(1); - const content = (captured[0].body as { data: { content: string } }).data.content; + expect(captured.length).toBeGreaterThanOrEqual(1); + const content = (captured[captured.length - 1].body as { data: { content: string } }).data.content; expect(content).toContain("OPENAI_API_KEY=[REDACTED]"); expect(content).toContain("PUBLIC_FLAG=enabled"); expect(content).toContain("AUTHORIZATION=[REDACTED]"); expect(content).not.toContain("sk-test-secret-value"); expect(content).not.toContain("live-token-value"); + } finally { + w.stop(); + } });(Apply similar changes to the other two redaction tests.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fs-watcher.test.ts` around lines 148 - 213, Tests for redaction call FilesystemWatcher.flush() directly which bypasses the watcher pipeline; update each redaction test (the dotenv, settings.json, and request.txt cases) to follow the existing async watcher pattern by creating the FilesystemWatcher, calling w.start(), writing the file with writeFileSync, awaiting the existing wait() helper to allow the watcher debounce to detect the change, then calling w.stop() before asserting captured results so the full file-detection → debouncing → flush code path in FilesystemWatcher is exercised; ensure you reuse the same logger/captured setup and apply this change consistently to the three tests referencing FilesystemWatcher.start/stop/flush.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/fs-watcher.test.ts`:
- Around line 148-213: Tests for redaction call FilesystemWatcher.flush()
directly which bypasses the watcher pipeline; update each redaction test (the
dotenv, settings.json, and request.txt cases) to follow the existing async
watcher pattern by creating the FilesystemWatcher, calling w.start(), writing
the file with writeFileSync, awaiting the existing wait() helper to allow the
watcher debounce to detect the change, then calling w.stop() before asserting
captured results so the full file-detection → debouncing → flush code path in
FilesystemWatcher is exercised; ensure you reuse the same logger/captured setup
and apply this change consistently to the three tests referencing
FilesystemWatcher.start/stop/flush.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70817c7c-2965-40a4-8ac6-acd36c2d34b2
📒 Files selected for processing (2)
integrations/filesystem-watcher/watcher.mjstest/fs-watcher.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- integrations/filesystem-watcher/watcher.mjs
1 participant
Summary
/agentmemory/observe.env,.env.*) as intentional text previews so they go through the same client-side redaction path instead of depending onextname()behaviorWhy
mem::observealready has server-side redaction paths, but the fs-watcher sends preview content over the configured transport first. For remote/plain HTTP setups, preview redaction needs to happen in the watcher beforefetch().Conflict / duplicate check
integrations/filesystem-watcher/watcher.mjsandtest/fs-watcher.test.ts, but it is about loading watcher config from agentmemory env files.Test Plan
PATH=/opt/homebrew/bin:$PATH npm test -- test/fs-watcher.test.ts --reporter=basicfailed on the two new redaction testsPATH=/opt/homebrew/bin:$PATH npm test -- test/fs-watcher.test.ts -t "redacts" --reporter=basicPATH=/opt/homebrew/bin:$PATH npm test -- --reporter=basic→81 passed,880 passedPATH=/opt/homebrew/bin:$PATH npm run buildPATH=/opt/homebrew/bin:$PATH node --check integrations/filesystem-watcher/watcher.mjsgit diff --checkNote: the initial build attempt exposed a local missing optional
rolldownnative binding; reinstallingnode_moduleswith the same npm version restored the optional dependency, after which build passed.Summary by CodeRabbit