Skip to content

Commit 884b371

Browse files
yotsudaclaude
andcommitted
fix(tools): never drop the sub-agent 🔑 notice — wrap every return through one helper
The "🔑 Your agent_id is: ..." notice is the only signal a freshly allocated sub-agent has for learning its own ID. It used to be emitted inline by: - GetCurrentLocation success path (line ~192) - InvokeExpression case "success" branch (only when startupNotice was set) - StartConsole new + reuse paths Every other return path forgot to add it. Sub-agents could lose their allocated ID forever if their first call landed on: - GetCurrentLocation with no ready pipe — delegated to StartConsole with the already-allocated agent_id, so the inner ResolveAgentId saw isNewlyAllocated=false and the inner success path stayed silent. - InvokeExpression cases "timeout", "Completed" (cached), "error" — branches built their own response and never appended the notice. - InvokeExpression busy auto-route — recurses with is_subagent:false, inner stays silent, outer''s busyResponse never adds the notice. - Any drift bail / var1 enforcement / pipe-error early return. Fix: add a single PrependAgentIdNoticeIfNew helper and a per-method local Wrap function, then route every return through Wrap. The helper is a no-op when isNewlyAllocated=false, so the common path (is_subagent=false or already-known agent_id) is unchanged. Inline 🔑 emissions removed from the three legacy sites so we don''t double-up. Trade-off: the notice now appears at the very top of the response instead of being interleaved with the startupNotice block. AI parses the same text either way; emit-from-one-place is what makes future branches automatically correct. 4 unit tests added: success-path notice (regression guard), error-path notice (the previously-broken case), default-agent silence, provided-agent-id silence. Build clean. xunit: net8 162/162, net9 275/275 (+4 from this change). Note: Unit/Proxy/** is net9-only by csproj convention because the Proxy project itself is net9-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c723f0f commit 884b371

2 files changed

Lines changed: 152 additions & 39 deletions

File tree

PowerShell.MCP.Proxy/Tools/PowerShellTools.cs

Lines changed: 49 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,20 @@ private static (string agentId, bool isNewlyAllocated, string? error) ResolveAge
110110
return ("default", false, null);
111111
}
112112

113+
/// <summary>
114+
/// Prepends the 🔑 agent_id notice to a tool response when a new sub-agent
115+
/// ID was just allocated. No-op for the common case (isNewlyAllocated=false),
116+
/// so this can be applied unconditionally to every return path of a tool method.
117+
/// Centralizing the emit here is what guarantees the notice can't be dropped by
118+
/// timeout / cached-completed / error / busy-route / drift-bail / auto-start
119+
/// branches the way it was when each branch had to remember to add it.
120+
/// </summary>
121+
private static string PrependAgentIdNoticeIfNew(string body, bool isNewlyAllocated, string agentId)
122+
{
123+
if (!isNewlyAllocated) return body;
124+
return $"🔑 Your agent_id is: {agentId} — pass this in all subsequent tool calls.\n\n{body}";
125+
}
126+
113127
[McpServerTool]
114128
[Description("Retrieves the current location and all available drives (providers) from the PowerShell session. Returns current_location and other_drive_locations array. Call this when you need to understand the current PowerShell context, as users may change location during the session. When executing multiple invoke_expression commands in succession, calling once at the beginning is sufficient.")]
115129
public static async Task<string> GetCurrentLocation(
@@ -122,17 +136,24 @@ public static async Task<string> GetCurrentLocation(
122136
CancellationToken cancellationToken = default)
123137
{
124138
var (agentId, isNewlyAllocated, error) = ResolveAgentId(is_subagent, agent_id);
139+
// Wrap every return so the 🔑 notice can never be dropped on a sub-agent's
140+
// first call regardless of which branch builds the response.
141+
string Wrap(string r) => PrependAgentIdNoticeIfNew(r, isNewlyAllocated, agentId);
125142
if (error != null)
126-
return error;
143+
return Wrap(error);
127144

128145
// Find a ready pipe
129146
var (readyPipeName, consoleSwitched, closedConsoleMessages, allPipesStatusInfo, _) = await FindReadyPipeAsync(pipeDiscoveryService, agentId, cancellationToken);
130147

131148
if (readyPipeName == null)
132149
{
133-
// No ready pipe - auto-start (StartConsole includes busy info collection)
150+
// No ready pipe - auto-start (StartConsole includes busy info collection).
151+
// Pass is_subagent: false because the sub-agent ID was already allocated
152+
// here; if we passed it as true the inner StartConsole would skip its own
153+
// ResolveAgentId allocation but our Wrap would still add the notice. Either
154+
// shape works; passing false makes the inner call's intent explicit.
134155
Console.Error.WriteLine($"[INFO] No ready PowerShell console found, auto-starting... Reason: {allPipesStatusInfo}");
135-
return await StartConsole(powerShellService, pipeDiscoveryService, agent_id: agentId, is_subagent: is_subagent, cancellationToken: cancellationToken);
156+
return Wrap(await StartConsole(powerShellService, pipeDiscoveryService, agent_id: agentId, is_subagent: false, cancellationToken: cancellationToken));
136157
}
137158

138159
try
@@ -168,18 +189,13 @@ public static async Task<string> GetCurrentLocation(
168189
{
169190
response.Append(completedOutputs);
170191
}
171-
if (isNewlyAllocated)
172-
{
173-
response.AppendLine($"🔑 Your agent_id is: {agentId} — pass this in all subsequent tool calls.");
174-
response.AppendLine();
175-
}
176192
response.Append(result);
177-
return response.ToString();
193+
return Wrap(response.ToString());
178194
}
179195
catch (Exception ex)
180196
{
181197
Console.Error.WriteLine($"[ERROR] GetCurrentLocation failed: {ex.Message}");
182-
return $"Failed to get current location: {ex.Message}\n\nPlease try again. A new console will be started automatically if needed.";
198+
return Wrap($"Failed to get current location: {ex.Message}\n\nPlease try again. A new console will be started automatically if needed.");
183199
}
184200
}
185201

@@ -248,8 +264,11 @@ public static async Task<string> InvokeExpression(
248264
}
249265

250266
var (agentId, isNewlyAllocated, resolveError) = ResolveAgentId(is_subagent, agent_id);
267+
// Wrap every return so the 🔑 notice can never be dropped on a sub-agent's
268+
// first call regardless of which branch builds the response.
269+
string Wrap(string r) => PrependAgentIdNoticeIfNew(r, isNewlyAllocated, agentId);
251270
if (resolveError != null)
252-
return resolveError;
271+
return Wrap(resolveError);
253272

254273
var sessionManager = ConsoleSessionManager.Instance;
255274
// Find a ready pipe
@@ -283,7 +302,7 @@ public static async Task<string> InvokeExpression(
283302
var (success, locationResult) = await StartConsoleInternal(powerShellService, agentId, null, autoStartLocation, cancellationToken);
284303
if (!success)
285304
{
286-
return locationResult; // Error message
305+
return Wrap(locationResult); // Error message
287306
}
288307

289308
// Pick up the freshly-spawned pipe so the rest of this call
@@ -292,7 +311,7 @@ public static async Task<string> InvokeExpression(
292311
readyPipeName = sessionManager.GetActivePipeName(agentId);
293312
if (readyPipeName == null)
294313
{
295-
return "Failed to acquire newly-started console pipe.";
314+
return Wrap("Failed to acquire newly-started console pipe.");
296315
}
297316

298317
await SetConsoleTitleAsync(powerShellService, readyPipeName, cancellationToken);
@@ -324,7 +343,7 @@ public static async Task<string> InvokeExpression(
324343
var var1Error = PipelineHelper.CheckVar1Enforcement(pipeline, var1, var2);
325344
if (var1Error != null)
326345
{
327-
return var1Error;
346+
return Wrap(var1Error);
328347
}
329348

330349
// Cwd drift detection: if the user typed `cd` in the visible console
@@ -360,7 +379,7 @@ public static async Task<string> InvokeExpression(
360379
}
361380
bailResponse.AppendLine($"ℹ️ User changed cwd in console {driftConsoleName} from '{drift.Value.AiCwd}' to '{drift.Value.LiveCwd}'.");
362381
bailResponse.AppendLine($"Pipeline NOT executed. Re-issue to run at '{drift.Value.LiveCwd}', or prepend `Set-Location -LiteralPath '{drift.Value.AiCwd.Replace("'", "''")}';` to revert.");
363-
return bailResponse.ToString();
382+
return Wrap(bailResponse.ToString());
364383
}
365384

366385
// Execute the command
@@ -418,7 +437,7 @@ public static async Task<string> InvokeExpression(
418437
var (startSuccess, startError) = await StartConsoleInternal(powerShellService, agentId, null, startLoc, cancellationToken);
419438
if (!startSuccess)
420439
{
421-
return startError;
440+
return Wrap(startError);
422441
}
423442

424443
// Set console window title
@@ -465,7 +484,7 @@ public static async Task<string> InvokeExpression(
465484
busyResponse.AppendLine($"ℹ️ Auto-routed to {newConsoleName} at {startLoc} (source console busy with {jsonResponse.Reason}). Pipeline executed automatically — no re-send needed.");
466485
busyResponse.AppendLine();
467486
busyResponse.Append(retryResult);
468-
return busyResponse.ToString();
487+
return Wrap(busyResponse.ToString());
469488
}
470489
break;
471490

@@ -522,7 +541,7 @@ public static async Task<string> InvokeExpression(
522541
timeoutResponse.AppendLine();
523542
timeoutResponse.AppendLine(scopeWarning);
524543
}
525-
return timeoutResponse.ToString();
544+
return Wrap(timeoutResponse.ToString());
526545

527546
case PipeStatus.Completed:
528547
// Snapshot AI-intended cwd from the cached completion.
@@ -589,10 +608,10 @@ public static async Task<string> InvokeExpression(
589608
{
590609
cachedResponse.Append("Result cached. Will be returned on next tool call.");
591610
}
592-
return cachedResponse.ToString();
611+
return Wrap(cachedResponse.ToString());
593612

594613
case "error":
595-
return jsonResponse.Message ?? $"Error from PowerShell.MCP module: {jsonResponse.Error}";
614+
return Wrap(jsonResponse.Message ?? $"Error from PowerShell.MCP module: {jsonResponse.Error}");
596615

597616
case "success":
598617
// Snapshot AI-intended cwd at successful completion. This
@@ -632,10 +651,6 @@ public static async Task<string> InvokeExpression(
632651
if (!string.IsNullOrEmpty(startupNotice))
633652
{
634653
successResponse.AppendLine(startupNotice);
635-
if (isNewlyAllocated)
636-
{
637-
successResponse.AppendLine($"🔑 Your agent_id is: {agentId} — pass this in all subsequent tool calls.");
638-
}
639654
successResponse.AppendLine();
640655
}
641656
if (completedOutput.Length > 0)
@@ -670,7 +685,7 @@ public static async Task<string> InvokeExpression(
670685
// successResponse.AppendLine();
671686
// successResponse.AppendLine(jsonHint);
672687
// }
673-
return successResponse.ToString();
688+
return Wrap(successResponse.ToString());
674689
}
675690
}
676691
}
@@ -681,7 +696,7 @@ public static async Task<string> InvokeExpression(
681696
}
682697

683698
// Fallback: return result as-is (shouldn't happen with new DLL)
684-
return result;
699+
return Wrap(result);
685700
}
686701
catch (Exception ex)
687702
{
@@ -692,7 +707,7 @@ public static async Task<string> InvokeExpression(
692707
var otherClosed = pipeDiscoveryService.DetectClosedConsoles(agentId);
693708
var closedMessages = new List<string> { $" - ⚠ Console {consoleName} was closed" };
694709
closedMessages.AddRange(otherClosed);
695-
return $"Command execution failed: {ex.Message}\n{string.Join("\n", closedMessages)}\nPlease try again. A new console will be started automatically if needed.";
710+
return Wrap($"Command execution failed: {ex.Message}\n{string.Join("\n", closedMessages)}\nPlease try again. A new console will be started automatically if needed.");
696711
}
697712
}
698713

@@ -909,8 +924,11 @@ public static async Task<string> StartConsole(
909924
CancellationToken cancellationToken = default)
910925
{
911926
var (agentId, isNewlyAllocated, resolveError) = ResolveAgentId(is_subagent, agent_id);
927+
// Wrap every return so the 🔑 notice can never be dropped on a sub-agent's
928+
// first call regardless of which branch builds the response.
929+
string Wrap(string r) => PrependAgentIdNoticeIfNew(r, isNewlyAllocated, agentId);
912930
if (resolveError != null)
913-
return resolveError;
931+
return Wrap(resolveError);
914932

915933
var forceNew = !string.IsNullOrEmpty(reason);
916934

@@ -969,13 +987,9 @@ await powerShellService.ExecuteSilentAsync(
969987
reuseResponse.Append(reuseCompletedOutput);
970988
}
971989
reuseResponse.AppendLine("ℹ️ Did not launch a new console. An existing standby console is available and will be reused. To force a new console, provide the reason parameter.");
972-
if (isNewlyAllocated)
973-
{
974-
reuseResponse.AppendLine($"🔑 Your agent_id is: {agentId} — pass this in all subsequent tool calls.");
975-
}
976990
reuseResponse.AppendLine();
977991
reuseResponse.Append(reuseLocationResult);
978-
return reuseResponse.ToString();
992+
return Wrap(reuseResponse.ToString());
979993
}
980994
// No standby console found, fall through to create a new one
981995
}
@@ -985,7 +999,7 @@ await powerShellService.ExecuteSilentAsync(
985999
var (success, startResult) = await StartConsoleInternal(powerShellService, agentId, startupCommands, resolvedPath, cancellationToken);
9861000
if (!success)
9871001
{
988-
return startResult; // Error message
1002+
return Wrap(startResult); // Error message
9891003
}
9901004

9911005
// Set console window title
@@ -1015,13 +1029,9 @@ await powerShellService.ExecuteSilentAsync(
10151029
response.AppendLine();
10161030
}
10171031
response.AppendLine("PowerShell console started successfully with PowerShell.MCP module imported.");
1018-
if (isNewlyAllocated)
1019-
{
1020-
response.AppendLine($"🔑 Your agent_id is: {agentId} — pass this in all subsequent tool calls.");
1021-
}
10221032
response.AppendLine();
10231033
response.Append(startResult);
1024-
return response.ToString();
1034+
return Wrap(response.ToString());
10251035
}
10261036

10271037
/// <summary>

Tests/Unit/Proxy/PowerShellToolsTests.cs

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,109 @@ public async Task GetCurrentLocation_PipeError_ReturnsErrorMessage()
135135
Assert.Contains("Pipe communication failed", result);
136136
}
137137

138+
[Fact]
139+
public async Task GetCurrentLocation_SubAgentNewAllocation_ReadyPipe_EmitsAgentIdNotice()
140+
{
141+
// Sub-agent's first call must include the 🔑 agent_id notice in the response
142+
// so the AI knows which ID to pass to subsequent tool calls. Success-path
143+
// regression guard.
144+
_mockPipeDiscoveryService
145+
.Setup(s => s.FindReadyPipeAsync(It.IsAny<string>(), It.IsAny<CancellationToken>(), It.IsAny<bool>()))
146+
.ReturnsAsync(new PipeDiscoveryResult(TestPipeName, false, new List<string>(), null));
147+
148+
_mockPowerShellService
149+
.Setup(s => s.GetCurrentLocationFromPipeAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
150+
.ReturnsAsync("Location [FileSystem]: C:\\test");
151+
152+
_mockPipeDiscoveryService
153+
.Setup(s => s.CollectAllCachedOutputsAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
154+
.ReturnsAsync(new CachedOutputResult("", ""));
155+
156+
var result = await PowerShellTools.GetCurrentLocation(
157+
_mockPowerShellService.Object,
158+
_mockPipeDiscoveryService.Object,
159+
agent_id: null,
160+
is_subagent: true);
161+
162+
Assert.Contains("🔑 Your agent_id is:", result);
163+
}
164+
165+
[Fact]
166+
public async Task GetCurrentLocation_SubAgentNewAllocation_PipeError_StillEmitsAgentIdNotice()
167+
{
168+
// Regression: previously the error / timeout / cached-completed branches did
169+
// not emit 🔑. A sub-agent that hit any of those on its first call lost its
170+
// allocated ID forever. The wrap-every-return refactor closes that hole.
171+
_mockPipeDiscoveryService
172+
.Setup(s => s.FindReadyPipeAsync(It.IsAny<string>(), It.IsAny<CancellationToken>(), It.IsAny<bool>()))
173+
.ReturnsAsync(new PipeDiscoveryResult(TestPipeName, false, new List<string>(), null));
174+
175+
_mockPowerShellService
176+
.Setup(s => s.GetCurrentLocationFromPipeAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
177+
.ThrowsAsync(new InvalidOperationException("Pipe communication failed"));
178+
179+
var result = await PowerShellTools.GetCurrentLocation(
180+
_mockPowerShellService.Object,
181+
_mockPipeDiscoveryService.Object,
182+
agent_id: null,
183+
is_subagent: true);
184+
185+
Assert.Contains("🔑 Your agent_id is:", result);
186+
Assert.Contains("Failed to get current location", result);
187+
}
188+
189+
[Fact]
190+
public async Task GetCurrentLocation_DefaultAgent_DoesNotEmitAgentIdNotice()
191+
{
192+
// Non-sub-agent (default) calls must NOT carry the 🔑 notice — it would just
193+
// be noise. Verifies the wrap helper is a no-op when isNewlyAllocated=false.
194+
_mockPipeDiscoveryService
195+
.Setup(s => s.FindReadyPipeAsync(It.IsAny<string>(), It.IsAny<CancellationToken>(), It.IsAny<bool>()))
196+
.ReturnsAsync(new PipeDiscoveryResult(TestPipeName, false, new List<string>(), null));
197+
198+
_mockPowerShellService
199+
.Setup(s => s.GetCurrentLocationFromPipeAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
200+
.ReturnsAsync("Location [FileSystem]: C:\\test");
201+
202+
_mockPipeDiscoveryService
203+
.Setup(s => s.CollectAllCachedOutputsAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
204+
.ReturnsAsync(new CachedOutputResult("", ""));
205+
206+
var result = await PowerShellTools.GetCurrentLocation(
207+
_mockPowerShellService.Object,
208+
_mockPipeDiscoveryService.Object);
209+
210+
Assert.DoesNotContain("🔑", result);
211+
}
212+
213+
[Fact]
214+
public async Task GetCurrentLocation_ProvidedAgentId_DoesNotEmitAgentIdNotice()
215+
{
216+
// When the AI passes back a previously-issued agent_id, no allocation happens
217+
// so the 🔑 notice would be a duplicate. Must stay silent.
218+
_mockPipeDiscoveryService
219+
.Setup(s => s.FindReadyPipeAsync(It.IsAny<string>(), It.IsAny<CancellationToken>(), It.IsAny<bool>()))
220+
.ReturnsAsync(new PipeDiscoveryResult(TestPipeName, false, new List<string>(), null));
221+
222+
_mockPowerShellService
223+
.Setup(s => s.GetCurrentLocationFromPipeAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
224+
.ReturnsAsync("Location [FileSystem]: C:\\test");
225+
226+
_mockPipeDiscoveryService
227+
.Setup(s => s.CollectAllCachedOutputsAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
228+
.ReturnsAsync(new CachedOutputResult("", ""));
229+
230+
// Use a valid sa-* shaped ID so IsValidAgentId returns true
231+
var validSubAgentId = ConsoleSessionManager.Instance.AllocateSubAgentId();
232+
var result = await PowerShellTools.GetCurrentLocation(
233+
_mockPowerShellService.Object,
234+
_mockPipeDiscoveryService.Object,
235+
agent_id: validSubAgentId,
236+
is_subagent: true);
237+
238+
Assert.DoesNotContain("🔑", result);
239+
}
240+
138241
[Fact]
139242
public async Task GetCurrentLocation_ConsoleSwitched_SetsWindowTitle()
140243
{

0 commit comments

Comments
 (0)