Repository navigation
feat(server): project T3 activities onto mindwalk's trace model - #24
Conversation
The 3D surfaces need an ordered stream of file-touch events per thread, and
the citymap needs the paths that stream touched before it can raise ghost
buildings for files that no longer exist. Since each artifact needs the other,
they are built and served together at `GET /api/mindwalk/threads/:threadId`,
in mindwalk's own order: trace with paths only, citymap with those paths as
ghosts, backfill each target's fileId, recount the stats.
The classifier and its ~250 lines of shell-command scraping are ported
near-verbatim from mindwalk rather than reusing `toolActivity.ts`. Bash is 66%
of every tool call in a real store and Grep/Glob never appear at all, so the
scraping is load-bearing: without it `search` would be ~0 and the histogram
would be one undifferentiated `exec` bar. Each provider gets a thin adapter
reshaping an activity into `{ tool, input, result }`; Claude's payload already
is that shape, and Codex's `commandActions[]` gives typed read/search paths
that T3 has been storing and ignoring.
Verified differentially against the real Go binary over the same Claude
session: identical action on 1204/1204 matched events, every one of our 1284
events has an exactly matching target set in mindwalk's trace, and the
per-file touch map agrees on 146/150 paths with none differing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| * Pure string math — it never touches the disk, which is what lets the whole | ||
| * projection degrade gracefully when a worktree has been removed by hand. | ||
| */ | ||
| export function normalizePath(cwd: string, base: string, rawPath: string): NormalizedPath { |
There was a problem hiding this comment.
🟡 Medium mindwalk/TraceScrape.ts:54
normalizePath returns ~/.config/app.ts as an in-repository path (kind: "inside") instead of flagging it as an outside home touch. NodePath.isAbsolute does not expand ~, so tilde-prefixed paths fall through to the relative branch on line 70 and are returned verbatim. Since PATH_ONLY_RE deliberately accepts ~ paths, targetsFor will record these as bogus in-repo targets for any ~-prefixed string scraped from output. Expand ~ (and ~user) to the home directory before normalizing, or treat paths starting with ~ as absolute outside touches.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mindwalk/TraceScrape.ts around line 54:
`normalizePath` returns `~/.config/app.ts` as an in-repository path (`kind: "inside"`) instead of flagging it as an outside `home` touch. `NodePath.isAbsolute` does not expand `~`, so tilde-prefixed paths fall through to the relative branch on line 70 and are returned verbatim. Since `PATH_ONLY_RE` deliberately accepts `~` paths, `targetsFor` will record these as bogus in-repo targets for any `~`-prefixed string scraped from output. Expand `~` (and `~user`) to the home directory before normalizing, or treat paths starting with `~` as absolute outside touches.
| if (program === "sed") { | ||
| // only the read idiom: sed -n '…p' file — anything else can rewrite | ||
| if (!sedReadsOnly(args)) continue; | ||
| scriptArgs = 1; | ||
| } | ||
| let expectValue = false; |
There was a problem hiding this comment.
🟡 Medium mindwalk/TraceScrape.ts:308
commandReadPaths always reserves one non-flag argument as the sed script via scriptArgs = 1, even when the script was already consumed as the value of -e or -f. For sed -n -e '1p' src/a.ts, the loop skips '1p' as the -e value (setting expectValue), then decrements scriptArgs for src/a.ts, so the file path is never emitted. This drops a real read path, causing readCommand to return false for a valid read-only command. Consider initializing scriptArgs to 1 only when the script is positional, and decrementing it instead of setting expectValue when -e/-f supplies the script.
- let scriptArgs = 0;
if (program === "sed") {
// only the read idiom: sed -n '…p' file — anything else can rewrite
if (!sedReadsOnly(args)) continue;
- scriptArgs = 1;
+ let scriptArgs = 1;
}
- let expectValue = false;
+ let expectValue = false;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mindwalk/TraceScrape.ts around lines 308-313:
`commandReadPaths` always reserves one non-flag argument as the sed script via `scriptArgs = 1`, even when the script was already consumed as the value of `-e` or `-f`. For `sed -n -e '1p' src/a.ts`, the loop skips `'1p'` as the `-e` value (setting `expectValue`), then decrements `scriptArgs` for `src/a.ts`, so the file path is never emitted. This drops a real read path, causing `readCommand` to return `false` for a valid read-only command. Consider initializing `scriptArgs` to 1 only when the script is positional, and decrementing it instead of setting `expectValue` when `-e`/`-f` supplies the script.
| } | ||
|
|
||
| /** Go's `strings.FieldsFunc` over the pipeline separators: no empty segments. */ | ||
| function splitSegments(command: string): Array<string> { |
There was a problem hiding this comment.
🟡 Medium mindwalk/TraceScrape.ts:387
splitSegments splits on |, ;, &, and newlines without regard for shell quoting, so a command like rg 'foo|bar' src is broken into rg 'foo and bar' src. searchCommand then sees the bogus second segment, fails to recognize its program, and returns false — so a real search command is misclassified as exec. The same happens with quoted ; or &. Consider tokenizing with a quote-aware splitter so separators inside single or double quotes are preserved within a single segment.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mindwalk/TraceScrape.ts around line 387:
`splitSegments` splits on `|`, `;`, `&`, and newlines without regard for shell quoting, so a command like `rg 'foo|bar' src` is broken into `rg 'foo` and `bar' src`. `searchCommand` then sees the bogus second segment, fails to recognize its program, and returns `false` — so a real search command is misclassified as `exec`. The same happens with quoted `;` or `&`. Consider tokenizing with a quote-aware splitter so separators inside single or double quotes are preserved within a single segment.
| * mindwalk's `cleanExtractedPath`. `allowTopLevel` is false for output text, | ||
| * where a bare word with a dot is far more likely to be prose than a path. | ||
| */ | ||
| export function cleanExtractedPath(rawPath: string, allowTopLevel: boolean): string | null { |
There was a problem hiding this comment.
🟡 Medium mindwalk/TraceScrape.ts:197
cleanExtractedPath strips leading a/ and b/ from every path, so a real command argument like cat a/src.ts is reduced to src.ts. Callers that resolve this against the repo root record the wrong path and typically drop the read as nonexistent, since the actual file lives at a/src.ts. The a//b/ prefixes are diff notation and should only be stripped when parsing patch text, not command arguments or output. Consider making the diff-prefix cleanup contextual — for example, splitting it into a separate helper called only by parsePatchPaths.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mindwalk/TraceScrape.ts around line 197:
`cleanExtractedPath` strips leading `a/` and `b/` from every path, so a real command argument like `cat a/src.ts` is reduced to `src.ts`. Callers that resolve this against the repo root record the wrong path and typically drop the read as nonexistent, since the actual file lives at `a/src.ts`. The `a/`/`b/` prefixes are diff notation and should only be stripped when parsing patch text, not command arguments or output. Consider making the diff-prefix cleanup contextual — for example, splitting it into a separate helper called only by `parsePatchPaths`.
| return paths.sort(compareStrings); | ||
| } | ||
|
|
||
| function sedReadsOnly(args: ReadonlyArray<string>): boolean { |
There was a problem hiding this comment.
🟡 Medium mindwalk/TraceScrape.ts:338
sedReadsOnly returns true for sed -n 'w generated.txt' input.txt, so readCommand classifies a mutating command as a pure read and the write is absent from the trace. The check only rejects -i and requires -n, but a w command inside the script writes a named file. Consider rejecting scripts containing a w (write-file) command, e.g. flag when an arg matches /^'?.*\bw\b/ or contains w followed by a filename, so file-writing sed invocations are excluded.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mindwalk/TraceScrape.ts around line 338:
`sedReadsOnly` returns `true` for `sed -n 'w generated.txt' input.txt`, so `readCommand` classifies a mutating command as a pure read and the write is absent from the trace. The check only rejects `-i` and requires `-n`, but a `w` command inside the script writes a named file. Consider rejecting scripts containing a `w` (write-file) command, e.g. flag when an arg matches `/^'?.*\bw\b/` or contains `w` followed by a filename, so file-writing sed invocations are excluded.
| ]; | ||
|
|
||
| /** Whether a command is the session checking its own work. */ | ||
| export function verifyCommand(command: string): boolean { |
There was a problem hiding this comment.
🟡 Medium mindwalk/TraceScrape.ts:373
verifyCommand returns true for any command containing a substring like go test or pytest, so echo 'go test failed' and grep 'pytest' README.md are classified as verify even though they do not run a verification command. It also fires before any segment-level read-only/mutating analysis, so go test ./... && rm generated.txt is classified as verify despite the rm mutation. Both cases produce wrong action labels in the trace histogram. Consider anchoring the match to the actual program (e.g. segment-level program/argument parsing) and ensuring mixed mutating commands fall through to exec.
Also found in 1 other location(s)
apps/server/src/mindwalk/TraceEvent.ts:102
actionForacceptsverifyCommand(command)before checking whether every command segment is read-only. SinceverifyCommandis only a substring match, a mixed command such asgo test ./... && rm generated.txt(or evenecho 'go test') is classified asverifyrather thanexec, misrepresenting mutating/non-verification events in the trace histogram.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mindwalk/TraceScrape.ts around line 373:
`verifyCommand` returns `true` for any command containing a substring like `go test` or `pytest`, so `echo 'go test failed'` and `grep 'pytest' README.md` are classified as `verify` even though they do not run a verification command. It also fires before any segment-level read-only/mutating analysis, so `go test ./... && rm generated.txt` is classified as `verify` despite the `rm` mutation. Both cases produce wrong action labels in the trace histogram. Consider anchoring the match to the actual program (e.g. segment-level program/argument parsing) and ensuring mixed mutating commands fall through to `exec`.
Also found in 1 other location(s):
- apps/server/src/mindwalk/TraceEvent.ts:102 -- `actionFor` accepts `verifyCommand(command)` before checking whether every command segment is read-only. Since `verifyCommand` is only a substring match, a mixed command such as `go test ./... && rm generated.txt` (or even `echo 'go test'`) is classified as `verify` rather than `exec`, misrepresenting mutating/non-verification events in the trace histogram.
| Effect.map((rows) => rows.map(toActivity)), | ||
| ); | ||
|
|
||
| const countByThreadId: ProjectionThreadActivityRepositoryShape["countByThreadId"] = (input) => |
There was a problem hiding this comment.
🟡 Medium Layers/ProjectionThreadActivities.ts:184
countByThreadId returns only the row count, but upsert uses ON CONFLICT (activity_id) DO UPDATE and can change kind, payload, sequence, and other fields on an existing row without changing the count. When countByThreadId is used as a cache fingerprint (e.g., by MindwalkSnapshot), replacing an existing activity yields an unchanged count, so the cache treats it as a hit and indefinitely serves a stale trace/citymap. The fingerprint must include content or version changes, or upsert must invalidate the cache.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/persistence/Layers/ProjectionThreadActivities.ts around line 184:
`countByThreadId` returns only the row count, but `upsert` uses `ON CONFLICT (activity_id) DO UPDATE` and can change `kind`, `payload`, `sequence`, and other fields on an existing row without changing the count. When `countByThreadId` is used as a cache fingerprint (e.g., by `MindwalkSnapshot`), replacing an existing activity yields an unchanged count, so the cache treats it as a hit and indefinitely serves a stale trace/citymap. The fingerprint must include content or version changes, or `upsert` must invalidate the cache.
Five findings in code that is ours, each verified before acting. The other seven are in the ported scraping and are mindwalk's own behaviour. The snapshot cache keyed on an activity count, which could not see two of its inputs. User messages become marks but add no activity row, so a new message served stale until some later activity moved the count. And `upsert` is ON CONFLICT DO UPDATE, so a row can be rewritten in place without the count moving at all. The key is now a content fingerprint over both row sets, including payloads; fetching them costs one indexed query on a hit, against a miss that reads every file in the repository. It is also more precise than the count it replaces: an activity of a kind the trace never reads no longer forces a rebuild. Lens bounds were computed before the lens filter, so an `agent:<id>` trace reported the whole thread's span and any duration derived from it was wrong. They now follow the events actually included. Message draining assumed iteration order was monotonic in `createdAt`. It is not: the activity query orders `sequence IS NULL` rows first, so an unsequenced row can carry a later timestamp than sequenced rows behind it, attaching user-message marks to the wrong `seq`. Draining now follows a high-water mark. The ACP adapter hard-coded `isError: false` on all five branches while its neighbours already read `status`, reporting failed Cursor and Grok calls as clean and skewing the error rate the histogram shows. The cache test now asserts the property rather than the mechanism: noise does not invalidate, a user message does, and an in-place rewrite does. Refs #22. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Twelve findings: five taken, seven declined. The split is between code that is ours and code ported from mindwalk, and each declined one was checked against Taken — all in our own codeThe cache key could not see two of its inputs. It was an activity count. User messages become marks but add no activity row, so a new message served stale until some later activity moved the count. And Lens bounds were computed before the lens filter, so an Message draining assumed monotonic The ACP adapter hard-coded The cache test now asserts the property rather than the mechanism: noise does not invalidate, a user message does, an in-place rewrite does. It failed against my first attempt at the fix — I had documented payload-only rewrites as an accepted gap, which is exactly the case the finding named, so the payload is now in the fingerprint. Declined — all seven are mindwalk's behaviour, ported verbatim
They are real defects and the analysis is sound. But this is a deliberate port, and the value of the ported scraper is that it produces the same classifications as the reference — the differential run in the PR description showed 1204/1204 identical actions precisely because the heuristics match, warts included. Fixing them here would trade a measurable equivalence for an unmeasurable improvement, and every future comparison would have to remember which differences were intentional. Recorded as post-port work alongside the four found in #16 and #18. 229 tests pass; typecheck and lint clean. |
| messages.listByThreadId({ threadId }), | ||
| ]); | ||
|
|
||
| const fingerprint = fingerprintRows(traceRows, messageRows); |
There was a problem hiding this comment.
🟡 Medium mindwalk/MindwalkSnapshot.ts:105
The cache-hit path at line 107 returns the stale cached snapshot when the thread's activity and message rows are unchanged, even if the repository root, commit, dirty state, session provider, or shell title/model have changed. Because the fingerprint only hashes traceRows and messageRows, a commit or worktree change that produces no new rows keeps the cache hit forever, so CitymapBuilder's commit/dirty-state invalidation is bypassed and the old citymap and trace metadata are served indefinitely.
The fingerprint should also incorporate context (root, commit, dirty state), session (provider), and shell (title, model) so metadata changes invalidate the cache. Consider building the cache key or fingerprint from these values alongside the row hash.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mindwalk/MindwalkSnapshot.ts around line 105:
The cache-hit path at line 107 returns the stale cached snapshot when the thread's activity and message rows are unchanged, even if the repository root, commit, dirty state, session provider, or shell title/model have changed. Because the fingerprint only hashes `traceRows` and `messageRows`, a commit or worktree change that produces no new rows keeps the cache hit forever, so `CitymapBuilder`'s commit/dirty-state invalidation is bypassed and the old citymap and trace metadata are served indefinitely.
The fingerprint should also incorporate `context` (root, commit, dirty state), `session` (provider), and `shell` (title, model) so metadata changes invalidate the cache. Consider building the cache key or fingerprint from these values alongside the row hash.
Second review round, both findings on the fix from the first. The snapshot embeds a citymap, so repository state is one of its inputs: a new commit changes the map without adding a single activity row, and the row-only fingerprint would have served the old one forever — bypassing the commit-and-dirty invalidation CitymapBuilder already does correctly. The builder is now consulted before the cache check, which is cheap because it caches the walk itself, and its repo identity joins the key. Messages were fingerprinted by id and timestamp while buildTrace reads role and text, both of which the repository rewrites in place as a turn streams. Now included. A failed citymap build no longer gains ghost buildings. Moving the build ahead of the trace briefly made the empty fallback pick them up, which would have invented buildings for a repository that was never read. Refs #22. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Folding the citymap's repo identity into the key created a trap: on a non-git root the failure fallback's repo block is indistinguishable from a successful build's — same root, no commit, not dirty — so once a build failed, the empty citymap would be served under that fingerprint forever, even after the build started succeeding. A degraded result should not be memoized at all. Refs #22. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Two further rounds, both on my own fix rather than the original branch — worth recording, since it took three passes to get one cache key right. Round two. The snapshot embeds a citymap, so repository state is an input the row fingerprint could not see: a new commit changes the map without adding a single activity row, and the key would have bypassed the commit-and-dirty invalidation Moving the build ahead of the trace briefly made the empty fallback pick up ghost buildings, inventing buildings for a repository that was never read. Caught by an existing test, reverted. Round three, a High of my own making. Folding repo identity into the key created a trap: on a non-git root the failure fallback's repo block is indistinguishable from a successful build's — same root, no commit, not dirty — so one failure would have pinned the empty citymap under that fingerprint permanently, even once builds started succeeding. A degraded result is no longer memoized at all. Final tally across three rounds: eight taken, seven declined. All seven declined are mindwalk's own scraping heuristics, cited against |
The 3D surfaces need the ordered stream of file-touch events the scenes and playback deck consume.
GET /api/mindwalk/threads/:threadId?lens=now returns{ trace, citymap }, built in mindwalk's own order (server.go:834-841): trace with paths only → citymap with those paths asghostPaths→ backfillTarget.fileId→ recompute stats against the real file count. The two are mutually dependent, which is why they ship as one endpoint rather than two.Code in
apps/server/src/mindwalk/; wire contract inpackages/contracts/src/trace.ts, shaped byte-identically toapps/web/src/mindwalk/types.tsso the ported scenes read it unchanged. Mindwalk's classifier and its path scraping are ported rather than reimplemented, with MIT attribution.Differentially verified against the real Go binary
The citymap builder set the bar at zero diffs. That bar is not reachable here — the inputs genuinely differ — so the divergence is characterised instead.
mindwalk tracewas run over the Claude session behind the store's largest thread and diffed against our projection of the same thread:The 4 gaps are files touched only by events T3 never recorded, which is the same finding as the event-count difference below rather than a separate one.
Event counts differ, 1,284 ours against mindwalk's 1,819, and ours is the more correct number. The JSONL holds 1,819
tool_useblocks but only 1,348 unique ids — Claude replays history into the file on resume and compaction, and mindwalk emits an event per replay. T3's log is append-only and records each call once.Three things worth a reviewer's attention
A second deviation from verbatim, beyond the one the design enumerated. Codex's error grade is
exact, notestimated. Mindwalk sniffs Codex failures from output text because rollout JSONL is all it has; T3 storesstatusandexitCodestructurally. Reportingestimatedwould be precisely the lie that grade exists to prevent.verifyis effectively dead on this repo — 1 event in 3,805. Mindwalk matchesgo test,npm test,pnpm test,pytest,cargo test,make test; this repo runsvp test runand CMake. Left faithful and recorded as fog rather than quietly widened.~5% of Claude tool calls never reach T3's activity store. 65 of 1,348 unique calls, spread evenly across every tool (Bash −23, Edit −15, Read −13, Write −2) rather than concentrated in any type — the fingerprint of dropped ingest events, not a mapping gap. Out of scope here and recorded as fog, but it is a T3 ingest-completeness question worth someone's time, and the trace is what made it measurable.
Two design assumptions corrected during the build
Provider must come from the session row: the thread-shell query returns active threads only, so an archived thread would have rendered an empty city. And an unnamed provider is now identified by payload shape rather than assumed.
Verification
229 tests pass across the new modules and the server suite. Typecheck clean on both
apps/serverandpackages/contracts.Refs #22.
Model: Claude Opus 5 via Claude Code.
🤖 Generated with Claude Code
Note
Add GET /api/mindwalk/threads/:threadId endpoint to project T3 activities onto a trace model
MindwalkSnapshotServicethat builds aMindwalkSnapshot(trace + citymap) for a thread, with a 16-entry LRU cache invalidated by content fingerprint over activities, messages, and repo identity.buildTraceinTraceProjection.tsto fold provider activities and user messages into aTrace, interleaving user-message marks, compaction marks, and subagent marks, with lens filtering for main vs. agent subtrees.ProviderTraceAdapter.tsfor Claude, Codex, ACP (Cursor/Grok), and OpenCode, normalizing tool call payloads into provider-agnostic{call, result}tuples; unknown providers fall back to shape-sniffing.TraceScrape.tsfor extracting file paths from command output and shell commands, andTraceEvent.tsfor classifying tool calls into actions and buildingTraceEventrecords with file targets and touch strengths.http.tsunderEnvironmentMindwalkHttpApi, returning the snapshot or 404; projection failures surface as a structured internal error with codetrace_projection_failed.Macroscope summarized 7ad10e3.