fix: drag crash, backward compat, MCP input validation - #131
Conversation
| @@ -598,7 +604,7 @@ export function Sidebar() { | |||
|
|
|||
There was a problem hiding this comment.
🟢 Suggestion
Problem: Using as a React fallback when is undefined (lines 604, 1297) means React treats these elements as new nodes every render, causing unnecessary DOM re-creation and potential animation glitches during drag-reorder animations.
Why it matters: Minor performance/stability issue during reorder — the crash is fixed, but the key instability could cause subtle UI jank or animation flicker when dragging nameless nodes.
Suggested fix:
If possible, generate a stable synthetic key when is absent — e.g. use a hash of or a monotonically increasing ID:
If is also absent (unlikely), fall back to for that case only.
There was a problem hiding this comment.
Fixed in 63396b1 — now using claudeSessionId as key fallback instead of index, with a prefixed string fallback (node-N, child-N) as last resort.
nox-0x
left a comment
There was a problem hiding this comment.
Three solid fixes: null guards prevent drag crashes, backward-compat defaults handle old session formats gracefully, and MCP tool input validation with helpful error messages replaces confusing undefined errors. Left one non-blocking suggestion about React key stability for anonymous nodes. LGTM ✅
| .string() | ||
| .optional() | ||
| .describe("Agent name (e.g. 'Dashboard@autonomOS')"), | ||
| name: z |
There was a problem hiding this comment.
🟡 Warning
Problem: set_manager error message says "Missing required parameter 'agent'" (line ~217), but agent is declared optional in the schema — neither agent nor name is actually required. The error message is therefore inconsistent with the schema contract.
Why it matters: MCP clients using schema-based validation will be confused if they see "Missing required 'agent'" after already omitting it intentionally. It undermines trust in the error messages.
Suggested fix:
`Missing required parameter. Usage: set_manager(agent: "AgentName", manager: "ManagerName")`Same fix applies to `kill_agent's error on line ~165.
There was a problem hiding this comment.
Fixed in 63396b1 — error messages now say "provide 'agent' or 'name'" instead of claiming agent is required. Applied to both kill_agent and set_manager in both MCP transports.
nox-0x
left a comment
There was a problem hiding this comment.
Good fixes overall. Left one non-blocking inline comment about misleading error message wording (parameter declared optional but error says "required agent"). Solid backward-compat tests and null guards. LGTM ✅
Three reliability fixes: 1. Sidebar drag crash: add null guards on all .toLowerCase() calls in hierarchy rendering — prevents TypeError when org nodes have undefined names during drag reorder. 2. Backward compatibility: relax readSessions() validation to not require autonomousMode (old sessions lack it), default missing fields (persistedAt, autonomousMode), and guard buildOrgChart() against nameless sessions. 3. MCP kill_agent/set_manager: accept 'name' as alias for 'agent' parameter (agents naturally try it), add input validation in both HTTP and channel server handlers, update tool schemas to match runtime behavior, and return helpful error messages instead of "Agent undefined not found". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- backward-compat.test.ts: 8 tests verifying old session formats (missing autonomousMode, persistedAt, status) load correctly and appear in org chart - mcp-validation.test.ts: 7 tests verifying kill_agent and set_manager schemas declare name alias and don't require agent Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests now save/restore the entire sessions.json instead of writing raw entries alongside real data. Fixes ENOENT on CI (no ~/.autonomos/) and test pollution between suites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fe61872 to
a648eb5
Compare
nox-0x
left a comment
There was a problem hiding this comment.
Solid fixes. Null guards resolve the drag crash, backward-compat defaults handle old sessions correctly, and MCP input validation with name alias is clean. Backward compat test suite is a nice addition. One non-critical note: the kill_agent/set_manager error messages still say "Missing required parameter 'agent'" even though agent is now optional in the schema — consider updating to "Missing required parameter. Usage: kill_agent(agent: "..." | name: "...")" to match the dual-parameter contract. Not blocking. LGTM ✅
- Use claudeSessionId as React key fallback instead of index (avoids DOM re-creation during drag-reorder of nameless nodes) - Error messages now say "provide 'agent' or 'name'" instead of "Missing required parameter 'agent'" (consistent with optional schema) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three critical fixes for production data loss: 1. Resume doesn't clear exited status: createSession() now passes status: "running" to persistSession(), so the merge logic no longer preserves the stale "exited" status on resume. 2. Test isolation: Tests from PR #131 wrote to production sessions.json. All test files now use isolated temp directories via _setConfigDirForTesting() / _resetCacheForTesting(). Added getConfigDir() with test override support to configDir.ts. 3. Write race on sessions.json: Added in-memory cache to persisted.ts — readSessions() populates once, all mutations go through the cached array, eliminating TOCTOU races. Added backup-on-shrink before destructive writes. getPersistedSessions() returns a defensive copy to prevent external cache corruption. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three critical fixes for production data loss: 1. Resume doesn't clear exited status: createSession() now passes status: "running" to persistSession(), so the merge logic no longer preserves the stale "exited" status on resume. 2. Test isolation: Tests from PR #131 wrote to production sessions.json. All test files now use isolated temp directories via _setConfigDirForTesting() / _resetCacheForTesting(). Added getConfigDir() with test override support to configDir.ts. 3. Write race on sessions.json: Added in-memory cache to persisted.ts — readSessions() populates once, all mutations go through the cached array, eliminating TOCTOU races. Added backup-on-shrink before destructive writes. getPersistedSessions() returns a defensive copy to prevent external cache corruption. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…134) Three critical fixes for production data loss: 1. Resume doesn't clear exited status: createSession() now passes status: "running" to persistSession(), so the merge logic no longer preserves the stale "exited" status on resume. 2. Test isolation: Tests from PR #131 wrote to production sessions.json. All test files now use isolated temp directories via _setConfigDirForTesting() / _resetCacheForTesting(). Added getConfigDir() with test override support to configDir.ts. 3. Write race on sessions.json: Added in-memory cache to persisted.ts — readSessions() populates once, all mutations go through the cached array, eliminating TOCTOU races. Added backup-on-shrink before destructive writes. getPersistedSessions() returns a defensive copy to prevent external cache corruption. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Three reliability fixes affecting daily use:
.toLowerCase()calls in sidebar hierarchy rendering now have null guards — preventsTypeErrorwhen org nodes have undefined names during drag reorderreadSessions()validation to not requireautonomousModefield (old sessions lack it), added defaults for missing fields (persistedAt,autonomousMode), guardedbuildOrgChart()against nameless sessionskill_agentandset_managernow acceptnameas alias foragentparameter, with helpful error messages instead of "Agent undefined not found". Both HTTP and channel server handlers updated, tool schemas match runtime behavior.Changes
Sidebar.tsx.toLowerCase()calls, defensivechildren ?? [], safekeypropsorgChart.tsbuildOrgChart(), default name to "Unknown" intoNode()persisted.tsautonomousMode/persistedAt, guardupdatePersistedSessionByNamemcp.tskill_agent+set_manager: acceptnamealias, input validation, helpful errorschannel-server/index.tsnamealias + validation forkill_agent,set_manager,sendmcp/tools.tsTOOL_KILL_AGENT+TOOL_SET_MANAGERschemas to declarenamepropertymcp-tools.test.tsagent, both params optional)Test plan
make check— 70/70 tests pass, biome clean, tsc cleanstatus,template,autonomousMode) — hierarchy should renderkill_agent(name: "X")— should work (not "Agent undefined not found")kill_agent()with no params — should return helpful usage message🤖 Generated with Claude Code