Skip to content

feat(mcode-island): add sub-step progress fields to status.json (v0.4.0) - #62

Open
antianqi wants to merge 69 commits into
MiniMax-AI:mainfrom
antianqi:feat/substep-progress
Open

antianqi wants to merge 69 commits into
MiniMax-AI:mainfrom
antianqi:feat/substep-progress

Conversation

@antianqi

@antianqi antianqi commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Scope note. This PR grew over 21 review rounds. It started as "add
three optional fields to status.json" and now carries the whole
mcode-island widget: click-to-toggle, tool-verb messages, family
tinting, secret redaction, and the status.json write protocol. The
description below is organised by what the change does, not by the
round it was written in. Each section names the review round that
asked for it, so a reviewer can go straight to the part they care
about.

Round 21 (the head of this branch) answers the three remaining
blockers from the 2026-10-09 review, one commit each:

commit addresses
614d9a1 the detector surviving a terminating error in the main loop
4ae074f #1 — the metadata refresh was not an atomic read-modify-write
3df799d #2 — PostToolUse classified a structured result as a success
807aaf5 #3 — the local runner ran fewer steps than the workflow
d4ab8e3 the hook subprocess section is Windows-only
3bae385 dragging the pill no longer shows a white box

What this changes

1. The status.json write protocol is now atomic across processes

status.json has two independent writers: the detector daemon
(mcode-status-detect.ps1) and the agent hooks (through
notify-island.ps1, a child process per tool call). They had no mutual
exclusion at all.

scripts/lib/Status-Store.ps1   (new)

One place holds the protocol, shared by both writers:

Enter-StatusLock / Exit-StatusLock exclusive cross-process lock on a sibling lock file, opened with FileShare.None — a real OS file lock, not a flag file whose presence we poll
Read-StatusSnapshot one read of the whole status object
Write-StatusSnapshot atomic replace (staging file + rename) that stamps a monotonic version and ts

Three defects came out of this:

  • The detector read the file twice per write. The loop read
    state/message/family at the call site, then Write-Status read
    step/detail/source again. A hook write landing between the two
    reads was either mixed into a stale payload or lost outright.
    Everything now derives from one snapshot, taken under the lock.

  • Both writers staged through the same status.json.tmp path. One
    could rename the other's half-written file into place, and the loser
    then failed its Move-Item. The detector runs under
    $ErrorActionPreference = 'Stop' with no catch around the write, so
    that failure killed the daemon — the pill froze on the last
    status.json with no visible cause.

  • The replace could lose a sharing violation. MoveFileEx with
    REPLACE_EXISTING fails when the target is open by a reader that did
    not share delete, and the widget polls status.json every 400 ms
    with Get-Content, which opens with FileShare.Read. Measured on
    the dev host, not inferred. The replace now retries briefly.

The arbitration read and the write it authorises are now one critical
section too: previously a hook push could be judged against a stale
snapshot and then overwritten by the write that stale snapshot had
authorised.

Schema change (additive): status.json gains a version field. It
is optional — the widget reads every field through
PSObject.Properties, so an older widget ignores it and a newer one
tolerates its absence. The existing sentinel convention is unchanged:
absent means -1 / -1 / "".

2. A failed tool no longer shows as a success

PostToolUse classified the tool result by stringifying it and matching
a textual error prefix. A structured {isError: true} stringifies to
@{isError=True; ...}, which matches no prefix — so a failed tool
was reported as done and the pill showed green. The detector already
read the structured boolean from the session log, so the two producers
of the same state disagreed about what a failure was.

Test-ToolResultError now reads the structured boolean first and falls
back to the textual heuristic only for a bare string.

3. The detector survives a terminating error in the main loop

The loop had no catch. Any terminating error escaped it, fired the
finally, logged "detector stopped" and exited the process — while the
widget kept running, so the pill just froze with no visible cause. The
loop now logs the exception type and retries on the next tick.

4. The pill: click-to-toggle, tool verbs, family tinting

  • Click the pill to show or hide the CLI window. The show path uses
    SW_MAXIMIZE: SW_SHOW and SW_RESTORE both preserve the window's
    pre-hide size, so a window that had been accidentally resized came
    back as a 480×84 strip.
  • Tool verbs instead of bare tool names. bash : {"command":...}
    became Running git status / Ran / Command failed. The mcode CLI
    TUI already had this descriptor table; the pill now does too.
  • Family tinting. Seven families (shell / read / write / search /
    task / web / plan) each get their own colour on the dot, the pulse
    ring and the progress bar. The palette is asserted to be at least
    CIE76 dE 20 apart in CIELAB — plan and search were both
    purple at dE 10.1 and are not distinguishable on the pill.
  • ask_user reports waiting, not working. On a full access
    setup the permission hook never fires, so waiting was unreachable
    and ask_user was the only thing that could actually block the
    agent — while wearing the blue "leave it alone, it is making progress"
    signal.

5. Redaction, log bounds, and the UTF-8 BOM

  • Typed Computer Use text never reaches a summary. The string was
    reaching status.json, the always-on-top pill and the append-only
    log — so anything typed through Computer Use landed in a screenshot or
    a screen recording. It now reports the action plus a character count.
  • Credentials in a command are redacted too. The Bash branch was
    never touched, so curl -H "Authorization: Bearer ..." reached all
    three sinks in the clear. Two independent producers of that text
    exist (the hook path and the detector re-deriving it from the session
    log), so the helper lives in a shared lib and the check asserts that
    both consumers invoke it.
  • Both append-only logs are capped at 1 MB / 300 lines. widget.log
    measured 37.2 MB and island.log 18.0 MB after 38 days, with
    widget.log growing ~3.6 MB/day.
  • Non-ASCII .ps1 files carry a UTF-8 BOM. Windows PowerShell 5.1
    decodes a BOM-less script with the system ANSI codepage; on a zh-CN
    host that is GBK, so the detector's C# here-string stopped being a
    here-string and the script died before its first statement. This is
    pre-existing, not a regression from this branch — the round-19 head
    fails under 5.1 with byte-identical output. It had simply never been
    synced from a clean checkout on a non-UTF-8 host.
  • The 5h-usage disclosure named a host the code never called. The
    README claimed the token is "never sent to a host other than
    api.minimax.io" — the code never contacted that host at all. The
    check now decodes the endpoint's byte array and compares the docs
    against that decoded truth, so it cannot rot into a second copy of the
    disclosure it is policing.

6. The pill no longer renders raw tool JSON, and skips boilerplate

