Built-in orchestrator agent + docs/toolchain cleanup - #41
Merged
Merged
Conversation
Add a built-in orchestrator agent plus self-configurable provisioning: the orchestrator can register MCP integrations, create specialized Argos agents restricted to an MCP, and attach durable instructions. Includes the daemon orchestration runtime, pi agent profile manager, agent-runtime config merge, shared domain schemas, tests, and SDD specs (builtin-orchestrator-agent, self-configurable-orchestrator).
Refresh live docs for the current toolchain: pnpm -> bun commands and tsgo -> TypeScript 7 (tsc) across features/issues/architecture; fix AGENTS.md (bun run start, TS 7) and README (broken icon path, stale screenshot URLs -> local landing shots). Close out and archive the already-implemented electron-vite-to-vite-plugin-electron goal, and resolve the last tail items on five archived goals (skill-draft-confirmation-card, tape-trace-ui, remote-agent-switch, splash-cinematic-reveal, windows-arm64-support).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughChangesThe daemon now seeds a protected, disabled-by-default orchestrator agent. It exposes validated tools for agent, MCP server, and managed skill provisioning with rollback. Managed skills use atomic persistence and SHA-256 records. Agent schemas include orchestration, memory, compaction, and persona settings. Documentation commands now use Bun. Orchestrator runtime and provisioning
Repository documentation and development commands
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OrchestrationRuntime
participant Daemon
participant PiAgentProfileManager
participant ArgosAgentRuntime
OrchestrationRuntime->>Daemon: Invoke provisioning tool
Daemon->>PiAgentProfileManager: Persist or validate managed skills
Daemon->>ArgosAgentRuntime: Create, update, or validate agent
ArgosAgentRuntime-->>Daemon: Return agent state
Daemon-->>OrchestrationRuntime: Return provisioning result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
Confidence Score: 4/5The unrestricted integration fallback must be fixed before merging because agents with omitted allowlists can receive all available MCP and plugin tools. The changed merge semantics convert omitted capability lists from explicit denial to unrestricted access in downstream tool filtering, while standalone managed-skill failures can also leave filesystem and database state inconsistent. Files Needing Attention: packages/agent-runtime/src/configMerge.ts; apps/daemon/src/index.ts
|
| Filename | Overview |
|---|---|
| packages/agent-runtime/src/configMerge.ts | Adds orchestration configuration but changes omitted integration allowlists from deny-all arrays to unrestricted undefined values. |
| apps/daemon/src/index.ts | Wires provisioning, MCP lifecycle, validation, and rollback; standalone skill mutations can leave disk and database state inconsistent after persistence errors. |
| apps/daemon/src/host/argosOrchestrationRuntime.ts | Exposes the new provisioning operations as orchestration MCP tools and delegates them through injected authority ports. |
| apps/daemon/src/host/piAgentProfileManager.ts | Adds normalized, hashed, disk-backed managed skills and per-agent registries with atomic individual file replacement. |
| packages/agent-runtime/src/argosAgentRuntime.ts | Seeds and protects the opt-in orchestrator agent while preserving its enabled state across daemon restarts. |
| packages/shared-contracts/src/domainSchemas.ts | Extends shared agent configuration schemas for orchestration support. |
Prompt To Fix All With AI
### Issue 1
packages/agent-runtime/src/configMerge.ts:18-20
**Omitted allowlists grant full access**
When an agent omits its MCP or plugin allowlist, these fields now resolve to `undefined`, which downstream filtering treats as unrestricted and exposes every available MCP tool, including plugin-owned servers. Restore explicit deny-all defaults so omitted capability lists do not expand the agent's access.
**How this was verified:** The changed merge output was traced into the daemon and MCP filters, both of which skip filtering when the allowlists are undefined.
```suggestion
enabledMcpServerIds: overrideConfig.enabledMcpServerIds ?? baseConfig.enabledMcpServerIds ?? [],
enabledPluginIds: overrideConfig.enabledPluginIds ?? baseConfig.enabledPluginIds ?? [],
enabledSkillNames: overrideConfig.enabledSkillNames ?? baseConfig.enabledSkillNames ?? [],
```
### Issue 2
apps/daemon/src/index.ts:536-555
**Skill mutations lack compensation**
If `updateArgosAgent` fails after a standalone skill write or removal, the filesystem and registry have already changed without rollback, leaving either an unattached orphaned skill or an allowlist entry that references a deleted skill. Add compensation or an atomic coordination layer so a reported persistence failure cannot leave these stores inconsistent.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "docs: toolchain cleanup and SDD close-ou..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+18
to
+20
| enabledMcpServerIds: overrideConfig.enabledMcpServerIds ?? baseConfig.enabledMcpServerIds, | ||
| enabledPluginIds: overrideConfig.enabledPluginIds ?? baseConfig.enabledPluginIds, | ||
| enabledSkillNames: overrideConfig.enabledSkillNames ?? baseConfig.enabledSkillNames, |
There was a problem hiding this comment.
When an agent omits its MCP or plugin allowlist, these fields now resolve to undefined, which downstream filtering treats as unrestricted and exposes every available MCP tool, including plugin-owned servers. Restore explicit deny-all defaults so omitted capability lists do not expand the agent's access.
How this was verified: The changed merge output was traced into the daemon and MCP filters, both of which skip filtering when the allowlists are undefined.
Suggested change
| enabledMcpServerIds: overrideConfig.enabledMcpServerIds ?? baseConfig.enabledMcpServerIds, | |
| enabledPluginIds: overrideConfig.enabledPluginIds ?? baseConfig.enabledPluginIds, | |
| enabledSkillNames: overrideConfig.enabledSkillNames ?? baseConfig.enabledSkillNames, | |
| enabledMcpServerIds: overrideConfig.enabledMcpServerIds ?? baseConfig.enabledMcpServerIds ?? [], | |
| enabledPluginIds: overrideConfig.enabledPluginIds ?? baseConfig.enabledPluginIds ?? [], | |
| enabledSkillNames: overrideConfig.enabledSkillNames ?? baseConfig.enabledSkillNames ?? [], |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent-runtime/src/configMerge.ts
Line: 18-20
Comment:
**Omitted allowlists grant full access**
When an agent omits its MCP or plugin allowlist, these fields now resolve to `undefined`, which downstream filtering treats as unrestricted and exposes every available MCP tool, including plugin-owned servers. Restore explicit deny-all defaults so omitted capability lists do not expand the agent's access.
**How this was verified:** The changed merge output was traced into the daemon and MCP filters, both of which skip filtering when the allowlists are undefined.
```suggestion
enabledMcpServerIds: overrideConfig.enabledMcpServerIds ?? baseConfig.enabledMcpServerIds ?? [],
enabledPluginIds: overrideConfig.enabledPluginIds ?? baseConfig.enabledPluginIds ?? [],
enabledSkillNames: overrideConfig.enabledSkillNames ?? baseConfig.enabledSkillNames ?? [],
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+536
to
+555
| async writeAgentSkill(agentId, input) { | ||
| const agent = await configPresenter.getArgosAgent(agentId); | ||
| if (!agent) throw new Error(`Argos agent not found: ${agentId}`); | ||
| const skill = piProfiles.writeManagedSkill(agentId, input); | ||
| const enabledSkillNames = Array.from(new Set([...(agent.config?.enabledSkillNames ?? []), skill.name])); | ||
| await configPresenter.updateArgosAgent(agentId, { | ||
| config: { ...agent.config, enabledSkillNames }, | ||
| }); | ||
| return { skill, enabledSkillNames }; | ||
| }, | ||
| async removeAgentSkill(agentId, name) { | ||
| const agent = await configPresenter.getArgosAgent(agentId); | ||
| if (!agent) throw new Error(`Argos agent not found: ${agentId}`); | ||
| const removed = piProfiles.removeManagedSkill(agentId, name); | ||
| const normalizedName = name.trim().toLowerCase(); | ||
| const enabledSkillNames = (agent.config?.enabledSkillNames ?? []).filter((item) => item !== normalizedName); | ||
| await configPresenter.updateArgosAgent(agentId, { | ||
| config: { ...agent.config, enabledSkillNames }, | ||
| }); | ||
| return { removed, name: normalizedName, enabledSkillNames }; |
There was a problem hiding this comment.
If updateArgosAgent fails after a standalone skill write or removal, the filesystem and registry have already changed without rollback, leaving either an unattached orphaned skill or an allowlist entry that references a deleted skill. Add compensation or an atomic coordination layer so a reported persistence failure cannot leave these stores inconsistent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/daemon/src/index.ts
Line: 536-555
Comment:
**Skill mutations lack compensation**
If `updateArgosAgent` fails after a standalone skill write or removal, the filesystem and registry have already changed without rollback, leaving either an unattached orphaned skill or an allowlist entry that references a deleted skill. Add compensation or an atomic coordination layer so a reported persistence failure cannot leave these stores inconsistent.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Pull request overview
Adds a new built-in, opt-in “orchestrator” Argos agent plus daemon-side provisioning capabilities (agent/MCP/managed skills), and then follows through with broad SDD/doc/toolchain command updates (pnpm→bun, tsgo→TypeScript 7) and small README/archival cleanups.
Changes:
- Seed a protected, disabled-by-default
argos-orchestratoragent and enforce orchestrator invariants in runtime config updates. - Add daemon orchestration provisioning actions (agent create/update, MCP server upsert/assignment, disk-backed managed skills, provision/validate operations) with new test coverage.
- Update docs/SDD artifacts and repository guidance to current bun + TypeScript 7 toolchain, plus README asset path fixes.
Reviewed changes
Copilot reviewed 76 out of 78 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Fix icon path and switch screenshots to repo-local images. |
| packages/shared-contracts/src/domainSchemas.ts | Extend agent config schema (orchestration, memory, compaction) and subagent slot shape. |
| packages/agent-runtime/src/index.ts | Export orchestrator constants/config from agent-runtime package API. |
| packages/agent-runtime/src/configMerge.ts | Preserve extension-policy optionality and add orchestrationEnabled merge semantics. |
| packages/agent-runtime/src/argosAgentRuntime.ts | Add orchestrator builtin identity/config and seeding + config invariants enforcement. |
| docs/issues/yobrowser-cdp-graceful-degradation/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/windows-release-build-arch/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/windows-arm64-duckdb-upgrade/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/windows-arm64-duckdb-upgrade/plan.md | Update lockfile/tooling references (bun.lock) and bun commands. |
| docs/issues/usage-dashboard-empty-state/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/thought-block-visual-alignment/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/telegram-message-markdown-render/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/telegram-message-markdown-render/plan.md | Replace pnpm test/typecheck commands with bun equivalents. |
| docs/issues/settings-navigation-selection-lag/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/session-list-stable-alphabetical-sort/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/session-list-stable-alphabetical-sort/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/scheduled-tasks-loading-loop/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/remove-china-specific-defaults/tasks.md | Replace pnpm commands with bun equivalents (plus i18n note). |
| docs/issues/react-doctor-top-3/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/prcheck-format-onboarding/spec.md | Update acceptance criteria command to bun. |
| docs/issues/prcheck-format-onboarding/plan.md | Update repro and gate commands to bun. |
| docs/issues/openai-compatible-video-prompt-duration-fallback/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/openai-compatible-video-prompt-duration-fallback/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/onboarding-provider-mcp-handoff/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/onboarding-provider-mcp-handoff/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/merged-activity-groups/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/mcp-server-start-regression/tasks.md | Update bun filter/typecheck commands for UI. |
| docs/issues/main-logger-console-recursion/spec.md | Update toolchain wording (pnpm→bun engine requirements). |
| docs/issues/mac-native-feel-audit/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/guided-onboarding-first-chat-confirm/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/floating-button-position-persistence/tasks.md | Replace pnpm commands with bun equivalents (including bun run test). |
| docs/issues/daemon-tier2-coming-soon-routes/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/daemon-tier2-coming-soon-routes/spec.md | Replace pnpm commands with bun equivalents. |
| docs/issues/daemon-tier2-coming-soon-routes/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/daemon-provider-model-backend/tasks.md | Replace pnpm commands with bun equivalents (filters and gates). |
| docs/issues/daemon-provider-model-backend/plan.md | Replace pnpm commands with bun equivalents (filters and gates). |
| docs/issues/daemon-disconnect-visibility/tasks.md | Replace pnpm commands with bun equivalents (filters and gates). |
| docs/issues/daemon-browser-provider-catalog/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/cua-driver-v0-2-0-sync/plan.md | Replace pnpm plugin build/validate commands with bun equivalents. |
| docs/issues/browser-web-bootstrap-stuck/plan.md | Replace pnpm build:web with bun equivalent. |
| docs/issues/browser-settings-desktop-only-tabs/plan.md | Replace pnpm commands with bun equivalents. |
| docs/issues/agent-exec-utility-process-crash/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/issues/acp-operational-health-check/tasks.md | Replace pnpm commands with bun equivalents (filters and gates). |
| docs/issues/acp-existing-thread-inline-options/plan.md | Replace pnpm commands with bun equivalents. |
| docs/features/self-configurable-orchestrator/tasks.md | Add SDD tasks checklist for provisioning/validation feature slice. |
| docs/features/self-configurable-orchestrator/spec.md | New spec for orchestrator-driven provisioning + managed skills persistence. |
| docs/features/self-configurable-orchestrator/plan.md | New plan for provisioning ports, managed skills, atomic rollback, validation. |
| docs/features/daemon-self-update/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/features/daemon-self-update/plan.md | Replace pnpm build:daemon with bun equivalent in test strategy. |
| docs/features/builtin-orchestrator-agent/tasks.md | Add SDD tasks checklist for built-in orchestrator agent slice. |
| docs/features/builtin-orchestrator-agent/spec.md | New spec for seeding and protecting an opt-in orchestrator agent. |
| docs/features/builtin-orchestrator-agent/plan.md | New plan for runtime seeding, config merge behavior, tests, and exports. |
| docs/features/agent-state-semantics/plan.md | Replace pnpm commands with bun equivalents throughout plan steps. |
| docs/features/acp-v1-reliability/tasks.md | Replace pnpm commands with bun equivalents. |
| docs/features/acp-v1-reliability/spec.md | Replace pnpm commands with bun equivalents. |
| docs/features/acp-v1-reliability/plan.md | Replace pnpm commands with bun equivalents in gate script block. |
| docs/archives/windows-arm64-support/tasks.md | Mark remaining archival item complete with evidence notes. |
| docs/archives/tape-trace-ui/tasks.md | Update archival completion notes to bun + TypeScript 7 context. |
| docs/archives/splash-cinematic-reveal/tasks.md | Close manual verification item with code-complete rationale. |
| docs/archives/skill-draft-confirmation-card/tasks.md | Close quality gates item with bun-based verification notes. |
| docs/archives/remote-agent-switch/tasks.md | Close manual e2e item as covered by tests (archival note). |
| docs/archives/electron-vite-to-vite-plugin-electron/tasks.md | Add “complete/archived” banner and preserve historical plan text below. |
| docs/archives/electron-vite-to-vite-plugin-electron/spec.md | Archive spec document content (migrated goal). |
| docs/archives/electron-vite-to-vite-plugin-electron/plan.md | Archive implementation plan document content (migrated goal). |
| docs/architecture/tape-subsystem/spec.md | Replace pnpm gate references with bun equivalents. |
| docs/architecture/memory-subsystem/tasks.md | Replace pnpm gate references with bun equivalents. |
| docs/architecture/extract-ui/tasks.md | Replace pnpm launch/build instructions with bun equivalents. |
| docs/architecture/extract-ui/plan.md | Replace pnpm guidance with bun equivalents and update tsgo wording. |
| docs/architecture/baselines/test-failure-groups.md | Update baseline test command references to bun. |
| apps/desktop/test/main/presenter/argosAgentRuntime/argosAgentRuntime.test.ts | Add regression tests for orchestrator seeding and extension policy semantics. |
| apps/daemon/test/piAgentProfileManager.test.ts | Update constructor signature and add managed skills persistence/validation tests. |
| apps/daemon/test/daemonArgosAgentRuntime.test.ts | Update daemon runtime tests to expect both built-in agents. |
| apps/daemon/test/argosOrchestrationRuntime.test.ts | New tests for provisioning tool exposure and delegation via injected actions. |
| apps/daemon/src/index.ts | Seed orchestrator on daemon start; wire provisioning actions + validation/rollback logic. |
| apps/daemon/src/host/piAgentProfileManager.ts | Add managed skills directory + registry, hashing, and Pi settings skill-location registration. |
| apps/daemon/src/host/daemonArgosAgentRuntime.ts | Expose ensureBuiltinOrchestratorAgent() on daemon host wrapper. |
| apps/daemon/src/host/argosOrchestrationRuntime.ts | Add provisioning tool definitions + call dispatch via injected provisioning actions. |
| AGENTS.md | Update command/toolchain guidance (bun start; TypeScript 7 note). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+149
to
+152
| const skillPath = path.join(skillDir, "SKILL.md"); | ||
| const temporaryPath = `${skillPath}.${process.pid}.${Date.now()}.tmp`; | ||
| fs.writeFileSync(temporaryPath, content, "utf8"); | ||
| fs.renameSync(temporaryPath, skillPath); |
Comment on lines
+332
to
+334
| const temporaryPath = `${registryPath}.${process.pid}.${Date.now()}.tmp`; | ||
| fs.writeFileSync(temporaryPath, `${JSON.stringify({ version: 1, skills }, null, 2)}\n`, "utf8"); | ||
| fs.renameSync(temporaryPath, registryPath); |
Comment on lines
+379
to
+385
| const ArgosSubagentSlotSchema = zod.object({ | ||
| id: zod.string().min(1), | ||
| targetType: zod.enum(["self", "agent"]), | ||
| targetAgentId: zod.string().min(1).optional(), | ||
| displayName: zod.string(), | ||
| description: zod.string(), | ||
| }); |
Comment on lines
+461
to
+468
| const effectiveConfig = await configPresenter.resolveArgosAgentConfig(agentId); | ||
| const configuredServers = (await configPresenter.getMcpServers()) as Record<string, any>; | ||
| const expectedServers = effectiveConfig.enabledMcpServerIds ?? []; | ||
| const managedSkills = piProfiles.validateManagedSkills(agentId); | ||
| const expectedSkills = agent.config?.enabledSkillNames ?? []; | ||
| const checks = [ | ||
| { name: "model", ok: Boolean(effectiveConfig.defaultModelPreset?.modelId) }, | ||
| { name: "enabled", ok: !requireEnabled || agent.enabled }, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
apps/daemon/src/index.ts (1)
494-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hard-coded
"argos"id with the exported constant.
BUILTIN_ARGOS_AGENT_IDis exported from@argos/agent-runtimeand is already re-exported by that package's index. Use it here so that the guard cannot drift from the runtime definition.♻️ Proposed change
async updateAgent(agentId, updates) { - if (agentId === "argos") throw new Error("The protected default Argos agent cannot be changed by provisioning."); + if (agentId === BUILTIN_ARGOS_AGENT_ID) { + throw new Error("The protected default Argos agent cannot be changed by provisioning."); + }Add the import to the existing
@argos/agent-runtimeimport statement:-import { ... } from "`@argos/agent-runtime`"; +import { BUILTIN_ARGOS_AGENT_ID, ... } from "`@argos/agent-runtime`";🤖 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 `@apps/daemon/src/index.ts` around lines 494 - 499, In the updateAgent method, import and use the exported BUILTIN_ARGOS_AGENT_ID from `@argos/agent-runtime` instead of the hard-coded "argos" literal in the protected-agent guard, preserving the existing error behavior.
🤖 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 `@apps/daemon/src/host/argosOrchestrationRuntime.ts`:
- Around line 354-364: Validate required string arguments before coercion in the
argos_agents_create and argos_agents_update branches: require a non-empty string
for args.name and args.agentId, respectively, and throw a clear error when
validation fails. Do not use String() until after validation; preserve the
existing optional-field handling and update payload behavior.
- Around line 126-147: Gate argos_mcp_server_upsert before persisting or
starting new stdio MCP servers by requiring confirmation or an approved
allowlist for configurations containing type "stdio". Update the
argos_mcp_server_upsert handler and its mcpRuntime.startServer flow, preserving
existing behavior for approved definitions and non-stdio server types while
preventing unapproved model-specified commands, arguments, or environment values
from executing.
In `@apps/daemon/src/host/piAgentProfileManager.ts`:
- Around line 318-327: Update readManagedSkillsRegistry to avoid silently
returning an empty registry when JSON parsing fails: propagate the parse error
or otherwise preserve the existing corrupt registry and report the failure. Keep
the missing-file behavior returning an empty list, while ensuring
validateManagedSkills and subsequent writeManagedSkill operations cannot treat
malformed data as a valid empty registry.
In `@apps/daemon/src/index.ts`:
- Around line 446-457: Update redactMcpServers to prevent credentials in fields
beyond env and customHeaders from reaching the orchestrator model. Use an
allowlist of explicitly non-sensitive MCP configuration fields, or
comprehensively redact credential-bearing values including args, baseUrl, and
additional header maps, while preserving the server structure needed by
listMcpServers.
- Around line 625-641: Isolate every rollback operation in the provisioning
catch block so one failure cannot prevent subsequent cleanup: guard the
synchronous piProfiles.removeProfile call, and handle failures from
updateMcpServer, removeMcpServer, and related MCP restore steps while continuing
through all snapshots and setMcpEnabled. Collect rollback errors and include
their details in the final thrown “Agent provisioning rolled back” error,
distinguishing partial rollback failures from the original provisioning error.
In `@docs/architecture/extract-ui/tasks.md`:
- Line 17: Update the “E2E launch” task to start the development server using
the repository’s filtered managed-background-process pattern rather than running
bun run dev in the foreground. Include log capture, readiness polling before
validating the UI, and PID-based shutdown/cleanup after the check.
In `@docs/archives/electron-vite-to-vite-plugin-electron/spec.md`:
- Around line 78-80: Update
docs/archives/electron-vite-to-vite-plugin-electron/spec.md lines 78-80 to
restrict build-time define inlining to public values and avoid exposing private
credentials through VITE_* defines; update lines 39-40 to remove private OAuth
and database values from the public environment contract; update
docs/archives/electron-vite-to-vite-plugin-electron/plan.md lines 45-46 to
remove the claim that this approach protects sensitive secrets.
In `@docs/archives/remote-agent-switch/tasks.md`:
- Around line 10-13: Keep the manual validation tasks open when their checks
were unavailable: in docs/archives/remote-agent-switch/tasks.md lines 10-13,
leave the live-channel e2e task deferred until a channel run completes; in
docs/archives/splash-cinematic-reveal/tasks.md lines 11-14, leave the visual
sign-off deferred until human verification completes.
In `@docs/archives/skill-draft-confirmation-card/tasks.md`:
- Line 10: Update the checklist entry to accurately record formatter
verification: run bun run format and document its result alongside the existing
lint check, or explicitly state that only bun run format:check was performed. Do
not claim the format command ran unless it was actually executed.
In `@docs/features/acp-v1-reliability/plan.md`:
- Around line 345-346: Update the test commands in the reliability plan to use
the focused desktop main-process script, test:main, instead of the broad test
wrapper, and retain an explicit acpProvider.test.ts filter for the ACP provider
test.
In `@docs/issues/remove-china-specific-defaults/tasks.md`:
- Line 7: Remove the unavailable “bun run i18n” quality gate from the listed
documents: docs/issues/remove-china-specific-defaults/tasks.md:7-7,
docs/features/acp-v1-reliability/plan.md:342-342,
docs/features/acp-v1-reliability/spec.md:41-41,
docs/issues/prcheck-format-onboarding/plan.md:14-14,
docs/issues/react-doctor-top-3/plan.md:12-12, and
docs/issues/session-list-stable-alphabetical-sort/plan.md:24-24. Keep only
validation commands that exist in package.json; no package.json change is
requested.
In `@docs/issues/telegram-message-markdown-render/plan.md`:
- Around line 20-21: Update the test command in the plan to reference the
desktop test file via the full repository-relative path
apps/desktop/test/main/presenter/remoteControlPresenter/telegramClient.test.ts,
or use the equivalent `@argos/desktop-scoped` command with test/main/....
In `@packages/agent-runtime/src/argosAgentRuntime.ts`:
- Around line 106-113: Update the built-in orchestrator configuration merge in
the relevant initialization and update paths to reassert only mandatory
capability flags, such as disabledAgentTools, while preserving user-provided
systemPrompt and permissionMode values. Use BUILTIN_ARGOS_ORCHESTRATOR_CONFIG
only as defaults for those editable fields, and ensure both stored configuration
and requested updates retain user edits while mandatory flags remain enabled.
In `@packages/agent-runtime/src/configMerge.ts`:
- Around line 18-20: Update mergeArgosConfig so enabledMcpServerIds,
enabledPluginIds, and enabledSkillNames fall back to an empty array when neither
overrideConfig nor baseConfig provides a value. Preserve configured override and
base values while ensuring undefined never represents an unrestricted allowlist.
---
Nitpick comments:
In `@apps/daemon/src/index.ts`:
- Around line 494-499: In the updateAgent method, import and use the exported
BUILTIN_ARGOS_AGENT_ID from `@argos/agent-runtime` instead of the hard-coded
"argos" literal in the protected-agent guard, preserving the existing error
behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fcf729f-e5dc-4e42-a24c-22825f3d8dae
📒 Files selected for processing (78)
AGENTS.mdREADME.mdapps/daemon/src/host/argosOrchestrationRuntime.tsapps/daemon/src/host/daemonArgosAgentRuntime.tsapps/daemon/src/host/piAgentProfileManager.tsapps/daemon/src/index.tsapps/daemon/test/argosOrchestrationRuntime.test.tsapps/daemon/test/daemonArgosAgentRuntime.test.tsapps/daemon/test/piAgentProfileManager.test.tsapps/desktop/test/main/presenter/argosAgentRuntime/argosAgentRuntime.test.tsdocs/architecture/baselines/test-failure-groups.mddocs/architecture/extract-ui/plan.mddocs/architecture/extract-ui/tasks.mddocs/architecture/memory-subsystem/tasks.mddocs/architecture/tape-subsystem/spec.mddocs/archives/electron-vite-to-vite-plugin-electron/plan.mddocs/archives/electron-vite-to-vite-plugin-electron/spec.mddocs/archives/electron-vite-to-vite-plugin-electron/tasks.mddocs/archives/remote-agent-switch/tasks.mddocs/archives/skill-draft-confirmation-card/tasks.mddocs/archives/splash-cinematic-reveal/tasks.mddocs/archives/tape-trace-ui/tasks.mddocs/archives/windows-arm64-support/tasks.mddocs/features/acp-v1-reliability/plan.mddocs/features/acp-v1-reliability/spec.mddocs/features/acp-v1-reliability/tasks.mddocs/features/agent-state-semantics/plan.mddocs/features/builtin-orchestrator-agent/plan.mddocs/features/builtin-orchestrator-agent/spec.mddocs/features/builtin-orchestrator-agent/tasks.mddocs/features/daemon-self-update/plan.mddocs/features/daemon-self-update/tasks.mddocs/features/self-configurable-orchestrator/plan.mddocs/features/self-configurable-orchestrator/spec.mddocs/features/self-configurable-orchestrator/tasks.mddocs/issues/acp-existing-thread-inline-options/plan.mddocs/issues/acp-operational-health-check/tasks.mddocs/issues/agent-exec-utility-process-crash/tasks.mddocs/issues/browser-settings-desktop-only-tabs/plan.mddocs/issues/browser-web-bootstrap-stuck/plan.mddocs/issues/cua-driver-v0-2-0-sync/plan.mddocs/issues/daemon-browser-provider-catalog/plan.mddocs/issues/daemon-disconnect-visibility/tasks.mddocs/issues/daemon-provider-model-backend/plan.mddocs/issues/daemon-provider-model-backend/tasks.mddocs/issues/daemon-tier2-coming-soon-routes/plan.mddocs/issues/daemon-tier2-coming-soon-routes/spec.mddocs/issues/daemon-tier2-coming-soon-routes/tasks.mddocs/issues/floating-button-position-persistence/tasks.mddocs/issues/guided-onboarding-first-chat-confirm/plan.mddocs/issues/mac-native-feel-audit/tasks.mddocs/issues/main-logger-console-recursion/spec.mddocs/issues/mcp-server-start-regression/tasks.mddocs/issues/merged-activity-groups/plan.mddocs/issues/onboarding-provider-mcp-handoff/plan.mddocs/issues/onboarding-provider-mcp-handoff/tasks.mddocs/issues/openai-compatible-video-prompt-duration-fallback/plan.mddocs/issues/openai-compatible-video-prompt-duration-fallback/tasks.mddocs/issues/prcheck-format-onboarding/plan.mddocs/issues/prcheck-format-onboarding/spec.mddocs/issues/react-doctor-top-3/plan.mddocs/issues/remove-china-specific-defaults/tasks.mddocs/issues/scheduled-tasks-loading-loop/plan.mddocs/issues/session-list-stable-alphabetical-sort/plan.mddocs/issues/session-list-stable-alphabetical-sort/tasks.mddocs/issues/settings-navigation-selection-lag/plan.mddocs/issues/telegram-message-markdown-render/plan.mddocs/issues/telegram-message-markdown-render/tasks.mddocs/issues/thought-block-visual-alignment/plan.mddocs/issues/usage-dashboard-empty-state/plan.mddocs/issues/windows-arm64-duckdb-upgrade/plan.mddocs/issues/windows-arm64-duckdb-upgrade/tasks.mddocs/issues/windows-release-build-arch/plan.mddocs/issues/yobrowser-cdp-graceful-degradation/tasks.mdpackages/agent-runtime/src/argosAgentRuntime.tspackages/agent-runtime/src/configMerge.tspackages/agent-runtime/src/index.tspackages/shared-contracts/src/domainSchemas.ts
Comment on lines
+345
to
+346
| bun run test -- test/main/presenter/llmProviderPresenter | ||
| bun run test -- test/main/presenter/acpProvider.test.ts |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json scripts =="
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({scripts:p.scripts || {}, workspaceMode:p.workspaceMode, devDependencies:p.devDependencies, dependencies:p.dependencies}, null, 2))"
fi
echo
echo "== turbo/package config files =="
git ls-files | rg '(^|/)(package\.json|turbo\.json|bun\.lockb?$|bun\.lock$|vercel\.json$|pnpm-workspace\.yaml$|package\.lock$|yarn\.lock$|package\.json)$' | sed -n '1,120p'
echo
echo "== files around docs/features/acp-v1-reliability/plan.md =="
git ls-files docs/features/acp-v1-reliability/plan.md
sed -n '320,365p' docs/features/acp-v1-reliability/plan.md 2>/dev/null || true
echo
echo "== package manager/workspace references to scripts or turbo =="
rg -n '"(test|turbo|vitest|--filter|--)":"|turbo|\\btest\\b|vitest' package.json .github bun.lock 2>/dev/null | sed -n '1,220p' || true
echo
echo "== inspect docs command context =="
rg -n "bun run test -- test/main/presenter/llmProviderPresenter|bun run test -- test/main/presenter/acpProvider" docs/features/acp-v1-reliability/plan.mdRepository: dvaJi/argos
Length of output: 16773
🌐 Web query:
Turbo 2.10.5 turbo run script -- extra arguments forward passed through
💡 Result:
To pass extra arguments through to the underlying scripts when using Turborepo (including version 2.10.5), you should use the double-dash separator (--) [1][2]. Any arguments provided after this separator will be forwarded directly to the tasks being executed [1][2]. The standard syntax is: turbo run [tasks] -- [args passed to tasks] For example, if you want to pass a flag to a lint script, you would run: turbo lint -- --fix Special Considerations for Yarn v1: If you are using Yarn v1 (classic), the package manager may intercept the first -- separator [1][3]. In this environment, you often need to provide two sets of double dashes to ensure the arguments are correctly passed through to the underlying script [1][3]: yarn turbo run lint -- -- --fix This works because Yarn v1 consumes the first -- before passing the remaining arguments to the Turborepo command [1][3]. Later versions of Yarn and other package managers like npm and pnpm typically do not require this extra separator and will function correctly with a single -- [4].
Citations:
- 1: https://turborepo.dev/docs/reference/run
- 2: https://github.com/vercel/turbo/blob/main/skills/turborepo/references/cli/RULE.md
- 3: https://v2-9-1-canary-1.turborepo.dev/docs/reference/run
- 4: [turborepo] passing arguments to running tasks leads to redundant warnings (yarn) vercel/turborepo#5702
🌐 Web query:
Bun run script -- forwarded arguments to script via package scripts turbo
💡 Result:
To pass arguments to a script when using Bun and Turborepo, use the double-dash (--) syntax. This signals that any subsequent arguments should be forwarded directly to the underlying command [1][2]. General Syntax The standard way to pass arguments through both Bun and Turborepo to an underlying task is: bun run -- When Turborepo is involved, the arguments are passed to the task defined in your package.json. If your script is a wrapper for a turbo command (e.g., "build": "turbo run build"), you may need an additional double-dash to ensure the arguments reach the final underlying task [1][3]: bun run build -- -- Key Considerations 1. Forwarding Behavior: Any flags placed after the first -- are treated as passthrough arguments for the script [4][5]. 2. Turborepo Specifics: Turborepo uses -- to separate its own options from arguments passed to the underlying tasks [1][2]. If you are chaining commands (e.g., a package.json script runs turbo, which runs a build script), you might need to use -- multiple times to "drill" the arguments down to the target process [3]. 3. Shell Behavior: Bun executes scripts in a subshell (bash, zsh, or sh on Unix; Bun Shell on Windows) [4][5]. While Bun aims to behave like other package managers, there are known historical differences regarding how shell features (like
Citations:
- 1: https://turborepo.dev/docs/reference/run
- 2: https://v2-8-6.turborepo.dev/docs/reference/run
- 3: Can't pass arguments to the underlying command when using
--filtervercel/turborepo#1355 - 4: https://bun.sh/docs/runtime
- 5: https://github.com/oven-sh/bun/blob/1498d7b7/docs/runtime/index.mdx
- 6: bun runs package.json scripts without a shell, breaking $1, $@ and possibly other shell features oven-sh/bun#22127
- 7: console argument in quotes is split by space oven-sh/bun#6780
- 8: Pre- and post- scripts erroneously receive arguments oven-sh/bun#5199
🌐 Web query:
Turborepo 2 script args turbo run script_name -- args passed to npm/yarn package script
💡 Result:
To pass arguments to the underlying scripts in Turborepo 2, use the -- separator [1][2]. Any arguments placed after -- will be passed directly to the tasks specified in the turbo run command [1][3]. Basic Syntax: turbo run <script_name> -- <args_passed_to_scripts> For example, to pass the --watch flag to a test script: turbo run test -- --watch [2] Key Behavior and Considerations: 1. Target Tasks: All arguments provided after -- are passed to all tasks named in the run command [1][3]. 2. Yarn v1 Requirement: If you are using Yarn v1, the package manager consumes the first -- [1][4]. To successfully pass arguments through to the underlying script, you must use two sets of double dashes [1][5]: yarn turbo run <script_name> -- -- -- 3. Script Definition: If you are calling turbo via an npm/yarn script in your package.json, ensure you include -- at the end of the turbo command so that the package manager knows to pass subsequent arguments through to the underlying script [6]: "scripts": { "test": "turbo run test --" } Running npm run test -- --watch will then correctly pass --watch to the task.
Citations:
- 1: https://turborepo.dev/docs/reference/run
- 2: https://github.com/vercel/turbo/blob/main/skills/turborepo/references/cli/RULE.md
- 3: docs(run): fix passthrough arg behavior vercel/turborepo#10167
- 4: https://v2-8-19-canary-3.turborepo.dev/docs/reference/run
- 5: https://v2-9-1-canary-1.turborepo.dev/docs/reference/run
- 6: Can't pass arguments to the underlying command when using
--filtervercel/turborepo#1355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/desktop/test scripts and vitest config references =="
node -e "const fs=require('fs'); const p=JSON.parse(fs.readFileSync('apps/desktop/package.json','utf8')); console.log(JSON.stringify({scripts:p.scripts || {}}, null, 2))"
echo
rg -n "vitest|test:main|test|turbo run test|turbo run test:" apps/desktop/package.json apps/desktop/turbo.json packages apps/desktop/test 2>/dev/null | sed -n '1,220p'
echo
echo "== root/app desktop turbo files =="
cat -n turbo.json | sed -n '1,180p'
cat -n apps/desktop/turbo.json | sed -n '1,180p'
echo
echo "== Vitest config files =="
git ls-files | rg '(^|/)vitest\.config\.(ts|js|mjs|mts|cjs)$|vitest\.config'Repository: dvaJi/argos
Length of output: 26153
Use the focused desktop test script for these steps.
test runs Turbo for both @argos/desktop and @argos/daemon, while the test:main script is fixed to apps/desktop/test/main. These steps need a focused desktop main-process command and an explicit acpProvider.test.ts filter, not another test wrapper.
🤖 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 `@docs/features/acp-v1-reliability/plan.md` around lines 345 - 346, Update the
test commands in the reliability plan to use the focused desktop main-process
script, test:main, instead of the broad test wrapper, and retain an explicit
acpProvider.test.ts filter for the ACP provider test.
Comment on lines
+20
to
+21
| - Run `bun run test test/main/presenter/remoteControlPresenter/telegramClient.test.ts` (extended) and a new `telegramMarkdown.test.ts` covering core conversion rules, table fallback, and chunk-boundary behavior. | ||
| - Run `bun run typecheck:node` to confirm no signature break in callers (Poller, Adapter). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate files =="
git ls-files | rg '(^docs/issues/telegram-message-markdown-render/plan\.md$|^package\.json$|turbo\.json$|bun\.workspaces|apps/desktop)' | sed -n '1,120p'
echo
echo "== Plan file context =="
cat -n docs/issues/telegram-message-markdown-render/plan.md | sed -n '1,60p'
echo
echo "== Repo/workspace configuration =="
for f in package.json bun.workspaces turbo.json apps/desktop/package.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
done
echo
echo "== Focused test path existence =="
for p in apps/desktop/test/main/presenter/remoteControlPresenter/telegramClient.test.ts \
test/main/presenter/remoteControlPresenter/telegramClient.test.ts; do
[ -f "$p" ] && echo "FOUND $p" || echo "MISSING $p"
done
echo
echo "== Test command occurrences in docs and js/ts files =="
rg -n "bun run test|test/main/presenter/remoteControlPresenter/telegramClient\.test\.ts|apps/desktop/test/main/presenter/remoteControlPresenter/telegramClient\.test\.ts" -S "$PWD" || trueRepository: dvaJi/argos
Length of output: 24888
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Desktop vitest config =="
sed -n '1,220p' apps/desktop/vitest.config.ts
echo
echo "== Root vitest config candidates =="
for f in vitest.config.ts vitest.config.mjs apps/desktop/vitest.config.ts apps/daemon/vitest.config.ts; do
[ -f "$f" ] && echo "--- $f ---" && sed -n '1,220p' "$f"
done
echo
echo "== Static path availability =="
python3 - <<'PY'
import pathlib
p=pathlib.Path("test/main/presenter/remoteControlPresenter/telegramClient.test.ts")
ap=pathlib.Path("apps/desktop/test/main/presenter/remoteControlPresenter/telegramClient.test.ts")
for path in (p, ap):
print(f"{path}: file={path.is_file()}, from root exists={path.exists()},"
f" resolved={path.resolve()}")
PYRepository: dvaJi/argos
Length of output: 5225
Use a path that points to the desktop test file.
From the repository root, test/main/presenter/remoteControlPresenter/telegramClient.test.ts does not exist; the file is under apps/desktop/test/main/presenter/remoteControlPresenter/telegramClient.test.ts. Use the full repository-relative path, or a command scoped to @argos/desktop with test/main/....
🤖 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 `@docs/issues/telegram-message-markdown-render/plan.md` around lines 20 - 21,
Update the test command in the plan to reference the desktop test file via the
full repository-relative path
apps/desktop/test/main/presenter/remoteControlPresenter/telegramClient.test.ts,
or use the equivalent `@argos/desktop-scoped` command with test/main/....
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 14
🧹 Nitpick comments (1)
apps/daemon/src/index.ts (1)
494-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hard-coded
"argos"id with the exported constant.
BUILTIN_ARGOS_AGENT_IDis exported from@argos/agent-runtimeand is already re-exported by that package's index. Use it here so that the guard cannot drift from the runtime definition.♻️ Proposed change
async updateAgent(agentId, updates) { - if (agentId === "argos") throw new Error("The protected default Argos agent cannot be changed by provisioning."); + if (agentId === BUILTIN_ARGOS_AGENT_ID) { + throw new Error("The protected default Argos agent cannot be changed by provisioning."); + }Add the import to the existing
@argos/agent-runtimeimport statement:-import { ... } from "`@argos/agent-runtime`"; +import { BUILTIN_ARGOS_AGENT_ID, ... } from "`@argos/agent-runtime`";🤖 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 `@apps/daemon/src/index.ts` around lines 494 - 499, In the updateAgent method, import and use the exported BUILTIN_ARGOS_AGENT_ID from `@argos/agent-runtime` instead of the hard-coded "argos" literal in the protected-agent guard, preserving the existing error behavior.
🤖 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 `@apps/daemon/src/host/argosOrchestrationRuntime.ts`:
- Around line 354-364: Validate required string arguments before coercion in the
argos_agents_create and argos_agents_update branches: require a non-empty string
for args.name and args.agentId, respectively, and throw a clear error when
validation fails. Do not use String() until after validation; preserve the
existing optional-field handling and update payload behavior.
- Around line 126-147: Gate argos_mcp_server_upsert before persisting or
starting new stdio MCP servers by requiring confirmation or an approved
allowlist for configurations containing type "stdio". Update the
argos_mcp_server_upsert handler and its mcpRuntime.startServer flow, preserving
existing behavior for approved definitions and non-stdio server types while
preventing unapproved model-specified commands, arguments, or environment values
from executing.
In `@apps/daemon/src/host/piAgentProfileManager.ts`:
- Around line 318-327: Update readManagedSkillsRegistry to avoid silently
returning an empty registry when JSON parsing fails: propagate the parse error
or otherwise preserve the existing corrupt registry and report the failure. Keep
the missing-file behavior returning an empty list, while ensuring
validateManagedSkills and subsequent writeManagedSkill operations cannot treat
malformed data as a valid empty registry.
In `@apps/daemon/src/index.ts`:
- Around line 446-457: Update redactMcpServers to prevent credentials in fields
beyond env and customHeaders from reaching the orchestrator model. Use an
allowlist of explicitly non-sensitive MCP configuration fields, or
comprehensively redact credential-bearing values including args, baseUrl, and
additional header maps, while preserving the server structure needed by
listMcpServers.
- Around line 625-641: Isolate every rollback operation in the provisioning
catch block so one failure cannot prevent subsequent cleanup: guard the
synchronous piProfiles.removeProfile call, and handle failures from
updateMcpServer, removeMcpServer, and related MCP restore steps while continuing
through all snapshots and setMcpEnabled. Collect rollback errors and include
their details in the final thrown “Agent provisioning rolled back” error,
distinguishing partial rollback failures from the original provisioning error.
In `@docs/architecture/extract-ui/tasks.md`:
- Line 17: Update the “E2E launch” task to start the development server using
the repository’s filtered managed-background-process pattern rather than running
bun run dev in the foreground. Include log capture, readiness polling before
validating the UI, and PID-based shutdown/cleanup after the check.
In `@docs/archives/electron-vite-to-vite-plugin-electron/spec.md`:
- Around line 78-80: Update
docs/archives/electron-vite-to-vite-plugin-electron/spec.md lines 78-80 to
restrict build-time define inlining to public values and avoid exposing private
credentials through VITE_* defines; update lines 39-40 to remove private OAuth
and database values from the public environment contract; update
docs/archives/electron-vite-to-vite-plugin-electron/plan.md lines 45-46 to
remove the claim that this approach protects sensitive secrets.
In `@docs/archives/remote-agent-switch/tasks.md`:
- Around line 10-13: Keep the manual validation tasks open when their checks
were unavailable: in docs/archives/remote-agent-switch/tasks.md lines 10-13,
leave the live-channel e2e task deferred until a channel run completes; in
docs/archives/splash-cinematic-reveal/tasks.md lines 11-14, leave the visual
sign-off deferred until human verification completes.
In `@docs/archives/skill-draft-confirmation-card/tasks.md`:
- Line 10: Update the checklist entry to accurately record formatter
verification: run bun run format and document its result alongside the existing
lint check, or explicitly state that only bun run format:check was performed. Do
not claim the format command ran unless it was actually executed.
In `@docs/features/acp-v1-reliability/plan.md`:
- Around line 345-346: Update the test commands in the reliability plan to use
the focused desktop main-process script, test:main, instead of the broad test
wrapper, and retain an explicit acpProvider.test.ts filter for the ACP provider
test.
In `@docs/issues/remove-china-specific-defaults/tasks.md`:
- Line 7: Remove the unavailable “bun run i18n” quality gate from the listed
documents: docs/issues/remove-china-specific-defaults/tasks.md:7-7,
docs/features/acp-v1-reliability/plan.md:342-342,
docs/features/acp-v1-reliability/spec.md:41-41,
docs/issues/prcheck-format-onboarding/plan.md:14-14,
docs/issues/react-doctor-top-3/plan.md:12-12, and
docs/issues/session-list-stable-alphabetical-sort/plan.md:24-24. Keep only
validation commands that exist in package.json; no package.json change is
requested.
In `@docs/issues/telegram-message-markdown-render/plan.md`:
- Around line 20-21: Update the test command in the plan to reference the
desktop test file via the full repository-relative path
apps/desktop/test/main/presenter/remoteControlPresenter/telegramClient.test.ts,
or use the equivalent `@argos/desktop-scoped` command with test/main/....
In `@packages/agent-runtime/src/argosAgentRuntime.ts`:
- Around line 106-113: Update the built-in orchestrator configuration merge in
the relevant initialization and update paths to reassert only mandatory
capability flags, such as disabledAgentTools, while preserving user-provided
systemPrompt and permissionMode values. Use BUILTIN_ARGOS_ORCHESTRATOR_CONFIG
only as defaults for those editable fields, and ensure both stored configuration
and requested updates retain user edits while mandatory flags remain enabled.
In `@packages/agent-runtime/src/configMerge.ts`:
- Around line 18-20: Update mergeArgosConfig so enabledMcpServerIds,
enabledPluginIds, and enabledSkillNames fall back to an empty array when neither
overrideConfig nor baseConfig provides a value. Preserve configured override and
base values while ensuring undefined never represents an unrestricted allowlist.
---
Nitpick comments:
In `@apps/daemon/src/index.ts`:
- Around line 494-499: In the updateAgent method, import and use the exported
BUILTIN_ARGOS_AGENT_ID from `@argos/agent-runtime` instead of the hard-coded
"argos" literal in the protected-agent guard, preserving the existing error
behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fcf729f-e5dc-4e42-a24c-22825f3d8dae
📒 Files selected for processing (78)
AGENTS.mdREADME.mdapps/daemon/src/host/argosOrchestrationRuntime.tsapps/daemon/src/host/daemonArgosAgentRuntime.tsapps/daemon/src/host/piAgentProfileManager.tsapps/daemon/src/index.tsapps/daemon/test/argosOrchestrationRuntime.test.tsapps/daemon/test/daemonArgosAgentRuntime.test.tsapps/daemon/test/piAgentProfileManager.test.tsapps/desktop/test/main/presenter/argosAgentRuntime/argosAgentRuntime.test.tsdocs/architecture/baselines/test-failure-groups.mddocs/architecture/extract-ui/plan.mddocs/architecture/extract-ui/tasks.mddocs/architecture/memory-subsystem/tasks.mddocs/architecture/tape-subsystem/spec.mddocs/archives/electron-vite-to-vite-plugin-electron/plan.mddocs/archives/electron-vite-to-vite-plugin-electron/spec.mddocs/archives/electron-vite-to-vite-plugin-electron/tasks.mddocs/archives/remote-agent-switch/tasks.mddocs/archives/skill-draft-confirmation-card/tasks.mddocs/archives/splash-cinematic-reveal/tasks.mddocs/archives/tape-trace-ui/tasks.mddocs/archives/windows-arm64-support/tasks.mddocs/features/acp-v1-reliability/plan.mddocs/features/acp-v1-reliability/spec.mddocs/features/acp-v1-reliability/tasks.mddocs/features/agent-state-semantics/plan.mddocs/features/builtin-orchestrator-agent/plan.mddocs/features/builtin-orchestrator-agent/spec.mddocs/features/builtin-orchestrator-agent/tasks.mddocs/features/daemon-self-update/plan.mddocs/features/daemon-self-update/tasks.mddocs/features/self-configurable-orchestrator/plan.mddocs/features/self-configurable-orchestrator/spec.mddocs/features/self-configurable-orchestrator/tasks.mddocs/issues/acp-existing-thread-inline-options/plan.mddocs/issues/acp-operational-health-check/tasks.mddocs/issues/agent-exec-utility-process-crash/tasks.mddocs/issues/browser-settings-desktop-only-tabs/plan.mddocs/issues/browser-web-bootstrap-stuck/plan.mddocs/issues/cua-driver-v0-2-0-sync/plan.mddocs/issues/daemon-browser-provider-catalog/plan.mddocs/issues/daemon-disconnect-visibility/tasks.mddocs/issues/daemon-provider-model-backend/plan.mddocs/issues/daemon-provider-model-backend/tasks.mddocs/issues/daemon-tier2-coming-soon-routes/plan.mddocs/issues/daemon-tier2-coming-soon-routes/spec.mddocs/issues/daemon-tier2-coming-soon-routes/tasks.mddocs/issues/floating-button-position-persistence/tasks.mddocs/issues/guided-onboarding-first-chat-confirm/plan.mddocs/issues/mac-native-feel-audit/tasks.mddocs/issues/main-logger-console-recursion/spec.mddocs/issues/mcp-server-start-regression/tasks.mddocs/issues/merged-activity-groups/plan.mddocs/issues/onboarding-provider-mcp-handoff/plan.mddocs/issues/onboarding-provider-mcp-handoff/tasks.mddocs/issues/openai-compatible-video-prompt-duration-fallback/plan.mddocs/issues/openai-compatible-video-prompt-duration-fallback/tasks.mddocs/issues/prcheck-format-onboarding/plan.mddocs/issues/prcheck-format-onboarding/spec.mddocs/issues/react-doctor-top-3/plan.mddocs/issues/remove-china-specific-defaults/tasks.mddocs/issues/scheduled-tasks-loading-loop/plan.mddocs/issues/session-list-stable-alphabetical-sort/plan.mddocs/issues/session-list-stable-alphabetical-sort/tasks.mddocs/issues/settings-navigation-selection-lag/plan.mddocs/issues/telegram-message-markdown-render/plan.mddocs/issues/telegram-message-markdown-render/tasks.mddocs/issues/thought-block-visual-alignment/plan.mddocs/issues/usage-dashboard-empty-state/plan.mddocs/issues/windows-arm64-duckdb-upgrade/plan.mddocs/issues/windows-arm64-duckdb-upgrade/tasks.mddocs/issues/windows-release-build-arch/plan.mddocs/issues/yobrowser-cdp-graceful-degradation/tasks.mdpackages/agent-runtime/src/argosAgentRuntime.tspackages/agent-runtime/src/configMerge.tspackages/agent-runtime/src/index.tspackages/shared-contracts/src/domainSchemas.ts
🛑 Comments failed to post (1)
docs/archives/electron-vite-to-vite-plugin-electron/spec.md (1)
78-80: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not expose private credentials through
VITE_*build defines.Both documents describe private values as secrets and require build-time inlining. The supplied Vite configuration places every
VITE_*value in the packaged main bundle.
docs/archives/electron-vite-to-vite-plugin-electron/spec.md#L78-L80: restrict build-time inlining to public values.docs/archives/electron-vite-to-vite-plugin-electron/spec.md#L39-L40: remove private OAuth and database values from the public environment contract.docs/archives/electron-vite-to-vite-plugin-electron/plan.md#L45-L46: remove the claim that this approach protects sensitive secrets.📍 Affects 2 files
docs/archives/electron-vite-to-vite-plugin-electron/spec.md#L78-L80(this comment)docs/archives/electron-vite-to-vite-plugin-electron/spec.md#L39-L40docs/archives/electron-vite-to-vite-plugin-electron/plan.md#L45-L46🤖 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 `@docs/archives/electron-vite-to-vite-plugin-electron/spec.md` around lines 78 - 80, Update docs/archives/electron-vite-to-vite-plugin-electron/spec.md lines 78-80 to restrict build-time define inlining to public values and avoid exposing private credentials through VITE_* defines; update lines 39-40 to remove private OAuth and database values from the public environment contract; update docs/archives/electron-vite-to-vite-plugin-electron/plan.md lines 45-46 to remove the claim that this approach protects sensitive secrets.
Address review feedback: there is no \i18n\ script in package.json, so the 25 \�un run i18n\ references (converted from \pnpm run i18n\ by the toolchain pass) are dead. Annotate them as N/A across live docs, dedupe the already-correct remove-china entry, and sharpen the skill-draft format-gate note to state \ ormat:check\ (not \ ormat\) was the verifier.
Address AI review findings on the orchestrator feature: - configMerge: default enabledMcpServerIds/PluginIds/SkillNames to [] (was undefined = unrestricted allowlist); restores parity with the desktop AgentRepository original. - argosAgentRuntime: orchestrator re-seed/update now preserves user systemPrompt/permissionMode and only reasserts mandatory capability flags (orchestration/subagent enabled). - argosOrchestrationRuntime: reject stdio MCP server registration (arbitrary local command execution); http/sse still allowed; validate required string args before String() coercion. - index.ts: redactMcpServers via allowlist (mask args/baseUrl/unknown fields, not just env/customHeaders); isolate every provisioning rollback step and collect failures; use BUILTIN_ARGOS_AGENT_ID constant; relax validateProvisionedAgent model check to accept assistantModel. - piAgentProfileManager: throw on corrupt skills registry instead of silent empty; atomic-rename with Windows overwrite fallback for skill and registry writes. - domainSchemas: ArgosSubagentSlot requires targetAgentId for targetType 'agent' and forbids it for 'self'.
Apply oxfmt to the two daemon files touched by the prior commit (argosOrchestrationRuntime.ts, index.ts). Pure formatting (line wrapping); no logic change.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/daemon/src/host/piAgentProfileManager.ts (2)
147-164: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the skill file and registry update consistent.
writeManagedSkillreplacesSKILL.mdbefore reading and writingskills-registry.json. If a registry operation fails, the new file remains without matching metadata.removeManagedSkilldeletes the skill directory before writing the registry, so a write failure leaves metadata for a missing skill. Stage both updates and restore the previous state on failure.Also applies to: 168-177
🤖 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 `@apps/daemon/src/host/piAgentProfileManager.ts` around lines 147 - 164, The writeManagedSkill and removeManagedSkill operations must update the skill file/directory and skills-registry.json transactionally. Stage the filesystem and registry changes, and if either operation fails, restore the previous skill content or directory and registry state so they remain consistent.
124-130: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate managed-skills registry before using it.
readManagedSkillsRegistryreturns[]for valid JSON withoutskills, and it passes records with invalid fields through tolistManagedSkillsandvalidateManagedSkills. An invalid record name such as../escapereachespath.join(this.getManagedSkillsDir(agentId), record.name, "SKILL.md")and resolves outside the managed skills directory. Parse the registry asunknown, validate the envelope and each record, and reject invalid data.🤖 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 `@apps/daemon/src/host/piAgentProfileManager.ts` around lines 124 - 130, Update readManagedSkillsRegistry, and the ManagedAgentSkillRecord validation used by listManagedSkills and validateManagedSkills, to parse registry JSON as unknown and reject invalid envelopes or records, including unsafe names such as path traversal values. Require the expected skills array and validate each record’s fields before returning it; do not allow malformed data to reach path.join in validateManagedSkills.
♻️ Duplicate comments (1)
apps/daemon/src/host/argosOrchestrationRuntime.ts (1)
127-146: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not advertise unsupported MCP transports.
The tool schema advertises
stdioand local-command fields. The runtime must not accept this transport. Publish only the HTTP and SSE contract.🤖 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 `@apps/daemon/src/host/argosOrchestrationRuntime.ts` around lines 127 - 146, Update the “argos_mcp_server_upsert” tool schema to advertise only the supported “sse” and “http” transports. Remove “stdio” from the type enum and remove the local-command fields “command” and “args” from the config properties, while preserving the existing HTTP/SSE configuration fields.
🤖 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 `@apps/daemon/src/host/argosOrchestrationRuntime.ts`:
- Around line 371-382: Enforce a closed HTTP/SSE transport allowlist in
apps/daemon/src/host/argosOrchestrationRuntime.ts lines 371-382 by rejecting any
config.type other than "http" or "sse" before
requireProvisioning().upsertMcpServer, including missing and unrecognized
values; retain the existing stdio rejection intent. In
apps/daemon/src/host/argosOrchestrationRuntime.ts lines 127-146, remove stdio
and local-command fields from the published tool schema. In
apps/daemon/test/argosOrchestrationRuntime.test.ts lines 68-101, add coverage
confirming missing and unrecognized transports reject and do not call
upsertMcpServer.
In `@apps/daemon/src/host/piAgentProfileManager.ts`:
- Around line 318-330: Update atomicRename so the delete-and-retry fallback runs
only for the known Windows error indicating an existing destination; rethrow all
other fs.renameSync failures without removing targetPath. Preserve the normal
rename path and the retry behavior for the expected destination-exists case.
In `@apps/daemon/test/argosOrchestrationRuntime.test.ts`:
- Around line 68-101: Add test cases in the “rejects stdio MCP registration and
validates required string args” test for MCP configurations with a missing
config.type and an unrecognized transport type; assert both reject and that
upsertMcpServer is not called for either case.
In `@docs/issues/cua-driver-v0-2-0-sync/plan.md`:
- Line 27: Update the i18n entry in the plan checklist to a non-executable note,
replacing the “Run i18n” command wording with the format “i18n: N/A — no root
script” while preserving the stated reason.
---
Outside diff comments:
In `@apps/daemon/src/host/piAgentProfileManager.ts`:
- Around line 147-164: The writeManagedSkill and removeManagedSkill operations
must update the skill file/directory and skills-registry.json transactionally.
Stage the filesystem and registry changes, and if either operation fails,
restore the previous skill content or directory and registry state so they
remain consistent.
- Around line 124-130: Update readManagedSkillsRegistry, and the
ManagedAgentSkillRecord validation used by listManagedSkills and
validateManagedSkills, to parse registry JSON as unknown and reject invalid
envelopes or records, including unsafe names such as path traversal values.
Require the expected skills array and validate each record’s fields before
returning it; do not allow malformed data to reach path.join in
validateManagedSkills.
---
Duplicate comments:
In `@apps/daemon/src/host/argosOrchestrationRuntime.ts`:
- Around line 127-146: Update the “argos_mcp_server_upsert” tool schema to
advertise only the supported “sse” and “http” transports. Remove “stdio” from
the type enum and remove the local-command fields “command” and “args” from the
config properties, while preserving the existing HTTP/SSE configuration fields.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 641c3ed9-9287-41f7-ba6f-66557a908cb0
📒 Files selected for processing (33)
apps/daemon/src/host/argosOrchestrationRuntime.tsapps/daemon/src/host/piAgentProfileManager.tsapps/daemon/src/index.tsapps/daemon/test/argosOrchestrationRuntime.test.tsapps/desktop/test/main/presenter/argosAgentRuntime/argosAgentRuntime.test.tsdocs/archives/skill-draft-confirmation-card/tasks.mddocs/features/acp-v1-reliability/plan.mddocs/features/acp-v1-reliability/spec.mddocs/features/acp-v1-reliability/tasks.mddocs/features/agent-state-semantics/plan.mddocs/issues/acp-existing-thread-inline-options/plan.mddocs/issues/agent-exec-utility-process-crash/tasks.mddocs/issues/cua-driver-v0-2-0-sync/plan.mddocs/issues/floating-button-position-persistence/tasks.mddocs/issues/guided-onboarding-first-chat-confirm/plan.mddocs/issues/mac-native-feel-audit/tasks.mddocs/issues/merged-activity-groups/plan.mddocs/issues/onboarding-provider-mcp-handoff/plan.mddocs/issues/onboarding-provider-mcp-handoff/tasks.mddocs/issues/openai-compatible-video-prompt-duration-fallback/plan.mddocs/issues/openai-compatible-video-prompt-duration-fallback/tasks.mddocs/issues/prcheck-format-onboarding/plan.mddocs/issues/react-doctor-top-3/plan.mddocs/issues/remove-china-specific-defaults/tasks.mddocs/issues/session-list-stable-alphabetical-sort/plan.mddocs/issues/session-list-stable-alphabetical-sort/tasks.mddocs/issues/windows-arm64-duckdb-upgrade/plan.mddocs/issues/windows-arm64-duckdb-upgrade/tasks.mddocs/issues/windows-release-build-arch/plan.mddocs/issues/yobrowser-cdp-graceful-degradation/tasks.mdpackages/agent-runtime/src/argosAgentRuntime.tspackages/agent-runtime/src/configMerge.tspackages/shared-contracts/src/domainSchemas.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- docs/issues/remove-china-specific-defaults/tasks.md
- docs/issues/merged-activity-groups/plan.md
- docs/issues/floating-button-position-persistence/tasks.md
- docs/issues/react-doctor-top-3/plan.md
- docs/issues/onboarding-provider-mcp-handoff/tasks.md
- docs/issues/session-list-stable-alphabetical-sort/tasks.md
- docs/issues/openai-compatible-video-prompt-duration-fallback/plan.md
- docs/issues/mac-native-feel-audit/tasks.md
- docs/issues/windows-arm64-duckdb-upgrade/tasks.md
- docs/issues/windows-release-build-arch/plan.md
- docs/features/acp-v1-reliability/plan.md
- docs/issues/session-list-stable-alphabetical-sort/plan.md
- docs/features/acp-v1-reliability/tasks.md
- docs/issues/onboarding-provider-mcp-handoff/plan.md
- docs/issues/yobrowser-cdp-graceful-degradation/tasks.md
- docs/archives/skill-draft-confirmation-card/tasks.md
- docs/issues/acp-existing-thread-inline-options/plan.md
- docs/issues/agent-exec-utility-process-crash/tasks.md
- docs/issues/prcheck-format-onboarding/plan.md
- apps/desktop/test/main/presenter/argosAgentRuntime/argosAgentRuntime.test.ts
- docs/issues/openai-compatible-video-prompt-duration-fallback/tasks.md
- docs/features/acp-v1-reliability/spec.md
- docs/issues/guided-onboarding-first-chat-confirm/plan.md
- docs/features/agent-state-semantics/plan.md
- docs/issues/windows-arm64-duckdb-upgrade/plan.md
- apps/daemon/src/index.ts
- packages/agent-runtime/src/argosAgentRuntime.ts
- packages/shared-contracts/src/domainSchemas.ts
Comment on lines
+371
to
+382
| case "argos_mcp_server_upsert": { | ||
| const serverName = this.requireString(args, "serverName"); | ||
| const serverConfig = this.asRecord(args.config) ?? {}; | ||
| // stdio servers run an arbitrary local command with model-supplied args/env. | ||
| // The orchestrator may only register http/sse (URL) transports; stdio servers | ||
| // must be configured manually by the user to avoid local code execution. | ||
| if (serverConfig.type === "stdio") { | ||
| throw new Error( | ||
| "The orchestrator cannot register stdio MCP servers (that would allow arbitrary local command execution). Configure stdio servers manually via Settings, or use an http/sse transport.", | ||
| ); | ||
| } | ||
| result = await this.requireProvisioning().upsertMcpServer(serverName, serverConfig); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce a closed HTTP/SSE transport allowlist.
A missing or unrecognized config.type bypasses the exact "stdio" check. It then reaches the daemon authority that persists and starts the MCP server.
apps/daemon/src/host/argosOrchestrationRuntime.ts#L371-L382: reject every transport except"http"and"sse"before delegation.apps/daemon/src/host/argosOrchestrationRuntime.ts#L127-L146: removestdioand local-command fields from the published tool schema.apps/daemon/test/argosOrchestrationRuntime.test.ts#L68-L101: verify that missing and unrecognized transports do not callupsertMcpServer.
Proposed fix
- type: { type: "string", enum: ["stdio", "sse", "http"] },
- command: { type: "string" },
- args: { type: "array", items: { type: "string" } },
- env: { type: "object", additionalProperties: true },
+ type: { type: "string", enum: ["sse", "http"] },
baseUrl: { type: "string" },
customHeaders: { type: "object", additionalProperties: { type: "string" } },
@@
- if (serverConfig.type === "stdio") {
+ if (serverConfig.type !== "http" && serverConfig.type !== "sse") {
throw new Error(
- "The orchestrator cannot register stdio MCP servers (that would allow arbitrary local command execution). Configure stdio servers manually via Settings, or use an http/sse transport.",
+ "The orchestrator can register only http or sse MCP servers.",
);
}📍 Affects 2 files
apps/daemon/src/host/argosOrchestrationRuntime.ts#L371-L382(this comment)apps/daemon/src/host/argosOrchestrationRuntime.ts#L127-L146apps/daemon/test/argosOrchestrationRuntime.test.ts#L68-L101
🤖 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 `@apps/daemon/src/host/argosOrchestrationRuntime.ts` around lines 371 - 382,
Enforce a closed HTTP/SSE transport allowlist in
apps/daemon/src/host/argosOrchestrationRuntime.ts lines 371-382 by rejecting any
config.type other than "http" or "sse" before
requireProvisioning().upsertMcpServer, including missing and unrecognized
values; retain the existing stdio rejection intent. In
apps/daemon/src/host/argosOrchestrationRuntime.ts lines 127-146, remove stdio
and local-command fields from the published tool schema. In
apps/daemon/test/argosOrchestrationRuntime.test.ts lines 68-101, add coverage
confirming missing and unrecognized transports reject and do not call
upsertMcpServer.
Comment on lines
+318
to
+330
| /** | ||
| * Atomic-rename with a Windows fallback: POSIX rename atomically replaces an | ||
| * existing destination, but on Windows rename fails when the target exists, so | ||
| * remove the target and retry. Keeps updates portable across platforms. | ||
| */ | ||
| private atomicRename(temporaryPath: string, targetPath: string): void { | ||
| try { | ||
| fs.renameSync(temporaryPath, targetPath); | ||
| } catch { | ||
| fs.rmSync(targetPath, { force: true }); | ||
| fs.renameSync(temporaryPath, targetPath); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate target file"
find . -path './apps/daemon/src/host/piAgentProfileManager.ts' -print
echo
echo "## relevant sections"
sed -n '280,370p' ./apps/daemon/src/host/piAgentProfileManager.ts | cat -n | sed 's/^/316+/'
echo
echo "## search atomicRename usages"
rg -n "atomicRename\\(|atomic" ./apps/daemon/src/host/piAgentProfileManager.ts apps/daemon/src || true
echo
echo "## file stats"
wc -l ./apps/daemon/src/host/piAgentProfileManager.tsRepository: dvaJi/argos
Length of output: 5450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## imports"
sed -n '1,80p' ./apps/daemon/src/host/piAgentProfileManager.ts | cat -n | sed 's/^/1+/'
echo
echo "## writeSkill/writeSettings/writeManagedSkillsRegistry surrounding sections"
sed -n '110,170p' ./apps/daemon/src/host/piAgentProfileManager.ts | cat -n | sed 's/^/110+/'
sed -n '230,290p' ./apps/daemon/src/host/piAgentProfileManager.ts | cat -n | sed 's/^/230+/'
sed -n '345,358p' ./apps/daemon/src/host/piAgentProfileManager.ts | cat -n | sed 's/^/345+/'
echo
echo "## deterministic Node fs.renameSync error-shape probe from /tmp"
node - <<'JS'
const fs = require('fs');
const path = require('path');
const tmpdir = require('os').tmpdir();
const targetPath = path.join(tmpdir, `atomic-rename-probe-dir-${process.pid}-${Date.now()}`);
fs.mkdirSync(targetPath);
fs.writeFileSync(path.join(targetPath, 'placeholder.txt'), 'keep');
function atomicRename(temporaryPath, targetPath) {
try {
fs.renameSync(temporaryPath, targetPath);
} catch {
fs.rmSync(targetPath, { force: true });
try {
fs.renameSync(temporaryPath, targetPath);
} catch (retryError) {
console.log(JSON.stringify({
retryErrorCode: retryError.code,
retryErrorMessage: retryError.message,
destinationExistsAfterRetry: fs.existsSync(targetPath),
}, null, 2));
}
}
}
const sourcePath = path.join(tmpdir, `atomic-rename-source-${process.pid}-${Date.now()}`);
fs.writeFileSync(sourcePath, 'skill');
console.log('destinationBefore=', fs.existsSync(targetPath));
atomicRename(sourcePath, targetPath);
console.log('destinationAfter=', fs.existsSync(targetPath));
console.log('destinationFiles=', fs.existsSync(targetPath) ? fs.readdirSync(targetPath) : null);
fs.rmSync(targetPath, { force: true, recursive: true });
fs.rmSync(sourcePath, { force: true });
JSRepository: dvaJi/argos
Length of output: 11407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Node Probe: file target destination exists"
node - <<'JS'
const fs = require('fs');
const path = require('path');
const os = require('os');
const targetPath = path.join(os.tmpdir(), `atomic-rename-probe-target-file-${process.pid}-${Date.now()}`);
fs.writeFileSync(targetPath, 'keep');
const temporaryPath = path.join(os.tmpdir(), `atomic-rename-probe-temporary-${process.pid}-${Date.now()}.tmp`);
fs.writeFileSync(temporaryPath, 'new content');
try {
fs.renameSync(temporaryPath, targetPath);
} catch (error) {
console.log(JSON.stringify({
firstErrorCode: error.code,
firstErrorMessage: error.message,
destinationBeforeRetry: fs.existsSync(targetPath),
destinationContentsBeforeRetry: fs.existsSync(targetPath) ? fs.readFileSync(targetPath, 'utf8') : null,
}, null, 2));
fs.rmSync(targetPath, { force: true });
try {
fs.renameSync(temporaryPath, targetPath);
} catch (retryError) {
console.log(JSON.stringify({
retryErrorCode: retryError.code,
retryErrorMessage: retryError.message,
destinationAfterRetry: fs.existsSync(targetPath),
temporaryAfterRetry: fs.existsSync(temporaryPath),
}, null, 2));
}
} finally {
fs.rmSync(targetPath, { force: true, recursive: true });
fs.rmSync(temporaryPath, { force: true });
}
JS
echo
echo "## Node Probe: temp source missing and target immutable"
tmpbase=$(mktemp -d)
targetPath="$tmpbase/registry.json"
temporaryPath="$tmpbase/registry.json.${process.pid}.tmp"
fs.writeFileSync(targetPath, JSON.stringify({ version: 1, skills: [{ name: "old" }] } + '\n'));
try {
fs.renameSync(temporaryPath, targetPath);
} catch (error) {
console.log(JSON.stringify({
firstErrorCode: error.code,
firstErrorMessage: error.message,
}, null, 2));
fs.rmSync(targetPath, { force: true });
try {
fs.renameSync(temporaryPath, targetPath);
} catch (retryError) {
console.log(JSON.stringify({
retryErrorCode: retryError.code,
retryErrorMessage: retryError.message,
destinationAfterRetry: fs.existsSync(targetPath),
sourceWasDeletedByNode: false,
}, null, 2));
}
} finally {
fs.rmSync(tmpbase, { force: true, recursive: true });
}
JSRepository: dvaJi/argos
Length of output: 327
🌐 Web query:
Node.js fs.renameSync Windows path already exists EPERM destination exists
💡 Result:
When you encounter an EPERM error with fs.renameSync on Windows, it is typically due to platform-specific differences in how file system locks and operations are handled [1][2][3]. Unlike POSIX systems (Linux/macOS) where rename operations are atomic and often override existing destinations regardless of locks, Windows enforces mandatory file locking [2][3]. On Windows, fs.renameSync uses the MoveFileEx system call, which fails if the target path is already in use or if specific Windows security policies prevent the replacement [1][2]. Common causes for this EPERM error include: 1. Mandatory File Locking: If any process (including antivirus software, search indexers, IDEs, or other Node.js processes like nodemon) has a handle on the file or the destination directory, Windows will block the rename operation [1][4][3]. 2. Destination Already Exists: While fs.rename is documented to overwrite the destination, on Windows, this often fails if the target is a directory or if the file is locked [1][5]. 3. Permission Restrictions: You may lack the necessary write permissions to modify the destination file or directory, especially if it resides in a protected system location [3]. Recommended Solutions: - Ensure Files are Closed: If you are creating or writing to the file immediately before renaming it, ensure the file handle is completely closed (e.g., waiting for the 'finish' or 'close' event of a WriteStream) before calling renameSync [3]. - Use a Retry Strategy: Since these locks are often transient (e.g., an antivirus scanner holding a file for a split second), implement a retry mechanism with a backoff delay [6]. - Manual Copy and Delete: For more complex scenarios, such as moving directories or files that consistently trigger locks, the most reliable approach is to manually copy the source to the destination and then delete the source [4]. Libraries like fs-extra provide utilities (e.g., fsExtra.move) that handle these cross-platform edge cases automatically [4]. - Check for Zombie Processes: Ensure no development servers or watchers are holding locks on the target files [1][4]. In summary, EPERM on Windows is a signal that the operating system has restricted access to the file at that moment, usually due to a lock held by another process or an operating system policy regarding overwriting files [2][3].
Citations:
- 1: EPERM when renaming files on Windows nodejs/node#29481
- 2: Improved workarounds for win32 isaacs/node-graceful-fs#119
- 3: https://errornotes.dev/en/errors/nodejs/fix-eperm-operation-not-permitted-rename-error-in-nodejs-on-windows
- 4: https://stackoverflow.com/questions/71134302/fs-rename-unable-to-rename-directories-with-contents-on-windows-10
- 5: fs.rename doesn't work as documented nodejs/node#21957
- 6: https://nodejs.org/api/fs.html
Limit the rename fallback to the expected existing-destination error.
atomicRename() currently treats every fs.renameSync failure as “destination already exists” and deletes targetPath before retrying. If the rename failed because of a permission, missing source, or filesystem error, this can remove the managed skill file or registry before the retry, losing persisted profile state. Apply the delete-and-rename sequence only when the exact Windows existing-destination error is known, or avoid deleting the destination on failure.
🤖 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 `@apps/daemon/src/host/piAgentProfileManager.ts` around lines 318 - 330, Update
atomicRename so the delete-and-retry fallback runs only for the known Windows
error indicating an existing destination; rethrow all other fs.renameSync
failures without removing targetPath. Preserve the normal rename path and the
retry behavior for the expected destination-exists case.
Comment on lines
+68
to
+101
| it("rejects stdio MCP registration and validates required string args", async () => { | ||
| const upsertMcpServer = vi.fn(async () => ({ ok: true })); | ||
| const createAgent = vi.fn(async (input) => ({ id: "x", ...input })); | ||
| const updateAgent = vi.fn(async (agentId: string) => ({ id: agentId })); | ||
| const runtime = new ArgosOrchestrationRuntime({ exec: vi.fn() }, async () => []); | ||
| runtime.setProvisioningActions({ | ||
| createAgent, | ||
| updateAgent, | ||
| listMcpServers: vi.fn(), | ||
| upsertMcpServer, | ||
| setAgentMcpServers: vi.fn(), | ||
| listAgentSkills: vi.fn(), | ||
| writeAgentSkill: vi.fn(), | ||
| removeAgentSkill: vi.fn(), | ||
| provisionAgent: vi.fn(), | ||
| validateAgent: vi.fn(), | ||
| }); | ||
|
|
||
| // stdio servers run arbitrary local commands and cannot be registered by the orchestrator | ||
| await expect( | ||
| call(runtime, "argos_mcp_server_upsert", { serverName: "evil", config: { type: "stdio", command: "rm" } }), | ||
| ).rejects.toThrow(/stdio/); | ||
| expect(upsertMcpServer).not.toHaveBeenCalled(); | ||
|
|
||
| // http/sse transports are still permitted | ||
| await call(runtime, "argos_mcp_server_upsert", { | ||
| serverName: "mail", | ||
| config: { type: "http", baseUrl: "https://example.com" }, | ||
| }); | ||
| expect(upsertMcpServer).toHaveBeenCalledWith("mail", expect.objectContaining({ type: "http" })); | ||
|
|
||
| // required string args are validated before String() coercion | ||
| await expect(call(runtime, "argos_agents_create", { description: "no name" })).rejects.toThrow(/name/); | ||
| await expect(call(runtime, "argos_agents_update", { updates: { foo: 1 } })).rejects.toThrow(/agentId/); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Cover closed transport validation.
Add cases for a missing config.type and an unrecognized type. Both cases must reject before upsertMcpServer runs.
🤖 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 `@apps/daemon/test/argosOrchestrationRuntime.test.ts` around lines 68 - 101,
Add test cases in the “rejects stdio MCP registration and validates required
string args” test for MCP configurations with a missing config.type and an
unrecognized transport type; assert both reject and that upsertMcpServer is not
called for either case.
| - Run `pnpm run i18n`. | ||
| - Run `pnpm run lint`. | ||
| - Run `bun run format`. | ||
| - Run `i18n (N/A -- no root script)`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the unavailable i18n check as a note, not a command.
Run i18n (N/A -- no root script) is not executable. A reader may copy it and receive a command-not-found error. Use a checklist note such as i18n: N/A — no root script.
🧰 Tools
🪛 LanguageTool
[style] ~27-~27: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... cua-driver. - Run bun run format. - Run i18n (N/A -- no root script). - Run ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 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 `@docs/issues/cua-driver-v0-2-0-sync/plan.md` at line 27, Update the i18n entry
in the plan checklist to a non-executable note, replacing the “Run i18n” command
wording with the format “i18n: N/A — no root script” while preserving the stated
reason.
Run oxfmt on 25 files with pre-existing formatting drift (shadcn components, UI stores, shared-contracts events). Restores a green format:check on master so build-check stops failing on unrelated drift.
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two logical commits bundled together per request.
1. feat(orchestrator): builtin orchestrator agent
Adds a built-in orchestrator agent and self-configurable provisioning. The orchestrator can register MCP integrations, create specialized Argos agents restricted to an MCP, and attach durable instructions.
2. docs: toolchain cleanup and SDD close-out
Verification
Summary by CodeRabbit
New Features
Documentation