Skip to content

Commit 5e09518

Browse files
committed
fix(mcode-island): the pill never renders raw tool JSON (round-20 #6)
The running-state message was built as "$verb " + (ConvertTo-Json $args -Compress) truncated to 60 chars so the always-on-top pill showed things like Running {"command":"$ErrorActionPreference=\u0027Continue\u0027\n... Reading task {"task_id":"bg_0f858500-...","wai... Escaped quotes, a JSON key, and a truncation that lands mid-token. It was the noisiest thing on screen and the least informative. The `done` and `fail` branches were always fine -- they render the verb alone. Only the `running` branch serialised arguments. Format-ToolArgs now returns the single most informative field per tool: command for shell, file_path for read/write/edit, pattern for grep/glob, query for web_search, url for web_fetch, name for skill, task_id for the task-output family. Unknown tools get a generic sweep of the common field names before giving up. `skill` is not in $TOOL_ACTIONS or $TOOL_FAMILIES, so it always took the JSON path -- one of the most frequent tools on screen was the least readable. That is the case that made this worth fixing rather than tuning. There is no fallback to JSON. If Format-ToolArgs returns nothing the pill wears the bare verb: "Using" reads fine, "Using {\"name\":...}" does not. An unknown tool should degrade to less information, never to a dump. Also fixes the display of multi-line commands. A PowerShell command is frequently several lines; the old code collapsed and truncated the whole thing, so the visible text was usually the middle of a script fragment. It now takes the first non-empty line. Redaction is unchanged and still applied on this path -- the summary runs through Protect-SecretText exactly as the JSON did, so a credential in a command still cannot reach the pill or island.log by this route. Tests: 16 new assertions in test-substep-progress.mjs, negative-first. The blanket rule is asserted per case: no result may contain a JSON key, an escaped quote, or a \uXXXX sequence. Cases cover bash, multi-line bash, read/write/edit, grep, glob, web_search, web_fetch, skill, task_output, an unknown tool with an unrecognised argument shape, and a bearer token inside a command. Validation: sub-step suite 47 pass / 0 fail, smoke 116 pass / 0 fail, all 30 .ps1 parse, and the detector still initialises cleanly under Windows PowerShell 5.1 (exit 0) -- which is the interpreter it runs under in production, and the one that breaks on a malformed file. One trap worth recording, because it produced a very misleading failure: the second parameter is deliberately NOT named `$args`. That is a reserved automatic variable in PowerShell, and binding a parameter to it yields the unbound-argument Object[] rather than the object passed in. The symptom was an empty summary for every single tool, which reads as "no data" rather than "bad binding". Verified directly: function T($n, $args) { $args.command } T bash ([PSCustomObject]@{ command = 'npm test' }) # -> ''
1 parent 4876ef1 commit 5e09518

2 files changed

Lines changed: 202 additions & 11 deletions

File tree

‎plugins/antianqi/mcode-island/mcode-status-detect.ps1‎

Lines changed: 83 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,83 @@ function Get-ToolFamily($name) {
446446
return $null
447447
}
448448

449+
# Human-readable one-liner for a tool's arguments (round-20 #6).
450+
#
451+
# The running-state message used to be built as
452+
# "$verb " + (ConvertTo-Json $args -Compress) truncated to 60 chars
453+
# which put this on an always-on-top, one-line pill:
454+
#
455+
# Running {"command":"$ErrorActionPreference=\u0027Continue\u0027\n...
456+
#
457+
# Escaped quotes, a JSON key, and a truncation that can land mid-token.
458+
# `skill` is not in $TOOL_ACTIONS or $TOOL_FAMILIES, so it always took
459+
# that path -- one of the most frequent tools on screen was also the
460+
# least readable.
461+
#
462+
# Contract: return the single most informative field for the tool, or ''
463+
# when there is nothing worth showing. Callers render the verb alone in
464+
# that case. Raw JSON is never returned -- there is no fallback to it.
465+
#
466+
# Unknown tools get a generic sweep of the common field names before
467+
# giving up, because the tool taxonomy is hand-maintained and does not
468+
# cover every tool mcode ships.
469+
# NB: the second parameter must NOT be called `$args`. That name is a
470+
# reserved automatic variable in PowerShell, and binding a parameter to
471+
# it yields the unbound-argument Object[] rather than the object passed in
472+
# -- verified: `function T($n, $args) { $args.command }` called with
473+
# `[PSCustomObject]@{command='npm test'}` returns an empty string, because
474+
# the parameter arrives as Object[]. Symptom is a summary that is empty for
475+
# every single tool, which looks like "no data" rather than "bad binding".
476+
function Format-ToolArgs($name, $toolArgs) {
477+
if (-not $toolArgs) { return '' }
478+
$key = ([string]$name).ToLowerInvariant()
479+
$raw = ''
480+
switch ($key) {
481+
'bash' { $raw = [string]$toolArgs.command }
482+
'shell' { $raw = [string]$toolArgs.command }
483+
'read' { $raw = [string]$toolArgs.file_path }
484+
'write' { $raw = [string]$toolArgs.file_path }
485+
'edit' { $raw = [string]$toolArgs.file_path }
486+
'notebookedit' { $raw = [string]$toolArgs.notebook_path }
487+
'grep' { $raw = [string]$toolArgs.pattern }
488+
'glob' { $raw = [string]$toolArgs.pattern }
489+
'list' { $raw = [string]$toolArgs.path }
490+
'search' { $raw = [string]$toolArgs.query }
491+
'web_search' { $raw = [string]$toolArgs.query }
492+
'web_fetch' { $raw = [string]$toolArgs.url }
493+
'skill' { $raw = [string]$toolArgs.name }
494+
'task' { $raw = [string]$toolArgs.description }
495+
'task_output' { $raw = [string]$toolArgs.task_id }
496+
'task_append' { $raw = [string]$toolArgs.task_id }
497+
'task_query' { $raw = [string]$toolArgs.task_id }
498+
default {
499+
foreach ($f in 'name','command','file_path','path','pattern','query','url','description','task_id') {
500+
if ($toolArgs.PSObject.Properties[$f]) {
501+
$v = [string]$toolArgs.$f
502+
if (-not [string]::IsNullOrWhiteSpace($v)) { $raw = $v; break }
503+
}
504+
}
505+
}
506+
}
507+
if ([string]::IsNullOrWhiteSpace($raw)) { return '' }
508+
509+
# The pill is a single line. A PowerShell or bash command is frequently
510+
# multi-line, so take the first non-empty line rather than letting the
511+
# body spill in and get truncated mid-token.
512+
$first = ''
513+
foreach ($ln in ($raw -split "`r?`n")) {
514+
if ($ln.Trim() -ne '') { $first = $ln.Trim(); break }
515+
}
516+
if ([string]::IsNullOrWhiteSpace($first)) { return '' }
517+
518+
# Same redaction the hook path applies, so a credential in a command
519+
# cannot reach the pill or island.log through this route either.
520+
$first = Protect-SecretText $first
521+
$first = ($first -replace '\s+', ' ').Trim()
522+
if ($first.Length -gt 60) { $first = $first.Substring(0, 57) + $DOTS }
523+
return $first
524+
}
525+
449526
# ask_user is the one tool whose "in flight" state means the agent is BLOCKED,
450527
# not busy. When the questionnaire pops, nothing progresses until the user
451528
# comes back and answers. Reporting it as `working` tells the user "leave it
@@ -543,17 +620,12 @@ function Infer-State($msg) {
543620
}
544621
$verb = Get-ToolVerb $tool 'running'
545622
if (-not $verb) { $verb = Get-ToolVerbFallback $tool 'running' }
546-
if ($args) {
547-
# ConvertTo-Json is a single .NET call; keep it (no pipeline leak).
548-
$argsJson = $args | ConvertTo-Json -Compress -Depth 2 -WarningAction SilentlyContinue
549-
# Redact BEFORE truncating (round-20). This message reaches
550-
# status.json, island.log and the pill, and `$args` is the raw
551-
# tool argument object straight out of the session log -- so a
552-
# Bash call carrying a bearer token or an API key was persisted
553-
# verbatim even after the hook path was redacted.
554-
$argsJson = Protect-SecretText $argsJson
555-
if ($argsJson.Length -gt 60) { $argsJson = $argsJson.Substring(0, 57) + $DOTS }
556-
return @{ state=$S_WORKING; message="$verb $argsJson"; family=(Get-ToolFamily $tool) }
623+
# Readable summary, never raw JSON (round-20 #6). When there is
624+
# nothing worth showing the pill wears the bare verb, which reads
625+
# fine -- "Using" beats "Using {\"name\":\"...\"}".
626+
$summary = Format-ToolArgs $tool $args
627+
if ($summary) {
628+
return @{ state=$S_WORKING; message="$verb $summary"; family=(Get-ToolFamily $tool) }
557629
}
558630
return @{ state=$S_WORKING; message=$verb; family=(Get-ToolFamily $tool) }
559631
}

‎plugins/antianqi/mcode-island/scripts/test-substep-progress.mjs‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,125 @@ ${block[0].replace(/\$payload\s*=$/, '')}
478478
`expected 2 -KeepSubStep call sites, found ${keeps.length}`);
479479
}
480480