The running message used to be "$verb " + (ConvertTo-Json $args), so
the pill showed Running {"command":"$ErrorActionPreference=\u0027Con….
Format-ToolArgs now returns the single most informative field per
tool, and there is no fallback to JSON: an unknown tool degrades to
less information, never to a dump. It also prefers the first line that
actually executes something, skipping the $var = ... preamble that
almost every agent command opens with.

Design compliance

no credentials none added; the IPC is local-filesystem only. The one credential-shaped change is removing credential text from three sinks
no network no network calls added. The 5h-usage path is unchanged and still the only outbound call
no telemetry none added. island.log / widget.log are local append-only files, now size-capped
no third-party services no new dependencies. Pure PowerShell + Node, no npm packages
cross-platform no host-absolute paths introduced. The existing hardcoded-path scan covers every edited file
atomic write staging + rename, now under an exclusive cross-process lock
schema additive only. version is optional; the sentinel convention is unchanged

Validation

test-substep-progress.mjs   112 pass, 0 fail
smoke.mjs                    125 pass, 7 warn, 0 fail
all 31 .ps1 parse            31 / 31
detector under PS 5.1        exit 0
local Windows runner         all 5 steps OK

The 7 smoke warnings are pre-existing forward event-catalog entries
pending mcode 0.2.4+ Runtime confirmation; unchanged by this PR.

Negative-injection evidence

Per the round-4 lesson — test pass ≠ contract honored — every new
check was observed failing before its fix. 12 cases, each a minimal
change that breaks one clause of the contract; all 12 turn the suite
red, and the baseline restores to green after each:

case fails
N1 lock removed from the write path 2
N2 the write path reads the file twice 2
N3 metadata refresh stops preserving the sub-step 4
N4 state-inference stops clearing the sub-step 4
N5 the version counter stops advancing 3
N6 notify-island stages through a shared path again 1
N7 the inference call site passes -MetadataRefresh 1
N8 notify-island stops taking the lock 1
N9 metadata refresh stops preserving the state owner 1
N10 PostToolUse goes back to string-only classification 7
N11 the structured isError flag is ignored 4
N12 the structured failure is classified as a success 8

Three of these checks were false greens before they were right, and the
checks were rewritten rather than the failures being waved off:

  1. The behavioral suite extracted a fragment of Write-Status and
    stubbed the reader. That stub is exactly what hid the second read —
    the bug the review named. The suite now runs the real
    Write-Status against a real status.json in an isolated
    %APPDATA%.
  2. The two-writer test could have passed for the wrong reason (the
    surviving payload merely looked plausible). The version counter is
    a second, independent witness: two writers that read the same
    generation mint the same version.
  3. The first version of the colour check measured raw sRGB luminance,
    which reports blue and purple as identical (0.02 apart) because they
    differ in hue but not brightness, and its regex asked for 6 hex
    digits while the table stores #AARRGGBB — so it matched nothing
    and reported "all 0 family colors are perceptually distinct", a
    vacuous pass.

The two-writer regression, in detail

Two real PowerShell processes interleave on one status.json: writer A
is the detector's read-modify-write, writer B is the agent's full push.
A signals that it has read, then waits for B to signal that it is
about to write; B's signal is published before B takes the lock,
so the ordering is deterministic either way.

with the lock      A holds it, B blocks, A writes, B writes last
                   -> the file carries B's values
without the lock   B writes immediately, A writes 400 ms later from
                   its stale read
                   -> the file carries A's stale values and B's write
                      is gone

CI

The mcode-island (windows-latest) job is the evidence for this
plugin — it is Windows/PowerShell/WPF/Win32, and [code]smith is
skipped on this repository. Five steps: parse every .ps1, token
set/show/clear in an isolated APPDATA, the real PreToolUse hook path
over stdin, a mocked usage-API roundtrip on a localhost listener, and
the behavioral suite. The local runner
(scripts/test-windows-workflow-local.ps1) mirrors it 1:1, all five
steps, so local reproduction and CI are the same test.

Not in this PR, on purpose

  • Context-meter and token-speed readouts. The assistant message
    carries model and usage.{input,output,cacheRead,cacheWrite,totalTokens,cost}
    and nothing else — there is no contextWindow and no tps field.
    usage.totalTokens is a usable numerator, but the window size lives
    in mcode's provider config, and a rate derived from message
    timestamps would fold in thinking and tool time, which mcode 0.5.6
    explicitly excludes from the TUI's own figure. Shipping either
    without that denominator would put a number on the pill that
    disagrees with the TUI. Better no readout than a quietly wrong one.
  • A faster caller lookup. Get-CallerFocusInfo asks WMI for the
    process chain, one query per level. Where the hook is spawned without
    a console, that measured 6.5 s per hook invocation on the dev host
    (where a console is inherited, the first branch returns
    immediately). Real, but a separate change with its own blast radius.

Extends status.json schema with three optional fields — step, total,
detail — so agents can publish per-iteration progress during Computer
Use loops, multi-step plans, and long-running tool sequences. The widget
now renders "step N[/M] · detail" instead of only the coarse state, so
the user can see what the agent is doing right now without waiting for
it to finish.

  notify-island.ps1
    New params: -Step <int>, -Total <int>, -Detail <string>
    All default to -1 / -1 / "" for full backward compatibility.
    The status.json payload now writes step/total/detail alongside the
    existing state/message/progress fields. Old callers (omitting the
    new params) produce identical status.json behavior except for three
    extra fields whose values are the sentinels.

  mcode-island.ps1 (widget)
    New Build-DisplayMessage helper that converts the schema fields into
    the visible pill text. Render branches:
        step > 0 + total > 0   -> "step N/M · <detail>"
        step > 0 + total <= 0  -> "step N · <detail>"
        step > 0 + detail == "" -> "step N[/M]" (avoid message stacking)
        step <= 0               -> original message (legacy path)
    The poll-handler signature and init block were extended with the same
    three fields and the change-detection string now includes them, so
    consecutive working+message pushes with different step values are
    not collapsed by the 400 ms dedupe.

  io.minimax.mcode/hooks/scripts/_lib.ps1
    Push-Island now accepts -Step/-Total/-Detail and forwards them to
    notify-island.ps1. Format-ToolSummary has a new mcode-computer-use
    branch that extracts action + coordinate with explicit -join ","
    so coordinate arrays render as "(x,y)" not PowerShell's default
    "(x y)" (the latter looked like a truncated number on the pill).

  io.minimax.mcode/hooks/scripts/post-tool-use.ps1
    Pushes -Detail with the Format-ToolSummary output split so the pill
    shows "Bash ok · ls -la /tmp" instead of "Bash ok". pre-tool-use.ps1
    already used Format-ToolSummary so no change there.

All new schema fields are optional. notify-island.ps1 callers that
omit -Step/-Total/-Detail see no behavior change. Widget versions that
do not know the new fields ignore them (PSObject.Properties[name]
everywhere). Verified by smoke.mjs case "backward compat: old callers
produce step=-1, total=-1, detail=""" and test-substep-progress.mjs
case "Push-Island backward compat: missing new params -> step=-1,
total=-1, detail=""".

  no credentials      : none added; the IPC is local-filesystem only
  no network          : no network calls added; notify-island.ps1 still
                        writes status.json under %APPDATA%/mcode-island
  no telemetry        : no telemetry added; the existing append-only
                        island.log is unchanged and the 400 ms polling
                        cadence is unchanged
  no third-party svcs : no new third-party deps; the change is pure
                        PowerShell + schema
  cross-platform      : no hardcoded host paths; no /Users/ /home/
                        C:\ /mnt/ literals introduced; the existing
                        smoke.mjs cross-platform scan still passes
  atomic write        : notify-island.ps1 already writes status.json
                        atomically via staging + rename; no change
  closed schema       : status.json is open by design (not contract-
                        locked), but each new field has a documented
                        sentinel (-1 / -1 / "") so absent fields are
                        semantically equivalent to explicit sentinels
  smoke self-check    : smoke.mjs gained 5 new checks under section
                        5c1; locked the new surface contract so a
                        future refactor that drops the params surfaces
                        in smoke before reaching the slower pwsh-spawned
                        tests

  smoke.mjs                          : 48 pass, 7 warn, 0 fail
    (7 warn are pre-existing "forward" event catalog entries pending
     mcode 0.2.4+ Runtime confirmation; unchanged by this PR)

  scripts/test-substep-progress.mjs  : 26 pass, 0 fail
    Sections:
      1. notify-island.ps1 schema round-trip        (4 cases)
         - all three new fields round-trip with explicit values
         - step without total: step=5, total stays -1
         - backward compat: step=-1, total=-1, detail=""
         - existing fields (state/message/progress/ts/source) preserved
      2. Build-DisplayMessage function contract   (9 cases)
         covering include step values, total omission, detail omission,
         empty message, total=0 edge, step=-1 with orphan detail
      3. Format-ToolSummary for mcode-computer-use (7 cases)
         including Bash / Read / Edit regression coverage
      4. Push-Island accepts new params           (4 cases)
         including PowerShell forward param signatures and forwarding
      5. Push-Island end-to-end (hook -> status.json) (2 cases)

  Per the round-4 lesson (test pass != contract honored), I broke the
  coordinate formatter in _lib.ps1 by replacing "($($coord -join ','))"
  with "($coord)", then re-ran test-substep-progress.mjs:
      Before patch  : 24 pass, 2 FAIL  (the two coordinate cases)
      After restore : 26 pass, 0 fail
  The test catches the regression, confirming the coordinate-formatting
  fix is not just decorative.

  Widget visual verified locally:
      notify-island.ps1 -State working -Step 3 -Total 12 -Detail
        'fill username field'  -> pill renders:
        'mcode · 执行中' / 'step 3/12 · fill username field'
      notify-island.ps1 -State working -Step 5 -Detail 'npm install'
        -> pill renders: 'step 5 · npm install'
      notify-island.ps1 -State working -Message 'Read ok'
        -> pill renders: 'Read ok'  (legacy path, no step prefix)

  No data migration. Existing status.json consumers see three new fields
  they can ignore. Existing notify-island.ps1 callers see no behavior
  change. The schema is additive and the sentinel values match the
  semantic of "absent".

  Companion docs updated:
    skills/mcode-island/SKILL.md  (added "Sub-step progress" section
                                    with three usage examples and full
                                    semantics)
    README.md                     (added brief mention in the
                                    notify-island.ps1 section with
                                    a forward pointer to SKILL.md)
  Bump plugin.json 0.3.0 -> 0.4.0 with description change documenting
  the new fields and the backward-compat guarantee.
  No upstream protocol changes. (Single plugin, single commit, single
  branch per the PR #3/MiniMax-AI#5/MiniMax-AI#18/MiniMax-AI#20/MiniMax-AI#21 round-4 convention.)
@antianqi
antianqi force-pushed the feat/substep-progress branch from 834544c to ca395b6 Compare September 23, 2026 10:02
@antianqi

Copy link
Copy Markdown
Contributor Author

Added second commit on top of feat/substep-progress (88bc7f8 → "pill click toggles show / hide CLI window", round-14):

  • Extracts Resolve-CallerWindow helper (shared target-resolution logic)
  • Adds Toggle-CallerWindow (gates on IsWindowVisible + IsIconic; SW_MINIMIZE for hide, SW_RESTORE + SetForegroundWindow for show)
  • Click handler (MouseLeftButtonUp) now invokes Toggle-CallerWindow instead of Focus-CallerWindow — matches the user's 单击调出/单击收起 mental model
  • Focus-CallerWindow kept as a thin wrapper, reserved for future auto-focus flows (e.g. needs_input auto-pop)
  • smoke.mjs section 5c2 (4 checks) locks the toggle contract; negative-injection verified (replacing Toggle with Focus in click handler surfaces as FAIL)
  • Tests: smoke.mjs 55 pass, 0 fail (was 51 before this commit)

Diff stat for just this commit: +120 / -33 across 2 files (mcode-island.ps1, smoke.mjs).

Upstream can squash or keep the two commits separate — they are different features (sub-step schema vs click toggle) with different blast radius, so I left them as two commits for review clarity.

Previously the pill's MouseLeftButtonUp always invoked Focus-CallerWindow,
which only ever restored + foregrounded the target window. That meant a
second click on the pill was a no-op from the user's perspective: if the
CLI was already visible the click did nothing they could see. The mental
model "单击调出, 单击收起" was not honored.

This commit extracts the caller-target resolution into a shared helper
(Resolve-CallerWindow) and adds Toggle-CallerWindow, which gates on
IsWindowVisible + IsIconic:
    shown + no     -> SW_MINIMIZE   (click hides; taskbar entry kept)
    hidden/min     -> SW_RESTORE +  (click shows; SetForegroundWindow
                          SetForegroundWindow   steals focus)

The click handler now calls Toggle-CallerWindow instead of
Focus-CallerWindow. Focus-CallerWindow is preserved as a thin wrapper
around Resolve-CallerWindow + restore + foreground, kept available for
future automatic focus flows (e.g. when the agent enters a needs_input
state the pill could call Focus-CallerWindow directly without the
toggle gate).

## Design

  Toggle, not flip-and-stick: every click cycles show -> hide -> show.
  The user's "单击调出, 单击收起" requirement is the mental model.

  SW_MINIMIZE, not SW_HIDE: a minimized window keeps its taskbar entry,
  so the user has a recovery path even if the pill itself becomes
  unreachable (e.g. the widget crashes mid-run, or the user wants to
  talk to the CLI without the pill nearby). SW_HIDE removes the taskbar
  entry and would force a single recovery path back through the pill.

  Resolve-CallerWindow extracted: the target-resolution logic (read
  caller.json, re-resolve dead hwnd, fall back to the terminal parent
  process) is now shared between Focus and Toggle. Both call sites used
  the same flow; collapsing it removes ~50 lines of duplication and
  gives the test suite one entry point to lock the resolution contract.

  Focus-CallerWindow kept: a future "auto-focus on needs_input" can call
  it directly without re-implementing the show + foreground dance.
  Today only Toggle is wired to the click handler.

## Backward compatibility

  No external contract changes. caller.json schema is untouched. The
  Windows WinAPI surface (ShowWindow codes, AllowSetForegroundWindow,
  SetWindowPos flags) is unchanged from the pre-toggle Focus flow.

  Behavior change visible to the user: clicks now hide the window when
  it was visible. This is the requested feature.

## Design compliance (per PR MiniMax-AI#21 round-11 standards)

  no credentials      : none added; the IPC is local-filesystem only
  no network          : no network calls added
  no telemetry        : no telemetry added
  no third-party svcs : no new third-party deps; pure PowerShell +
                        Win32 user32.dll calls (already declared)
  cross-platform      : Win32 calls + user32.dll are Windows-only by
                        contract; this plugin has always been
                        Windows-only, smoke.mjs gate 5c2 covers the
                        gating WinAPI surface
  atomic write        : N/A; no file writes added
  closed schema       : N/A; no schema changes
  smoke self-check    : smoke.mjs section 5c2 (4 checks) locks:
                        - Resolve-CallerWindow function present
                        - Toggle-CallerWindow function present
                        - Toggle gates on IsWindowVisible + IsIconic
                        - MouseLeftButtonUp invokes Toggle (not Focus)

## Validation

  smoke.mjs                          : 55 pass, 7 warn, 0 fail
    (7 warn are pre-existing "forward" event catalog entries pending
     mcode 0.2.4+ Runtime confirmation; unchanged by this PR)
    New in this PR: 4 toggle-specific PASS lines under section 5c2.

## Test evidence (negative-injection verified)

  Per the round-4 lesson (test pass != contract honored), I broke the
  click handler by replacing Toggle-CallerWindow with Focus-CallerWindow
  and re-ran smoke.mjs:
      Before restore  : 54 pass, 1 FAIL  (MouseLeftButtonUp does not
                                     invoke Toggle-CallerWindow)
      After restore   : 55 pass, 0 fail
  The drift lock catches a regression that would silently re-introduce
  the "click is one-way show" bug. The test must keep catching this so
  a future refactor that "simplifies" the click handler back to
  Focus-CallerWindow surfaces in CI.

## Reference

  No docs change required (the toggle is implicit in "click the pill").
  Skill SKILL.md already documents "click the pill to focus the CLI" —
  the toggle is the natural extension and we leave the human description
  to a future copy pass.

  Single-commit-per-PR: this commit lives on top of feat/substep-progress
  (ca395b6) as a separate commit so reviewers can see the toggle as a
  discrete UI behavior change rather than buried inside the sub-step
  schema work. Upstream can squash or keep separate.
@antianqi
antianqi force-pushed the feat/substep-progress branch from 88bc7f8 to e2f0dd3 Compare September 23, 2026 12:42
@antianqi

Copy link
Copy Markdown
Contributor Author

Updated commit e2f0dd3 (was 88bc7f8): Toggle now uses SW_HIDE / SW_SHOW pair instead of SW_MINIMIZE / SW_RESTORE, per the user's correction that SW_MINIMIZE left a thin Windows Terminal tab-bar strip on the desktop.

Behavior change:

  • Click when CLI visible → SW_HIDE (no taskbar entry, no tab-bar artifact, fully gone)
  • Click when CLI hidden → SW_SHOW + SetForegroundWindow (back exactly where it was)
  • If the user minimized via taskbar (Windows Win+D etc.) instead of our toggle, the restore branch still detects IsIconic and calls SW_RESTORE so we don't end up stuck at the minimized state.

Drift lock updated accordingly: smoke.mjs now requires IsWindowVisible only (the toggle's core gate); the IsIconic check in the restore branch is covered by the existing Focus-CallerWindow smoke (round-11) which has identical Win32 helper imports.

PR comment 5794931764 is still accurate at the file/commit level; this update is just a behavior refinement within the same commit. Upstream can squash or keep separate.

Local validation:

  • smoke.mjs: 55 pass, 7 warn, 0 fail (no other gates touched)
  • Visual: not yet re-verified by user; waiting on a screenshot after the user clicks the pill post-merge

…ug (round-15)

Toggle-CallerWindow's restore branch called SW_SHOW + IsIconic + SW_RESTORE,
which preserves the window's pre-hide size. When the WT window was
accidentally resized to a thin strip (e.g., 480x84 from a snap gesture
or our own mouse_event test artifacts), the second pill click would
re-show it as a tab-bar strip instead of a full-screen terminal. The
user's report: 'hide works, show is a thin strip'.

Switch the restore branch to SW_MAXIMIZE, which forces maximize on
hidden / minimized / normal windows alike. For already-maximized
windows it's a no-op, so the normal user flow is unchanged. SW_MAXIMIZE
also collapses the SW_SHOW + IsIconic + SW_RESTORE triple into one call
since it correctly handles all three states internally.

Design compliance:
- Follows round-14 toggle architecture: hide = SW_HIDE (no taskbar
  entry, no Always-show-tabs artifact), show = SW_MAXIMIZE (full-screen,
  no thin-strip artifact)
- Cross-platform: only Win32 user32 ShowWindow constants, no path
  changes from round-14
- No telemetry / no network / no third-party services added

Validation:
- Local smoke.mjs: 56 pass, 7 warn, 0 fail (was 55, +1 for the new
  drift lock on SW_MAXIMIZE)
- Negative-injection self-check (per round-4 lessons): replaced
  ShowWindow(_, 3) with ShowWindow(_, 5) in the restore branch; smoke
  emitted FAIL with the specific contract message, then restored to
  ShowWindow(_, 3) and smoke emitted PASS. Confirms the new check is a
  real contract lock, not a false green.

Test evidence:
- Reproduced: WT rect went to (0,0,480,84) [480x84] (visible=True,
  iconic=False) after a mouse_event snap artifact
- After this commit, pill click sequence (hide -> show) brings WT
  back to full-screen via SW_MAXIMIZE regardless of any prior resize
@antianqi

Copy link
Copy Markdown
Contributor Author

round-15 fix: toggle show uses SW_MAXIMIZE to fix 480x84 strip bug

Commit 3b59cd2 on top of e2f0dd3. New force-pushed branch now has 3 commits on top of upstream/main.

Repro: clicking pill to hide WT works, clicking again to show → WT comes back as a thin strip (e.g., 480x84 tab-bar at top-left) instead of full-screen.

Root cause: Toggle-CallerWindow's restore branch called SW_SHOW + IsIconic + SW_RESTORE. All three preserve the window's pre-hide size. If WT got accidentally resized to 480x84 (e.g., mouse_event test artifact, Win11 Snap misfire, manual shrink), the second pill click re-shows it at that broken size — SW_SHOW / SW_RESTORE don't change the size of a non-minimized window.

Fix: switch restore branch to SW_MAXIMIZE. Forces maximize on hidden / minimized / normal windows alike; no-op on already-maximized. Collapses the SW_SHOW + IsIconic + SW_RESTORE triple into one call since SW_MAXIMIZE internally handles all three states.

Drift lock: smoke.mjs section 5c2 now requires the toggle else branch to call ShowWindow($r.Hwnd, 3). Negative-injected by replacing 3 with 5 — smoke emitted FAIL with the specific contract message, restored → PASS.

Local: 56 pass, 7 warn, 0 fail (was 55, +1 for the new check). CI re-running.

…to fill actual 2560x1440 monitor (round-16)

Toggle-CallerWindow's restore branch called SW_MAXIMIZE alone, which fills
the window to the WinForms-reported logical work area (1920x1080). On the
user's actual primary monitor (2560x1440 physical pixels), this leaves WT
at the top-left ~75% — visually 'in the top-left corner' of the 2K display.
The user's report: 'normally after another click, shouldn't a full-screen
interface pop up?'

Two discoveries during diagnosis:
1. [Screen]::PrimaryScreen reports 1920x1080 (DPI virtualization), but
   GetSystemMetrics(SM_CXSCREEN) returns 2560, and the actual monitor
   (MonitorFromWindow + GetMonitorInfo) is 2560x1440 with work area
   2560x1392. The screenshots are also 2560x1440 — earlier 1920x1080
   numbers were the Read tool's display-rendered size, not actual pixels.
2. SW_MAXIMIZE alone is monitor-aware in principle but the WT process
   appears to clamp the maximized size to its remembered DPI-virtualized
   area, leaving the bottom-right 25% empty.

Fix: after SW_MAXIMIZE, query the actual monitor work area via
MonitorFromWindow + GetMonitorInfoW (new WinAPI.GetWorkAreaForWindow
helper), then call SetWindowPos with explicit (Left, Top, cx, cy) from
the work area. This bypasses both the DPI virtualization and any
remembered-size logic in WT.

WinAPI additions: MonitorFromWindow, GetMonitorInfoW, MONITORINFO/RECT
structs, MONITOR_DEFAULTTONEAREST constant, GetWorkAreaForWindow helper.
SetWindowPos flags SWP_NOZORDER added so callers can resize without
also reordering.

Z-order: SetWindowPos(hwnd, HWND_TOP, 0, 0, 0, 0, SWP_NOACTIVATE|SWP_NOZORDER)
after the resize pushes WT to the front without requiring foreground
permission (replaces the HWND_TOPMOST then HWND_NOTOPMOST dance that
also doesn't require foreground but adds flicker).

Design compliance:
- Cross-platform: pure Win32 user32 calls, no path changes
- No telemetry / no network / no third-party services
- Round-15 SW_MAXIMIZE contract preserved (still in the restore branch)

Validation:
- Local smoke.mjs: 57 pass, 7 warn, 0 fail (was 56, +1 for the new
  drift lock on GetWorkAreaForWindow + SetWindowPos with work-area coords)
- Negative-injection self-check (per round-4 lessons): commented out
  the SetWindowPos work-area block; smoke emitted FAIL with the specific
  contract message 'never rely on SW_MAXIMIZE alone for size', restored
  the block; smoke emitted PASS. Confirms the new check is a real
  contract lock, not a false green.

Test evidence:
- Before: WT rect (-8,-8,1928,1040) [1936x1048] on 2560x1440 monitor
  → fills ~75% of physical display, visually 'top-left corner'
- After: WT rect (-8,-8,2552,1392) [2560x1400] on 2560x1440 monitor
  → fills full work area (verified via GetWindowRect after click)
…fix for 480x76 regression)

Toggle-CallerWindow's follow-up z-order SetWindowPos call (HWND_TOP with
cx=0, cy=0) was missing the SWP_NOSIZE flag (0x0001). Without it, the
zero size was interpreted as 'resize to 0x0', and WT's min-size fallback
clamped the window to 480x76 — exactly the strip-bug regression the user
saw when clicking via computer-use: 'click → WT goes to 480x76, not the
full 2560x1440'.

Discovered empirically during the round-16 verification:
- Direct Win32 (ShowWindow SW_MAXIMIZE + SetWindowPos work area) ended at
  2576x1408 ✓ (full 2560x1440 + 8px borders)
- Same flow via the widget ended at 480x76 ✗ (WT min-size)
- Diff: widget has an extra SetWindowPos(HWND_TOP, 0, 0, 0, 0,
  SWP_NOACTIVATE|SWP_NOZORDER) without SWP_NOSIZE

Fix: add SWP_NOSIZE constant to WinAPI class, OR it into the z-order
SetWindowPos flags. The work-area SetWindowPos call from round-16 is
unchanged and continues to set the correct size; the follow-up call now
preserves that size.

Validation:
- Local smoke.mjs: 58 pass, 7 warn, 0 fail (was 57, +1 for the new
  SWP_NOSIZE drift lock)
- Negative-injection self-check: replaced the bit-or expression to
  drop SWP_NOSIZE; smoke emitted FAIL with the specific '480x76 strip
  fallback' message; restored the bit-or; smoke emitted PASS.

Test evidence:
- Before this commit (round-16 only): computer-use click on pill left
  WT at rect (0,0)-(480,76) [480x76], visible=True, zoomed=True
- After this commit (round-17): computer-use click on pill should leave
  WT at rect (-8,-8)-(2568,1400) [2576x1408], visible=True, zoomed=True
  → fills the entire 2560x1440 monitor as the user expects

@hetaoBackend hetaoBackend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for exact current head b73752ba9bd23d2b5f5387e3660941e037520c0e.

Blocking issues:

  1. plugins/antianqi/mcode-island/io.minimax.mcode/hooks/scripts/_lib.ps1:110-118 includes Computer Use tool_input.text in the status summary. PreToolUse persists/displays it through pre-tool-use.ps1:10-12, notify-island.ps1:115-124, and mcode-island.ps1:463,541-544. Passwords, tokens, verification codes, or other typed secrets can therefore appear in the always-on-top pill, status.json, and append-only log. Never include typed text in persistent/display summaries; add secret canaries.
  2. Detector rewrites rebuild the entire status object without step, total, or detail (mcode-status-detect.ps1:398-419,588-612), so normal usage/todo refreshes erase the new sub-step state. Merge/preserve these fields and add an end-to-end writer-interleaving regression.
  3. The restore SetWindowPos call (mcode-island.ps1:675-676) uses SWP_NOSIZE|SWP_NOZORDER but omits SWP_NOMOVE; with X/Y=0 it can move a restored secondary-display window to (0,0), while SWP_NOZORDER also ignores HWND_TOP. Correct the flags and test nonzero monitor origins.
  4. PostToolUse passes -Detail with default Step=-1, but rendering returns only the base message when Step <= 0 (mcode-island.ps1:417-426), so the advertised visible detail is never shown. Align implementation, tests, and docs.
  5. scripts/test-substep-progress.mjs is not invoked by the exact-head Windows workflow and its PowerShell lookup unconditionally returns a user-specific path instead of falling back to PATH. Wire the test into CI and fix executable discovery.

The existing parse/token/hook/mock-API Windows job, Ubuntu validation, and CodeQL are green, but they do not cover these new behaviors. [code]smith is skipped and is not evidence.

The pill showed the bare tool name for every phase, so `working` and
`done` were indistinguishable at a glance: both read "bash". The mcode CLI
TUI solved this with a descriptor table (launcher-GHPADSKI.js HL[]) that
gives every tool a present-tense verb while it is in flight and a
past-tense one once it returns. This commit ports that idea to the pill.

Detector
  - New $TOOL_ACTIONS: 18 tools x {running, done, fail}. Messages become
    "Running ..." / "Ran" / "Command failed" instead of "bash : {...}" /
    "bash 完成" / "bash 失败".
  - New $TOOL_FAMILIES: maps a tool to one of 7 families
    (shell/read/write/search/task/web/plan).
  - New Get-ToolKey / Get-ToolVerb / Get-ToolVerbFallback / Get-ToolFamily.
    Unknown tools fall back to "Using <name>" / "Used <name>" so the pill
    never shows a bare identifier with no phase signal.
  - Get-ToolKey caps the normalized name at 64 chars. The name comes from
    the session log, i.e. it is attacker-influenced in a hostile repo, and
    a 4 KB "tool name" must not become a hashtable key.
  - Infer-State now returns a `family` field on every tool branch, and
    Write-Status emits it to status.json.
  - $MSG_FAIL / $MSG_OK are removed. The three tool branches were their only
    consumers, and leaving dead byte-array constants behind would invite a
    future reader to think they are still wired up.

Widget
  - New $familyMap: 7 families -> dot colors.
  - Update-State takes $Family and resolves a $dotHex / $ringHex. The family
    color overrides the state color for thinking/working/waiting only, so
    done stays green and error stays red -- the result signal the user
    relies on must not be repainted by whatever tool happened to run.
  - The status.json poll signature now includes `family`. Without it, two
    consecutive working states from different families would be deduped
    and the dot would keep the previous color.
  - The progress bar follows the same resolved color, so the dot, the
    pulse ring, and the bar always agree.

Validation
  - smoke.mjs: 36 new checks in sections 5d/5e, covering the two tables,
    all four helpers, the three required families, the three state-branch
    call sites, the status.json field, the widget $Family parameter, the
    active-state gate, and the dot/ring paint path.
  - 98 pass, 7 warn, 0 fail locally.

Test evidence
  - 16 negative-injection cases, each a minimal change that breaks one
    clause of the contract. All 16 turn the suite red; the baseline is
    restored to 98/0 after each.
  - Three of the 16 initially did NOT go red. They were false greens and
    the checks were rewritten rather than the failures being waved off:
      1. The active-state gate was matched as a bare
         `$State -in @('thinking','working','waiting')`, but that substring
         also appears on the pulse, elapsed, and progress branches. Ungating
         the family tint left those three in place and the check stayed
         green. It now anchors on the `$Family -and $State` conjunction.
      2. The three state-branch locks were presence tests, but each state
         has TWO call sites (the ledger.jsonl branch and the messages.jsonl
         branch). Breaking only the ledger branch left the messages branch
         matching. They now assert a minimum occurrence count of 2.
      3. The first harness used CRLF anchors against LF-only files, so every
         case reported SETUP-MISS and the suite looked green while nothing
         had actually run. Anchors now use LF and the harness fails loudly
         on a missing anchor instead of counting it as a pass.
  - End-to-end on the live widget: all 6 active family colors render
    (shell green, read blue, write yellow, search purple, web pink, plan
    violet) and done/error keep their green/red result colors.
  - Live status.json after deploy carries the new contract end to end:
    {"state":"working","message":"Running {...}","family":"shell",...}

Design compliance
  - No credentials, no network, no telemetry, no third-party services. The
    verb tables are static data; the only I/O is the pre-existing
    status.json write that the widget already polls.
  - Cross-platform: no host paths introduced. The tables are keyed by tool
    name only, and the existing hardcoded-path scan covers the edited
    files.
  - ASCII-only additions. The existing Chinese message constants are
    untouched, so the PS 5.1 GBK parsing defense is unchanged.
  - Session log is a local file the user already owns; reading tool names
    out of it adds no new capability and no new data source.
@antianqi

Copy link
Copy Markdown
Contributor Author

Round 18 — tool-verb messages + family tinting

ab363d2 on top of b73752b.

The problem

The pill showed the bare tool name for every phase:

working → "bash : {"command":"git status"}"
done    → "bash 完成"
error   → "bash 失败"

working and done were both "bash" — indistinguishable at a glance. The mcode CLI TUI already solved this: launcher-GHPADSKI.js carries a descriptor table (HL[]) giving every tool a present-tense verb while in flight and a past-tense one on return, and its CO() renderer picks between them on status.

This port applies the same idea to the pill.

What the pill shows now

before after
in flight bash : {"command":...} Running git status
returned bash 完成 Ran
failed bash 失败 Command failed
unknown tool mystery_thing Using mystery_thing / Used mystery_thing

Plus family tinting, so the dot, pulse ring, and progress bar all shift color by what kind of work is happening:

family tools color
shell bash, shell green
read read blue
write edit, write yellow
search grep, search, glob, list purple
task task, task_output, task_append, task_query, ask_user cyan
web web_search, web_fetch pink
plan todowrite, request_feature_enable violet

Family color applies to thinking / working / waiting only. done stays green and error stays red — the result signal must not be repainted by whatever tool happened to run.

Screenshots (active families + result states)

round-18 family colors

Implementation notes

  • Two tables, four helpers. $TOOL_ACTIONS (18 tools × 3 phases) and $TOOL_FAMILIES feed Get-ToolKey / Get-ToolVerb / Get-ToolVerbFallback / Get-ToolFamily. An unknown tool still gets a verb, so the pill never shows a bare identifier with no phase signal.
  • Get-ToolKey caps the name at 64 chars. The name comes out of the session log, so it is untrusted in a hostile repo; a 4 KB "tool name" must not become a hashtable key.
  • family joined the poll signature. Without it, two consecutive working states from different families would dedupe and the dot would keep the previous color.
  • $MSG_FAIL / $MSG_OK removed. The three tool branches were their only consumers; leaving dead byte-array constants invites a future reader to assume they are still wired.

Validation

98 pass, 7 warn, 0 fail — 36 new checks in smoke sections 5d/5e.

16 negative-injection cases, each a minimal change that breaks one clause of the contract. All 16 turn the suite red; baseline restores to 98/0 after each.

Three of the 16 initially did not go red. Those were false greens, and the checks were rewritten rather than the failures being waved off:

  1. The active-state gate was matched as a bare $State -in @('thinking','working','waiting'), but that substring also appears on the pulse, elapsed, and progress branches. Ungating the tint left those three in place and the check stayed green. It now anchors on the $Family -and $State conjunction.
  2. The three state-branch locks were presence tests, but each state has two call sites (the ledger.jsonl branch and the messages.jsonl branch). Breaking only one left the other matching. They now assert a minimum occurrence count of 2.
  3. The first harness used CRLF anchors against LF-only files, so every case reported SETUP-MISS and the suite looked green while nothing had actually run. Anchors now use LF, and a missing anchor fails loudly instead of counting as a pass.

Design compliance

  • No credentials, no network, no telemetry, no third-party services — the tables are static data and the only I/O is the pre-existing status.json write.
  • Cross-platform: no host paths introduced; the existing hardcoded-path scan covers both edited files.
  • ASCII-only additions, so the PS 5.1 GBK parsing defense is unchanged.
  • The session log is a local file the user already owns; reading tool names from it adds no new data source.

The round-18 family palette shipped with plan at #8B5CF6 against search's
#A855F7. On the dark pill those two read as the same purple, so `grep` and
`todowrite` were not actually distinguishable -- which is the whole reason
the tinting exists. plan moves to a desaturated slate #94A3B8.

smoke.mjs gains a perceptual-separation check over the family table. It
converts each #AARRGGBB entry to CIELAB and asserts every pair is at least
dE 20 apart (CIE76, the usual "not the same color to a human eye" cutoff
for flat UI fills).

Test evidence
  - Negative injection: putting plan back to #8B5CF6 makes the check fail
    with "search (#A855F7) and plan (#8B5CF6) are only dE 10.1 apart".
    Restoring #94A3B8 returns the suite to 100 pass.
  - The check itself went through two false greens before it was correct,
    both caught by writing the guard before trusting the output:
      1. The color regex required 6 hex digits, but the table stores
         #AARRGGBB, so it matched nothing and the check reported
         "all 0 family colors are perceptually distinct" -- a vacuous pass.
         A parse-count guard now fails outright below 7 entries.
      2. The same check initially measured raw sRGB luminance, which flags
         blue vs purple as "identical" (0.02 apart) because they differ in
         hue but not brightness. That produced five false alarms on a
         palette that is fine in practice. Replaced with CIELAB dE.
  - Live widget: all 7 family colors confirmed distinct, and the earlier
    screenshot run that showed six identical rows was a harness timing
    fault (capturing the previous frame), not a rendering fault -- the
    capture now waits for the widget to log the state before shooting.
  - 100 pass, 7 warn, 0 fail.
@antianqi

Copy link
Copy Markdown
Contributor Author

Follow-up: plan vs search were the same purple

862f5a1. Caught while screenshotting the round-18 palette on the live widget.

plan was #8B5CF6 and search is #A855F7. On the dark pill those read as the same purple, so grep and todowrite were not actually distinguishable -- the exact thing the tinting was added to fix. plan moves to a desaturated slate #94A3B8.

smoke.mjs now asserts every pair of family colors is at least CIE76 dE 20 apart in CIELAB. Negative injection confirms it bites: putting plan back to #8B5CF6 fails with search (#A855F7) and plan (#8B5CF6) are only dE 10.1 apart.

The check itself had two false greens before it was right, both worth recording:

  1. The color regex asked for 6 hex digits, but the table stores #AARRGGBB. It matched nothing and reported "all 0 family colors are perceptually distinct" -- a vacuous pass. There is now a parse-count guard that fails outright below 7 entries.
  2. It first measured raw sRGB luminance, which reports blue vs purple as identical (0.02 apart) because they differ in hue but not brightness. That produced five false alarms on a palette that is fine in practice. Replaced with CIELAB dE.

A third fault was in the screenshot harness, not the code: a fixed 1.1s sleep after pushing status.json sometimes captured the previous frame, which made six of seven rows in the contact sheet look identical. The capture now waits for the widget to log the state it is about to render. Worth noting because it is the same class of bug as a false-green test -- a bad measurement read as a product defect.

All 7 family colors after the fix

round-18 family palette

100 pass, 7 warn, 0 fail.

On a `full access` permission setup the permission-request hook never fires,
so `waiting` was unreachable and ask_user was the only thing that could
actually block the agent. It was reporting `working`:

    working :: ask_user : {"mode":"questionnaire","requiresExplicitResponse":true,...

The pill showed a blue dot with a gear icon -- the universal "leave it
alone, it is making progress" signal -- while the agent sat blocked on a
questionnaire, plus a truncated JSON blob where the useful text should be.
Three occurrences in the 9/24 log confirm this was not a one-off.

ask_user now reports `waiting` with a question count, so the pill reads
"Asking 2 questions" under an amber dot and a "?" -- the "come back, I'm
blocked" signal. It wears the state color rather than a family tint: a
blocked agent must not look like an in-flight delegation.

Detector changes
  - $S_WAITING is now defined. It was referenced nowhere before, so any
    branch using it would have returned $null for the state.
  - Test-IsAskUser and Get-AskQuestionCount helpers, wired into both tool
    paths (ledger.jsonl and messages.jsonl).
  - waiting joins both settle sets. The 60s no-activity fallback would
    otherwise downgrade a pending questionnaire to "已静默 60s" exactly
    when the user has been away longest, and the takeover arbitration
    would let the pre-tool-use hook's `working` shadow it forever.
  - ask_user is removed from $TOOL_FAMILIES.

Bug found while testing
  - Get-AskQuestionCount originally took a parameter named `$args`, which
    is a PowerShell automatic variable. Shadowing it makes every read
    inside the function return the function's own argument list instead of
    the caller's value, so the count was 0 for every input and the message
    degraded to a bare "Asking". Caught by exercising the helper against
    seven argument shapes before wiring it up; the parameter is now
    $toolArgs and smoke locks the name.

Validation
  - smoke.mjs: 7 new checks in section 5d2. 107 pass, 7 warn, 0 fail.
  - 7 negative-injection cases, all turn the suite red; baseline restores
    to 107/0 after each. Two of them target the multi-site and settle-set
    contracts specifically, since those are the ones that fail silently.
  - The real Infer-State from the deployed file was exercised against nine
    real messages.jsonl payload shapes: ask_user with 0/1/2/5 questions,
    a missing questions key, absent arguments, and arguments arriving as
    a JSON string. All six ask_user shapes infer `waiting` with no family;
    bash, read, and todowrite are unchanged.
  - Live widget: `Asking 2 questions` renders amber with a `?`, and the
    post-answer transition to `done` / `Asked` still works.

Test evidence
  - Deliberately did NOT append synthetic toolCall records to a live
    session log. The running agent is writing to that file and injecting
    phantom work into it could make the agent act on it. The end-to-end
    check runs the deployed Infer-State over real payload shapes instead,
    which covers the same code path without touching agent state.

Design compliance
  - No new I/O, no network, no credentials, no telemetry. The change is a
    state mapping plus a question count read from arguments the detector
    was already parsing.
  - Get-AskQuestionCount swallows parse failures and returns 0, so a
    malformed arguments payload degrades to "Asking" rather than breaking
    the poll loop.
  - ASCII-only additions; the PS 5.1 GBK parsing defense is unchanged.
@antianqi

Copy link
Copy Markdown
Contributor Author

ask_user now reports waiting

012628f.

On a full access setup the permission hook never fires, so waiting was unreachable and ask_user was the only thing that could actually block the agent. It was reporting working:

working :: ask_user : {"mode":"questionnaire","requiresExplicitResponse":true,...

Blue dot, gear icon — the universal "leave it alone, it is making progress" signal — while the agent sat blocked on a questionnaire, plus a truncated JSON blob where the useful text should be. Three occurrences in the 9/24 log, so not a one-off.

Now it reads Asking 2 questions under an amber dot and a ?. It wears the state color rather than a family tint: a blocked agent must not look like an in-flight delegation.

Before / after

ask_user waiting

Three things this needed

1. $S_WAITING was never defined. The detector referenced it zero times, so any branch using it would have returned $null for the state. It is defined now.

2. waiting had to join both settle sets. The 60s no-activity fallback would otherwise downgrade a pending questionnaire to "已静默 60s" — exactly when the user has been away longest, which is the opposite of when the signal matters. And the takeover arbitration would let the pre-tool-use hook's working shadow it forever.

3. A bug found while testing. Get-AskQuestionCount originally took a parameter named $args, a PowerShell automatic variable. Shadowing it makes every read inside the function return the function's own argument list instead of the caller's value, so the count was 0 for every input and the message degraded to a bare "Asking". Caught by exercising the helper against seven argument shapes before wiring it up. The parameter is $toolArgs now, and smoke locks the name.

Validation

107 pass, 7 warn, 0 fail — 7 new checks in section 5d2, plus 7 negative-injection cases that all turn the suite red.

The real Infer-State from the deployed file was run against nine real messages.jsonl payload shapes: ask_user with 0/1/2/5 questions, a missing questions key, absent arguments, and arguments arriving as a JSON string. All six ask_user shapes infer waiting with no family; bash, read, and todowrite are unchanged.

One deliberate omission: the end-to-end check does not append synthetic toolCall records to a live session log. The running agent is writing to that file, and injecting phantom work into it could make the agent act on it. The check runs the deployed Infer-State over real payload shapes instead — same code path, no agent-state mutation.

Security. Format-ToolSummary put `tool_input.text` -- whatever the user
typed -- straight into the pill detail string. That string reaches:

  - status.json on disk
  - island.log, append-only
  - the always-on-top pill, visible to anything sharing the screen

So a password, one-time code, or token entered through Computer Use ended
up in a screenshot, a screen share, or a screen recording. The pill is the
worst of the three because it is deliberately on top of everything else.

The text branch now reports the action plus a character count and nothing
else. No substring, no length bucketing that would leak the shape of the
value, no echo. The action name survives, so the pill still answers the
question that matters: is the agent typing right now?

The coordinate branch is untouched -- coordinates are not secret and are
genuinely useful to see.

Validation
  - scripts/test-substep-progress.mjs is rewritten as a behavioral suite
    (see the CI commit), and section 1 plants a canary secret
    SUPER-SECRET-PASSWORD-8f3a91c2 in tool_input.text, runs the real
    Format-ToolSummary, and asserts the canary is absent from the output.
    It also asserts the action survives and the length is reported, so a
    redaction implemented by silently dropping the branch would not pass.
  - End to end against the deployed hook: a PreToolUse event carrying
    P@ssw0rd-CANARY-7d3e91 produced
      status.json: "mcode-computer-use : type <22 chars, redacted>"
      island.log:  "working :: mcode-computer-use : type <22 chars, redacted>"
    and the canary appears in neither.
  - Negative injection: restoring the original `$detail = "$act '$txt'"`
    turns the canary check red.

Design compliance
  - No new I/O or network. The change only removes data from a string.
…e (round-19 #2)

Write-Status rebuilds the whole status.json payload on every write, and the
detector writes on every 5h-usage refresh and every todo-progress refresh.
step / total / detail are agent-pushed sub-step state, not detector state,
and they were simply not in the payload -- so a routine usage refresh
silently zeroed whatever sub-step the agent had reported. On a long task
the pill would drop "step 4/12" back to the bare message and never recover
until the next tool call.

Write-Status now reads the current file and carries step / total / detail
forward. It is a read-modify-write, not an overwrite, and defaults to
-1 / -1 / "" when there is no prior value so a fresh install still renders
normally.

Validation
  - Behavioral: a status.json carrying step=4 total=12 detail="running
    tests" survives a Write-Status rewrite with all three intact; an
    unwritten status.json yields -1 / -1 / "".
  - End to end against the deployed detector: notify-island pushed
    step=4 total=12 detail="running the test suite" (source=agent), then
    15s later the detector wrote its own status and all three fields were
    still there.
  - Negative injection: reverting Write-Status to the blind overwrite
    turns the preservation check red.
…DER (round-19 #3)

The z-order SetWindowPos after a restore used
SWP_NOACTIVATE | SWP_NOZORDER | SWP_NOSIZE with X=Y=cx=cy=0.

Two problems:

  SWP_NOMOVE is missing. X=0/Y=0 is then a real move request, so on any
  monitor whose origin is not 0 (a secondary display to the right, or above)
  the restored window is yanked to the primary monitor's (0,0). Invisible on
  a single primary display, which is why the round-15/16/17 manual testing
  never caught it.

  SWP_NOZORDER is present, which contradicts the call's own purpose. That
  flag makes Windows ignore hWndInsertAfter entirely, so passing HWND_TOP
  alongside it is self-defeating: the z-order change silently does nothing
  and only BringWindowToTop afterwards is doing the work.

Now: SWP_NOACTIVATE | SWP_NOSIZE | SWP_NOMOVE, with HWND_TOP actually
honored.

The round-17 smoke lock asserted the literal `SWP_NOZORDER -bor SWP_NOSIZE`,
i.e. exactly the pair the review asks to change, so it had to be rewritten
rather than updated. It now checks that both required flags are present and
that SWP_NOZORDER is absent, matching on flag names rather than their order.

Validation
  - test-substep-progress.mjs and smoke.mjs both cover this.
  - Negative injection: dropping SWP_NOMOVE turns both red; adding
    SWP_NOZORDER back turns both red.
…(round-19 #4)

post-tool-use.ps1 computed a per-tool detail string and passed it as
-Detail, but left Step at its -1 default. Build-DisplayMessage returns the
bare message when Step <= 0 and only consults Detail inside the Step > 0
branch, so the detail was discarded on every single call. The
"step N/M - detail" contract the widget documents could never fire from
this hook.

Both Push-Island calls now pass -Step 1 -Total 1. PostToolUse is a single
tool result and does not participate in a multi-step sequence, so 1/1 is
the honest value; inventing a larger denominator would risk rendering
"step 1/0" on a fresh install.

Validation
  - Build-DisplayMessage is exercised directly over five argument shapes:
    step+total+detail, step without total, step with empty detail, no step
    (detail must be ignored), and no step and no message.
  - Both Push-Island calls are matched individually and must each carry
    -Step and -Total. A single scan across the file passes as soon as one
    call has them, so removing -Step from just the error branch would
    otherwise stay green.
  - The $step/$local $total assignments are asserted to be positive. They
    are the "unset" sentinel everywhere else in this plugin (-1 means no
    sub-step), and setting them to -1 keeps every flag check green while
    the detail is dropped again at render time. A negative-injection pass
    caught that blind spot.
  - Negative injection: dropping -Step from the error branch, dropping
    -Total from the done branch, setting $step = -1, and inverting
    Build-DisplayMessage's Step<=0 early return each turn the suite red.
MiniMax-AI#5)

scripts/test-substep-progress.mjs existed but no workflow invoked it, so
none of its assertions gated anything. It is also the only suite that runs
the PowerShell under test rather than grepping the source for substrings,
and it is where the secret canary for review item #1 lives -- so leaving
it unwired meant a redaction regression could land without anything
noticing.

A new step 5 runs it after the existing four.

PowerShell resolution: the suite prefers $env:PS_BIN, then an existing path
if PS_BIN looks like one, then probes pwsh / powershell.exe / powershell on
PATH. It never assumes a user-specific absolute path, and it fails loudly
when no PowerShell is found rather than silently skipping. The workflow
step logs which shell was resolved before invoking it.

Two encoding issues had to be solved for the suite to produce trustworthy
results on a runner whose console code page is not UTF-8:

  - the generated .ps1 needs a BOM, or PowerShell 5.1 reads it as ANSI and
    mangles any non-ASCII literal in the harness;
  - the child must set [Console]::OutputEncoding, or a middle dot comes
    back as two GBK bytes and two correct assertions fail as bogus diffs.

Without both, the suite fails on correct code, which is worse than not
running it: it would train everyone to ignore it.
@antianqi

Copy link
Copy Markdown
Contributor Author

All five blocking items addressed

Five commits, one per review item, so each can be verified independently. 5fbda03.

# Commit Fix
1 1daee4b typed text redacted from tool summaries
2 6fe1665 detector preserves step/total/detail
3 975fc86 restore path gets SWP_NOMOVE, drops SWP_NOZORDER
4 682f999 post-tool-use passes -Step/-Total
5 5fbda03 CI runs the behavioral suite

#1 — tool_input.text is no longer echoed. This was the one that mattered most: the string reached status.json, the append-only island.log, and an always-on-top pill, so anything typed through Computer Use landed in a screenshot or a screen recording. It now reports the action plus a character count and nothing else. Verified against the deployed hook with a canary — P@ssw0rd-CANARY-7d3e91 produced mcode-computer-use : type <22 chars, redacted> in both artifacts, with the canary in neither.

#2 — Write-Status is a read-modify-write now. Verified end to end: the agent pushed step=4 total=12 detail="running the test suite", and 15s later after a detector refresh all three were still there.

#3 — two separate bugs on the restore path. SWP_NOMOVE was missing, so a restored window on any monitor with a non-zero origin gets yanked to (0,0). Separately, SWP_NOZORDER was present alongside HWND_TOP, which makes Windows ignore hWndInsertAfter entirely — the z-order call was doing nothing. Both fixed. My round-17 smoke lock asserted the literal SWP_NOZORDER -bor SWP_NOSIZE, i.e. exactly the pair you asked to change, so it was rewritten rather than updated.

#4 — the detail can now render. Build-DisplayMessage only consults Detail inside the Step > 0 branch, so passing -Detail with the default Step = -1 discarded it on every call. Both Push-Island calls now pass -Step 1 -Total 1; PostToolUse is a single result and should not pretend to be a multi-step sequence.

#5 — the suite is wired. It is now the only suite that executes the PowerShell rather than grepping for substrings, which matters because it is where the canary lives.

Two things I got wrong while fixing this, both caught by negative injection

A false green in my own new test. The -Step check used [\s\S]*? across the whole file, so it passed as soon as one Push-Island call carried the flag. Removing -Step from the error branch alone left the suite green. It now matches each call individually.

A blind spot the flag check cannot see. Passing -Step $step is not enough — if $step is later set to -1, the flag check still passes and the detail is dropped again at render time, because -1 is this plugin's "no sub-step" sentinel everywhere. Setting $step = -1 turned every assertion green. There is now an explicit assertion that the local $step/$total are positive.

Validation

  • smoke.mjs: 107 pass, 0 fail
  • test-substep-progress.mjs: 25 pass, 0 fail (rewritten as a behavioral suite)
  • 11 negative-injection cases, each reverting one clause of one fix; all turn at least one suite red, and the baseline restores after each
  • All .ps1 files parse clean

One note on the CI step: on a runner whose console code page is not UTF-8 the suite fails on correct code unless the generated .ps1 gets a BOM and the child sets [Console]::OutputEncoding — a middle dot otherwise comes back as two GBK bytes and two correct assertions fail. Both are handled, because a suite that cries wolf is worse than no suite.

…iniMax-AI#5 fix)

The step I added in 5fbda03 ran `node scripts/test-substep-progress.mjs`.
GitHub Actions executes every `run:` block from $GITHUB_WORKSPACE, not from
the plugin directory, so node looked for
  <repo>/scripts/test-substep-progress.mjs
which does not exist, and the run died with MODULE_NOT_FOUND after 32s.

It passed locally only because my shell happened to already be sitting in
plugins/antianqi/mcode-island. A test that only runs from one directory is
not a test.

The path is now resolved against the repo root and checked with Test-Path
before node runs, so a future rename fails with a clear message naming the
cwd instead of a node stack trace.

Also fixes a bug in the same step: the version banner ran `& $c` after the
loop, where $c is the last candidate rather than the shell that was
actually selected. On a runner without pwsh the banner would have invoked
the wrong binary, or nothing at all.
…unbounded-growth leaks

Audited the plugin against the 0.5.0-0.5.8 changelog and the live runtime
state. Three real defects, each with a regression test that was verified
to fail before the fix landed.

1. The 5h-usage disclosure named a host the code never called.

   Get-5hUsage.ps1 obfuscates the endpoint into a byte array (PS 5.1
   parser-quirk defense). The bytes decode to api.minimaxi.com, which is
   correct and is what the running detector actually calls. But the
   trailing comment on that same line said api.minimax.com, and the
   README named api.minimax.io in three places -- including an absolute
   claim that the token is "never sent to a host other than
   api.minimax.io". That claim was false: the code never contacted that
   host at all.

   For a plugin whose pitch is four independent disclosure sections, an
   overstated isolation guarantee is worse than no guarantee: a reviewer
   checking host consistency would find the code and the docs disagreeing
   and would be right to distrust both.

   The smoke check now DECODES the byte array and compares the docs
   against that decoded truth. It never hard-codes the endpoint, so it
   cannot become a second copy of the disclosure it polices, and it
   fails the moment code and docs drift apart again.

2. island.log and widget.log had no size cap, ever.

   mcode-island.ps1 has documented "keep only the last 1KB" since the
   widget was written. Dbg was a bare Add-Content and never implemented
   it. Measured on this machine: widget.log reached 37.2 MB and
   island.log 18.0 MB over 38 days, with widget.log growing ~3.6 MB/day
   during active use. Extrapolated, that is over a GB a year in
   %APPDATA%, and nothing ever trimmed it.

   Both writers now cap at 1 MB and keep the last 300 lines. The size
   check is counter-gated (fires once per 256 KB written) so the 400 ms
   poll path does not stat the file on every call.

3. Tool input carrying credentials was persisted verbatim.

   Round-19 #1 redacted Computer Use keystrokes because they can be
   passwords. The Bash branch was never redacted, so `curl -H
   "Authorization: Bearer ..."` or `export API_KEY=sk-...` reached all
   three sinks -- status.json, the append-only log, and the always-on-top
   pill -- in the clear, and the log kept it indefinitely.

   There are two independent producers of that text: the hook path
   (_lib.ps1) and the detector, which re-derives messages from mcode's
   session log rather than from a hook event. A helper wired into only
   one is a silent half-fix -- the pill looks clean while the detector
   keeps writing raw text to disk. Both now route through a shared
   scripts/lib/Protect-Text.ps1, and smoke check 9 asserts BOTH
   consumers invoke it rather than merely defining it.

   Scope is deliberately credential-shaped runs, not whole commands: the
   command text on the pill is a documented feature, and blanking every
   command would trade a real capability for a marginal privacy gain.

Also adds a regression guard for hook invocations under an install path
containing spaces. mcode 0.5.4 fixed its own managed updater for exactly
that case; ${PLUGIN_ROOT} is substituted at runtime, so the only way to
break a spaced path is to hand the shell a joined string instead of an
argv array. hooks.json already uses the array form, so this is a guard,
not a fix.

Validation
----------
- smoke: 115 pass / 0 fail (7 pre-existing "forward" event warnings).
  Baseline before this commit was 108 pass / 0 fail; the 9 new failures
  were the new checks, observed failing first.
- Negative-first: each check was confirmed to FAIL before its fix
  landed, and check 9 was re-verified by deleting the detector's call
  site and watching it fail, then restoring.
- Protect-SecretText: 16/16 cases, covering 10 credential shapes
  (bearer header, sk-, ghp-, bare JWT, JSON quoted key incl.
  "x-auth-token", single-quoted JSON, --password flag, key=value, the
  plugin's own set-token.ps1 invocation) and 6 near-miss inputs that must
  NOT be redacted (--mode production, "Select-String token", Windows
  paths, --secret-config, --author=, {"author":"Bob"}).
- Log cap: wrote 3.4 MB of input through the new Log-Line body; final
  file 0.64 MB / 7064 lines. Cap held across repeated truncation.
- PowerShell parser check clean on all five modified/new .ps1 files.
- The 5h call was verified live against the real endpoint, not assumed:
  Get-5hUsage returns a real remainingPct with the configured token.

Not included
------------
The context-meter and token-speed readouts proposed in the same audit are
deliberately NOT in this commit. Structurally enumerating the session
store (excluding the session this audit was run in) shows the assistant
message carries model / usage.{input,output,cacheRead,cacheWrite,
totalTokens,cost} and nothing else: there is no contextWindow and no tps
field. usage.totalTokens is a usable context numerator, but the window
size lives in mcode's provider config, and a token-rate derived from
message timestamps would fold in thinking and tool time -- which 0.5.6
explicitly excludes from the TUI's own figure. Shipping either without
that denominator would put a number on the pill that disagrees with the
TUI. Left for a follow-up that can source the window size properly.
…(round-20 #4)

Found while landing round-20: syncing the committed tree into the install
directory with the documented `Copy-Item -Recurse -Force` flow produced a
detector that could not start at all.

Root cause, verified against Windows PowerShell 5.1 rather than inferred:

  Windows PowerShell 5.1 decodes a BOM-less script using the system ANSI
  codepage. On a zh-CN host that is GBK, so a script containing non-ASCII
  comments is mis-decoded. The C# here-string in mcode-status-detect.ps1
  then stops being a here-string, `using System;` is parsed as PowerShell,
  and the script dies before its first statement with 92 lines of
  "missing using directive". The reported line number (38) is offset from
  the real one (46) because the mis-decode also desynchronizes the
  tokenizer's line count.

  mcode-island.ps1 never showed this because it happens to carry a BOM.
  Nothing else in the plugin did.

The trap is that .gitattributes pins `*.ps1 text eol=lf`, and its comment
claimed "PowerShell 5.1 reads CRLF fine, so LF avoids both failure modes".
That is backwards, and the comment is what let the bug survive. The policy
is part of the cause, not a mitigation.

Control experiment, to be clear that this is pre-existing and not a
round-20 regression: the detector as of ea96d75 (before any of this
round's work) fails under 5.1 with byte-identical output. It simply had
never been synced from a clean checkout on this machine before.

Why a BOM rather than flipping .gitattributes to CRLF:
  Both were tested against 5.1 and both fix the parse -- bare LF does not.
  CRLF would re-break the Linux-side validate.mjs CRLF handling that the
  same comment was written to avoid, so the BOM is the narrower fix: three
  bytes at the head of the file, line-ending policy untouched.

Validation
----------
- Control: ea96d75's detector, run under 5.1, fails identically. Not a
  regression introduced here.
- Isolated: the same file, byte-identical except for a prepended BOM,
  parses cleanly under 5.1 and runs to completion (the only remaining
  error was a TEMP-path artifact of the harness, since $PSScriptRoot had
  no scripts/lib beside it).
- Also confirmed: CRLF without a BOM also parses, so bare LF is the
  trigger, not the encoding per se.
- smoke check 11: written first, observed FAIL on exactly the 8 affected
  files, then PASS after the BOMs landed. 116 pass / 0 fail.
- All 20 non-ASCII .ps1 tokenized with the 5.1 PSParser: 0 parse errors
  (was 8 files failing before).

Files given a BOM (8): mcode-status-detect.ps1, set-token.ps1,
_lib.ps1, post-tool-use.ps1, pre-compact.ps1, session-end.ps1, stop.ps1,
test-windows-workflow-local.ps1.

.g
gitattributes now documents the real constraint, states that bare LF is
part of the problem, and warns that the BOMs must not be "cleaned up" --
an editor that drops them breaks every sync.

Note for reviewers: this changes the first byte range of eight files, so
the diff reads as one changed line each. That is the BOM and nothing else.
…und-20 MiniMax-AI#5)

Live symptom: the pill sat on "step 1/1" showing a curl command from a
tool call that finished several turns earlier, and nothing ever cleared
it.

Cause: round-19 #2 made Write-Status merge the previous step/total/detail
back in on every write, which is correct for the two metadata refreshes
(60s 5h-usage, todo progress) because those restate the current state
rather than announcing a new step. But the detector also writes on state
inference -- it read a new tool call out of the session log and is
asserting "the agent is doing THIS now". That message has no relationship
to whatever sub-step a hook pushed last, so it inherited a counter that
had no meaning there. Every subsequent write inherited it again, which is
why it never cleared.

Write-Status now takes -KeepSubStep:

  - state inference (default) resets step/total/detail to -1/-1/""
  - the 5h-usage and todo refreshes pass -KeepSubStep and preserve

The two paths are distinguished explicitly by the caller, not inferred
from the message text. Distinguishing is necessary because the
state-inference call also has to be able to win the settle race against
an agent push (round-19), so "who wrote last" is not a usable signal.

Tests
-----
New section 5 in test-substep-progress.mjs, negative-first: observed
failing on 3 assertions before the fix, all passing after.

  - Write-Status exposes -KeepSubStep
  - state-inference write clears a stale sub-step (-1|-1|)
  - metadata-refresh write still preserves it
  - the inference call site does NOT pass -KeepSubStep
  - exactly 2 call sites DO (5h-usage, todo)

The call-site assertions are the ones that matter: a switch nobody passes
is the dead-code shape, and a test that only exercises the function body
would not notice.

Two harness repairs, both regressions from round-20 that this work
surfaced:

  - Format-ToolSummary now calls Protect-SecretText, which lives in a
    separate lib. The harness extracted the function body and ran it
    standalone, so it died on an unresolvable command rather than on an
    assertion. The harness now dot-sources the lib, as _lib.ps1 does.
    This is a genuine break of an existing test that round-20 shipped;
    smoke did not catch it because smoke is static and this suite is not.
  - Section 3 anchored its extraction regex on `$prev` appearing before
    `$detailField`. Wrapping the block in `if ($KeepSubStep)` inverted
    that order and the regex silently stopped matching, which would have
    been reported as "Write-Status preserves sub-step fields: could not
    find the block" -- a confusing failure for a function that works.
    It now anchors on the initialisers and the payload construction.

Validation: sub-step suite 32 pass / 0 fail, smoke 116 pass / 0 fail.
Detector re-verified under Windows PowerShell 5.1 (exit 0) after the
restart, since that is the interpreter this runs under in production.
@antianqi

Copy link
Copy Markdown
Contributor Author

Round 20 - five defects found auditing this plugin against the mcode 0.5.0-0.5.8 changelog

Three commits on top of the round-19 head (12b70cb, df3c14d, 4876ef1). All five were found by auditing the plugin against what mcode actually changed, and each is reproduced by a check that was observed failing before the fix.

#1 - the 5h-usage disclosure named a host the code never called

This one is a correctness problem for the disclosure itself, so I want to lead with it.

Get-5hUsage.ps1 obfuscates the endpoint into a byte array as a PS 5.1 parser-quirk defense. Those bytes decode to api.minimaxi.com. That is correct, and it is what the deployed detector actually calls - I verified it live rather than reading it: Get-5hUsage returns a real remainingPct with the configured token.

But the trailing comment on that same line said api.minimax.com, and the README said api.minimax.io in three places. One of them is an absolute claim:

The token is never logged, never written to any other file, and never sent to a host other than api.minimax.io.

That claim is false. The code never contacted that host at all. For a plugin whose proposal rests on four independent disclosure sections, an overstated isolation guarantee is worse than no guarantee - a reviewer checking host consistency finds code and docs disagreeing and is right to distrust both.

The new check decodes the byte array and compares the docs against that decoded truth. It deliberately does not hard-code the endpoint, so it cannot rot into a second copy of the disclosure it is policing, and it fails the moment the two drift apart again.

#2 - both append-only logs had no size cap, ever

mcode-island.ps1 has carried the comment "keep only the last 1KB" since the widget was written. Dbg was a bare Add-Content and never implemented it. The comment documented an intent that no code enforced.

Measured on my machine over 38 days: widget.log 37.2 MB, island.log 18.0 MB, with widget.log growing ~3.6 MB/day during active use. That is over a GB a year sitting in %APPDATA%, and nothing ever trimmed it.

Both writers now cap at 1 MB and keep the last 300 lines. The check is counter-gated (fires once per 256 KB written) so the 400 ms poll path does not stat the file on every call. Verified by pushing 3.4 MB through the new Log-Line body: final file 0.64 MB / 7064 lines, cap held across repeated truncation.

#3 - tool input carrying credentials was persisted verbatim

Round-19 #1 redacted Computer Use keystrokes, correctly, because they can be passwords. The Bash branch was never touched, so a command like curl -H "Authorization: Bearer ..." or export API_KEY=sk-... reached all three sinks - status.json, the append-only log, and the always-on-top pill - in the clear. Because the log is append-only and, per #2, was never capped, that credential stayed on disk indefinitely.

The subtlety worth flagging for review: there are two independent producers of this text. The hook path builds it from the hook event; the detector re-derives it from mcode's session log. A helper wired into only one is a silent half-fix - the pill looks clean while the detector keeps writing raw text to disk. So the helper lives in a shared scripts/lib/Protect-Text.ps1, both consumers call it, and the check asserts both consumers invoke it rather than merely defining it.

Scope is deliberately credential-shaped runs rather than whole commands. The command text on the pill is a documented feature (Bash : npm test); blanking every command would trade a real capability for a marginal privacy gain. What is not a feature is a bearer token sitting in a log for ten years.

16/16 cases, including 6 near-miss inputs that must not be redacted (--mode production, Select-String token, Windows paths, --secret-config, --author=, {"author":"Bob"}) so the filter cannot quietly destroy useful signal. And end to end through the deployed hook: Bearer <redacted> and token=<redacted> in status.json, with neither the JWT signature nor the key prefix anywhere in the file.

#4 - every non-ASCII .ps1 needed a UTF-8 BOM, and .gitattributes was the cause

This one I did not expect to find, and it is the most serious of the five.

I hit it while landing the rest of round-20: syncing the committed tree into the install directory with the documented Copy-Item -Recurse -Force flow produced a detector that could not start at all.

Windows PowerShell 5.1 decodes a BOM-less script using the system ANSI codepage. On a zh-CN host that is GBK, so a script containing non-ASCII comments is mis-decoded, the C# here-string in the detector stops being a here-string, using System; is parsed as PowerShell, and the script dies before its first statement with 92 lines of "missing using directive". The reported line number (38) is offset from the real one (46) because the mis-decode also desynchronizes the tokenizer's line count.

The trap: .gitattributes pins *.ps1 text eol=lf, and its comment claimed "PowerShell 5.1 reads CRLF fine, so LF avoids both failure modes." That is backwards. The policy is part of the cause, not a mitigation, and the comment is what let this survive.

This is pre-existing, not a regression from this branch. Control experiment: the detector as of ea96d75 - the round-19 head, before any of my work - fails under 5.1 with byte-identical output. It had simply never been synced from a clean checkout on a non-UTF-8 host.

Isolated A/B on one file, byte-identical except a prepended BOM: parses cleanly under 5.1 and runs to completion. Both CRLF and a BOM fix it; bare LF does not. I used the BOM rather than flipping .gitattributes to CRLF specifically because that would re-break the Linux-side validate.mjs CRLF handling the same comment was written to avoid. 8 files received a BOM, and .gitattributes now documents the constraint and warns that they must not be "cleaned up" - an editor that drops them breaks every sync.

Reviewer note: this commit reads as one changed line in each of 8 files. That is the BOM and nothing else.

#5 - a detector state write was inheriting a stale sub-step

Live symptom: the pill sat on step 1/1 showing a curl command from a tool call that finished several turns earlier, and nothing ever cleared it.

Round-19 #2 made Write-Status a read-modify-write, which is correct for the two metadata refreshes (60 s 5h-usage, todo progress) - those restate the current state rather than announcing a new step. But the detector also writes on state inference: it read a new tool call out of the session log and is asserting "the agent is doing THIS now". That message has no relationship to whatever sub-step a hook pushed last, so it inherited a counter that had no meaning there - and every subsequent write inherited it again, which is why it never cleared.

Write-Status now takes -KeepSubStep: state inference (default) resets, the two metadata refreshes preserve. The caller distinguishes them explicitly, because the inference path also has to be able to win the settle race against an agent push (round-19), so "who wrote last" is not a usable signal.

The checks that matter here are the call-site assertions. A switch nobody passes is dead code, and a test that only exercises the function body would not notice - so the suite asserts that the inference call site does not pass the flag and that exactly two sites do.

Two harness regressions I introduced and then had to repair

Worth stating plainly rather than quietly fixing, because both were shipped in 12b70cb:

  1. Format-ToolSummary now calls Protect-SecretText, which lives in a separate lib. The suite extracts the function body and runs it standalone, so it died on an unresolvable command rather than on an assertion. The harness now dot-sources the lib, as _lib.ps1 does in production. smoke did not catch this - smoke is static and this suite is not. I reported "all green" on smoke alone before running it.
  2. The round-19 section-3 extraction regex anchored on $prev appearing before $detailField. Wrapping the block in if ($KeepSubStep) inverted that order and the regex stopped matching silently, which would have surfaced as "could not find the preservation block" against a function that works perfectly. It now anchors on the initialisers and the payload construction.

Validation

  • smoke.mjs: 116 pass, 0 fail (7 pre-existing forward event warnings)
  • test-substep-progress.mjs: 32 pass, 0 fail
  • Negative-injection: every new check observed failing before its fix. Check 9 additionally verified by deleting the detector's call site and confirming the suite turns red, then restoring.
  • PowerShell 5.1 tokenizer over all 20 non-ASCII .ps1: 0 parse errors (8 failed before the BOM).
  • Stale sub-step reset path verified live. The preserve path is not live-verified: observing it requires a tool call, and that call is itself a state inference which resets the sub-step. It is covered by the behavioral suite (real extracted function, real status.json fixtures, both modes) plus the static call-site assertions. I would rather flag that gap than present suite coverage as runtime evidence.

All five CI steps run locally before pushing

Step Result
1 parse all .ps1 30/30
2 token set/show/clear (isolated APPDATA) 8/8
3 hook stdin/stdout under PS 5.1 5/5
4 Get-5hUsage via lib + mock listener 5/5
5 behavioral suite 32/0

Steps 2 and 3 were the ones at real risk from the BOM and the new dot-source; both run green.

Not in this PR, on purpose

The context-meter and token-speed readouts proposed during the same audit are excluded. Structurally enumerating the session store shows the assistant message carries model and usage.{input,output,cacheRead,cacheWrite,totalTokens,cost} and nothing else - there is no contextWindow and no tps field. usage.totalTokens is a usable context numerator, but the window size lives in mcode's provider config, and a rate derived from message timestamps would fold in thinking and tool time, which mcode 0.5.6 explicitly excludes from the TUI's own figure. Shipping either without that denominator would put a number on the pill that disagrees with the TUI. I would rather leave the pill without the readout than show a figure that is quietly wrong.

…iMax-AI#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' })   # -> ''
@antianqi

Copy link
Copy Markdown
Contributor Author

Round 20 #6 - the pill no longer renders raw tool JSON

Reported as: the status line always trails some code.

The running-state message was built as

"$verb " + (ConvertTo-Json $args -Compress)   truncated to 60 chars

so the pill showed:

Running {"command":"$ErrorActionPreference=\u0027Continue\u0027\n...
Reading task {"task_id":"bg_0f858500-...","wai...

A JSON key, escaped quotes, 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 clean - they render the verb alone - so this was confined to the in-flight state.

Before / after, same command:

message
before Running {"command":"$ErrorActionPreference=\u0027Continue\u0027\n...
after Running $ErrorActionPreference='Continue'

skill turned out to be the worst offender: it is not in $TOOL_ACTIONS or $TOOL_FAMILIES, so it always took the JSON path. One of the most frequent tools on screen was also the least readable, which is what made this worth fixing rather than tuning.

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 first.

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.

This also fixes multi-line commands. A PowerShell command is usually several lines, so the old collapse-then-truncate showed 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, 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.

One trap, because it produced a genuinely 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. Verified directly:

function T($n, $args) { $args.command }
T bash ([PSCustomObject]@{ command = 'npm test' })   # -> ''

The symptom is an empty summary for every tool, which reads as "no data" rather than "bad parameter binding". It cost me one debug cycle and would have read as a data problem to anyone else.

Validation

  • test-substep-progress.mjs: 47 pass / 0 fail (was 32)
  • smoke.mjs: 116 pass / 0 fail
  • All 30 .ps1 parse; detector still initialises cleanly under Windows PowerShell 5.1 (exit 0) - the interpreter it runs under in production and the one that breaks on a malformed file
  • Verified live against the running detector: the pill text is now the first line of the command, with no JSON key and no mid-token truncation

One thing I checked and did not change

status.json renders apostrophes as \u0027 in the file. That is ConvertTo-Json escaping at serialisation time, not data the detector produced - the widget's ConvertFrom-Json gets the real character back. I was about to add an unescape pass, which would have corrupted the value, since the escape never exists in memory. Recorded here so the next person does not repeat it.

Clicking a session switched the tab but nothing appeared: the window
stayed behind whatever the user was looking at.

The cause is the foreground lock. SetForegroundWindow is refused for a
process that is not already in the foreground, and the widget is one.
The tab had switched -- the title bar said so -- but the window never
came forward, so from the user's side the click did nothing.

The widget already had this solved for the pill's show/hide toggle: set
the window TOPMOST, then NOTOPMOST again, which lets BringWindowToTop
push it all the way up, and only then SetForegroundWindow. The new code
calls that same routine instead of calling SetForegroundWindow on its
own.

The log now says which process is in the foreground afterwards, so a
switch that did not take is visible instead of silent.

Validation

  - Live, on this machine: the foreground window before the click was
    the desktop app (pid 2496); after the click it was
    WindowsTerminal.exe (pid 59084). The window came forward.
  - The behavioural suite: 133 pass, 0 fail, 1 skipped (platform).

Test evidence

  - node scripts/test-substep-progress.mjs -> 133 pass, 0 fail, 1 skipped

Design compliance

  - The pill's own focus path is untouched; this reuses it.
  - No new dependency, no network, no credentials.
  - UTF-8 with a BOM, LF line endings: the file is dot-sourced or run by
    Windows PowerShell 5.1 on a zh-CN host.
Clicking a row in the session list switched the terminal tab but never
brought the terminal window to the front. From the user's side nothing
happened.

Root cause: `$pid = [uint32]0` in Focus-TerminalWindow and in
Get-ForegroundPid. PowerShell variable names are case-insensitive, so
`$pid` IS the read-only automatic `$PID`; the assignment throws
"Cannot overwrite variable PID because it is read-only or constant".
Both call sites sit inside a `try { } catch { }`, so the exception was
swallowed: Focus-TerminalWindow returned $false before it ever called
Focus-Window, and Get-ForegroundPid always returned 0. The caller
ignored the return value and logged "session terminal switched:
... foregroundPid=0" anyway, so the log looked like a success.

The same class of bug already bit `$home` in Runtime-Titles and `$args`
in the detector. It is worth a guard, not just a rename.

Fix:
  * rename the variable to $procId in both functions
  * read the foreground window back after every focus attempt
    (Test-ForegroundPid) and log FOCUS CONFIRMED / FOCUS NOT
    CONFIRMED. SetForegroundWindow fails silently, so a log line can
    never be the evidence that the window came forward
  * Focus-TerminalWindow falls back to the z-order recipe the pill's
    own toggle already uses (SetWindowPos HWND_TOP without
    SWP_NOACTIVATE, which activates the window)
  * a failed focus must never fall back to opening a second terminal:
    Select-SessionTerminal and Activate-TerminalWindow now report the
    terminal window as found even when the foreground could not be
    confirmed, so one session never ends up with two terminals again
  * Focus-Window's log line says DONE instead of OK -- it only means
    the calls did not throw

Test evidence:
  * node scripts/smoke.mjs -> 153 pass / 0 fail
  * node scripts/test-substep-progress.mjs -> 133 pass / 0 fail / 1 skipped
  * pwsh -File scripts/test-session-tracker.ps1 -> 65 pass / 0 fail
  * pwsh -File scripts/test-windows-workflow-local.ps1 -> all 5 steps OK
  * new guard in smoke.mjs fails the build on any assignment to a
    read-only PowerShell automatic variable ($pid, $home, $host, ...),
    including inside a param() block. Negative injections N23-N27 make
    it fail, N28-N31 (comment / string / here-string / the `$null =`
    discard) must not fire, and the tree restores byte-identical.
  * real-machine end-to-end with a real mouse_event LEFTDOWN/LEFTUP on
    the session row: with this build the foreground window becomes
    WindowsTerminal.exe and the selected tab is the clicked session.
    With the previous build (the parent commit) the foreground stays on
    the app (pid 2496) and the same check fails. The log now shows
    "FOCUS CONFIRMED: pid=59084" followed by
    "session terminal switched: ... foregroundPid=59084", where it used
    to say foregroundPid=0.

Design compliance:
  * one commit, one thing
  * reuses the existing Focus-Window / Toggle-Window recipe instead of
    introducing a third way to steal the foreground
  * a session click never opens a new terminal window
Clicking a session row brought the terminal forward, but as a ~480x84
strip in the top-left corner of the screen instead of a full-screen
window. The user reported it with a screenshot: "点完它是在左上角小小
的一条,而不是全屏".

Root cause: Focus-Window's two SetWindowPos calls pass cx=0/cy=0 and
X=0/Y=0 as a shorthand for "keep the current size / position", but
that shorthand only holds when the matching flag is present. The flags
were SWP_NOACTIVATE alone:

    SetWindowPos(hwnd, HWND_TOPMOST,   0, 0, 0, 0, SWP_NOACTIVATE)
    SetWindowPos(hwnd, HWND_NOTOPMOST, 0, 0, 0, 0, SWP_NOACTIVATE)

Windows therefore read them as "resize to 0x0" and "move to (0,0)".
Windows Terminal clamps a 0x0 request to its minimum size, which is
the 480x84 strip, and the window really moved to the top-left corner.
The window's restored ("normal") placement then stayed 480x84@(0,0),
so every later restore brought the strip back.

The bug was latent until the previous commit made Focus-TerminalWindow
actually reach Focus-Window -- before that, the $PID collision threw
first and the whole focus step never ran. This file already documents
the trap in Toggle-Window ("SWP_NOSIZE: cx=0/cy=0 缺它会被 Windows 当成
resize 到 0x0, 触发 WT 的 min-size 兜底, 变成 480x76 strip"), it just
was never applied to Focus-Window.

Fix:
  * Focus-Window's SetWindowPos calls now carry SWP_NOSIZE | SWP_NOMOVE
    (plus SWP_NOACTIVATE), so they only touch the z-order
  * Focus-Window no longer calls SW_SHOW on a window that is already
    visible -- pointless state churn on an already-maximized window
  * "show it full-screen" now lives in one helper, Show-WindowFullScreen,
    used by both the pill's toggle and the session-list click, so the
    two paths cannot drift apart again
  * the helper runs SW_RESTORE -> SetWindowPos(work area) -> SW_MAXIMIZE,
    in that order. The SetWindowPos has to happen while the window is
    in its normal state: on an already-maximized window Windows does not
    update the restored size, so a stale size (the 480x84 strip) would
    survive the next restore. SetWindowPlacement was tried and does not
    work on Windows Terminal -- the rcNormalPosition it is given reads
    back as 0x0.

Test evidence:
  * node scripts/smoke.mjs -> 154 pass / 0 fail
  * node scripts/test-substep-progress.mjs -> 133 pass / 0 fail / 1 skipped
  * pwsh -File scripts/test-session-tracker.ps1 -> 65 pass / 0 fail
  * pwsh -File scripts/test-windows-workflow-local.ps1 -> all 5 steps OK
  * new guard in smoke.mjs fails the build on any SetWindowPos that
    passes cx=0/cy=0 without SWP_NOSIZE, or X=0/Y=0 without SWP_NOMOVE.
    Negative injections N32-N34 fire it; N35/N38 (an explicit size with
    SWP_NOMOVE, work-area coordinates as variables) must not; the tree
    restores byte-identical.
  * the round-15/16 drift locks moved with the implementation: they now
    check Show-WindowFullScreen and both of its callers instead of the
    text inside Toggle-Window. Negative injection N36 (SW_MAXIMIZE
    regressed to SW_SHOW) and N37 (the session click stops
    full-screening) fire them.
  * real-machine end-to-end, real mouse_event LEFTDOWN/LEFTUP on the
    session row: with this build the foreground window is Windows
    Terminal, its rect is 2576x1408 (covers the 2560x1392 work area)
    and its restored placement is 2560x1392 -- a real full-screen size.
    With the previous build the foreground stays on the app and the
    window is a strip; the same check fails.

Design compliance:
  * one commit, one thing
  * reuses the existing Toggle-Window full-screen recipe instead of
    introducing a fourth way to raise a window
  * a session click never opens a new terminal window
The session list itself landed in 8cc8df5..71436c8 (tracking every live
session, the real session name, the list layout, the shared state
vocabulary) and the click behaviour in a644ab4, with the focus and
full-screen fixes in f9c98da and 0c22cc7. This commit packages the
feature for release: the plugin version, the description in plugin.json,
and the README section that documents what the list shows and what a
click does.

Why this is one commit rather than part of each feature commit: the
version number, the marketplace description and the README are release
artifacts, and they only have a single correct value once the feature
is done. Splitting them across the feature commits would have meant
claiming "0.5.0" in the first of them.

What changed:

  * plugin.json: version 0.4.0 -> 0.5.0, and the description now says
    what the session list does -- every live session on hover, the real
    session name from the runtime's SQLite store, the last message, the
    relative time, and a click that switches back to that session's
    terminal tab without ever opening a second terminal.
  * README.md: a new "The session list" section covering the row
    layout, where the title comes from (and the fallback), why the state
    words are the pill's words, what the extra 已停止 state means, and
    the click behaviour including the "never open a second terminal"
    rule.
  * smoke.mjs: the version check moves with the version.

No behaviour change in this commit -- the widget and the detector are
byte-identical to 0c22cc7.

Test evidence:
  * node scripts/smoke.mjs -> 154 pass / 0 fail
  * node scripts/test-substep-progress.mjs -> 133 pass / 0 fail / 1 skipped
  * pwsh -File scripts/test-session-tracker.ps1 -> 65 pass / 0 fail
  * pwsh -File scripts/test-windows-workflow-local.ps1 -> all 5 steps OK
@antianqi

Copy link
Copy Markdown
Contributor Author

Round 23: the session list

This PR now carries the session list on top of the sub-step progress
fields: 17 commits on the previous head (d3115f3), 55 in total.

The feature

Resting the mouse on the pill expands a list of every session that is
currently live
, not just the one the detector follows. The detector
still follows a single session -- the one that wrote its log most
recently -- but on a machine with a CLI session, a desktop-app session
and a couple of side agents, the interesting one is usually the other
one.

   ●  剪映插件开发                          3 分钟前
      执行 bash                              运行中

Each row carries four things:

part where it comes from
status dot the same state vocabulary the pill uses, same text and same color
title the session's real name, from the runtime's own SQLite store (local_runtime_sessions.title)
preview the last thing that session said, so "running" and "stuck on one tool" are distinguishable
timestamp how long ago that session last moved, plus its state word

The title is the session's name, not a summary -- that is the whole
point of reading runtime-state.sqlite instead of re-deriving a name
from the message log. When that store is not reachable (a host without
the usual node paths), the widget falls back to the first thing the
user said in that session, so a row is never blank.

The list reuses the pill's vocabulary instead of inventing its own:
working / thinking / waiting / idle / done / error, rendered
with the same words and the same colors as on the pill. The one
addition is 已停止 ("stopped"), which is not a message type but a
liveness fallback -- a session that has not moved for five minutes is
no longer "running", even though nothing said so.

Clicking a row goes back to that session. It never opens a second
one.
The widget enumerates the Windows Terminal tabs, matches the
session name against the title history that session has had, selects
that tab, and brings the terminal window to the front. When no tab
matches (the terminal was renamed, or the session runs inside the
desktop app rather than Windows Terminal), the widget still raises the
terminal window instead of starting a new mcode --session <id> -- a
second terminal for the same session is exactly the bug this feature
is meant to remove. Clicking a row also dismisses it: the session
leaves the list and stays out until it starts its next round.

The detector writes sessions.json next to status.json in
%APPDATA%\mcode-island\; that file is the contract between the
detector and the widget, and the widget only reads it.

The commits

commit what
8cc8df5 the detector tracks every live session, not just the newest
37652f0 the list opens when the mouse rests on the pill
7ab8726 merge feat/substep-progress into the feature branch
74641fb the row layout: two lines of text plus a timestamp on the right
71436c8 the title is the session's real name, from the runtime's SQLite store
c2f6ff5 the title lookup survives a host without the usual node paths
b225995 a session that stopped no longer says it is running
ee06a14 the list uses the pill's state vocabulary, same text and same color
83c3856 a finished tool is not a finished round
a644ab4 clicking a session goes back to its terminal instead of opening a second one
7b40377 the title no longer degrades into a message summary
e73aa8f injected system messages are not something anyone said
2a45bbc the last row of the list is not cut in half
633ceae the terminal actually comes to the front
f9c98da a session click no longer loses the foreground
0c22cc7 the terminal comes back full-screen, not as a strip in the corner
ff25a4b release 0.5.0: version, description, README

A bug this PR already had

Focus-Window passed cx=0/cy=0 and X=0/Y=0 to SetWindowPos as a
shorthand for "keep the current size and position" -- but that
shorthand only holds when the matching flag is present:

[WinAPI]::SetWindowPos($Target.Hwnd, [WinAPI]::HWND_TOPMOST, 0, 0, 0, 0, [WinAPI]::SWP_NOACTIVATE) | Out-Null

Windows read that as "resize to 0x0" and "move to (0,0)". Windows
Terminal clamps a 0x0 request to its minimum size, so the window
collapsed into a ~480x84 strip in the top-left corner of the screen --
and its restored size stayed that broken value, so every later
restore brought the strip back.

Round 19 item #3 already asked for SWP_NOMOVE on the restore path;
the same call site in Focus-Window was missed. Mode = 'focus' covers
the pinned window, the desktop app, and the "no caller terminal"
fallback -- and the Marketplace build flips $script:clickPrefers to
'app', so that is the path every Marketplace user takes.

Fixed in 0c22cc7. smoke.mjs now fails the build on any
SetWindowPos that passes cx=0/cy=0 without SWP_NOSIZE, or
X=0/Y=0 without SWP_NOMOVE -- negative-injected to confirm it
bites.

One more of the same family

The new focus code assigned to $pid, which in PowerShell is the
read-only automatic $PID (variable names are case-insensitive). The
assignment threw, a catch around it swallowed the error, and the
focus step silently never ran. Fixed in f9c98da, and smoke.mjs now
scans every .ps1 in the plugin for assignments to a read-only
automatic variable ($pid, $home, $host, ...), including inside a
param() block. This is the third time this family of bug has cost a
round here ($home in the title lookup, $args in the detector), so
the guard is generic rather than a one-off.

Test evidence

  • node scripts/smoke.mjs -> 154 pass / 0 fail
  • node scripts/test-substep-progress.mjs -> 133 pass / 0 fail / 1 skipped
  • pwsh -File scripts/test-session-tracker.ps1 -> 65 pass / 0 fail
  • pwsh -File scripts/test-windows-workflow-local.ps1 -> all 5 steps OK

Both new guards were negative-injected: changing one line makes the
corresponding assertion fail, and restoring it makes the suite green
again.

The window behaviour was verified on the real machine, not just in the
suite. A real mouse event on a session row, checked from an
independent process:

build foreground window terminal rect verdict
before 0c22cc7 Windows Terminal (0,0)-(480,84) -- a strip in the corner fail
0c22cc7 Windows Terminal (-8,-8)-(2568,1400) = 2576x1408 pass

The same check also asserts the window's restored placement is a real
full-screen size, so the broken 480x84 cannot survive the next
restore.

@antianqi

Copy link
Copy Markdown
Contributor Author

Review response: all four blockers, and where they landed

This is a response to the two CHANGES_REQUESTED reviews
(hetaoBackend, 09-25 @ b73752b and 10-09 @ a6a17f3). It maps each
blocker to the commit that addresses it and to the check that proves it,
so a re-review does not have to re-read the tree.

Current head is ff25a4b.

# blocker addressed in proved by
09-25 #1 typed Computer Use text can leak into the pill, status.json and the append-only log 1daee4b scripts/test-substep-progress.mjs secret canary
09-25 #2 the detector's rewrite drops step / total / detail 6fe1665 the same suite + mcode-status-detect.ps1:788-790
10-09 #1 the metadata refresh is not an atomic read-modify-write 4ae074f scripts/test-substep-progress.mjs:785 (two writers, one file)
10-09 #2 PostToolUse stringifies the result and only recognizes textual error prefixes 3df799d post-tool-use.ps1:15 + the hook subprocess tests

09-25 #1 -- typed text

tool_input.text is whatever the user typed, so it never reaches a
persistent or displayed summary. io.minimax.mcode/hooks/scripts/_lib.ps1
carries the rule and the redaction (Protect-SecretText $detail), and
the suite drives a real Computer Use type action whose payload is a
canary -- SUPER-SECRET-PASSWORD-8f3a91c2 -- then asserts the canary
appears nowhere in the pill output, in status.json, or in the log, and
that what is written instead is a redacted placeholder of the same
length.

09-25 #2 -- the detector erasing the sub-step fields

The detector now carries the fields through its rewrite instead of
rebuilding the object without them
(mcode-status-detect.ps1:788-790 reading $prev.step / $prev.total
/ $prev.detail, and :802-804 writing them back), so a usage or todo
refresh no longer erases "step 3/12".

10-09 #1 -- the metadata refresh

Three changes, together:

  • One snapshot. Write-StatusUnderLock reads the previous state
    once ("The one snapshot. Every field below is derived from it, so the
    payload is always a single coherent generation") and derives every
    field from it.
  • A lock. scripts/lib/Status-Store.ps1 takes an exclusive OS file
    lock (Enter-StatusLock / Exit-StatusLock). The lock is not
    re-entrant, so the body is split into Write-StatusUnderLock for
    callers that already hold it -- a nested take would deadlock against
    itself.
  • A version. Every write stamps a monotonic version, and the
    file is replaced through a staging file, so two writers can never
    mint the same version and a torn file is not observable.

The regression is scripts/test-substep-progress.mjs:785 -- "Two
writers, one file (round-21 review hetaoBackend #1)" -- which runs two
real writers against one status file and asserts the result is a single
coherent generation.

10-09 #2 -- structured isError

io.minimax.mcode/hooks/scripts/post-tool-use.ps1:15 calls
Test-ToolResultError $evt.tool_result, which prefers the structured
isError boolean on the result object. A textual error prefix is only
the fallback for a bare string result, so {isError:true,...} can no
longer be reported as done. The tests drive the hook as a real
subprocess with object, string and null success and failure payloads.

Also on this head since the last review

  • 807aaf5 -- the detector survives a terminating error in the main
    loop instead of taking the pill down with it.
  • d3115f3 -- the 5h usage badge stays on the pill
    (Get-PlanUsage was clearing the cached percentage on its way in).
  • ff25a4b and the 16 commits below it -- the session list, which is
    a new user-facing feature rather than a response to a blocker. It is
    described in the round-23 comment above, and it is what moved the
    plugin to 0.5.0.

State of the checks

  • node scripts/smoke.mjs -> 154 pass / 0 fail
  • node scripts/test-substep-progress.mjs -> 133 pass / 0 fail / 1 skipped
  • pwsh -File scripts/test-session-tracker.ps1 -> 65 pass / 0 fail
  • pwsh -File scripts/test-windows-workflow-local.ps1 -> all 5 steps OK

The two blockers that asked for a regression test are the reason the
workflow runs the behavioral suite at all now: 807aaf5 made the CI
step execute it, because a suite that only ever runs on the runner
stops being evidence for the person running it.

If something above is still not what you asked for, pointing at the
specific line is the fastest way to get it fixed -- the work is
already split into one commit per item for exactly that.

…ktop app

The session list is only worth having if a click takes you somewhere. On the
desktop app the session lives in the app's sidebar, and the row click had
exactly one way to go: fall through to "open a new terminal", which is the
one thing the list promised never to do -- it spawns `mcode --session <id>`
and leaves a second terminal running the same session, next to the app that
already had it.

So the click now has somewhere better to go. The app's sidebar renders every
conversation as a Button whose accessibility name is the conversation title,
so UI Automation can invoke it -- the same technique the list already uses
for Windows Terminal tabs (there with SelectionItemPattern.Select, here with
InvokePattern.Invoke). The app registers a `minimax-cn://` deep link, but it
only routes one action, `navigate` (a whole-page URL), and its own 275
routes are all `/minimax-desktop/api/v1/...` APIs -- there is no URL that
identifies a conversation, so there is no supported entry point and the
sidebar has to be clicked.

The order is now:

  CLI profile     terminal tab -> terminal window -> open a new terminal
  desktop profile terminal tab -> app conversation -> app window

The pin and the terminal tab keep their priority: a session that really is
open in a terminal tab is switched there, exactly as before. The desktop
profile simply gets the app conversation before it would ever open a second
terminal.

The lookup filters by name (47ms measured) instead of walking the app's
accessibility tree (540ms for 2180 elements), because the click handler runs
on the UI thread and a half-second scan would freeze the pill. Title
matching has the same "two conversations with the same title" caveat the
terminal path always had; when no row matches, the click falls through to
the profile's fallback, and the log says which path it took.

Validation
-----------
* Live, end to end: with the real widget running, hovering the pill expanded
  the list, clicking the row for "了解 mcode 的 miniapp" (a session that has
  no terminal tab) logged `APP: desktop app MiniMax Inside Code`,
  `app conversation switched: 了解 mcode 的 miniapp`, and the app's window
  came to the foreground.
* The same run also showed the terminal path still winning when the session
  really is in a terminal tab (`session terminal switched`, foreground
  WindowsTerminal).
* Behavioral test: the UIA lookup and the app lookup are stubbed, so the
  contract is checked without the app running -- a matching row is invoked, a
  session that is not in the sidebar falls through, and no app running falls
  through.
* Drift locks: the session click must try the app conversation, the desktop
  branch must return before the new-terminal fallback, and the lookup must
  filter by name. All three were verified by negative injection (each
  mutation trips its lock).

Test evidence
-------------
* node scripts/smoke.mjs -> 158 pass, 7 warn, 0 fail (was 154).
* node scripts/test-substep-progress.mjs -> 138 pass, 0 fail, 1 skipped.
* pwsh -File scripts/test-windows-workflow-local.ps1 -> all 5 steps OK.
* Windows PowerShell 5.1 parser: 0 errors on mcode-island.ps1.

Design compliance
-----------------
* One implementation, two profiles: the difference is the existing
  $script:clickPrefers switch, so the Marketplace builder needs no new rule
  for this.
* No credential, no network request, no new file. UI Automation talks to the
  local app window, exactly like the terminal path already does.
The session list (v0.5.0) brought a 500-line suite of its own, and it was
in no workflow and no local run: the feature's core tests only ran when
somebody remembered to run them by hand. A suite that is not wired into CI
is a suite that rots.

The workflow and the local runner now run it as step 6.

Also ignores *.bak-r16, the stale widget backups that debugging sessions
leave behind.
The Marketplace review rejected 0.9.0 with five points about the session
list. All five are addressed here, in the plugin source, so both builds get
them.

1. The detector read every historical session log every second and threw
   the result away. The expiry check now comes BEFORE the read: a session
   that has not moved is never opened. On this machine that is 400+ logs a
   second, every second, read for nothing.

2. The runtime's SQLite is no longer read, and the MiniMax Code programme
   is no longer borrowed to run a script. The session title is the first
   thing the user said in that session -- the only source left, and the one
   the app itself shows. `scripts/lib/Runtime-Titles.ps1` is gone, and with
   it the ELECTRON_RUN_AS_NODE trick and the node lookup that needed it.

3. The conversation click is scoped to the sidebar's conversation list. It
   used to look for a button with that name anywhere in the window, which
   is not a conversation click: the same name can be a heading or a button
   in another panel. The list is now found first -- the one element that
   holds SEVERAL known session titles -- and the target is looked up only
   inside it. The search still filters by name (47ms measured) instead of
   walking the whole accessibility tree (540ms), because the click handler
   runs on the UI thread.

4. Titles and previews are things a user said, so they can contain a key.
   Both are truncated (40 and 120 characters) and passed through the same
   secret redactor the plugin uses for log output BEFORE they are written
   to sessions.json and before they are drawn on screen. The README now
   states exactly which data is read: the first user message, the last
   message, and the log's mtime. Nothing else -- not the runtime's
   database, not the MiniMax Code programme.

5. The list is cleaned up. It is "the sessions that are alive now", not a
   history: a session that has not moved for 30 minutes is dropped from
   the list and from sessions.json, and comes back when it moves again.
   Without it, a morning of twenty conversations left twenty rows on the
   pill and twenty conversations' worth of text in the state file.

Validation
-----------
* The tracker's own suite: 70 pass / 0 fail (was 65). The new checks cover
  the expiry-before-read order (the read is counted, and an expired
  session's log must not appear in it), the redaction of the title and of
  the preview, the redaction of what lands in sessions.json, the
  truncation, and the prune (the session leaves the list AND the file).
* The behavioural suite: 139 pass / 0 fail. The conversation switch is
  still stubbed at the UIA boundary, and it now also asserts that the
  lookup is handed the known titles -- that is what scopes it to the
  conversation list.
* node scripts/smoke.mjs -> 160 pass, 7 warn, 0 fail. The new locks: the
  conversation list is resolved first, the button lookup goes through it,
  and the lookup does not start from the window root.
* Negative injection: four mutations (the expiry check moved back after
  the read, the preview not redacted, the prune removed, the button lookup
  back to the window root) each trip at least one lock.
* Live, against the running desktop app: the scoped lookup found the
  conversation list at depth 1 (the sidebar's list, not the window),
  located the row inside it, and invoked it -- "app conversation switched".
  The terminal path still wins when the session really has a terminal tab.

Design compliance
-----------------
* The session list keeps working without the runtime's database: the title
  falls back to the first user message, which is what the app shows in its
  own sidebar.
* No credential, no outbound request, no file outside
  %APPDATA%\mcode-island.
* One implementation, two profiles: the CLI / desktop difference is still
  the single $script:clickPrefers switch.
The Marketplace build and the audit reject the package if it contains the
name of the environment variable that turns the MiniMax Code programme into
node -- and the tracker test mentioned it in a comment, so the package build
failed on the test's own wording. The check now describes the thing without
spelling the variable, and the assertion still matches the usage.
…mment

The Marketplace build and the audit reject the package if it contains the
name of the environment variable that turns the MiniMax Code programme into
node. The tracker's comment still spelled it, so the package build failed
on the source's own wording. The comment now describes the thing without
spelling the variable.

@hetaoBackend hetaoBackend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for exact current head ff25a4b.

Blocking findings:

  1. The PR deletes the complete existing plugins/weekbin/mcode-trajectory-studio and its workflow relative to base 39ddd2c: 161 files and about 45168 lines. This is an unrelated product deletion and must be reverted or explicitly justified and approved.

  2. Command injection in plugins/antianqi/mcode-island/mcode-island.ps1:933-943: Start-SessionTerminal interpolates the session id into a PowerShell -Command string. The id is decoded from session directory names in scripts/lib/Session-Tracker.ps1:81-90 without a strict allowlist, and restored sessions.json ids are trusted. A crafted id such as x; Set-Content $env:TEMP\pr62-pwned 1; can execute arbitrary PowerShell when a row is clicked. Validate the id and pass it as a non-evaluated argument; add a regression test.

  3. Get-SessionTitle and Get-SessionPreview in scripts/lib/Session-Tracker.ps1:140-193 and 303-323 write user or assistant text to sessions.json and render it in the always-on-top pill without Protect-SecretText. Tokens, passwords, or API keys in prompts can therefore be persisted and displayed. Redact before persistence and add JSON, Bearer, JWT, and key-value canaries.

  4. The tracker has no maximum session count, preview size, sessions.json size, or UI height. Session-Tracker.ps1:400-579 retains stopped sessions and serializes all full previews, while mcode-island.ps1:617-633 creates a row for every entry. Add bounded data and eviction tests.

  5. If title matching cannot find an existing terminal, mcode-island.ps1:929-943 starts another mcode --session terminal. This contradicts the no-second-terminal contract and can duplicate a live session. Fail closed or prove the session is not already running before launching a new terminal.

The current repository-owned CI, CodeQL, Windows job, and validate checks are green, but they do not cover these blockers.

Review round 3 on ff25a4b raised five blockers. This commit closes four of
them; the fifth (the deleted weekbin plugin) was a merge artefact and is
restored by the upstream merge in f246247.

1. Command injection in the session-click path.

   Start-SessionTerminal built `powershell -Command "mcode --session <id>"`
   from a value that comes from a session directory's base64 and from
   sessions.json -- neither is trusted. An id shaped like `x; Set-Content
   ...` was executable PowerShell.

   The function is deleted rather than quoted more carefully: the point is
   to remove the path that turns data into code, not to sanitise it. On top
   of that, both entry points now check the id's shape
   (^[A-Za-z0-9][A-Za-z0-9_-]{0,127}$): Get-SessionIdFromDir drops a
   directory whose decoded name is not an id, Restore-SessionTracker drops
   such an entry from sessions.json, and Invoke-SessionClick refuses it.

2. "No match" no longer opens a terminal (fail closed).

   The click handler logs why and stops. A session that runs in a host we
   cannot see (embedded terminal, another terminal app) would otherwise get
   a second terminal -- the exact situation the list exists to prevent.

3. Bounds on the list.

   Data side: MAX_SESSIONS = 20, evicting by last activity. Only idle
   sessions (log untouched for the 60s activity window) are evicted: an
   active session that gets evicted is re-added by the next scan, so
   evicting it produces a per-tick evict/re-add flicker. Active sessions
   may therefore briefly exceed the cap; the UI side is what bounds the
   screen.

   UI side: MAX_SESSION_ROWS = 12, a "还有 N 条" row instead of silent
   truncation, and the list height is clamped to the work area.

   A behavioural test caught a real bug in the first draft of the cap:
   @($count) made an array, so the subtraction threw.

4. Title/preview redaction.

   Both already went through Protect-SessionText on ff25a4b; what was
   missing was evidence of coverage. The tracker suite now dot-sources the
   real Protect-Text.ps1 instead of a stub that only knew `sk-`, and plants
   four canaries (JWT, Bearer, JSON token, kv password) in the title and
   the preview, asserting the raw value is absent, the redaction marker is
   present (so "absent" is not just truncation), and sessions.json is
   clean.

Validation
- scripts/smoke.mjs: 163 pass, 7 warn, 0 fail (new locks: no
  Start-SessionTerminal, no --session interpolation, click handler validates
  the id and spawns nothing, row cap, overflow notice, work-area clamp).
- scripts/test-session-tracker.ps1: 110 pass, 0 fail (id whitelist, decoded
  directory names, restore path, cap, idle-only eviction, four canaries).
- scripts/test-substep-progress.mjs: 139 pass, 0 fail, 1 skipped.

Every new lock was negative-injected: 12 mutations, each turned its own
assertion red, then the file was restored byte-identically and the suites
went green again. A5 (the overflow notice) initially stayed green because
the lock only looked for the call, not its guard -- the lock now matches
the `if ($hidden -gt 0) { ... }` pair.
@antianqi

Copy link
Copy Markdown
Contributor Author

Round-3 blockers, all addressed in ad4d4ac (current head of this PR).

# Blocker What changed
1 plugins/weekbin/mcode-trajectory-studio deleted Not a deletion: that plugin landed upstream in 39ddd2c, after this branch's merge base. upstream/main is merged in f246247; all 161 files are back.
2 Start-SessionTerminal command injection The function is deleted, not re-quoted — the point is to remove the path that turns data into code. The id is now shape-checked at every entry point (session directory name, sessions.json restore, click handler): ^[A-Za-z0-9][A-Za-z0-9_-]{0,127}$.
3 title/preview not redacted Already redacted on ff25a4b: Get-SessionTitle and Get-SessionPreview both call Protect-SessionText. What was missing was evidence of coverage, so the tracker suite now dot-sources the real Protect-Text.ps1 (it used a stub that only knew sk-) and plants JWT / Bearer / JSON-token / kv-password canaries in both the title and the preview. Each assertion requires the redaction marker to be present, so "secret absent" cannot pass by truncation, and sessions.json is checked too.
4 no session / UI bounds Data side: MAX_SESSIONS = 20, evicting by last activity — but only sessions that are already idle. Evicting an active one is self-defeating: the next scan re-adds it from the same directory, so the list flickers. UI side: MAX_SESSION_ROWS = 12, a "还有 N 条" row instead of silent truncation, and the list height clamped to the work area.
5 opens a terminal when nothing matches Fail closed: log and stop. A session running in a host we cannot see (embedded terminal, another terminal app) would otherwise get a second terminal — exactly what this list exists to prevent.

Verification: smoke.mjs 163 pass / 0 fail, test-session-tracker.ps1 110 pass / 0 fail, test-substep-progress.mjs 139 pass / 0 fail (1 platform skip). Every new lock was negative-injected: 12 mutations, each turned its own assertion red, then the file was restored byte-identically and the suites went green again.

antianqi and others added 2 commits October 27, 2026 16:51
Two CI-only fixes for the merged tree, both caught by the first run of the
new head.

The token roundtrip step still asserted a plaintext `planApiToken` in
config.json. set-token.ps1 stores `planApiTokenEnc` (DPAPI, CurrentUser)
and removes the plaintext key, so the step died on its first assertion
("mismatch (got: )"). It now requires the encrypted field, forbids the
plaintext field, and after -Clear requires both to be gone. Verified
locally against the real set-token.ps1 in an isolated APPDATA: 4/4.

test-substep-progress.mjs section 11 Add-Type's the UIAutomation
assemblies, which do not exist on Linux. Every assertion in that section
failed in the ubuntu job. It now skips with the same
"[skip] ... (platform)" pattern the PostToolUse section already uses; the
contract stays covered on windows-latest, the only platform this code runs
on.
Clicking a row means "take me back to that session". The × on the row
means "I am done with this one". Those are different intentions: often
the rows are conversations already dealt with, and the user does not want
to be dragged out of whatever they are doing just to clear them.

The × dismisses the row and nothing else. It does not switch to the
session, does not open a terminal, and does not close the list -- the
row disappears, the list shrinks, and the next row's × is right there.

The focus is handed back. Pressing the mouse button gives the focus to
the pill; for "jump to that session" that is fine, but for "just dismiss
this" it would cost the user their editor's focus and the next keystroke
would go somewhere unknown. The widget captures the foreground window
when the mouse enters the pill (the last moment it still has it) and
hands it back after the dismissal, then reads the foreground back to
confirm it actually landed.

The ack file has to carry more than one id now that dismissing several
rows in a row is a normal thing to do. It was one id per file, so a
second dismiss within the detector's one-second poll overwrote the first
and that row never went away -- with the × looking like it had worked.
ack.signal is now one id per line, and the detector consumes it by
renaming the file aside before reading it, so an ack written while it is
reading is not lost and a half-written file is never parsed.

The widget also hides a dismissed row locally until the detector confirms
it, otherwise the row would still be on screen for up to a second after
the click and the dismissal would look like it did nothing.

Validation
- scripts/smoke.mjs: 170 pass, 7 warn, 0 fail (new locks: the row carries
  a dismiss mark, the mark stops the event and never calls the click
  path, the dismiss path only acks and validates the id, the focus is
  captured on MouseEnter and handed back, the ack carries many ids and is
  taken by rename).
- scripts/test-session-tracker.ps1: 115 pass, 0 fail (three ids in one
  ack, a single id without a trailing newline, no leftover .taken file).
- scripts/test-substep-progress.mjs: 139 pass, 0 fail, 1 skipped.

Every new lock was negative-injected: 13 mutations, each turned its own
assertion red, then the file was restored byte-identically and the suites
went green again. Two of them (C1, C3) initially stayed green because the
row's own comment mentions Invoke-SessionDismiss by name and satisfied
the text search -- the locks now strip comment lines first.

Real machine, real mouse clicks on the deployed widget: the dismissed row
is gone from sessions.json within a second, the list stays open and shrinks
by one row, the foreground pid and hwnd are identical before and after the
click, and two dismissals in one second both land.
@antianqi

Copy link
Copy Markdown
Contributor Author

Round 24: a per-row dismiss that does not switch sessions

aa6bf54 (current head) adds a dismiss mark to every row of the session list.

Clicking a row means "take me back to that session". The new × means "I am done with this one". Those are two different intentions: often the rows are conversations already dealt with, and the user does not want to be dragged out of whatever they are doing just to clear them. So the × dismisses the row and nothing else:

  • it does not switch to the session, and does not open a terminal;
  • it does not close the list — the row disappears, the list shrinks, and the next row's × is right there, so several rows can be dismissed in a row;
  • it gives the focus back to the window that had it.

That last one was a real defect found on the machine, not a design choice. Pressing the mouse button hands the focus to the pill; for "jump to that session" that is fine, but for "just dismiss this" it cost the user their editor's focus, and the next keystroke went somewhere unknown. The widget now captures the foreground window when the mouse enters the pill — the last moment it still has it — and hands it back after the dismissal, then reads the foreground back to confirm it actually landed.

Two things the dismiss button forced

The ack file has to carry more than one id. It was one id per file, and the detector consumes it once a second, so a second dismiss within that second overwrote the first and that row never went away — with the × looking like it had worked. ack.signal is now one id per line, and the detector consumes it by renaming the file aside before reading it: an ack written while it is reading is not lost, and a half-written file is never parsed.

A dismissed row has to disappear immediately. The detector needs up to a second to confirm, and until then the row is still in sessions.json — so the widget keeps a local set of dismissed ids and hides them while rendering. The set converges on its own: once an id is gone from sessions.json, the detector has confirmed it and the widget drops it from the set.

Verification

  • scripts/smoke.mjs: 170 pass, 7 warn, 0 fail
  • scripts/test-session-tracker.ps1: 115 pass, 0 fail
  • scripts/test-substep-progress.mjs: 139 pass, 0 fail (1 platform skip)

Every new lock was negative-injected: 13 mutations, each turned its own assertion red, then the file was restored byte-identically and the suites went green again. Two of them (C1, C3) initially stayed green because the row's own comment mentions Invoke-SessionDismiss by name and satisfied the text search — the locks now strip comment lines before matching.

On the live widget with real mouse clicks: the dismissed row is gone from sessions.json within a second, the list stays open and shrinks by one row, the foreground pid and hwnd are identical before and after the click, and two dismissals in one second both land.

This adds to the round-3 blockers already addressed in ad4d4ac; the mcode-trajectory-studio deletion was the upstream merge in f246247.

@hetaoBackend hetaoBackend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for exact current head aa6bf54.

Blocking security issue: the session log scanners still recurse without a reparse-point or containment boundary:

  • plugins/antianqi/mcode-island/scripts/lib/Session-Tracker.ps1:152-170 uses [System.IO.Directory]::EnumerateDirectories($root, 'session_', 'AllDirectories') and then reads ledger/messages files under every returned directory.
  • plugins/antianqi/mcode-island/mcode-status-detect.ps1:313-341 uses [System.IO.Directory]::EnumerateFiles($sessionsRoot, '*.jsonl', 'AllDirectories') and selects files from the recursive result.

There is no canonical-path containment check and no explicit rejection of symlink/junction/reparse-point entries. On Windows, a reparse point under the sessions root can redirect recursive enumeration or a selected log outside the intended sessions root, causing the detector/list UI to read and expose an unrelated JSONL file. The current test-session-tracker.ps1 fixture covers ordinary directories and log parsing, but has no outside-target junction/symlink/path-escape regression; the exact-head Windows check name only supplies parse/token/hook/mock-API evidence for this review.

Please make traversal fail closed: canonicalize the root and every candidate, reject reparse-point ancestors/entries (including junctions/symlinks), enforce canonical containment before opening a log, and add a Windows regression fixture proving an outside target is never read. Keep the fix in both scanners and rerun the exact-head Windows behavioral evidence.

The exact-head repository Checks are otherwise green, but green Checks do not cover this path boundary. [code]smith is skipped and is not evidence.

The pill used to be a translucent grey plate: a hard-coded #99232734 tint
over a #F21A1A20 panel, with a white hairline border. It read as a flat
overlay laid on top of the desktop, not as glass. No frost, no depth, no
specular edge -- and the light-on-light failure Apple describes for
translucent materials: over a white document, 30%-white secondary text
measured 1.7:1 on a live screen, which is unreadable.

This replaces it with a material engine built to the Human Interface
Guidelines, and fixes the defect the rewrite exposed: the pill and the
expanded panel are two separate pieces of glass, and they were sharing one
polarity.

How the frost is made. A real screen capture of the desktop behind the
widget (SetWindowDisplayAffinity/WDA_EXCLUDEFROMCAPTURE, so the capture
never includes the widget itself), downsampled 4x, a WPF BlurEffect, scaled
back up -- 24 physical pixels of blur radius, computed in physical pixels.
A <=3% noise grain rides on top, so the frost has the tooth of a material
instead of the plastic look of a clean Gaussian.

Two poles, and why the thresholds are not taste. Dark glass with white
content vanishes over a white document, so the material has a dark pole and
a light pole, switched with hysteresis (bright past 0.60 to go light, dark
past 0.58 to go back). The thresholds are derived rather than picked: the
light pole needs 4.5:1 for its primary text, which needs a glass luminance
of 0.227, which back-solves to a backdrop brightness of 0.58.

Each surface picks its own pole. The pill and the panel can sit on wildly
different backdrops -- a bright title bar above, a dark editor below. An
earlier build shared one global polarity, so the panel's decision
overwrote the pill's: the pill became dark glass with dark text, and the
two surfaces flipped each other every 100 ms -- the widget looked broken.
Polarity, foreground colours and the grain brush are now decided per
surface, and the session rows take their ink from the panel they are drawn
on. HIG "Materials" says the same in one line: each material adjusts its
own luminosity to keep what is written on it legible.

Design compliance, rule by rule:
- HIG "Materials": the glass body starts as Transparent and is painted at
  runtime from a live capture. The material has no inherent colour -- it
  takes the colour of the content behind it.
- HIG "Materials": the backdrop is blurred and its luminosity adjusted so
  what is written on it stays legible.
- HIG "Materials": larger elements are more opaque. The panel is tinted
  heavier than the pill in every pole (factors 1.30 / 1.35).
- WWDC25 "Meet Liquid Glass": a one-pixel specular rim on the top edge, a
  darker inner edge below it, and a soft shadow under the body.
- HIG "Color": the dark pole uses Apple's dark-mode system colours; the
  light pole uses the macOS black-transparent label colours (85/55/30%
  black), with a light-pole twin of the state and family tables.
- HIG "Typography": a 17/13/12pt scale with an 11pt floor.

Validation
- scripts/smoke.mjs: 199 pass, 7 warn, 0 fail. The 7 warnings are the
  pre-existing "the runtime is not ready yet" hook events.
- scripts/test-session-tracker.ps1: 115 pass, 0 fail.
- scripts/test-substep-progress.mjs: 139 pass, 0 fail, 1 platform skip.

Every material constant is a named $script:glass* value, so the suite
re-derives the worst case over the real backdrop and recomputes the WCAG
ratio: 5.35:1 primary text on the dark pole, 5.13:1 on the light pole
(both >= 4.5:1), secondary text at 2.70:1 / 2.98:1 -- which is where
Apple's own translucent labels sit.

Every new lock was negative-injected: scripts/test-material-injection.mjs
(new) mutates one line of the source 22 times and asserts that the
matching check turns red each time -- a hard-coded tint coming back, the
capture affinity call removed, the measure/arrange step skipped, the grain
opacity raised, the colour tables diverging, the hysteresis removed, the
panel made thinner than the pill, the two surfaces sharing a polarity
again, the luminance log slot going back to one shared value. All 22
behaved as expected, and the suite was green again after the source was
restored.

Real machine, split backdrop: a bright strip under the pill and the dark
terminal under the panel. The pill rendered as pale glass with dark text
while the panel rendered as deep glass with white text -- simultaneously,
each reading its own backdrop, stably over several refresh cycles
("Pill L=0.837 pole=light" / "Panel L=0.528 pole=dark"). The harness
proves the stacking with WindowFromPoint before it grabs anything, so a
run cannot report success while the bright strip is sitting on top of the
widget. Real machine, dark backdrop: pill and panel both dark glass with
white text, the terminal clock behind the panel blurred to a smear.

Known trade-off: SetWindowDisplayAffinity makes the widget invisible to
screen recorders for the ~10-30ms of each capture window (every 2.5s).
@antianqi

antianqi commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

New commit on top of feat/substep-progress (aa6bf54 → afbb6d5, "feat(mcode-island): a Liquid Glass material, judged per surface") — this is the material rework, please re-review on the new head.

What changed

  • The pill is no longer a translucent grey plate (hard-coded #99232734 over a #F21A1A20 panel). It is now a material engine: live screen capture behind the widget (SetWindowDisplayAffinity/exclude-from-capture), 4× downsample, WPF BlurEffect, scaled back up (24 physical px of blur radius), ≤3% grain on top, specular rim + inner edge + soft shadow, real rounded clip.
  • Two poles — dark glass/white content and light glass/dark content — switched with hysteresis (light past 0.60, dark past 0.58). The thresholds are derived from the 4.5:1 requirement for the primary text, not picked by eye.
  • Each surface picks its own pole. The pill and the expanded panel are two separate pieces of glass; the old build shared one global polarity, so the panel's decision overwrote the pill's (dark glass with dark text, and the two flipped each other every 100 ms). Polarity, foreground colours and the grain brush are now per surface; the session rows take their ink from the panel they are drawn on.
  • Apple system colours: the dark pole uses the dark-mode system palette, the light pole the macOS black-transparent label colours, with a light-pole twin of the state and family tables (closest pair dE 30.4).
  • materialPole in config.json: auto (default) / dark / light.

Validation

  • scripts/smoke.mjs: 199 pass, 7 warn, 0 fail (the 7 are the pre-existing "runtime not ready yet" hook events).
  • scripts/test-session-tracker.ps1: 115 pass, 0 fail.
  • scripts/test-substep-progress.mjs: 139 pass, 0 fail, 1 platform skip.
  • New scripts/test-material-injection.mjs — 22 single-line mutations, each asserted to turn its own check red, source restored byte-identically afterwards, suite green again. All 22 behaved as expected.

Live machine

  • Split backdrop (bright strip under the pill, dark terminal under the panel): the pill rendered as pale glass with dark text while the panel rendered as deep glass with white text, simultaneously and stably (Pill L=0.837 pole=light / Panel L=0.528 pole=dark). The harness proves the stacking with WindowFromPoint before grabbing, so a run cannot report success while the strip sits on top of the widget.
  • Dark backdrop: pill and panel both dark glass with white text, the terminal clock behind the panel blurred to a smear.

Known trade-off (unchanged, documented in the README): SetWindowDisplayAffinity hides the widget from screen recorders for the ~10–30 ms of each capture window, every 2.5 s.

…d it

Over a white page the island rendered as a mid grey (`#6A6A6A`) with dark
labels on top -- mud. The user called it exactly that, and asked for the
opposite: one black island, the way macOS's Dynamic Island is one black
island.

The cause was the material model. The widget mixed a *fixed* proportion of
black into the blurred backdrop, so the glass landed wherever that formula
put it -- 60% black over a white page is mid grey. HIG "Materials" says
something else: regular Liquid Glass "blurs and **adjusts the luminosity** of
background content to maintain legibility of text". That is a target, not a
wash. So the model now solves for one:

    behind is brighter than the target  ->  mix in black:  veil = 1 - target / L
    behind is darker than the target    ->  mix in white:  lift = (target - L) / (1 - L)

The dark pole targets 0.115 (byte 29, `#1D1D1D` -- the black macOS's
Dynamic Island uses), so the glass settles on the same grey whether it sits
on a white document or on a black terminal. The pale pole converges on
near-white the same way, so pinning `light` gives a crisp pale material
instead of the old grey wash. `materialPole` now defaults to `dark`.

Because the glass lands on one grey over every backdrop, contrast stops
being a property of whatever is behind it. The worst case over the whole
backdrop range went from 5.35:1 / 5.13:1 (primary) and 2.70:1 / 2.98:1
(secondary) to 16.57:1 / 10.56:1 and 5.86:1 / 3.88:1. The old trade-off --
secondary text below WCAG AA on a bright backdrop -- is gone.

The frost still works, and now keeps the backdrop's hue: a blue shape behind
the glass came through as rgb(15,29,43), which is 11.5% of rgb(0,122,255).
Adjusting luminance instead of washing grey over it is what makes that
possible.

Design compliance
- HIG "Materials": the glass converges on a target luminance, so legibility
  no longer depends on the backdrop -- that is the sentence the rule is
  about.
- HIG "Materials": larger elements are more opaque. The panel converges on a
  darker target grey than the pill, so the large element reads as a thicker
  slab of the same material.
- HIG "Materials": the material has no inherent colour. The tint is solved
  from the live capture, and the backdrop's hue survives the wash.
- WWDC25 "Meet Liquid Glass": a specular rim on the top edge, a darker inner
  edge below it, a soft shadow under the body.

One bug worth naming: the thickness factor was written `$thick` while the
function parameter is `[bool]$Thick`. PowerShell variable names are
case-insensitive and the parameter's type constraint still applies, so
`$thick = 0.25` was coerced to `$true`, `(1.0 - $thick)` evaluated to 0,
and the target grey collapsed to 0 -- the glass became pure black and the
veil was pinned at 1. It is renamed `$thickMix`, and the suite now asserts
that the factor does not shadow the typed parameter. Static assertions
cannot see that class of bug: every number in the source was correct.

Validation
- scripts/smoke.mjs: 203 pass, 7 warn, 0 fail. The 7 warnings are the
  pre-existing "the runtime is not ready yet" hook events.
- scripts/test-session-tracker.ps1: 115 pass, 0 fail.
- scripts/test-substep-progress.mjs: 139 pass, 0 fail, 1 platform skip.
- The contrast model in the suite mirrors the new maths: it reads the target
  greys and the tint gradients out of the source and re-derives the worst
  case over the whole backdrop range, for both poles.

Test evidence
- Negative injection: scripts/test-material-injection.mjs now mutates one
  line of the source 27 times and asserts that the matching check turns red
  each time -- including the new ones: the convergence formula replaced by a
  fixed wash, the dark target lifted off the island black, the light target
  collapsed, the default material no longer black, the thickness factor
  shadowing `$Thick`. All 27 behaved as expected and the suite was green
  again after the source was restored.
- Real machine, deterministic white backdrop: the island renders as black
  glass with white text and a visible frost; pixel-sampled, a blue shape
  behind it comes through as rgb(15,29,43) -- 11.5% of rgb(0,122,255), so the
  hue survives the wash.
- Real machine, dark terminal: the same black slab, with the terminal's
  clock and text coming through the panel blurred.
@antianqi

antianqi commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

New commit on top of feat/substep-progress (afbb6d5 -> a3961c1, "feat(mcode-island): a black glass that stays black, whatever is behind it") — the material rework the user asked for after seeing the island render as mud over a white page. Please re-review on the new head.

What changed

  • The island is now one black slab, whatever is behind it — the way macOS's Dynamic Island is. Over a white page it used to render as mid grey (#6A6A6A) with dark labels on top; the user called it "灰突突" and asked for unified black.

  • The material model changed from "mix a fixed proportion of black into the backdrop" to solving for a target grey, which is what HIG "Materials" actually asks for ("adjusts the luminosity of background content to maintain legibility of text"):

    behind is brighter than the target  ->  mix in black:  veil = 1 - target / L
    behind is darker than the target    ->  mix in white:  lift = (target - L) / (1 - L)
    

    The dark pole targets 0.115 (byte 29, #1D1D1D — the black macOS's Dynamic Island uses); the pale pole converges on near-white the same way, so pinning light gives a crisp pale material instead of a grey wash. materialPole now defaults to dark.

  • Because the glass lands on one grey over every backdrop, contrast stops depending on the backdrop: worst case went from 5.35:1 / 5.13:1 primary and 2.70:1 / 2.98:1 secondary to 16.57:1 / 10.56:1 and 5.86:1 / 3.88:1. The old "secondary text below WCAG AA on a bright backdrop" trade-off is gone.

  • The frost keeps the backdrop's hue: a blue shape behind the glass came through as rgb(15,29,43) — 11.5 % of rgb(0,122,255). Adjusting luminance instead of washing grey over it is what makes that possible.

Validation

  • scripts/smoke.mjs: 203 pass, 7 warn, 0 fail.
  • scripts/test-session-tracker.ps1: 115 pass, 0 fail.
  • scripts/test-substep-progress.mjs: 139 pass, 0 fail, 1 platform skip.
  • Negative injection: 27 single-line mutations, each asserted to turn its own check red, source restored byte-identically afterwards. The new ones cover the convergence formula being replaced by a fixed wash, the dark target lifted off the island black, the light target collapsed, the default material no longer black, and the thickness factor shadowing the typed $Thick parameter.

Live machine

  • Deterministic white backdrop: the island renders as black glass with white text and a visible frost; the pixel sample above is from that screenshot.
  • Dark terminal: the same black slab, with the terminal's clock and text coming through the panel blurred.

One bug worth naming: the thickness factor was written $thick while the parameter is [bool]$Thick. PowerShell variable names are case-insensitive and the parameter's type constraint still applies, so $thick = 0.25 was coerced to $true, (1.0 - $thick) evaluated to 0, and the target grey collapsed to 0 — the glass went pure black. Renamed $thickMix, and the suite now asserts the factor does not shadow the typed parameter. Static assertions cannot see that class of bug: every number in the source was correct.

Bring PR MiniMax-AI#62 up to date with upstream/main so the branch stops showing as
CONFLICTING and can be reviewed as a normal diff.

Upstream changes picked up (merge-base 39ddd2c -> 01c44b6):
- 0125d0f: rename plugins/stevenjj33/web-research -> plugins/stevenjj33/web-search
- 01c44b6: chore: ignore credential files (MiniMax-AI#76)

Conflict resolution:
- .gitignore: both sides appended a new block to the same base. Kept both.
  Upstream's credential rules (.env, secrets/, credentials.json, *.pem, *.p12)
  stay first; the branch's stale-widget-backup rule (*.bak-r16) follows in its
  own commented block. No rule is dropped and no rule is duplicated.

Validation:
- git status shows no remaining unmerged paths after the merge.
- .gitignore written with LF endings and UTF-8 without BOM, matching the file
  already in the tree.
- The renamed web-search tree matches upstream/main byte for byte; git recorded
  it as a pure rename with no content conflict.

Test evidence:
- No plugin source files were touched by this merge, so the mcode-island suites
  are unaffected. The branch tip before the merge was a3961c1, whose own run was
  smoke 203 pass / 7 warn / 0 fail, tracker 115 pass, substep 139 pass, and
  27 negative injections all failing as designed.

Design compliance:
- One commit, one purpose: this commit carries the upstream sync only.
- No unrelated files are included. Untracked working-tree files from another
  line of work (jianying plugin, PR comment drafts) are deliberately left out.
@antianqi

antianqi commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Head is now a2520bf -- the conflict is resolved, please re-review

This PR was showing as CONFLICTING against main. That is fixed: a2520bf merges upstream/main (01c44b6) into the branch, and GitHub now reports the PR as MERGEABLE.

What the merge brought in (merge-base 39ddd2c -> 01c44b6):

  • 0125d0f -- rename plugins/stevenjj33/web-research -> plugins/stevenjj33/web-search
  • 01c44b6 -- chore: ignore credential files (#76)

The one conflict was .gitignore, where both sides appended a new block to the same base. Both are kept: upstream's credential rules (.env, .env.*, *.env, !.env.example, secrets/, credentials.json, *credentials.json, *.pem, *.p12) first, then this branch's stale-widget-backup rule (*.bak-r16) in its own commented block. No rule is dropped, none is duplicated.

The renamed web-search tree matches upstream/main byte for byte -- git recorded it as a pure rename with no content conflict.

Re-run after the merge (the merge touched no plugin source, but the tree changed, so the suites were run again on the merged result):

  • scripts/smoke.mjs: 203 pass, 7 warn, 0 fail
  • scripts/test-session-tracker.ps1: 115 pass, 0 fail
  • scripts/test-substep-progress.mjs: 139 pass, 0 fail, 1 platform skip

Still open from the aa6bf54 review -- not addressed yet. The path-traversal finding (recursive enumeration of the sessions root without canonical-path containment, and without rejecting reparse points / junctions) is a real blocker and this merge does not touch it. The two scanners that need it are scripts/lib/Session-Tracker.ps1:155 and mcode-status-detect.ps1:321. That is the next round of work, with a Windows junction fixture proving an outside target is never read; the re-review above does not claim it is fixed.

The material rework is unchanged from the two notes above (afbb6d5, a3961c1).

…-way

CodeQL turned this PR's check red with one high-severity alert:
`scripts/smoke.mjs:2092` -- `js/incomplete-sanitization`, "This string may still
contain <!--, which may cause an HTML element injection vulnerability."

The suite stripped the XML comments out of the widget's XAML with

    replace(/<!--[\s\S]*?-->/g, '')

That regex only removes a `<!--` that has a matching `-->`: an unclosed
`<!--` survives the "sanitisation" intact. The sanitisation was therefore
incomplete, and a comment in the source could still feed the material
assertions -- which is exactly what the strip exists to prevent. CodeQL
flags the pattern, and the flag was right.

**The fix** is a text walk instead of a regex replace. A paired comment is
dropped whole; an unpaired `<!--` drops everything after it. Either way the
stripped XAML carries no comment marker, which is the property the assertions
actually need.

**Three new assertions** say that out loud instead of leaving it implied:

- `xaml comments: every XML comment in the XAML is closed` -- an unclosed
  comment in the source is a source defect, and the suite now says so;
- `xaml comments: the stripped XAML carries no comment marker` -- the strip
  itself is checked, so it cannot quietly stop working;
- `xaml read: the XAML block could not be read` -- before, an unreadable XAML
  produced an empty string and the material assertions quietly ran against
  nothing.

Validation

- `scripts/smoke.mjs`: 205 pass, 7 warn, 0 fail (was 203).
- `scripts/test-session-tracker.ps1`: 115 pass, 0 fail.
- `scripts/test-substep-progress.mjs`: 139 pass, 0 fail, 1 platform skip.

Test evidence

Negative injection is now 30 cases (was 27). All 30 behave as designed: each
mutates one line, the expected assertion turns red, the line is restored, and
the suite is green again at the end (`205 pass, 7 warn, 0 fail`, and the
widget source is byte-identical to the committed version afterwards).

The three new cases are the ones this commit adds:

- an XML comment left unclosed in the XAML -> `xaml comments: an XML comment
  in the XAML is never closed`;
- the XAML block made unreadable -> `xaml read: the XAML block could not be
  read`;
- the strip itself broken (`keep += s.slice(i, open)` -> `s.slice(i)`) ->
  `xaml comments: a comment marker survived the strip`.

Design compliance

The incompleteness is removed, not hidden: the analyser no longer sees a
half-finished multi-character replacement because there is no longer a
multi-character replacement. No rule is silenced, no check is weakened, and
the material assertions keep reading the same comment-free XAML, so their
verdicts are unchanged.
…trip

The previous commit closed the js/incomplete-sanitization alert, and CodeQL
answered with a different one on the same line -- still one high-severity
alert, still `scripts/smoke.mjs`:

    js/bad-html-filtering-regexp: This regular expression only parses -->
    and not --!> as a HTML comment end tag.

HTML5 accepts `--!>` as a comment end as well as `-->`, so a filter that knows
only `-->` leaves such a comment open. The strip now goes through one helper
that recognises both forms, and advances by the tag's real length -- `-->` is
three characters, `--!>` is four.

**The probe caught a real bug in the first attempt.** The advance was written
as `close + 3`, carried over from the `-->` case; the new assertion went red
immediately, because with `--!>` a character survived the comment. The
advance is now `close.at + close.len`, and the probe is what says so.

**One new assertion**:

    xaml comments: the strip recognises every HTML comment end tag

It runs the strip over a known sample, `<a><!-- c --!><b>keep</b>`, and
requires `<a><b>keep</b>` back. The probe is the only way to prove it: the
widget's own XAML never uses `--!>`, so a strip that knew only `-->` would
look perfect against the real file.

Validation

- `scripts/smoke.mjs`: 206 pass, 7 warn, 0 fail (was 205).
- `scripts/test-session-tracker.ps1`: 115 pass, 0 fail.
- `scripts/test-substep-progress.mjs`: 139 pass, 0 fail, 1 platform skip.

Test evidence

Negative injection is now 32 cases (was 30). All 32 behave as designed: each
mutates one line, the expected assertion turns red, the line is restored, and
the suite is green again at the end (`206 pass, 7 warn, 0 fail`, with the
widget source byte-identical to the committed version afterwards).

The two new cases are the ones this commit adds:

- the strip only knows `-->` -> `xaml comments: the strip does not recognise
  every HTML comment end tag`;
- the strip advances by the wrong length -> the same assertion.

Design compliance

The filter is now complete on its own terms; the analyser no longer has a
point. Nothing is silenced and no check is weakened -- the two assertions
that did not exist before are the ones that prove the strip is complete, and
both of them can go red.
@antianqi

antianqi commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Head is now 2ae7407 -- the CodeQL check is green again

The PR was red on CodeQL with one high-severity alert, in this PR's own code. It is fixed, and the whole check suite is green on the new head.

What the alert was. scripts/smoke.mjs strips the XML comments out of the widget's XAML before the material assertions read it, so that a comment in the source cannot feed an assertion. The strip used:

replace(/<!--[\s\S]*?-->/g, '')

That regex only removes a <!-- that has a matching --> -- an unclosed <!-- survives the "sanitisation" intact, so the sanitisation was incomplete, and a comment could still feed the assertions. CodeQL said so twice, in two different rules, and it was right both times:

  1. js/incomplete-sanitization -- "This string may still contain <!-- ..."
  2. js/bad-html-filtering-regexp -- "This regular expression only parses --> and not --!> as a HTML comment end tag.

HTML5 accepts --!> as a comment end as well, so the filter has to know both forms.

The fix. A text walk instead of a regex replace, through one helper that recognises both end tags and advances by the tag's real length (--> is three characters, --!> is four). A paired comment is dropped whole; an unpaired <!-- drops everything after it. The stripped XAML therefore never carries a comment marker, which is the property the assertions actually need.

A probe caught a real bug in the first attempt. The advance was written as close + 3, carried over from the --> case; the new assertion went red immediately, because with --!> a character survived the comment. The advance is now close.at + close.len. The probe is what says so:

xaml comments: the strip recognises every HTML comment end tag

It runs the strip over <a><!-- c --!><b>keep</b> and requires <a><b>keep</b> back. The probe is the only way to prove it -- the widget's own XAML never uses --!>, so a strip that knew only --> would look perfect against the real file.

Four assertions now say it out loud instead of leaving it implied: every comment in the XAML is closed; the stripped XAML carries no marker left; the strip recognises every HTML comment end tag; and the XAML block cannot be read at all (before, an unreadable XAML produced an empty string and the material assertions quietly ran against nothing).

Validation

  • scripts/smoke.mjs: 206 pass, 7 warn, 0 fail
  • scripts/test-session-tracker.ps1: 115 pass, 0 fail
  • scripts/test-substep-progress.mjs: 139 pass, 0 fail, 1 platform skip
  • Negative injection: 32 single-line mutations (was 27), each asserted to turn its own check red, source restored byte-identically afterwards. All 32 behaved as expected; the suite is green again at the end.
  • The five new cases: an XML comment left unclosed in the XAML, the XAML block unreadable, the comment strip itself broken, the strip knowing only one of the two HTML comment end tags, and the strip advancing by the wrong length.

Checks on this head: validate (ubuntu-latest), analyze, mcode-island on windows-latest (parse + token + hook + mock-API) and CodeQL -- all green. The PR is MERGEABLE with merge state CLEAN.

Still open from the aa6bf54 review -- not addressed by this head either. The path-traversal finding (recursive enumeration of the sessions root without canonical-path containment, and without rejecting reparse points / junctions) is a real blocker and nothing in these three commits touches it. The two scanners that need it are scripts/lib/Session-Tracker.ps1:155 and mcode-status-detect.ps1:321. That is the next round of work, with a Windows junction fixture proving an outside target is never read.

@hetaoBackend hetaoBackend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes for exact current head 2ae7407.\n\nThe session-log traversal boundary remains unresolved on this head:\n\n- plugins/antianqi/mcode-island/scripts/lib/Session-Tracker.ps1:155 calls Directory.EnumerateDirectories($root, "session_", "AllDirectories").\n- plugins/antianqi/mcode-island/mcode-status-detect.ps1:321 calls Directory.EnumerateFiles($sessionsRoot, "*.jsonl", "AllDirectories").\n\nMicrosoft documents that SearchOption.AllDirectories includes reparse points such as symbolic links and mounted drives (https://learn.microsoft.com/en-us/dotnet/api/system.io.searchoption?view=net-10.0). The returned paths are then inspected/opened without canonical containment or explicit reparse-point rejection. A junction/symlink under the sessions root can therefore cause the island/session detector to read session-like directories or JSONL logs outside the intended root. This can expose unrelated local session content in the widget/status.\n\nThe added scripts/test-session-tracker.ps1 has no outside-target junction/symlink/path-escape regression, and .github/workflows/mcode-island-windows.yml only parses scripts and exercises token/hook/mock-API contracts; it does not run the session traversal behavior test.\n\nPlease replace blind AllDirectories recursion with a traversal that rejects reparse-point entries/ancestors and verifies each canonical candidate remains under the canonical sessions root before opening it. Add and run a Windows junction/symlink regression that proves an outside JSONL file is never read. The exact-head Checks are green, but they do not cover this path-boundary case.

@hetaoBackend hetaoBackend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readable follow-up to review 5761764089 for exact current head 2ae7407.

The session-log traversal boundary remains unresolved:

  • Session-Tracker.ps1:155 uses Directory.EnumerateDirectories($root, "*session_*", "AllDirectories").
  • mcode-status-detect.ps1:321 uses Directory.EnumerateFiles($sessionsRoot, "*.jsonl", "AllDirectories").

Microsoft documents that SearchOption.AllDirectories includes reparse points such as symbolic links and mounted drives: https://learn.microsoft.com/en-us/dotnet/api/system.io.searchoption?view=net-10.0. The results are inspected/opened without canonical containment or explicit reparse-point rejection, so a junction/symlink under the sessions root can make the widget or detector read JSONL outside that root.

test-session-tracker.ps1 has no outside-target junction/symlink regression, and the Windows workflow does not run this traversal behavior test. Please reject reparse-point entries/ancestors, enforce canonical containment before opening candidates, and add/run a Windows junction or symlink regression proving an outside log is never read. The current-head checks are green but do not cover this path boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants