Skip to content

Add mcode-trajectory-studio: read-only session trajectory inspection via the runtime SQLite projection - #56

Open
weekbin wants to merge 1 commit into
MiniMax-AI:mainfrom
weekbin:feat/weekbin-mcode-trajectory-studio
Open

weekbin wants to merge 1 commit into
MiniMax-AI:mainfrom
weekbin:feat/weekbin-mcode-trajectory-studio

Conversation

@weekbin

@weekbin weekbin commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

What changes

Adds one new Plugin: plugins/weekbin/mcode-trajectory-studio. It exposes seven MCP tools plus one Skill for read-only session-trajectory inspection of local MiniMax Code sessions, backed by the runtime's own SQLite projection, and ships a local Studio panel. Nothing outside plugins/weekbin/mcode-trajectory-studio/ is touched.

The Plugin reads the runtime's projection with the built-in node:sqlite, so it adds no native module and no ABI constraint of its own — mcode writes that file with better-sqlite3, and only SQLite's on-disk format is shared.

The confidentiality contract

This Plugin's whole risk surface is "what can leave this process", because it reads session data that routinely quotes credentials. The contract it implements:

Access is read-only, and it writes nothing. The projection is opened with readOnly: true. No session storage is written, moved or deleted, and the Plugin creates no files at all — not even a port file, which is why reusing a previous process's port is not attempted. When the projection is missing or unreadable it falls back to the session's own messages.jsonl and says so.

There is no network access and no telemetry. The only listener is the optional panel, bound to 127.0.0.1 as a non-overridable constant, and it refuses a non-loopback peer address.

summary detail returns no message text, tool arguments or tool results. full detail is opt-in per call.

full detail is redacted in two tiers, because a credential does not arrive in the shape one would expect. The runtime stores tool results as JSON text, not as objects: in a real projection of 525 sessions / 118,109 message rows, 109,462 rows carry tool_call_result_data as a string, so a credential reaches the redactor escaped — {\"api_key\":\"…\"}. A key/value rule that requires the key and its separator to be adjacent never fires on that. So: a string value that parses as JSON is walked again as structured data (and rewritten only when a redaction actually fired, so JSON tool results are never reformatted to protect nothing), and the key/value rule tolerates and re-emits any backslash run on either delimiter so text that is not valid JSON at all is still covered. Measured back through the shipped MCP path on that same projection: 54 escaped pairs whose key names a credential, 0 surviving.

Credential keys are judged by word boundary, not by an exact list. clientSecret, refreshToken, accessToken, authToken, privateKey, apiSecret and xApiKey are covered. Counters (inputTokens, total_tokens) and addressing identifiers (sessionId) deliberately survive: a session id is how the API addresses everything, and treating it as a secret returned "[redacted]" from /api/sessions and broke every following request.

Coverage also includes private key blocks, credentials inline in a connection string (including a password that itself contains @, which an earlier rule truncated and left half in the clear), the whole Authorization header with its scheme, a bare scheme and token, provider-prefixed keys (sk-, ghp_, glpat-, xox* …), length-anchored tokens (github_pat_, npm_, hf_, Google AIza/ya29., Azure AccountKey=), AWS key ids, and an unlabelled JWT. Rules are ordered most-specific-first and preserve shape, so the result stays valid JSON. Task descriptions and commands are redacted at the data source; a session title has only its credential substrings replaced. Every outbound payload is swept once more at the boundary — including the MCP failure branch, since an egress swept only when it succeeds is not a boundary.

The rules are idempotent, and that is asserted as a property. Text is swept at the data source, per record and again at the boundary, so a rule that re-redacts its own output compounds. redact.test.mjs asserts redact(redact(x)) === redact(x) over the corpus × four option sets, that no sweep grows a marker a bracket at a time, that a truncation marker is stable across sweeps, and that a JSON string with nothing to redact comes back byte-identical. Over-redaction is pinned from the other side too: npm_config_registry and HF_HOME survive intact, and a 13-digit epoch is not read as a phone number.

Personal data is masked on the egress that leaves the machine, and nowhere else. E-mail addresses and phone numbers are masked on the MCP path, whose output reaches a model context. The panel is the reader's own screen, where masking a customer's address would destroy the answer they opened it for; that difference is one explicit argument, not an implicit behaviour.

Paths are folded, not returned verbatim. The home directory, the data directory (including a deployment that places it outside the home directory) and any root named in MCODE_TRAJECTORY_REDACT_ROOTS collapse to ~ wherever they appear — inside a warning string, in a workspace field, in a tool argument, and in the doubled-backslash form a Windows path takes inside a JSON column. Another account's home keeps its shape and loses the name (/home/<user>/…). That setting can only fold more, never less, so unlike the listen address it is not a knob that can weaken anything.

Everything reachable has a ceiling, and truncation is reported rather than implied. Per string, per response (an event page is trimmed to a byte budget and reports truncated/omitted so the client pages on nextOffset), and per row — the SQLite read has an 8 MiB data_json ceiling whose test runs inside SQL, so an oversized row is returned as oversized with its byte count instead of being parsed into the process or silently dropped. A JSONL record with no newline cannot grow the read buffer past the 2 MiB line cap, and the lines it drops are counted in droppedOversized. Diagnostics are bounded too: warnings keeps the most recent 64 entries and reports the dropped count.

File reads are contained. Every read canonicalizes each traversed component, must land inside the canonical data directory, may not have a symlinked final component, is opened under O_NOFOLLOW, and on Linux has its opened descriptor re-verified through /proc/self/fd, so the containment decision and the read describe one inode. Where /proc is unavailable (macOS, Windows) that residual race is a documented, accepted boundary rather than an unstated one.

The panel is authorized, not merely fenced. A 256-bit capability is minted per process, carried in the URL fragment (so it never reaches the server, never lands in a request log, and never appears in a Referer), and required with a constant-time comparison on every /api/* route. Because one MCP server runs per session, two sessions get two panels with two capabilities and neither URL opens the other's. Host, Origin and Sec-Fetch-Site checks are kept as defence in depth. Assets are served under a deny-by-default CSP (default-src 'none') with no-referrer and no-store, and every node is rendered with createElement/textContent, so a session body is text and never markup — with a mutation-checked scan that fails the suite if a markup sink is ever introduced. The panel opens in summary detail, so no content is fetched until the reader ticks 显示正文, and the control is derived from the state so the two cannot drift apart.

The one place an external command runs gets an allowlisted environment. git rev-parse folds a project's worktrees into one sidebar group, and it runs on a directory that came out of session data. It receives a fixed allowlist of environment variables rather than the whole process.env, so no credential the host exports is inherited, plus -c core.fsmonitor=false -c credential.helper= and no askpass helper. The user's own ~/.gitconfig is deliberately kept, because safe.directory lives there and the probe reads no value from it; an alias.rev-parse planted in a repository's own .git/config cannot shadow the builtin, which was measured rather than assumed.

Documented limits (see README.md "Known limits" and DESIGN.md §6.6): the redactor recognises credential shapes and credential-named keys, so a high-entropy string with no label and no provider shape is not a credential to it; PII masking covers e-mail addresses and phone numbers only, and only on the MCP egress; an absolute path under a root the operator has not configured is returned verbatim; and the panel capability does travel into the session transcript with the tool result, because the runtime persists tool results and sends the context on later turns — it is contained by being process-scoped and loopback-only, and server/main.mjs --serve is the way to keep a capability out of a model context entirely. --doctor prints real paths on purpose: it is the diagnostic a user reads and decides whether to share.

Compatibility

The Plugin's floor and range are tied to the host's Node, not to a second copy of a native driver, because it uses the built-in node:sqlite:

Node Bundled SQLite FTS5 Behaviour
22.12.0 — node:sqlite absent entirely refuses to start, naming the floor
22.13.0 3.47.2 absent starts; trajectory_search degrades with a warning and the FTS5 test reports itself skipped
22.15.0 3.49.1 absent same
22.19.0 3.50.4 present starts; search available
22.21.1 / 22.23.2 3.50.4 / 3.51.3 present same
23.4.0 / 23.11.0 3.47.1 / 3.49.1 absent starts; search degrades — and note these bundle an older SQLite than 22.19.0 does
24.0.0 3.49.1 present starts; search available
24.16.0 / 24.19.0 / 24.20.0 3.53.0 / 3.53.3 / 3.53.4 present same

The two numbers that matter are therefore separate: the node:sqlite floor is 22.13.0, and FTS5 is not monotonic in the Node version — absent from 22.13.0 through 22.18.x and throughout 23.x, present from 22.19.0 and 24.0.0. The floor is enforced with a readable sentence rather than a module-resolution crash, the verified range mirrors mcode's own engines (>=22.19 <23 || >=24 <27), FTS5 is probed rather than inferred, and search degrades with an explicit warning where it is absent. Declaring these two numbers once is not enough on its own — the Node minimum is stated identically in both manifests and both READMEs, and node-version.test.mjs asserts that.

The suite executed green on every release in that table; on the FTS5-absent releases its single skip was the search test reporting itself skipped rather than failing. tools/compat-matrix.mjs ships with the Plugin so the table can be re-checked rather than trusted: it runs the suite under whatever Node executes it and fails unless fail === 0, unless the skip count matches whether that runtime's SQLite has FTS5, and unless every skip is the FTS5 search test. It is mutation-checked — an injected failure and an injected unrelated skip both make it exit 1.

The suite runs on Linux, macOS and Windows. The off-Linux runs are what surfaced three platform assumptions in the tests — a temporary path compared before canonicalization (/var on macOS, a short RUNNER~1 path on Windows), a POSIX-only assertion that a resolved data directory comes back verbatim, and a temporary directory removed while SQLite still held the file open (EBUSY on Windows) — all three fixed.

User value

After installing the Plugin, a user can ask what a session actually did and where its time went, without opening any session file by hand.

Example prompt:

用 mcode-trajectory-studio 技能总结我最近更新的会话:轮数、步数、LLM 与工具的墙钟时间、工具失败数,以及压缩(compaction)。

Expected result: the agent calls trajectory_summary and reports the folded totals — turns, steps, llmMs, toolMs, decodeMs, token counts, tool calls and failures, compactions, sub-agent dispatches — and marks TTFT as unavailable rather than estimating it, because the runtime does not persist time-to-first-token.

Example prompt:

打开这个会话的轨迹面板,我要自己看时间轴。

Expected result: the agent calls trajectory_studio and returns a 127.0.0.1 URL whose fragment carries a per-process capability. Opening it verbatim including the #t=… fragment shows a three-lane timeline, a scannable record stream, a per-record inspector, and a background-task list, in summary detail — message text, tool arguments and results are fetched only once the reader ticks 显示正文, since the timeline, timings, token usage and tool names need none of it. A URL with the fragment stripped loads the page shell and no data, and the page says so.

Plugin submission checklist

  • Plugin lives at plugins/<github-owner>/<plugin-name>.
  • plugin.json name matches the Plugin directory.
  • README.md includes a real example prompt and expected result.
  • LICENSE and plugin.json declare an open-source license (Apache-2.0).
  • Required executables, accounts, paid services, and supported platforms are disclosed.
  • Network destinations and data handled by the plugin are disclosed.
  • No credentials, private endpoints, hidden telemetry, installers, symlinks, or native binaries are included.
  • Every scaffold TODO has been replaced.
  • npm run check passes.

Required executables: node on PATH, 22.13.0 or newer — the hard floor for the built-in node:sqlite without --experimental-sqlite. Below it the Plugin exits with that sentence rather than a module-resolution error. The verified range is mcode's own: >=22.19 <23 || >=24 <27. git is optional and only used to fold a project's worktrees into one sidebar group. No accounts, no paid services, no npm dependencies, no native module and no build step.

Network and data behavior: no network access at runtime and no telemetry, and the Plugin creates no files. It reads the runtime's SQLite projection strictly read-only and falls back to the session's own messages.jsonl when the projection is missing. The optional Studio panel binds 127.0.0.1 only and requires a per-process capability on every route. What leaves a full payload is described under "The confidentiality contract" above.

Known limitation: SQLite needs a writable directory even for a read-only open, because a WAL database needs its -shm file. On a read-only mount, or when the data directory belongs to another user, the Plugin reports sqlite_unavailable:attempt to write a readonly database and falls back to messages.jsonl with the timing fields absent. Reading a live WAL database that another process is writing works normally.

Evidence

npm run check — the same command CI runs (validate then the repository suite):

$ npm run check
OK   plugin weekbin/mcode-trajectory-studio
...
Validated 29 hosted Plugins and all examples.

ℹ tests 523
ℹ pass 523
ℹ fail 0

The Plugin's own suite is 166 tests / 0 failures. tools/compat-matrix.mjs asserts the current run on whatever Node executes it — on Node 24.19.0 / SQLite 3.53.3 that is 166 pass, 0 fail, 0 skipped.

  • containment.test.mjs — a canary planted outside the data directory, reached for through a symlinked task directory, a two-hop link, a relative link, a symlinked output.log, a symlinked task root, a symlinked session directory and a symlinked messages.jsonl. Each case asserts the canary is absent and that the read is reported unavailable, with a positive control that must still succeed. It also pins the resource behaviour: an unterminated JSONL line takes the incremental cap's discard path and a line after it still folds.
  • redact.test.mjs — JSON object forms, nested envelopes, the escaped form the runtime actually stores ({\"api_key\":\"…\"}) and double escaping, Authorization: Bearer …, Basic, proxy-authorization, a header inside a shell command, a tool-call description, the provider and length-anchored token shapes, and the identifiers the API is keyed on. Each case asserts the secret is gone and that the surrounding payload survives, plus the four properties described above and the over-redaction pins.
  • panel-security.test.mjs — the capability (the old fixed header alone is 403; a wrong-length value is refused without throwing; a repeated header is refused; the right one is accepted and not echoed), cross-process isolation, the loopback bind plus unreachability on every non-loopback interface and a peer-address fence, the mutation-checked render-sink scan, and the privacy default (the toggle must not ship checked, the state default must be summary, and the control must be derived from the state).
  • protocol.test.mjs — the real MCP server over stdio, driven as a client the way mcode does: version negotiation, the seven tools and their annotations, a call against a fixture projection, full detail redacted on the wire (including a credential hidden inside a tool result, stored as the runtime stores it), the failure branch swept through the same path as the success branch, a reply trimmed to its byte budget with truncated/omitted reported, unknown tool, unknown method, an unanswered notification, --doctor, and the lifecycle — closing stdin ends the process with the panel running. Every wait is bounded, so a server that stops answering fails the suite instead of hanging it.
  • node-version.test.mjs — the floor, the verified range, the FTS5 claim against an actual CREATE VIRTUAL TABLE … USING fts5 on the running runtime, and that all four declarations state the same numbers.
  • store.test.mjs — the SQLite reads, the JSONL fallback, repository grouping (including a real worktree merge), provenance, the tool-call/task join, the agent-definition lookup, and the bounds: a row past the per-record byte cap is reported as oversized with the index and total still aligned and its siblings intact, the warning list is bounded with its dropped count accounted for, the folded-root list always includes the data directory and refuses a root that would rewrite every separator, and the environment handed to git is matched against an exact allowlist so ...process.env cannot come back — with the real-worktree test as the positive control that the restricted environment still runs git.
  • The pre-existing suites for client formatting, path portability and the package declarations.

Two claims in the contract above were measured against real data rather than reasoned about. Redaction: over the projection of 525 sessions / 118,109 message rows described earlier, the escaped credential pairs whose key names a credential went from none redacted to 54 pairs, 0 surviving when read back through the shipped MCP path. Rendering, in a real browser: at the default summary detail the first paint fetches no message text at all, and after enabling 显示正文 three planted payloads (<img src=x onerror=…>, <svg onload=…>, "><script>…</script>) render as literal text with zero on* inline handlers, zero javascript: URLs, no <img> and a clean console; a credential in the session title arrives as [redacted]; no token in localStorage and no cookie; and the response headers are default-src 'none' plus no-referrer.

tests/plugins/mcode-trajectory-studio/smoke.test.mjs audits the package against the repository layout (registry manifest, MCP descriptor, exactly one Skill, version coherence, package hygiene and size limits) and runs deliberate mutations through its manifest checkers, so a passing audit cannot be a false green.

The two CodeQL findings from the first round are addressed in the current tree: the client only ever receives a code from a closed set, the request is never interpolated into a response, and an unexpected error's detail goes to stderr. No new alerts are expected; the scan re-runs on push.

Comment thread plugins/weekbin/mcode-trajectory-studio/server/http.mjs Fixed
Comment thread plugins/weekbin/mcode-trajectory-studio/server/http.mjs Fixed
@weekbin
weekbin force-pushed the feat/weekbin-mcode-trajectory-studio branch 2 times, most recently from d1593db to dfc6bb5 Compare September 20, 2026 03:46
@weekbin

weekbin commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author
image

@weekbin
weekbin force-pushed the feat/weekbin-mcode-trajectory-studio branch from dfc6bb5 to 8b7cd94 Compare September 20, 2026 05:56

@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 8b7cd943719dd3ca6c103208d3b6269d609e26b9.

The read-only SQLite access and parameterized queries are good, but several confidentiality/containment contracts are not yet safe:

  1. Symlink root escapes. server/tasks.mjs:93-113 validates the lexical task path/final file but not a symlinked task directory; server/jsonl.mjs:19-53 traverses session directories and then uses stat() before checking isSymbolicLink(), so the check observes the resolved target. I reproduced both paths reading external output.log / messages.jsonl through parent-directory symlinks. Canonicalize every traversed component beneath the approved roots and add real escape canaries.
  2. Secret redaction misses common stored forms. server/redact.mjs:9-44,75-89 leaves JSON strings such as {"api_key":"..."} and {"env":{"TOKEN":"..."}} unchanged, and turns Authorization: Bearer token into Authorization=[redacted] token. Task descriptions/commands at server/tasks.mjs:52-60 also bypass redaction. Full-detail and task surfaces must redact these realistic encodings before returning them.
  3. Studio checks are CSRF fences, not authorization. server/http.mjs:205-225 checks Host/Origin/fixed x-trajectory-client/Fetch metadata, but any local process can send the fixed header; the URL minted at server/http.mjs:97-128 carries no capability. There is no binding between the caller and requested session/task IDs. Add a per-process capability token or enforce/document an equivalent single-user authorization boundary.
  4. The MCP readOnlyHint is false for trajectory_studio. server/mcp.mjs:35-45,124-133 marks every tool read-only, but starting Studio binds a listener and writes studio-port.json (server/http.mjs:172-190). The write also follows a pre-existing symlink. Correct the annotation and make persistence symlink-safe.
  5. Node compatibility is overstated. The server imports node:sqlite (server/sqlite.mjs:10-13) and launches plain node, but the manifests/package say Node >=22. Without --experimental-sqlite, the supported minimum is Node 22.13.0. Raise the minimum and test that exact version.

Local repository checks passed and substantive GitHub checks are green, but current tests do not cover the reproduced symlink/redaction cases. [code]smith is skipped and was not used as evidence.

weekbin added a commit to weekbin/MiniMax-Code-Plugins that referenced this pull request Sep 21, 2026
…ntial forms, gate the panel on a capability

Addresses the review on MiniMax-AI#56. Every finding was reproduced before the change and
re-checked after it; the new suites are written so that "refuse everything" cannot
pass them.

Symlink root escapes (tasks.mjs, jsonl.mjs). Every file read now goes through
`containedRealPath` / `openContainedRead` in fsutil.mjs and has to canonicalize
inside the data directory, with a non-symlink final component opened under
O_NOFOLLOW so the containment decision and the read describe one inode. Three
separate defects were reproduced: a symlinked task directory, a symlinked task
root, and a symlinked session directory. The session level of the JSONL walk had
no `isDirectory()` guard at all, and the `stat()` before `isSymbolicLink()` was
dead code, because stat resolves the link it was asked to detect.

Redaction missed the encodings credentials take on disk. The key/value rule
required the key and its separator to be adjacent, so `{"api_key":"…"}` never
matched, and the Authorization rule ran after the key/value rule, which consumed
`Authorization` and left the token of `Authorization: Bearer …` in the clear. The
rules are now ordered most-specific-first and shape-preserving, task descriptions
and commands are redacted at the data source, a session title has only its
credential substrings replaced, and every outbound payload is swept once more at
the boundary so a field added later cannot escape. The browser run caught one more
defect: key-name redaction matched `sessionId`, so `/api/sessions` returned
`"[redacted]"` and every following request 404'd.

The panel's fences were CSRF defences, not authorization. Any local process could
send the fixed header and read every session's tool arguments. A per-process
256-bit capability is now minted on each start, carried in the URL fragment (so it
is never sent to the server, logged, or leaked through Referer) and required,
constant-time, on every API route. mcode spawns one MCP server per session, so two
sessions get two panels with two capabilities and neither URL opens the other.

`trajectory_studio` declared `readOnlyHint: true` while opening a listener. The
annotation is corrected, and the port file is gone rather than hardened: once the
capability is per process, reusing a previous process's port is semantically wrong
(the old token is gone and the new one was never sent to the old page), and
removing the write removes the symlink hazard. The Plugin now creates no files.

The Node range was overstated. Measured across nine releases: `node:sqlite` needs
22.13.0, and FTS5 is not monotonic in the Node version - absent in 22.13.0 through
22.18.x and throughout 23.x, present from 22.19.0 and 24.0.0. The floor is now
enforced with a readable message instead of a module-resolution crash, the verified
range mirrors mcode's own `engines` field (`>=22.19 <23 || >=24 <27`), FTS5 is
probed rather than inferred, and search degrades with a warning where it is absent.
Found while measuring: SQLite needs a writable *directory* even for a read-only
open, because WAL needs its `-shm` file - now documented as a limitation.

Also, from the same review points: the panel binds `127.0.0.1` as a non-overridable
constant and refuses a non-loopback peer address; the client builds every node with
`createElement` and writes only `textContent`, guarded by a mutation-checked scan
for markup injection sinks; and the Node minimum is stated identically in both
manifests and both READMEs.

Tests: 105 in the Plugin suite, 462 repo-wide, 0 failures. The Plugin suite passes
105/0/0 on 22.19.0, 22.21.1, 24.0.0, 24.16.0 and 24.19.0, and 104/0/1 skipped on
22.13.0, 22.15.0, 23.4.0 and 23.11.0, the single skip being the FTS5 search test.
Browser evidence for the render layer is in `tools/panel-e2e.mjs`.
weekbin added a commit to weekbin/MiniMax-Code-Plugins that referenced this pull request Sep 21, 2026
…ntial forms, gate the panel on a capability

Addresses the review on MiniMax-AI#56. Every finding was reproduced before the change and
re-checked after it, and the new suites are written so that "refuse everything"
cannot pass them.

**Symlink root escapes** (`tasks.mjs`, `jsonl.mjs`). Every file read now goes through
`containedRealPath` / `openContainedRead` in `fsutil.mjs` and has to canonicalize
inside the data directory, with a non-symlink final component opened under
`O_NOFOLLOW` so the containment decision and the read describe one inode. Three
separate defects were reproduced: a symlinked task directory, a symlinked task root,
and a symlinked session directory. The session level of the JSONL walk had no
`isDirectory()` guard at all, and the `stat()` before `isSymbolicLink()` was dead
code, because `stat` resolves the link it was asked to detect.

**Redaction missed the encodings credentials take on disk.** The key/value rule
required the key and its separator to be adjacent, so `{"api_key":"…"}` never
matched, and the Authorization rule ran after the key/value rule, which consumed
`Authorization` and left the token of `Authorization: Bearer …` in the clear. The
rules are now ordered most-specific-first and shape-preserving, task descriptions and
commands are redacted at the data source, a session title has only its credential
substrings replaced, and every outbound payload is swept once more at the boundary so
a field added later cannot escape. One defect the browser run caught and no unit test
would have: key-name redaction matched `sessionId`, so `/api/sessions` returned
`"[redacted]"` and every following request 404'd.

**The panel's fences were CSRF defences, not authorization.** Any local process could
send the fixed header and read every session's tool arguments. A per-process 256-bit
capability is now minted on each start, carried in the URL fragment (so it is never
sent to the server, logged, or leaked through `Referer`) and required, constant-time,
on every API route. mcode spawns one MCP server per session, so two sessions get two
panels with two capabilities and neither URL opens the other's panel.

**`trajectory_studio` declared `readOnlyHint: true` while opening a listener.** The
annotation is corrected, and the port file is gone rather than hardened: once the
capability is per process, reusing a previous process's port is semantically wrong
(the old token is gone and the new one was never sent to the old page), and removing
the write removes the symlink hazard. The Plugin now creates no files.

**The Node range was overstated.** Measured across nine releases: `node:sqlite` needs
22.13.0, and FTS5 is not monotonic in the Node version — absent in 22.13.0 through
22.18.x and throughout 23.x, present from 22.19.0 and 24.0.0. The floor is enforced
with a readable message instead of a module-resolution crash, the verified range
mirrors mcode's own `engines` (`>=22.19 <23 || >=24 <27`), FTS5 is probed rather than
inferred, and search degrades with a warning where it is absent. Found while
measuring: SQLite needs a writable *directory* even for a read-only open, because WAL
needs its `-shm` file, now documented as a limitation.

**Render layer, unchanged but now guarded.** The client already built every node with
`createElement` and wrote only `textContent`; a mutation-checked scan now fails the
suite if that ever stops being true, and the panel binds `127.0.0.1` as a
non-overridable constant and refuses a non-loopback peer address.

**New evidence.** `test/protocol.test.mjs` spawns the real MCP server over stdio the
way mcode does and drives it as a client: version negotiation, the seven declared
tools and their annotations, a call against a fixture projection, full detail redacted
on the wire while the payload survives, an unknown tool and an unknown method, a
notification left unanswered, `--doctor`, and the lifecycle — closing stdin has to end
the process *with the panel running*, because a listener left behind leaks a port on
every session. Every wait is bounded, so a server that stops answering fails the suite
instead of hanging it. `tools/fixture.mjs` now owns the runtime-shaped projection that
the domain tests, the protocol suite and the E2E harness share, and
`tools/compat-matrix.mjs` asserts the compatibility table rather than printing it
(fail === 0; skip count matching whether this runtime's SQLite has FTS5; every skip
being the FTS5 search test). It is mutation-checked: an injected failure and an
injected unrelated skip both make it exit 1.

**Cross-platform.** This suite had never run off Linux. Its first run on Windows and
macOS found three platform assumptions, all of them in the tests rather than the
Plugin: a temporary path compared before canonicalization (`/var` on macOS, a short
`RUNNER~1` path on Windows), a POSIX-only assertion that a resolved data directory
comes back verbatim, and a temporary directory removed while SQLite still held the
file open (`EBUSY` on Windows, harmless on POSIX — it cascaded through 24 tests).
All three are fixed; both platforms now report 115 tests, 0 failures.

No workflow is added to this repository: what runs on the project's runners is the
maintainers' call. The job definitions are written out in the pull request instead.

Tests: 115 in the Plugin suite, 472 repo-wide, 0 failures. Verified on Node 22.12.0
(refused with a reason), 22.13.0, 22.15.0, 22.19.0, 22.21.1, 22.23.2, 23.4.0, 23.11.0,
24.0.0, 24.16.0, 24.19.0 and 24.20.0, with the outcome matching the documented range
on each. Version 0.1.1.
@weekbin
weekbin force-pushed the feat/weekbin-mcode-trajectory-studio branch from 6aed44f to 9d5b6a6 Compare September 21, 2026 03:03
@weekbin

weekbin commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the review, and for doing the reproduction work rather than just naming the files — the symlink cases in particular were reproduced exactly as you described, and the two jsonl.mjs ones were worse than the report suggested: the innermost session level had no isDirectory() guard at all, and the stat() before isSymbolicLink() was dead code, since stat resolves the link it was asked to detect.

All five findings are fixed in a single follow-up commit, 9d5b6a6. Nothing outside the Plugin directory is touched.

1. Symlink root escapes. Every read now goes through containedRealPath / openContainedRead in fsutil.mjs: the path is canonicalized (both sides, so a data directory reached through a symlink is not mistaken for an escape), it has to land inside the canonical data directory, the final component may not be a symlink, and it is opened under O_NOFOLLOW with the size read back by fstat — so the containment decision and the read describe the same inode. Containment is measured against dataDir, not against background-tasks, which also closes the case where the root itself is a link. test/containment.test.mjs adds the real escape canaries you asked for: a canary outside the root, reached for through a symlinked task directory, a two-hop link, a relative link, a symlinked output.log, a symlinked task root, a symlinked session directory and a symlinked messages.jsonl — each asserting the canary is absent and that the read reports unavailable, with a positive control so "refuse everything" cannot pass.

2. Redaction. The root cause was two bugs, not one. The key/value rule required the key and its separator to be adjacent, so {"api_key":"…"} never matched at all (the examples in the report happened to be saved by the provider-prefix rule, which is why it was easy to miss); and the Authorization rule ran after the key/value rule, which consumed Authorization and left the token in the clear — that is where Authorization=[redacted] <token> came from. The rules are now ordered most-specific-first and shape-preserving, so the output stays valid JSON. Task descriptions and commands are redacted at the data source rather than at each consumer, session titles have their credential substrings replaced in place while the rest of the title survives, and every outbound payload is swept once more at the boundary so a field added later cannot escape. One extra defect turned up while checking the result in a real browser and is now pinned by a test: key-name redaction was matching sessionId, so /api/sessions returned "[redacted]" and every following request 404'd.

3. Panel authorization — you were right that this was the most serious one. The Host/Origin/Sec-Fetch-Site/fixed-header set is a CSRF fence and I had written it up as if it were an authorization boundary. It now mints a 256-bit capability per process, carries it in the URL fragment (so it never reaches the server, never lands in a request log, and never appears in the Referer of an asset request), and requires it on every /api/* route with a constant-time comparison. The fixed x-trajectory-client header is gone. mcode spawns one MCP server per session, so two sessions get two panels with two separate capabilities and neither URL opens the other's — that is asserted directly, along with "the old fixed header alone is now 403", "a wrong-length value is refused without throwing" and "a repeated header is refused rather than compared".

4. readOnlyHint. Corrected. I did not harden the port file — I removed it. Once the capability is per process, reusing a previous process's port is semantically wrong: the old token is gone and the new one was never sent to the page that is open, so the reused port would serve a document that cannot authenticate. Deleting the write also deletes the symlink hazard rather than guarding it, and the Plugin now creates no files at all (asserted by a test that points PLUGIN_DATA at a temp directory and checks it stays empty). The tool description no longer implies the call is free of side effects.

5. Node compatibility. Measured rather than reasoned about, and it is more interesting than "22 or newer". node:sqlite needs 22.13.0. FTS5 is not monotonic in the Node version: absent in 22.13.0–22.18.x and throughout 23.x, present from 22.19.0 (SQLite 3.50.4) and 24.0.0 (3.49.1) — 23.4.0 bundles an older SQLite (3.47.1) than 22.19.0 does. The floor is now enforced with a readable sentence instead of a module-resolution crash, the verified range mirrors mcode's own engines (>=22.19 <23 || >=24 <27), FTS5 is probed rather than inferred from the version, and trajectory_search degrades to no matches plus an explicit warning where it is absent rather than throwing. Both manifests and both READMEs state the same two numbers. On the same pass I found that SQLite needs a writable directory even for a read-only open (WAL needs its -shm file), which is now documented as a limitation.

Extra tests. test/protocol.test.mjs spawns the real server over stdio the way mcode does and drives it as a client — version negotiation, the seven tools and their annotations, a call against a fixture projection, full detail redacted on the wire while the payload survives, unknown tool, unknown method, an unanswered notification, --doctor, and the lifecycle: closing stdin has to end the process with the panel running, because a listener left behind leaks a port on every session. Every wait is bounded, so a server that stops answering fails the suite rather than hanging it. The Plugin suite is 115 tests, 472 repo-wide, 0 failures.

Two things worth flagging while you re-review:

  • [code]smith is skipped on this repository (it reports conclusion: skipped with "not active on this PR" on every open PR, so it is not specific to this one), which is why the compatibility and cross-platform evidence is spelled out explicitly instead of being implied by a check name.
  • That cross-platform run was the first time this suite had ever executed off Linux, and it found three platform assumptions in the tests, not the Plugin: a temporary path compared before canonicalization (/var on macOS, a short RUNNER~1 path on Windows), a POSIX-only assertion that a resolved data directory comes back verbatim, and a temporary directory removed while SQLite still held the file open (EBUSY on Windows — it cascaded through 24 tests). Fixed in the same commit; both platforms now report 115 tests, 0 failures. I mention it because the previous round claimed macOS and Windows were verified by hand, and this is what that hand-run had not covered.

Happy to split the commit, rename anything, or drop tools/ out of the shipped package if you would rather it stay test-only.

@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 9d5b6a6a8da7ae32f2cbdf5eb32d9ef308fcf627.

The capability token, loopback bind, readOnlyHint correction and Node SQLite floor are substantive improvements, but confidentiality/resource blockers remain:

  1. plugins/weekbin/mcode-trajectory-studio/server/redact.mjs:135,167 only recognizes a narrow exact key set. Structured payloads containing clientSecret, refreshToken, accessToken, authToken, privateKey, apiSecret or xApiKey can pass through because value-only redaction has no key context. Add canary tests through both HTTP and MCP full-detail egress.
  2. /api/overview returns stats.workspaceDir from server/stats.mjs:34-41 through server/http.mjs:399-415 without the redactPath() applied to other workspace fields, leaking the absolute user path. Redact it and add an HTTP regression.
  3. Full event responses have only per-string limits; http.mjs:370-392 can return up to 1000 events and redact.mjs:181-183 uses an effectively unlimited entry budget, with no total byte/depth bound. Add a bounded aggregate response contract for HTTP and MCP.
  4. server/jsonl.mjs:79-91 appends chunks to buffer before checking the 2 MiB line limit. A single unterminated line can grow without bound until EOF. Enforce an incremental buffer cap and test a large no-newline record.
  5. server/fsutil.mjs:77-92 realpaths before opening; O_NOFOLLOW only protects the final component, leaving an intermediate-directory replacement race despite the no-window claim. Either implement directory-fd/openat-style containment or narrow the documented guarantee and test the accepted threat boundary.

These are not closed by the currently green repository/CodeQL checks. [code]smith is skipped and is not evidence.

@weekbin

weekbin commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the second round — the jsonl.mjs and fsutil.mjs items in particular, and for reproducing rather than naming. Everything below is what the Plugin does now; each item was reproduced before the change and re-checked after it.

File reads. Every read canonicalizes each traversed component, has to land inside the canonical data directory, may not have a symlinked final component, is opened under O_NOFOLLOW, and on Linux has the opened descriptor re-verified through /proc/self/fd — so the containment decision and the read describe one inode, and an intermediate directory swapped between canonicalization and open is refused. The cases you described are canaried with positive controls, so "refuse everything" cannot pass: a symlinked task directory, a symlinked session directory, a symlinked messages.jsonl, a symlinked task root, a two-hop link and a relative link. Where /proc is absent that residual race is stated as an accepted boundary rather than implied away.

Redaction. Rules run most-specific-first and preserve shape, and they now cover the form credentials actually take on disk. The runtime stores tool results as JSON text: in a real projection, 109,462 of 118,109 message rows carry tool_call_result_data as a string, so a credential arrives escaped — {\"api_key\":\"…\"} — which a rule that needs the key and its separator to be adjacent never sees. A string value that parses as JSON is walked as structured data (and rewritten only when a redaction actually fired, so JSON tool results are not reformatted to protect nothing), and the key/value rule tolerates and re-emits any backslash run on either delimiter. Measured through the shipped MCP path on that projection: the escaped pairs whose key names a credential go from none redacted to 54 pairs, 0 surviving. Structured keys are judged by word-boundary containment rather than an exact list, so clientSecret, refreshToken, accessToken, authToken, privateKey, apiSecret and xApiKey are in, while counters (inputTokens) and the addressing identifier (sessionId) are deliberately out — treating a session id as a secret returns "[redacted]" from /api/sessions and breaks every following request. A task's description and command are redacted at the data source, stats.workspaceDir is folded beside session.workspaceDir, and the rules are idempotent, asserted as a property over the corpus rather than by inspection, because text is swept at the source, per record and again at the boundary.

E-mail addresses and phone numbers are masked on the MCP egress only — that output reaches a model context, while the panel is the reader's own screen. The home directory, the data directory and operator-configured roots fold to ~ wherever they appear, including the doubled-backslash form a Windows path takes inside a JSON column, and another account's home keeps its shape and loses the name.

Bounding. Per string, per response, and per row. Per response: an event page is trimmed to an aggregate byte budget and reports truncated/omitted so the client pages on nextOffset, and MCP's record-list budget is half because MCP emits the payload twice (text and structuredContent). Per row: the SQLite read has an 8 MiB data_json ceiling whose test runs inside SQL, so one oversized row comes back as oversized with its byte count instead of being materialised. The git rev-parse probe receives an exact environment allowlist plus fixed -c core.fsmonitor=false -c credential.helper= rather than the whole process.env, and diagnostics are capped at 64 entries with the dropped count reported.

The JSONL line cap. Enforced per chunk rather than after appending. Measured inside a copy of the module with three lines of instrumentation: the pending buffer peaks at 2.06 MiB — the cap plus one chunk — for a 10, 100 and 400 MiB stream, so retention no longer tracks the file. Re-verifying that turned up one remaining defect, in that same code: an oversized line whose newline arrived in the same chunk as the byte that crossed the cap was discarded without being counted, because only the pending-buffer path incremented droppedOversized. Which path a line took — counted or silent — depended on nothing but where the chunk boundary fell. Both paths count now, they cannot double count (the discarding branch consumes the overflowing line's newline and returns before the per-line test), and a test asserts the two deliveries agree: same bytes, same folded output, same count. Worth noting for the record: my first pass flagged this area as still broken, and that flag was wrong — it was a defect in my own reproduction's line shape. Correcting my reproduction is what exposed the counting gap above.

The panel. A 256-bit capability per process, carried in the URL fragment (never sent to the server, so never logged and never in a Referer), required with a constant-time comparison on every /api/* route; the old fixed header is gone and is now one of the rejected cases. Host, Origin and Sec-Fetch-Site remain as depth. The panel opens in summary detail — the markup shipped the toggle checked while the state defaulted to full, so it fetched content the control said it was not fetching — and the control is derived from the state so the two cannot drift. Assets stay under a deny-by-default CSP with no-referrer, every node is built with createElement/textContent, and a mutation-checked scan fails the suite if a markup sink is introduced.

Node compatibility. node:sqlite needs 22.13.0, and FTS5 is not monotonic in the Node version — absent from 22.13.0 through 22.18.x and throughout 23.x, present from 22.19.0 and 24.0.0 — so those are two numbers rather than one. The floor is enforced with a readable sentence, the verified range mirrors mcode's own engines, FTS5 is probed rather than inferred, and both manifests state the same numbers.

Documented limits, in README.md under "Known limits" and in DESIGN.md §6.6: the redactor recognises credential shapes and credential-named keys, so a high-entropy string with no label and no provider shape is not a credential to it; PII masking covers e-mail addresses and phone numbers only, and only on the MCP egress; an absolute path under a root the operator has not configured is returned verbatim; and the panel capability does travel into the session transcript with the tool result, because the runtime persists tool results and sends the context on later turns — it is contained by being process-scoped and loopback-only, and server/main.mjs --serve keeps a capability out of a model context entirely.

Tests: 166 plugin, 523 repo-wide, 0 failures; tools/compat-matrix.mjs asserts 166 pass / 0 skip on this runtime. The browser checks behind the render claims are in tools/panel-e2e.mjs. Happy to split the change, to drop the escape-aware tier if you think it is more machinery than the problem warrants, or to answer anything the reproduction steps above do not cover.

@weekbin
weekbin force-pushed the feat/weekbin-mcode-trajectory-studio branch 2 times, most recently from ef28dc3 to 4dd8dd4 Compare September 22, 2026 04:14

@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 4dd8dd4af37cf2271fa3f071622801ed8461373c.

The composite credential redaction, overview workspace-path masking, no-newline JSONL cap, and documented containment boundary are materially improved, but current-head regressions remain:

  1. /api/overview fails with 500 internal_error for sessions with a non-empty agent definition. server/http.mjs:427 defines overview() without homeDir/redactRoots, while the agent branch references those undefined variables at :443-445. Add a populated-agent fixture and pass the required context explicitly.
  2. Aggregate event trimming returns the underlying page nextOffset rather than the last returned record (server/http.mjs:408-419, server/mcp.mjs:238-253). When a 1000-record page is cut after 209 records, nextOffset=1000 permanently skips 209-999; the web client uses it directly (web/js/flow.js:113-125). Derive the cursor from delivered records and test lossless pagination.
  3. The claimed SQLite byte cap uses length(data_json) (server/events.mjs:83-95), which counts TEXT characters, not UTF-8 bytes; multibyte rows over 8 MiB are fully parsed/returned. Enforce byte length and fix tests that use JavaScript character count as bytes.
  4. JSONL fallback has no offset and silently reports more-than-1000-record files as complete (server/jsonl.mjs:61-73,117-120; HTTP/MCP callers). Expose a real continuation or explicit truncation/omission state.
  5. Aggregate bounds cover event lists only. trajectory_tasks can return up to 2000 full tasks without boundPayloadList (server/mcp.mjs:122-130,271-282), producing a roughly 32 MB frame. Apply the same total-frame budget across record-list tools.

Exact-head repository/plugin tests and CodeQL are green, but existing fixtures miss these paths. [code]smith is skipped and is not evidence.

@weekbin

This comment was marked as outdated.

1 similar comment
@weekbin

This comment was marked as outdated.

@weekbin
weekbin force-pushed the feat/weekbin-mcode-trajectory-studio branch from 19b9aa2 to 992dd82 Compare September 28, 2026 08:52
@weekbin

weekbin commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Consolidating the two interim replies above, which are collapsed: this is the whole response to the third review, and the branch head is now 1857360.

The five findings

Each was reproduced on the reviewed head before anything was changed, and the new tests were then run against 4dd8dd4 to confirm they fail there — 11 failures, one per defect, so none is a test that passes either way.

1. /api/overview 500. overview() is a module-level function and its agent branch swept the system prompt with homeDir/redactRoots, names that exist only in the enclosing api(). So the failure was a ReferenceError, not a null: 500 internal_error, for exactly the sessions with a non-empty agent systemPrompt. It survived two rounds because every fixture stubbed getAgentDefinition: () => null, and a null definition skips the branch that reads the missing names. The context is now an explicit argument.

2. Pagination that skipped records. The cursor echoed the store's nextOffset, which counts records read; the budget counts records delivered. A 1000-record page cut to 209 answered nextOffset: 1000, so every client skipped 209-999 permanently — the bound was working and the session was still incomplete. The cursor is now offset + delivered whenever the list was trimmed, on both surfaces, and a test walks the real cursor over a 1000-record store asserting the union of pages is exactly 0-999.

Two consequences I settled rather than left implicit: the budget is soft for one record (a page that fits nothing would re-request the same offset forever; that record has already passed the per-string bounds, so the overshoot is bounded by them), and truncated is true when the budget was exceeded, not only when records were dropped — otherwise a reply breaking its budget by an order of magnitude describes itself as whole. A value JSON cannot serialise is now dropped and counted rather than returned into a reply it would break.

3. The byte cap counted characters. length() on a TEXT column returns characters, so a row of 4-byte code points passed the ceiling at a quarter of its real size and was parsed whole. Now length(CAST(data_json AS BLOB)) — the stored byte length, and for a blob a record-header read rather than a scan. The test fixture is one where the two provably disagree: 1107 code points, 4107 bytes, against a 2048-byte cap. Worth noting the previous assertion used JavaScript's UTF-16 length, which agreed only because the fixture was ASCII.

4. JSONL continuation. The fallback re-read from the top and the caller discarded the cursor, so a 5,000-record session returned one 1,000-record page and reported itself complete. It now pages on the same offset, with one record of look-ahead to distinguish "the file ends here" from "there is more", and total: null because the fold deliberately does not count the file. Lines dropped at the per-line cap now reach the client as droppedOversized — the store always counted them and both routes discarded it.

5. Record-list bounds, uniformly. Every tool returning records goes through the same frame budget (trajectory_get, trajectory_tasks, trajectory_list, trajectory_search); limit: 2000 of full tasks is roughly a 32 MB frame before structuredContent doubles it. I extended the same bound to the panel's overview task list, which had the identical defect one route over.

What I added beyond the fixes

A test per defect stops that defect. It does not stop the next one reaching review the same way, and the reason these five escaped is structural: a stub store can express getAgentDefinition: () => null, so the /api/overview defect was unreachable from every fixture in the repository while all of them were green. The shared fixture has carried the local_runtime_session_agent_definitions table since it was written and had no way to put a row in it.

  • tools/fixture.mjs grows an agent() helper. systemPrompt defaults to non-empty, because an empty one takes the same branch-skipping path as a null definition.
  • test/egress-contract.test.mjs contains no stubs: real projection, real Store, all seven tools and every route, with structural assertions — no route 500s on a populated session, every record-list tool reports truncated/omitted, the cursor names the first undelivered record.
  • tools/release-gates.mjs checks what no behavioural test can see, because the property is the relationship between code, docs and shipped surface: version coherence across the four places a version is declared; the manifests' Node floor and verified range against what node-version.mjs enforces; a byte ceiling written as length() on TEXT; documentation quoting an implementation the code no longer has; and every record-list tool declared a list or a single object, so a new tool forces the decision trajectory_tasks never got. Each gate names the defect it prevents.
  • tools/panel-e2e.mjs --smoke makes the manual browser harness judgeable over a real loopback socket. The interactive mode is unchanged — it holds its fixture until SIGINT, which is why it could not be the CI step itself.

What the CI run then found, which is the argument for it

.github/workflows/mcode-trajectory-studio.yml is paths:-scoped to this Plugin. Its first honest run failed, and it found four things no hand-run had:

  1. The workflow file was never parsed at all. persist-credentials was a step-level key; it is an action input, so the parser rejected the file — (Line 59, Col 9): Unexpected value 'persist-credentials' — and every run came back with zero jobs. It belongs under with:.
  2. Windows containment assertions were testing the path separator. isWithin compares a prefix plus path.sep, which is \ on windows-latest, so hard-coded /base/data/ok.log could never start with /base/data\. Fixtures are now built with path.resolve/path.join; neither assertion changed meaning.
  3. Windows EBUSY across nine tests. after hooks run in registration order, so a removal registered ahead of the store's close unlinks the projection while SQLite still holds it. Linux unlinks an open file happily, which is why it passed. One of the nine predates this branch — the previous round fixed this class in most of the suite and left one behind.
  4. A failing release could not be diagnosed from its own log. On Node 22.13.0 the gate printed "1 test(s) failed" and a 40-line tail that had already scrolled past the name. It now prints every not ok with its detail — verified by deliberately failing a test and reading the name back. That is how the last EBUSY hook was located, in its multi-line form, which the first sweep had missed.

The gates also caught three defects in this tree while being written: trajectory_search had neither a bound nor the truncated/omitted contract, the SKILL's version was not being read at all, and DESIGN.md still described the character-count SQL as a byte bound.

A separate flake, since it is a failure mode rather than a finding: the heaviest protocol call took 10.8 s on a two-CPU runner against a flat 10 s budget, so it reported a timeout and the property it verifies never ran. The fixture is now 150 records instead of 400 (~1.5× the frame budget rather than 4×, which is all the property needs) and the budget is 30 s.

Then the gates went green, and the Plugin was still wrong

This is the part I did not expect to have to write. Once all seven jobs were passing I audited the Plugin on its own terms — not against your findings, which are addressed above, but against the one property this Plugin claims for itself: it reports what it actually read. Gates test what I thought to write down. On this surface a defect is not a crash, it is a confident wrong number, which is exactly the shape that survives a green suite.

The first thing that audit found was a P0 I had introduced in the same round, in the performance work:

A projection-time string clip made redaction fail open. To stop the sweep walking a megabyte of text that would be cut on the way out anyway, I clipped projected fields before the redaction sweep. A cut landing inside a quoted value deletes that value's closing delimiter, and all three redaction layers fail open together: redactJsonText cannot JSON.parse the unterminated document, the escaped-document rule needs its closing delimiter, and the key/value rule's unquoted branch excludes ". A read or bash result over 256 KiB carrying api_key inside a JSON object — the runtime's normal storage shape, at a routine size — was delivered unredacted, through the one surface whose output reaches a model. At 4dd8dd4 the same payload redacts correctly, and the boundary sweep at egress does not catch it either, because the sweep sees the same broken structure.

The comment I wrote beside that clip reasoned about the failure mode and got it wrong. It considered "a secret cut in half" and concluded the prefix still matches; it did not consider that the cut removes the delimiter, which is what every rule needs.

It is removed rather than repaired. The work it was bounding is bounded by eventJsonBytes per row and eventPageBytes per page, and both already report themselves. Measured, both ways:

with the clip without
twenty 1 MiB records 350 ms 1.1 s
2000 records of ~4 KB 816 ms 811 ms

The shape that actually occurs does not move — those fields never reached the 256 KiB clip. The pathological shape goes from 350 ms to 1.1 s, against the 6.4 s the page budget alone once cost. Two further consequences went with it: the cut was invisible whenever redaction shrank a field below the egress bound, and the truncation marker counted omissions from the clipped string, under-reporting by about 4×. A 500,000-char value reported "truncated 242144 chars" when 480,000 had gone; it now reports 480,000.

The audit's test for that clip was itself defective, which is worth naming because it is the second time on this branch a passing test asserted nothing. It placed its canary past the cut and asserted the credential was absent — true, because it had been cut, not redacted; it passed with the redaction sweep deleted outright. Its replacement uses a canary with no token prefix, because ghp_… and sk-… are caught by a shape rule that needs no key/value structure and is therefore redacted even when the clip has destroyed it.

Eight P1s followed, all the same shape — a surface answering confidently beside one holding the truth:

  • The page byte budget was set on the store and dropped by both egresses, so a reply carrying 7 of 20 records answered truncated: false, omitted: 0. It is its own field now, because omitted: 0, truncated: true reads as a contradiction.
  • The artifact fallback keyed on "this page came back empty", which is also what a turn filter matching nothing looks like — so a narrowed question was answered from a second source, with that source's total. I want to be precise about what this was and was not: it did not lose records, because the projection is built from the artifact and offset n names the same record in either. What it did was let one cursor walk change source halfway through, and answer two questions about one session from two places. It keys on the session's own row count now.
  • The turn-filtered read built its predicate against the raw table, so one truncated write made a whole session unreadable by turn — the same "empty trajectory beside a full event list" shape the earlier normalisation was added for, reached through a different door.
  • turnId had two states and needed three. undefined is no filter; null and '' are the rows carrying no turn identity, which the turn fold reports as a group and which a caller holding that id back is asking about. Both fell through one if (turnId) and returned the entire session.
  • stats.turns dropped that group while the fold's GROUP BY kept it, so the same rows read as turns: 1 beside three turn cards.
  • getTimeline().total was the delivered count under a name that reads as the session's size. No such count exists — the query never read past the cap — so the field is gone and truncated is the answer.
  • The four per-session folds were cached against the session row's updated_at_ms, which the runtime does not touch while it indexes: a session that gained a record kept serving the statistics, turn summaries, timeline and task index it had before, and a task moving from running to completed kept its old status and duration. They are keyed on the rows they fold now. invalidateSession existed to break exactly that, was never called from anywhere, and is gone rather than left reading as protection.
  • A capped task index, a capped overview task list, a capped child list and an unparseable artifact line each dropped data and said nothing. Each reports itself now.

test/read-consistency.test.mjs grows thirteen cases. Ten fail against b3adc72; the other three are coverage for behaviour that was already right, and each was confirmed by mutation — ignoring the task offset, letting '' resolve to the newest session, and dropping the camelCase branch of TURN_KEY_SQL each break exactly one.

The panel was throwing the answers away

The server reported every cut above, and the client read four disclosure fields and rendered none of them. state.eventsTotal was assigned on every page and read nowhere, so a session with 7 of 20 records loaded said "7 行". The timeline's truncated was dropped at the client, so 6,000 points were drawn for a 7,000-record session with nothing on screen saying so — and a failed timeline request was turned into an empty axis by a .catch(() => []). The warning channel was read once during boot, before any warning existed, so every warning raised by the first session a reader opened reached nobody.

All of it is shown now. And tools/panel-e2e.mjs --smoke, which was the evidence for the panel, never loaded web/js/ at all: it fetched a page, checked headers and grepped two responses. Reverting the four client files to their pre-fix form left it at 8/8 and the suite at 203/203 — the race guards this Plugin shipped had no automated protection whatsoever. test/panel-dom.mjs is a hand-written DOM and fetch seam that runs the real browser modules; test/panel-flow.test.mjs drives them. Each of the six guards was reverted in a scratch copy and watched fail, and the smoke harness passed under every one of them, which is the blindness now demonstrated rather than asserted. The printed check count is the tally that decides the exit code, with a floor, so a deleted check fails rather than renumbering.

Two gates that could not fail

The zero-dependency gate was one regex over one file. It passed a package.json declaring lodash and left-pad, four ways of reaching a package that are not a static import, and any dependency in three of the four files in tools/. It reads the whole Plugin now, and its test proves each bypass fails. Two gaps turned up while checking that result rather than taking it: assigning the loader to a name and calling it on a later line put a statement boundary the chained pattern could not cross, and the first version of the fix for that reported join(", ") and a local function name as packages.

The test counts in the documentation were hand-kept and had drifted — 188 in two README rows, 166 in the translation, 202 in DESIGN, against 236 cases. A gate compares them now, computing the count by parsing test/*.test.mjs rather than running the suite, so it costs 15 ms and cannot recurse into itself. The same round corrected claims that were wrong rather than merely stale: the README said this Plugin adds no workflow, in the same commit range that added one; DESIGN's tool table listed five of the seven tools and named two parameters that do not exist; a duplicated table row sat after the table it belonged to.

Evidence

1857360 — github-hosted run, 7/7 green:

Job Result
contract gates (ubuntu, node 22) pass — 7 release gates, egress contract, panel smoke
suite ubuntu / macos / windows (node 22) pass
engines node 22.13.0 (floor, no FTS5) pass — 0 fail, 1 permitted skip
engines node 22.19.0 / node 24 pass

Locally: 236 plugin / 594 repo-wide on Node 24; 236 plugin, 0 fail, 1 skip on Node 22.13.0, downloaded for the purpose. Node 22.19.0 is covered by the matrix and was not run here.

What I would rather you judge than accept

The panel's tests run against a hand-written DOM, not a browser. No engine rendered the notices or the CSS. The DOM is faithful enough to drive the real modules and to fail when a guard is reverted, and that is the property I wanted from it; it is not a rendering test, and the layout is unverified by anything here.

The gates are stronger than the tests they sit beside and weaker than they sound. The zero-dependency check cannot follow a loader hidden behind a function that takes a computed name, and its one heuristic can still report a short lowercase argument in a file that has introduced a loader. The count gate is scoped to this Plugin's own suite by design — the repository-wide number belongs to the repository's gate, so it is corrected here but not enforced here.

One of the six panel guards is covered only incidentally. Reverting the page-token check alone, with the AbortSignal left in place, fails exactly one test rather than a test written for it. The other five guards fail under a revert on their own. I looked for a test that isolated it and could not write one that was not dependent on microtask ordering, so rather than ship a fragile one the evidence is recorded on the test that does catch it.

A workflow file introduced by a PR is not executed for that PR, so on this PR these jobs cannot gate until the file lands on main — the green run above is captured by dispatch from the fork, the route tool-map-windows.yml already documents. And record-list-egress's classification of trajectory_summary as a single-object tool is a decision written in the gate, not a property of the code; if you would rather it were a list, the gate should change.

[code]smith remains skipped on this repository and is not evidence — the verification above is the tests failing on 4dd8dd4 and on b3adc72, the mutations reverted one at a time, and passing on 1857360.

@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 1857360b4c8db293589dd77d166429001e06e540.

Several prior issues are fixed, but data-loss/reporting blockers remain:

  1. trajectory_tasks trims a page to the frame budget, then advances using the database page length rather than delivered records (server/mcp.mjs:360-371). A reproduced 802-task page returned 252, omitted 550, and nextOffset:null, permanently losing access to omitted tasks. Advance from delivered count and add lossless pagination tests.
  2. JSONL fallback loses corruption evidence: non-final look-ahead pages omit accumulated malformed (server/jsonl.mjs:151-159,285,303), and HTTP/MCP adopt fallback only when it contains a valid event (server/http.mjs:414-419, server/mcp.mjs:275-280). An all-corrupt artifact is reported as an empty SQLite session. Preserve source/malformed/oversized metadata even with zero valid events and expose it in the UI.
  3. Task index look-ahead requests 2001 entries but listBackgroundTasks() clamps to 2000 (server/tasks.mjs:19-28,120-130), so taskIndexTruncated can never become true; old tasks silently lose duration/task IDs.
  4. Overview task omissions are not rendered by the client, and tasksOmitted uses look-ahead row count rather than tasksTotal, under-reporting hundreds of omitted rows (server/http.mjs:498-521, web/js/flow.js:156-158).
  5. Raw task materialization can still load 2000 unbounded descriptions/commands before egress trimming, and the MCP transport lacks required JSON-RPC parse/batch validation/error responses.

Exact-head cross-platform suites, engine matrix, repository validation and CodeQL are green, but they do not cover these reproduced paths. [code]smith is skipped and is not evidence.

@weekbin
weekbin force-pushed the feat/weekbin-mcode-trajectory-studio branch 5 times, most recently from 131013f to 86c04fe Compare October 12, 2026 05:34
…via the runtime SQLite projection

A Plugin that answers questions about mcode's own sessions: which sessions exist, what
each one was for, how far it got, what the user actually asked for, what failed, and what
a background task captured. Read-only, over `~/.minimax/v2/sqlite/runtime-state.sqlite`
with the per-session `messages.jsonl` as a fallback. 13 MCP tools plus a loopback-only
web panel, zero runtime dependencies, no build step.

This commit is the plugin and everything four review rounds and a mutation sweep changed
in it. The rounds, in order, each closing reproduced defects rather than named ones:

1. Symlink escapes through task and session directories; credential redaction missing the
   forms the runtime actually stores (JSON text, escaped JSON, a key and its separator with
   a quote between); the panel's Host/Origin/Sec-Fetch-Site set described as authorization
   when it is a CSRF fence; `readOnlyHint` wrong on the one tool that starts a listener;
   and a Node floor that understated what `node:sqlite` needs.

2. Structured key names (`clientSecret`, `refreshToken`, `xApiKey`) judged by exact match,
   so they passed a value-only sweep; `stats.workspaceDir` reaching the panel unredacted
   beside the field above it that was folded; record lists bounded per string and not per
   response; a JSONL buffer that grew until EOF before the line cap was checked; and a
   documented containment boundary narrowed to match what the code enforces.

3. `/api/overview` throwing `ReferenceError` for any session with a non-empty agent
   definition — a fixture that stubbed the definition to `null` had hidden it; an event
   page trimmed to the byte budget returning the store's own cursor, so records 209–999
   of a 1000-record page were unreachable forever; and a byte ceiling implemented as a
   character count, which multibyte rows walked straight through.

4. `trajectory_tasks` advancing by rows read rather than delivered, so a byte-trimmed page
   answered "no more pages" with hundreds of tasks counted but unreachable; the JSONL
   fallback discarding `malformed` exactly when a page stopped on its look-ahead, and
   never adopting an all-corrupt artifact at all; a task-index look-ahead clamped back to
   its own cap so `capped` could never be true; `tasksOmitted` measured against the read
   window and published beside a `tasksTotal` it contradicted; and a list read that
   materialised 2,000 unbounded descriptions before egress trimming.

After those, the mutation sweep and the interfaces were audited the way a caller meets
them rather than the way they are written:

- Every record list derives its cursor from records **delivered**, counts omissions
  against the **session's own total**, and says which source it read.
- `trajectory_get` grew a `role` filter after measuring the question it opens. A 5,831-record
  session holds 11 user messages — 0.2%. Asking "what did the user say" cost one 3 MB call
  before and 19 KiB after.
- The redaction sweep is 35× cheaper on a 4 MiB response, after one rule was found
  retrying the key separator 64 times per character in text with no separator anywhere.
- `tools/mutation-sweep.mjs` runs all 36 mutation scripts serially and names its failure
  categories, because a tally nobody can reproduce is indistinguishable from a clean one.
  214 mutants, 0 survivors, 0 problems.
- `tools/browser-check.mjs` drives real Chrome over the DevTools protocol; it is kept out
  of the default suite because it needs a browser, and it has caught defects the unit
  tests could not see.

The panel carries a per-process 256-bit capability in the URL fragment — never sent to the
server, so never logged and never in a `Referer` — with constant-time comparison on every
`/api/*` route, and `Host`/`Origin`/`Sec-Fetch-Site` as depth. Every outbound payload is
swept once more at the boundary, so a field added later cannot leave unredacted.

The Plugin's own gates passed a defect the *host repository's* packaging check then rejected:
seventeen `tools/mutate-*.sh` files shipped mode 755. Nothing here could see it, and that is
the point — every script is launched as `bash <path>` and every gate reads file *contents*, so
the bit was inert to all of them. Sixteen of the seventeen predate the round that added the
last one. The ninth gate, `package-plainness`, now `lstat`s every shipped file for an
executable bit and for symlinks, and its two red-path tests were proved red by removing the
gate: a check seen only green is decoration.

CI is scoped to this Plugin: release gates, a stub-free egress contract suite driven
against the real projection, the full suite on ubuntu/macos/windows, and an engine matrix
that asserts the manifest's floor and verified range instead of describing them.
@weekbin
weekbin force-pushed the feat/weekbin-mcode-trajectory-studio branch from 86c04fe to 8e64799 Compare October 12, 2026 05:47
@weekbin

weekbin commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the fourth round. All five items are addressed — but I want to flag before you start that this branch has grown well past what you last reviewed, so the re-review is a materially bigger job than "check these five again".

Your five items

  1. trajectory_tasks advancing by rows read rather than delivered — the cursor now advances from bounded.items.length / the last carried row rather than the database page length (delivered and lastCarried in server/mcp.mjs), with lossless-pagination regression coverage.

  2. JSONL fallback dropping malformed, and an all-corrupt artifact reported as an empty session — malformedLines now travels on the look-ahead stop and on every early return in server/jsonl.mjs, and fallback is adopted on the artifact's existence rather than on it containing a valid event (server/http.mjs, server/mcp.mjs).

  3. taskIndexTruncated unreachable because listBackgroundTasks() clamped the look-ahead to 2000 — the look-ahead now asks for one row past the cap, so truncation is actually observable (server/tasks.mjs).

  4. tasksOmitted measured against the read window and never rendered — now max(0, tasksTotal - delivered) in server/http.mjs, and the panel renders it in web/js/flow.js.

  5. Unbounded task materialisation, and no JSON-RPC parse/batch validation — the task query selects only the columns it needs, extracts them with json_extract, bounds at the SQL LIMIT, and truncates via redactText before egress; the transport rejects non-object and malformed requests with -32600, accepts batch arrays, and filters notifications.

What you should know before re-reviewing

You reviewed 7 tools and 67 files. This is now 13 tools and 161 files.

  • New tools: trajectory_turns, trajectory_find, trajectory_agent, trajectory_assets, trajectory_changes, trajectory_ingress
  • New server modules: server/changes.mjs, server/ingress.mjs
  • 51 new test files, and 36 mutation scripts (214 mutants) that pin the behaviour the suite covers

I'd rather say that plainly than let you discover it halfway through.

Verification on this head (8e64799)

  • 10/10 check-runs green: contract gates, suite on ubuntu/macos/windows, engine matrix (24 / 22.19.0 / 22.13.0), the repository's own validate, and CodeQL
  • mutation sweep: 214 mutants, 0 survivors, 0 problems across 36 scripts
  • 763 declared test cases; 9 release gates
  • MCP driven over the real stdio protocol: all 13 tools answer, both path separators accepted, unknown session returns the documented error contract
  • the web panel driven in real Chrome over the DevTools protocol: 25/25, no console errors

One process note that may save you a step

The 14 commits are now squashed to one, so this reviews as a single-commit diff rather than a series.

Separately: 17 tools/mutate-*.sh shipped with mode 755 and were rejected by the repository's packaging check even though all of our own gates were green. That is fixed, and I added a gate so the class of defect fails locally instead of at the host repo — the instructive part is that every script is launched as bash <path>, so the bit was inert to every behavioural check we had.

Happy to walk through any area, or to split the diff by area if that makes reviewing easier. Thanks again for the rounds so far.

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.

3 participants