481+
// ---------------------------------------------------------------------------
482+
// 6. The pill never renders raw tool JSON (round-20 #6)
483+
// ---------------------------------------------------------------------------
484+
//
485+
// The detector's `running` message used to be built as
486+
// "$verb " + (ConvertTo-Json $args -Compress) truncated to 60 chars
487+
// which put this on screen:
488+
//
489+
// Running {"command":"$ErrorActionPreference=\u0027Continue\u0027\n...
490+
//
491+
// Escaped quotes, a JSON key, and a truncation that can land mid-token.
492+
// `skill` is not in $TOOL_ACTIONS / $TOOL_FAMILIES, so it always took that
493+
// JSON path -- meaning one of the most frequent tools on screen was the
494+
// least readable.
495+
//
496+
// The contract: a human-readable field, never the serialised argument
497+
// object. Unknown tools resolve to the verb alone rather than dumping
498+
// JSON, because "Using" is a fine pill and "Using {\"name\":...}" is not.
499+
{
500+
const detectPath = join(PLUGIN_ROOT, 'mcode-status-detect.ps1');
501+
const detect = readFileSync(detectPath, 'utf8');
502+
503+
let formatArgs;
504+
try {
505+
formatArgs = extractFn(detect, 'Format-ToolArgs');
506+
} catch (e) {
507+
bad('Format-ToolArgs is extractable', e.message);
508+
}
509+
510+
if (!formatArgs) {
511+
bad('tool summaries are human-readable, not raw JSON',
512+
'Format-ToolArgs not found: the detector still renders ConvertTo-Json output');
513+
} else {
514+
ok('Format-ToolArgs is extractable');
515+
516+
const protectLibPath = join(PLUGIN_ROOT, 'scripts', 'lib', 'Protect-Text.ps1');
517+
const harness = `
518+
$ErrorActionPreference = 'Stop'
519+
. '${protectLibPath.replace(/'/g, "''")}'
520+
${formatArgs}
521+
$cases = ConvertFrom-Json $env:ISLAND_TEST_CASES
522+
foreach ($c in $cases) {
523+
# $c.a is already a PSCustomObject -- the outer ConvertFrom-Json turned
524+
# the nested object into one. Re-parsing it fails on the '@'.
525+
$r = Format-ToolArgs $c.t $c.a
526+
'{0}={1}' -f $c.n, $r
527+
}
528+
`;
529+
530+
const cases = [
531+
{ n: 'bash', t: 'bash', a: { command: 'npm test' }, want: 'npm test' },
532+
{ n: 'bashMulti', t: 'bash', a: { command: "$x = 'Stop'\nGet-ChildItem" }, want: '$x' },
533+
{ n: 'read', t: 'read', a: { file_path: 'C:\\proj\\a\\file.ts' }, want: 'file.ts' },
534+
{ n: 'write', t: 'write', a: { file_path: 'C:\\proj\\a\\out.json' }, want: 'out.json' },
535+
{ n: 'edit', t: 'edit', a: { file_path: 'C:\\proj\\a\\x.ps1' }, want: 'x.ps1' },
536+
{ n: 'grep', t: 'grep', a: { pattern: 'TODO' }, want: 'TODO' },
537+
{ n: 'glob', t: 'glob', a: { pattern: '**/*.ps1' }, want: '*.ps1' },
538+
{ n: 'websearch', t: 'web_search', a: { query: 'mcode changelog' }, want: 'mcode changelog' },
539+
{ n: 'webfetch', t: 'web_fetch', a: { url: 'https://example.com/y' }, want: 'example.com' },
540+
// skill is absent from the tool taxonomy: this is the case
541+
// that was guaranteed to render as raw JSON.
542+
{ n: 'skill', t: 'skill', a: { name: 'docx' }, want: 'docx' },
543+
{ n: 'taskout', t: 'task_output', a: { task_id: 'bg_abc123' }, want: 'bg_abc123' },
544+
// Unknown tool with an unrecognised shape: verb-only, not JSON.
545+
{ n: 'unknown', t: 'wibble', a: { zzz: 1 }, want: '' },
546+
// Credential in a command must still be redacted on this path.
547+
{ n: 'secret', t: 'bash', a: { command: "curl -H 'Authorization: Bearer eyJhbGciOi.SUPERSECRET'" } },
548+
];
549+
550+
const out = runPs(
551+
`$env:ISLAND_TEST_CASES = '${JSON.stringify(cases).replace(/'/g, "''")}'\n` + harness);
552+
const got = {};
553+
for (const line of out.split(/\r?\n/)) {
554+
const m = line.match(/^(\w+)=(.*)$/);
555+
if (m) got[m[1]] = m[2];
556+
}
557+
558+
for (const c of cases) {
559+
if (!(c.n in got)) {
560+
bad(`summary ${c.n}`, `no output produced (got ${JSON.stringify(out.trim())})`);
561+
continue;
562+
}
563+
const v = got[c.n];
564+
// The blanket rule, asserted for every case including the
565+
// secret one: nothing JSON-shaped reaches the pill.
566+
const jsonish = /\{\s*"|":\s*"|\\u00[0-9a-f]{2}/i.test(v);
567+
if (jsonish) {
568+
bad(`summary ${c.n}`, `renders raw JSON: ${JSON.stringify(v)}`);
569+
continue;
570+
}
571+
if (c.want === '') {
572+
// Unknown tool with nothing worth showing: must be empty,
573+
// not merely "not JSON". A partial dump is still a dump.
574+
if (v !== '') {
575+
bad(`summary ${c.n}`, `expected empty, got ${JSON.stringify(v)}`);
576+
continue;
577+
}
578+
} else if (c.want !== undefined) {
579+
if (!v.toLowerCase().includes(String(c.want).toLowerCase())) {
580+
bad(`summary ${c.n}`, `expected to contain ${JSON.stringify(c.want)}, got ${JSON.stringify(v)}`);
581+
continue;
582+
}
583+
}
584+
if (c.n === 'secret' && /SUPERSECRET/.test(v)) {
585+
bad('summary secret', `credential survived: ${JSON.stringify(v)}`);
586+
continue;
587+
}
588+
ok(`summary ${c.n}${c.want !== undefined ? ` -> ${JSON.stringify(v)}` : ' (redacted)'}`);
589+
}
590+
591+
// A multi-line command must not spill its body onto a one-line pill.
592+
if ('bashMulti' in got && /Get-ChildItem/.test(got.bashMulti)) {
593+
bad('summary bashMulti', 'a multi-line command leaked its second line onto the pill');
594+
} else if ('bashMulti' in got) {
595+
ok('summary bashMulti keeps only the first line');
596+
}
597+
}
598+
}
599+
481600
// ---------------------------------------------------------------------------
482601
// summary
483602
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)