fix(agent): route connector workspaces and redact secrets - #303
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughThe PR adds a managed Sequence Diagram(s)sequenceDiagram
participant AgentManager
participant Sidecar
participant OoGuard
participant RealOo
AgentManager->>Sidecar: start with managed oo path
Sidecar->>OoGuard: execute connector command
OoGuard->>RealOo: bind workspace and run command
RealOo-->>OoGuard: connector output
OoGuard-->>Sidecar: redacted output and exit status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
electron/agent/oo-guard.ts (1)
39-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEscalate to
SIGKILLafter the size limit trips.When the output exceeds the limit, the guard sends
SIGTERMand then waits on theclosepromise. If the connector process ignoresSIGTERM, the guard never resolves. The tool call then hangs with no timeout. Add a bounded escalation toSIGKILL.♻️ Proposed change
+function killWithEscalation(child: ReturnType<typeof spawn>): void { + child.kill("SIGTERM") + const timer = setTimeout(() => child.kill("SIGKILL"), 5_000) + timer.unref?.() + child.once("close", () => clearTimeout(timer)) +} + async function runGuarded(command: string, args: string[]): Promise<number> {Then replace both
child.kill("SIGTERM")calls withkillWithEscalation(child).🤖 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 `@electron/agent/oo-guard.ts` around lines 39 - 56, Update both output-limit error handlers in the child.stdout and child.stderr listeners to call the existing or newly added killWithEscalation(child) helper instead of directly sending SIGTERM. Ensure the helper sends SIGTERM first, then escalates to SIGKILL after a bounded delay if the process has not exited, allowing the close flow to resolve even when SIGTERM is ignored.electron/agent/oo-guard-bin.ts (2)
28-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn the launcher path instead of
binDir.The function returns
binDiron both branches. The caller inelectron/agent/manager.ts(line 574) then rebuilds the launcher name withprocess.platform. The launcher filename is therefore encoded in two places, and the caller ignores theplatformoption of this helper. Return the created command path so the caller uses it directly.♻️ Proposed change
- return binDir + return path.join(binDir, "oo.cmd") } const commandPath = path.join(binDir, "oo") @@ await chmod(commandPath, 0o755) - return binDir + return commandPath }In
electron/agent/manager.ts:- await ensureOoGuardCommandBin({ + managedOoBinPath = await ensureOoGuardCommandBin({ binDir: commandBinDir, nodeBin: process.execPath, ooGuardCliPath, }) - managedOoBinPath = path.join(commandBinDir, process.platform === "win32" ? "oo.cmd" : "oo")🤖 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 `@electron/agent/oo-guard-bin.ts` around lines 28 - 44, Update the helper’s return value to provide the launcher path, using commandPath on the creation branch and the existing platform-specific launcher path on the early-return branch. Then update the caller around the manager’s launcher setup to use this returned path directly instead of rebuilding the filename from process.platform.
22-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
setlocalin the Windows launcher.
set ELECTRON_RUN_AS_NODE=1has no scope guard. If a cmd shell invokesoo.cmddirectly, the variable persists in that shell after the command returns. A later Electron invocation in the same shell then starts in Node mode.setlocallimits the assignment to the script.♻️ Proposed change
path.join(binDir, "oo.cmd"), - ["`@echo` off", "set ELECTRON_RUN_AS_NODE=1", `"${nodeBin}" "${ooGuardCliPath}" %*`, ""].join("\r\n"), + [ + "`@echo` off", + "setlocal", + "set ELECTRON_RUN_AS_NODE=1", + `"${nodeBin}" "${ooGuardCliPath}" %*`, + "endlocal & exit /b %ERRORLEVEL%", + "", + ].join("\r\n"), "utf8",🤖 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 `@electron/agent/oo-guard-bin.ts` around lines 22 - 29, Update the Windows launcher content in the platform === "win32" branch to scope ELECTRON_RUN_AS_NODE=1 to oo.cmd by adding setlocal before the assignment. Preserve the existing command invocation and generated line endings.electron/agent/oo-guard-bin.test.ts (1)
14-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the executable mode and quoting of the POSIX launcher.
The test checks the file content only. It does not check the mode set by
chmod(commandPath, 0o755). If that call is removed, the launcher stops working throughPATH, and this test still passes. A path that contains a single quote also exercisesshellQuote, which no test covers.💚 Proposed additions
-import { mkdtemp, readFile, rm } from "node:fs/promises" +import { mkdtemp, readFile, rm, stat } from "node:fs/promises"const source = await readFile(path.join(binDir, "oo"), "utf8") expect(source).toContain("ELECTRON_RUN_AS_NODE=1") expect(source).toContain("wanta-oo-guard.js") expect(source).toContain('"$@"') + const mode = (await stat(path.join(binDir, "oo"))).mode & 0o777 + expect(mode & 0o111).not.toBe(0)Note: skip the mode assertion on
win32if the suite runs there.🤖 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 `@electron/agent/oo-guard-bin.test.ts` around lines 14 - 27, Extend the POSIX launcher test for ensureOoGuardCommandBin to stat the generated oo file and assert it has executable mode 0o755, skipping this check on win32. Also use a launcher path containing a single quote and assert the generated source correctly quotes it, covering shellQuote behavior while preserving the existing argument and Electron mode assertions.electron/agent/oo-guard-core.test.ts (1)
78-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an idempotence test for output without credentials.
The redaction tests only cover payloads that contain credentials. Add a case for pretty-printed JSON with no sensitive key. That case pins the contract that
redactConnectorOutputreturns the input unchanged, and it guards the persisted-scrub comparison inelectron/agent/manager.ts(line 499). See the related issue onelectron/agent/oo-guard-core.tslines 50-61.💚 Proposed test
test("redacts credential fields from non-JSON errors", () => {test("leaves output without credentials unchanged", () => { const output = `{\n "data": {\n "id": 173107,\n "name": "CLI"\n }\n}\n` assert.equal(redactConnectorOutput(output), output) })🤖 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 `@electron/agent/oo-guard-core.test.ts` around lines 78 - 107, Add a test in the “connector output redaction” suite verifying that pretty-printed JSON without sensitive keys is returned byte-for-byte unchanged by redactConnectorOutput. Use the existing assertion style and include representative business fields such as id and name to cover the persisted-scrub comparison contract.
🤖 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.
Inline comments:
In `@electron/agent/manager.ts`:
- Around line 472-476: Update start() so it does not await
scrubPersistedConnectorOutputs(); launch the scrub asynchronously after the
sidecar is ready while preserving the existing catch-based warning and
diagnostic handling. If needed to prevent overlap with dispose(), retain the
background promise and have scrubPersistedConnectorOutputs() or its session loop
check this.disposed before continuing.
- Around line 496-509: Restrict the scrub loop around redactConnectorOutput to
connector executions only by adding a helper that inspects part.tool and
part.state.input. Allow connector tool parts, and allow shell parts only when
their recorded command invokes oo connector; skip read, unrelated bash, and
other tool outputs before persisting any part.update changes.
In `@electron/agent/oo-guard-core.ts`:
- Around line 2-28: Update normalizedKey and isSensitiveConnectorKey so
credential names are detected within prefixed or vendor-scoped fields, including
x-api-key and posthog_api_key, rather than requiring an exact set match; ensure
APIKey normalizes consistently with api_key. Also update
sensitiveAssignmentPattern to remove its leading word-boundary anchor so
underscore-prefixed assignment names are matched.
- Around line 50-61: Update redactConnectorOutput so the JSON branch compares
the redacted value with the original parsed value and returns the original
output unchanged when no sensitive content was modified. Only serialize the
redacted value when redaction actually occurs, preserving existing
trailing-newline behavior for changed output.
- Around line 98-111: Update resolveGuardWorkspaceTeam to filter out empty
strings after trimming session team values, so unset sessions do not participate
in the uniqueness check. If no non-empty session teams remain, fall back to
scope.teamName; preserve the existing ambiguity error when multiple distinct
non-empty teams are active and retain the empty-string result when no fallback
is present.
- Around line 63-78: Update isConnectorBusinessCommand and hasWorkspaceSelector
to parse past documented leading global options, including value-taking options
such as --lang zh, before identifying the connector command and its workspace
requirement; do not treat command-specific --json or undocumented -t as
global/selector support. When generating and appending a workspace selector,
insert it immediately before a -- terminator rather than after it, preserving
the remaining command arguments.
In `@electron/agent/oo-identity.ts`:
- Around line 54-56: Update the identity migration logic around the
organization/team replacement to remove every existing identity.team and
identity.organization entry, regardless of key order, before writing exactly one
organization entry when teamName is set. When teamName is unset, remove all
matching selectors without adding a replacement, and add tests covering both key
orders and removal.
---
Nitpick comments:
In `@electron/agent/oo-guard-bin.test.ts`:
- Around line 14-27: Extend the POSIX launcher test for ensureOoGuardCommandBin
to stat the generated oo file and assert it has executable mode 0o755, skipping
this check on win32. Also use a launcher path containing a single quote and
assert the generated source correctly quotes it, covering shellQuote behavior
while preserving the existing argument and Electron mode assertions.
In `@electron/agent/oo-guard-bin.ts`:
- Around line 28-44: Update the helper’s return value to provide the launcher
path, using commandPath on the creation branch and the existing
platform-specific launcher path on the early-return branch. Then update the
caller around the manager’s launcher setup to use this returned path directly
instead of rebuilding the filename from process.platform.
- Around line 22-29: Update the Windows launcher content in the platform ===
"win32" branch to scope ELECTRON_RUN_AS_NODE=1 to oo.cmd by adding setlocal
before the assignment. Preserve the existing command invocation and generated
line endings.
In `@electron/agent/oo-guard-core.test.ts`:
- Around line 78-107: Add a test in the “connector output redaction” suite
verifying that pretty-printed JSON without sensitive keys is returned
byte-for-byte unchanged by redactConnectorOutput. Use the existing assertion
style and include representative business fields such as id and name to cover
the persisted-scrub comparison contract.
In `@electron/agent/oo-guard.ts`:
- Around line 39-56: Update both output-limit error handlers in the child.stdout
and child.stderr listeners to call the existing or newly added
killWithEscalation(child) helper instead of directly sending SIGTERM. Ensure the
helper sends SIGTERM first, then escalates to SIGKILL after a bounded delay if
the process has not exited, allowing the close flow to resolve even when SIGTERM
is ignored.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b399b45-674a-46b9-9550-d6fe8598b14a
📒 Files selected for processing (15)
electron/agent/event-translator.test.tselectron/agent/event-translator.tselectron/agent/manager.test.tselectron/agent/manager.tselectron/agent/oo-guard-bin.test.tselectron/agent/oo-guard-bin.tselectron/agent/oo-guard-core.test.tselectron/agent/oo-guard-core.tselectron/agent/oo-guard.tselectron/agent/oo-identity.test.tselectron/agent/oo-identity.tselectron/agent/tool-sources.test.tselectron/agent/tool-sources.tselectron/main.tsvite.config.ts
alwaysmavs
changed the title
|
Addressed the AI review in e46b225. The update makes the history scrub non-blocking and connector-only, preserves unchanged JSON, expands prefixed credential redaction, parses leading oo global flags and argument terminators, ignores empty session scopes, deduplicates identity selectors, hardens process termination, and improves launcher behavior/tests. I also verified the bundled oo CLI 1.7.1 directly: its canonical persisted key is identity.team, so the migration now removes retired/duplicate selectors and writes exactly one team entry rather than adopting the review’s stale identity.organization assumption. Validation: lint and type-check passed; all 295 test files / 2208 tests passed; production build passed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/agent/manager.ts (1)
503-509: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake connector-output redaction complete before writing
.connector-output-redaction-v1.
client.session.listapplieslimitdirectly and exposes no cursor.client.session.messagesreturns one bounded array. A response at10_000can leave persisted connector outputs unredacted while the marker is written. Use a complete enumeration path, or write the marker only after completeness is known. Add a test with more than10_000records.🤖 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 `@electron/agent/manager.ts` around lines 503 - 509, The connector-output scrub loop around client.session.list and client.session.messages must not claim completion when either bounded response may be truncated. Replace the fixed-limit enumeration with a complete pagination/enumeration path, or track completeness and defer writing .connector-output-redaction-v1 until all sessions and messages are confirmed processed; add a test covering more than 10,000 records.
🤖 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.
Inline comments:
In `@electron/agent/manager.ts`:
- Around line 317-327: The shellConnectorCommandPattern used by
isPersistedConnectorToolPart must not identify “oo connector” text inside quoted
arguments. Replace the regex-only command detection with shell-aware parsing
that recognizes connector invocations only at executable command positions,
while preserving supported oo/WANTA_OO_BIN options; add a regression case
covering quoted text such as printf with “oo connector run demo”.
---
Outside diff comments:
In `@electron/agent/manager.ts`:
- Around line 503-509: The connector-output scrub loop around
client.session.list and client.session.messages must not claim completion when
either bounded response may be truncated. Replace the fixed-limit enumeration
with a complete pagination/enumeration path, or track completeness and defer
writing .connector-output-redaction-v1 until all sessions and messages are
confirmed processed; add a test covering more than 10,000 records.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53e90d56-4b9a-448e-b84c-98582ed57d2a
📒 Files selected for processing (9)
electron/agent/manager.test.tselectron/agent/manager.tselectron/agent/oo-guard-bin.test.tselectron/agent/oo-guard-bin.tselectron/agent/oo-guard-core.test.tselectron/agent/oo-guard-core.tselectron/agent/oo-guard.tselectron/agent/oo-identity.test.tselectron/agent/oo-identity.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- electron/agent/oo-guard-bin.test.ts
- electron/agent/oo-guard-core.test.ts
- electron/agent/oo-guard-bin.ts
- electron/agent/oo-guard.ts
- electron/agent/oo-guard-core.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@electron/agent/manager.ts`:
- Around line 346-354: Extend shellExecutesConnectorBusinessCommand to
recursively inspect executable $(...) command substitutions and assignment
right-hand sides using the existing quote-aware parsing rules, while preserving
depth protection and direct/nested command detection. Ensure connector commands
in forms such as quoted substitutions and assignments return true so persisted
output is scrubbed. Add regression tests covering both command-substitution and
assignment cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e81f289-0c4f-47ff-b97e-3fbfb9e5a3ab
📒 Files selected for processing (2)
electron/agent/manager.test.tselectron/agent/manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/agent/manager.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@electron/agent/manager.ts`:
- Around line 408-412: Update shellExecutesConnectorBusinessCommand to remove or
exclude shell comment text before calling executableCommandSubstitutions and
recursively evaluating command bodies, so commented commands such as # $(...)
return false. Add a regression test covering a commented connector command and
verify it does not trigger scrubbing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dea05b5-430f-41bd-ae5b-43ca7b48e17f
📒 Files selected for processing (2)
electron/agent/manager.test.tselectron/agent/manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/agent/manager.test.ts
1 participant
Problem
The Connections page could show a provider such as PostHog as connected while a chat call intermittently returned
app_not_foundorconnection_required. The behavior appeared probabilistic because Wanta's session-scoped Link tools selected the active team correctly, while Provider Skills could invoke a bareoo connector apps/runcommand that fell back to a different CLI identity.Provider/account switching also had a short stale-inventory window: explicit
connectionNamevalidation cached the previous account list for five seconds, so a newly connected or selected account could be rejected until that cache expired.Root cause
--teamselector.Fix
ooguard executable and shim for Agent-side calls.connector apps/runcommands to the active session workspace.--terminators.SIGTERMtoSIGKILLafter a bounded delay.identity.teamkey while collapsing retired or duplicate identity selectors into exactly one entry.connectionNamevalidation so newly switched accounts are immediately visible.User impact
Connected providers should no longer intermittently appear disconnected because two invocation paths used different workspace identities. Rapid provider/account switching no longer waits for the old connection-name cache to expire. Ambiguous cross-team raw CLI calls now stop safely instead of reaching the wrong workspace.
Safety and Compatibility
connector apps/runcommands are bound.identity.teamand the command-line selector remains--team.setlocal.Validation
pnpm run lintpnpm run ts-checkpnpm test— 295 files, 2208 tests passedpnpm run build:appgit diff --checkThe production build includes
dist-electron/wanta-oo-guard.js; only the existing bundle-size and deprecatedinlineDynamicImportswarnings remain.