feat: [opus with workflows] Throughline v1 — a pull request becomes a journey you can finish - #4
feat: [opus with workflows] Throughline v1 — a pull request becomes a journey you can finish#4AdiRishi wants to merge 4 commits into
Conversation
Turns the Effect starter into the PR comprehension system the documentation specifies: point it at a pull request, and an agent reconstructs the development journey behind it as an ordered sequence of clusters over the real diff. The guarantees are checked, not prompted. `@app/journey` is a new pure package holding seed-hunk derivation, the coverage validator, progress arithmetic, and `tl:` evidence resolution — functions from values to values, and the most heavily tested code in the repo. The server refuses to persist a journey that fails them. Coverage is structural rather than requested. Seeds come from a zero-context diff (computed facts, no agent involvement), and a split is *materialized from a cursor* rather than trusted: the agent says how many lines each part covers and the materializer computes where each part starts, so a refinement of a partition is a partition by construction. On top of that sits the ladder that makes "the agent always commits" an invariant of the system — validate, repair on the same thread, regenerate, then finish the artifact deterministically and record every fallback in the journey's provenance. Five seams carry the design: `GitHub` (one choke point, width-2 semaphore, TTL caches with single-flight, parked on rate limits, 4xx never retried), `Workspaces` (bare partial clones, `refs/pull/N/head`, diff materialization), `AnalysisHarness` (Codex and Claude behind detect/run-with-schema/cancel-via- scope, read-only enforced by sandbox and tool allowlist), `Ingestion`, and the WS RPC contracts. Three assumptions in the technical spec did not survive contact with the real libraries, and the code follows the libraries: - `@pierre/diffs` has no `loadDiffFiles` seam. Expansion is gated on `isPartial`, so diffs are parsed together with the full old/new blobs — which turns expand-context into native library behaviour. - `useCSSClasses` is not a CSS seam. Every file renders in its own shadow root, so emphasis rides `onPostRender` writing `data-*` markers plus one memoised `unsafeCSS` string. `data-line` is side-relative, so marking branches on `data-line-type`; proving that against a real CodeView is what kept it from highlighting the wrong code. - OpenAI's structured output rejects any optional schema property, so optionality is expressed as an empty array. Verified end to end against a real pull request: eight clusters in a comprehension order, 25 hints, resolved evidence links, and no fallbacks. Claude-Session: https://claude.ai/code/session_01MWkaeWcpTEcbBPdGSDkUKL
Each was invisible to the type checker and to unit tests, and each broke a documented promise. **The per-file header was hidden.** The emphasis stylesheet hid `[data-diffs-header]`, but `renderCustomHeader` portals its content into a slot *inside* that element — so the path, the inline/split control, and Mark read (the primary interaction of the whole product) were rendered and then hidden. The custom renderer already replaces the default header's content, so there was nothing to hide. **A hard reload of a deep journey URL failed.** `request` fails fast while disconnected so a click that cannot reach the server says so, which is right for a user action but wrong for a surface that fetches on mount: on a cold load the socket is still opening. `requestWhenConnected` waits for the supervisor's session and then asks once — not a retry, so ADR-0003's single-reconnector rule still holds — and every mount-time fetch uses it. **Staleness stopped updating once a journey was open.** The envelope carries the answer as of the moment it was fetched, and the artifact is immutable and cached forever, so a journey opened before a push kept claiming to be current. Staleness is now derived in the layout by comparing the pinned head against the live PR list, which is what "computed at the moment of display" has to mean. Verified against the fixture PR: pushing a commit turns the welcome row and the journey stale, Reanalyze starts a full rebuild, and the transition narrates real stages with observed counters. Claude-Session: https://claude.ai/code/session_01MWkaeWcpTEcbBPdGSDkUKL
…s break Packaging the app and running it found what dev never could. **The database from an older build made the app inert.** An earlier `1_initial` had a different shape — `journeys` keyed by owner/repo/number with no `pr_key`, `pr_state` a row per pull request rather than one document — and the migrator's only question is "what is the highest id you have run?". It read `1` in the ledger, skipped its work, and every statement afterwards failed with `no such column: pr_key`: total, silent, and worded to the reviewer as though retrying would help. That is a released app's upgrade path, and a contributor's every branch switch. `schemaGuard` asks the question the migrator does not — *can my statements run against this file?* — by comparing the tables and columns on disk against what the record is supposed to have produced. On a disagreement the file is moved aside, not deleted, and migrations run onto a clean one. Resetting is the right recovery precisely here: nothing in this database is a source of truth. A journey is a derived artifact, read progress costs one reanalysis, and the alternative is an app that opens and does nothing forever. Extra columns are tolerated, so downgrading does not destroy a newer install's data, and a file with no ledger is left alone because emptiness is not a mismatch. **Harness failures logged as a blank line.** `Data.TaggedError` leaves `message` empty unless a field is named `message`, and every generic rendering — log line, `exit.cause`, stack — reads `message`. All three harness errors carried a perfectly good `detail` and showed none of it, so the trace said `HarnessRunFailedError:` and stopped. That cost real diagnosis time; they now override `message`. **"Try again in a moment" was a lie for a defect.** A failure with nothing to say is a defect, and a defect fails identically on the next click. The fallback now says it was unexpected and points at the log, in `ui/failure` only — the welcome screen used to restate the same sentence in its own copy. Also: the packaged app ships the brand mark instead of the default Electron icon (derived at package time from the 1024px master, so the repo keeps one copy of it), the web tab gets its favicon, and a startup focus event no longer greets the reviewer with an error for a refresh they did not ask for and which the opening socket was about to make redundant. Verified against the real broken database: the guard displaces it with both WAL sidecars, migrates fresh, and the app opens working. Claude-Session: https://claude.ai/code/session_01MWkaeWcpTEcbBPdGSDkUKL
This repository is documentation-first, so the docs are normally right and the
code is wrong. Building the product inverted that in 57 specific places, and a
spec that disagrees with its implementation is worse than no spec: it is a
confident wrong answer. Every claim below was verified against the source that
disproves it before being changed.
Most of it was drift — names and shapes that moved during implementation.
`docs/technical/03-github.md` described an interface (`openPrs`, `cloneCredentials`,
`PrDetail`, `GitCredential`) none of whose members exist, credited `gh auth status`
as the identity probe when it is never invoked, and claimed `gh api --cache` as a
second cache layer that is deliberately unused. `02-domain-model.md`'s `Journey`
sketch was missing four required fields, so an artifact built to it would fail to
decode, and it credited a `SqlSchema` this codebase never imports.
`04-analysis.md` sketched a harness interface superseded by the session shape the
repair ladder needs, and described a "Regenerate" rung that does not exist.
Three corrections are about honesty rather than accuracy, and are the ones worth
reading. The transition's counters were documented as "files walked, symbols
traced, call sites followed", which reads better and cannot be derived — nothing
in a harness event stream distinguishes a symbol lookup from any other read, so
those counters could only have been invented. They are `changedFilesOpened`,
`filesRead`, `searchesRun`, and the doc now says why the duller names win. Hints
were documented as binding in every display mode; the guidance rail refuses to
bind an old-side hint in just-the-code, because the lines it points at are not on
screen — the refusal is the design, so the doc now states it as such. And the
single-accent rule was written too narrowly ("emphasis of the current cluster's
hunks") to describe code that also accents the current rail row, a filled meter,
and the live ingestion stage. Those are one idea — where the reviewer is and how
far they have come — so the rule is restated at that width, with what the accent
is still never spent on: alarm.
Five ADRs record the decisions taken while building that a future maintainer
would otherwise re-litigate: the schema guard (0008), the harness SDKs staying
external to the server bundle and why exec cannot reach into an asar (0009), the
one seam @pierre/diffs exposes for emphasis and the own-side line-number trap
(0010), the single injectable `gh` runner (0011), and split hunks declaring line
counts so the plan can materialize their starts (0012).
Also recorded: two library constraints that shaped the design and would look
arbitrary without explanation — OpenAI's structured output rejects any schema
with an optional property, so every field is required and absence is an empty
array; and a diff parsed without full old and new contents is permanently
unexpandable, which is why patches and contents are fetched as one bundle.
Claude-Session: https://claude.ai/code/session_01MWkaeWcpTEcbBPdGSDkUKL
📝 WalkthroughWalkthroughThroughline adds an end-to-end pull-request review workflow: GitHub and harness integrations, deterministic journey planning and validation, SQLite persistence, streaming RPCs, and a router-based web interface for ingestion progress, journey navigation, code reading, guidance, and settings. ChangesJourney workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
README.md-52-56 (1)
52-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the two development locations accurately.
The text says everything lives in “one directory,” then names
<repo>/.logsand a sibling data directory. State that development artifacts are split across those two locations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 52 - 56, Update the development storage description in README.md to state that artifacts are split across two locations: the repository’s <repo>/.logs directory and its sibling data directory. Keep the packaged <app-data>/throughline location and the existing explanation of what leaves the machine unchanged.apps/server/src/journeys/JourneyReader.ts-39-52 (1)
39-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
svginIMAGE_MEDIA_TYPESis unreachable.
dataUrlis only fedartifacts.oldBytes/newBytes, andWorkspaces.fileArtifactspopulates those solely whenentry.binaryis true (Lines 605-606 ofapps/server/src/workspace/Workspaces.ts). An SVG is text, so it never gets a data URL — it will render as source. Either drop the entry or serve SVG from the text side.Also applies to: 145-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/journeys/JourneyReader.ts` around lines 39 - 52, The svg entry in IMAGE_MEDIA_TYPES is unreachable because dataUrl only receives binary artifact bytes, while SVG files are handled as text. Remove the svg mapping from IMAGE_MEDIA_TYPES, or update the text-rendering path in JourneyReader to serve SVG content as a data URL; keep the existing behavior for other image types unchanged.apps/server/src/github/queries.ts-240-248 (1)
240-248: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShorthand branch skips the
number >= 1validation the URL branch applies.
owner/repo#0resolves to{ number: 0 }and becomes a realrepos/.../pulls/0request, while the URL path rejects it at Line 266. Reuse the same guard for both.The OpenGrep
command-injection.exec-jsfinding on Line 241 is a false positive — that isRegExp.prototype.exec.🐛 Apply the same numeric guard
if (shorthand !== null) { + const number = Number.parseInt(shorthand[3] as string, 10); + if (!Number.isInteger(number) || number < 1) return null; return { owner: shorthand[1] as string, repo: shorthand[2] as string, - number: Number.parseInt(shorthand[3] as string, 10), + number, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/github/queries.ts` around lines 240 - 248, Update the shorthand parsing branch in the pull-reference parser to require the parsed pull request number to be at least 1, matching the validation used by the URL branch. Reuse the existing numeric guard or shared validation logic rather than allowing `number: 0` to proceed; leave the RegExp.prototype.exec call unchanged.Source: Linters/SAST tools
apps/server/src/github/GitHub.ts-199-265 (1)
199-265: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd coverage for
discoverResetAtwhile the gate is saturated.All
API_CONCURRENCYpermits are returned as soon as the rate-limitedrunGheffect exits, sotapErrorcan reachdiscoverResetAt()and acquire a permit. Add a test that starts another rate-limited call while the second permit is already busy waiting for the reset, and assert the rate-limit discovery resolves instead of the module hanging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/github/GitHub.ts` around lines 199 - 265, Add test coverage for the request flow around discoverResetAt and the API concurrency gate: occupy the second permit with a rate-limited call waiting for reset, then start another rate-limited request and verify its reset discovery completes rather than hanging. Assert the rate-limit handling resolves while preserving the existing parking behavior.apps/server/src/analysis/prompts.ts-141-141 (1)
141-141: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winA PR body containing a code fence escapes its own fence.
fence()always uses a three-backtick delimiter, so a body with its own ``` block terminates the quote early and the remainder of the author-controlled text reads as prompt instructions. Cheap fix: pick a delimiter longer than the longest backtick run in the text.🛡️ Proposed fix
function fence(text: string): string { - return ["```", text, "```"].join("\n"); + const longest = Math.max(0, ...[...text.matchAll(/`+/g)].map((match) => match[0].length)); + const delimiter = "`".repeat(Math.max(3, longest + 1)); + return [delimiter, text, delimiter].join("\n"); }Also applies to: 427-429
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/analysis/prompts.ts` at line 141, Update the fence() helper used by the PR-body prompt interpolation to choose a backtick delimiter longer than the longest consecutive backtick run in the text, while retaining a minimum length of three. Apply this consistently to both opening and closing delimiters so embedded code fences cannot terminate the quoted author content early.apps/server/src/analysis/Ingestion.ts-713-758 (1)
713-758: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRung 3 of the ladder ("regenerate once") does not appear to be implemented.
The module header documents four rungs, but
runWithRepaironly asks, validates, and sends up toREPAIR_ROUNDScorrection turns on the same thread; narration that still fails falls straight through to a fallback rather than being discarded and rerun in a fresh session. Either the regeneration step is missing or the header overstates it — per the repo's documentation-first rule, one of the two should change.Based on learnings: documentation is authoritative and code should not contradict it unless the documents change first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/analysis/Ingestion.ts` around lines 713 - 758, Implement the documented “regenerate once” rung in the ingestion flow rather than removing it from the module documentation: after narration exhausts validation and correction rounds in runWithRepair, discard the failed result and rerun narration through a fresh HarnessSession once before using the fallback. Preserve existing validation and correction behavior, and ensure the regeneration is limited to one fresh-session attempt.Source: Learnings
apps/server/src/prs/PrList.ts-216-223 (1)
216-223: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
refreshingcan stay stuck attrue.
Effect.orElseSucceedonly covers the typed failure channel; a defect or an interrupt between Line 217 and Line 220 leaves the flag set, and the welcome screen shows a permanent spinner with no way to clear it. Reset it in anEffect.ensuringfinalizer instead.🔧 Proposed fix
refresh: Effect.gen(function* () { yield* Ref.set(refreshing, true); yield* publish("refreshing"); const sent = yield* github.refreshPrs.pipe(Effect.orElseSucceed(() => false)); - yield* Ref.set(refreshing, false); - yield* publish("refreshed"); return sent; - }), + }).pipe( + Effect.ensuring(Ref.set(refreshing, false).pipe(Effect.andThen(publish("refreshed")))), + ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/prs/PrList.ts` around lines 216 - 223, Update the refresh Effect in the refresh method so Ref.set(refreshing, false) runs in an Effect.ensuring finalizer, covering typed failures, defects, and interruption. Keep the existing refreshing/refreshed publication and return behavior, but remove the vulnerable inline reset.packages/journey/src/hunks.ts-309-316 (1)
309-316: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
additions/deletionscount context lines when the patch is not--unified=0.The header counts include context lines, so under any wider context width these totals overstate the change — which contradicts the tolerance claimed in the module comment on Lines 231-234. Either count
+/-body lines directly, or narrow the doc comment to state that the counts are only accurate at--unified=0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/src/hunks.ts` around lines 309 - 316, Update the hunk parsing logic around current.ranges.push so current.additions and current.deletions count only “+” and “-” body lines rather than using newLines and oldLines from the header, preserving accurate totals for any unified context width.packages/journey/src/hunks.ts-363-364 (1)
363-364: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winComparator never returns 0, so equal paths sort unpredictably.
indexDiff's determinism guarantee rests on this sort, but ties (two entries for the same path — e.g. a delete plus an add) return1, which violates the comparator contract and makes the resulting id assignment implementation-defined. Use a three-way comparison.🐛 Proposed fix
- const ordered = [...parsed].toSorted((left, right) => (left.path < right.path ? -1 : 1)); + const ordered = [...parsed].toSorted((left, right) => + left.path < right.path ? -1 : left.path > right.path ? 1 : 0, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/src/hunks.ts` around lines 363 - 364, Update the comparator used in indexDiff’s ordered sort to return 0 when left.path and right.path are equal, while preserving ascending path ordering for unequal values. Use a proper three-way comparison so duplicate paths receive deterministic ordering and ID assignment.packages/journey/src/plan.ts-277-283 (1)
277-283: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSplit parts sort lexicographically, so a seed split into ten or more parts comes out in the wrong order.
left.id < right.idordersh1.10beforeh1.2. Tiling still validates (the partition checks sort by line), but the artifact's hunk order — and therefore the reading order within the file — no longer follows the seed's lines. Compare the part index numerically, or fall back tooldStart/newStart.🐛 Proposed fix
const orderedHunks = [...hunks].toSorted((left, right) => { const leftSeed = seedOrder.get(left.seedId) ?? 0; const rightSeed = seedOrder.get(right.seedId) ?? 0; if (leftSeed !== rightSeed) return leftSeed - rightSeed; - return left.id < right.id ? -1 : left.id > right.id ? 1 : 0; + return ( + left.newStart - right.newStart || + left.oldStart - right.oldStart || + (left.id < right.id ? -1 : left.id > right.id ? 1 : 0) + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/src/plan.ts` around lines 277 - 283, Update the tie-breaker in the orderedHunks sort to compare split-part indices numerically rather than using lexicographic hunk IDs, so parts such as h1.2 precede h1.10. If IDs do not expose a usable numeric index, fall back to oldStart/newStart while preserving the existing seedOrder grouping.packages/journey/src/coverage.ts-435-441 (1)
435-441: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate
buildsOnentries are reported under the wrong code.A repeated-but-known dependency emits
cluster-builds-on-unknown, whose message contradicts the code. Since codes are the machine-readable half of a violation, add a dedicated code (e.g.cluster-builds-on-duplicate) toViolationCodeand use it here.🐛 Proposed fix
if (buildsOnSeen.has(dependency)) { violations.push({ - code: "cluster-builds-on-unknown", + code: "cluster-builds-on-duplicate", message: `${cluster.id} lists ${dependency} in buildsOn more than once`, clusterId: cluster.id, }); }and in the
ViolationCodeunion (line 33-34):| "cluster-builds-on-unknown" + | "cluster-builds-on-duplicate" | "cluster-builds-on-forward"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/src/coverage.ts` around lines 435 - 441, Add a dedicated duplicate-dependency value such as cluster-builds-on-duplicate to the ViolationCode union, then update the violation created in the buildsOn duplicate-detection branch of coverage validation to use it instead of cluster-builds-on-unknown; leave the message and detection logic unchanged.packages/journey/src/coverage.ts-594-620 (1)
594-620: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAnchor lower bound is unchecked at the validation boundary.
normalizeAnchorinplan.tsrejectsstartLine < 1, butvalidateHintsis the gate the server trusts before persisting, and it acceptsstartLine: 0(or negative) as long asendLine <= limit. Worth adding the lower-bound check here so the guarantee doesn't depend on the plan path being taken.🛡️ Proposed fix
- if (hint.anchor.endLine < hint.anchor.startLine) { + if (hint.anchor.startLine < 1 || hint.anchor.endLine < hint.anchor.startLine) { violations.push({ code: "hint-anchor-inverted",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/src/coverage.ts` around lines 594 - 620, Update validateHints around the existing hint-anchor range validation to reject anchors whose startLine is less than 1, alongside the current inverted and upper-bound checks. Emit the existing hint-anchor-out-of-range violation with the same contextual message and path, while preserving valid anchor handling and the separate unknown-file behavior.apps/web/src/index.css-16-18 (1)
16-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStylelint doesn't know Tailwind v4 at-rules — teach the config, don't change the CSS.
@themeis flagged as an unknown at-rule byscss/at-rule-no-unknown, andMenlo/Consolas/optimizeLegibilityare flagged byvalue-keyword-case. All four are correct as written (Tailwind v4 CSS-first config; CSS keywords are case-insensitive but conventionally cased). Sincepnpm checkmust pass, adjust the Stylelint config rather than the source — e.g. allowtheme,source,utility,applyin the at-rule ignore list and setvalue-keyword-caseto ignore font family names /optimizeLegibility.As per coding guidelines, "Run
pnpm checkandpnpm testsuccessfully before considering a task complete."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/index.css` around lines 16 - 18, Update the Stylelint configuration, not apps/web/src/index.css: allow Tailwind v4 at-rules including theme, source, utility, and apply in the scss/at-rule-no-unknown ignore list, and configure value-keyword-case to ignore the valid font family names Menlo and Consolas plus optimizeLegibility. Run pnpm check and pnpm test to verify the configuration.Sources: Coding guidelines, Linters/SAST tools
apps/web/src/journey/emphasis.ts-96-99 (1)
96-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
isChangedRow's doc contradicts whatapplyEmphasisactually marks.The comment on Line 96 states context is left alone, but
changedat Lines 130-131 also admitscontextandcontext-expanded, so in diff mode context rows falling inside a hunk's line range getdata-tl-dim/data-tl-focustoo. If that's intended (dimming the whole foreign region), move the "just-the-code" rationale up toisChangedRowso the next reader doesn't trust the narrower claim; if it isn't, gate the context branch on the plain-file item kind.Also applies to: 128-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/journey/emphasis.ts` around lines 96 - 99, Align the documentation and behavior around isChangedRow and the changed classification in applyEmphasis: either document that context and context-expanded rows within diff hunk ranges are intentionally emphasized, moving the just-the-code rationale to isChangedRow, or restrict the context branch to plain-file item kinds so only changed rows are marked. Ensure the final comment accurately describes the implemented behavior.apps/web/src/ui/Markdown.tsx-150-160 (1)
150-160: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAllowlist the href scheme before rendering an agent-authored link.
link.hrefcomes from harness-generated prose and is passed straight to an anchor. React 19 rejectsjavascript:hrefs, butdata:,blob:, and unknown schemes are not filtered — render those as plain text instead of a clickable link.🛡️ Proposed fix
+const SAFE_SCHEMES = new Set(["http:", "https:", "mailto:"]); + +function isSafeHref(href: string): boolean { + try { + return SAFE_SCHEMES.has(new URL(href, window.location.origin).protocol); + } catch { + return false; + } +}and gate the anchor on
isSafeHref(link.href), falling back to<>{label}</>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/ui/Markdown.tsx` around lines 150 - 160, Update the link-rendering logic in the Markdown component to validate link.href with the existing or introduced isSafeHref allowlist before rendering the external anchor. For unsafe data:, blob:, unknown, or other disallowed schemes, render only the label as plain text; preserve the current anchor attributes and styling for safe hrefs.apps/web/src/journey/CodeSurface.tsx-258-260 (1)
258-260: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the header state in item versions.
versionis the only signalCodeViewuses to decide whether to sync a record, andrenderCustomHeaderis evaluated through the item record. SinceClusterFileHeaderis part offilesand changes withcursorPath,displayMode, andread, the cursor can move without changing other fields and leave theRbadge on the previous file. Include a stable header signature in the hash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/journey/CodeSurface.tsx` around lines 258 - 260, Update the version hash in the item-building logic around `version` to include a stable signature of the current `ClusterFileHeader` state, including the header inputs that change with `cursorPath`, `displayMode`, and `read`. Ensure cursor movement changes the version so `CodeView` resynchronizes the record and moves the `R` badge correctly.apps/web/src/features/journey/FileScreen.tsx-298-307 (1)
298-307: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRegion walk sorts old- and new-side anchors into one numeric order.
In diff modes both sides are on screen, and sorting purely by
lineinterleaves old-side and new-side anchors, son/pcan jump backwards relative to what the reviewer sees. Sorting by side first (old before new for a deletion-only file, otherwise new) — or by the surface's render order — would keep the walk monotonic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/features/journey/FileScreen.tsx` around lines 298 - 307, Update the regions useMemo in FileScreen so anchors are ordered by the surface’s render order, keeping old-side and new-side regions from being interleaved solely by line number. Preserve ascending line order within each side and use old-before-new for deletion-only files, otherwise new-before-old, so n/p navigation remains monotonic.
🧹 Nitpick comments (14)
apps/server/src/journeys/JourneyReader.ts (1)
182-190: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrefer
Bufferfor base64 on the server path.
String.fromCharCode(...chunk)spreads up to 32k arguments per iteration for images as large as 2 MiB on a request thread.Buffer.from(bytes).toString("base64")is one call and avoids the intermediate string entirely.♻️ Proposed change
function toBase64(bytes: Uint8Array): string { - // Chunked so a large image cannot blow the argument limit of `fromCharCode`. - const chunk = 0x8000; - let binary = ""; - for (let index = 0; index < bytes.length; index += chunk) { - binary += String.fromCharCode(...bytes.subarray(index, index + chunk)); - } - return btoa(binary); + return Buffer.from(bytes).toString("base64"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/journeys/JourneyReader.ts` around lines 182 - 190, Update the server-side toBase64 function to use Buffer.from(bytes).toString("base64") directly, removing the chunking loop and intermediate binary string.apps/server/src/workspace/Workspaces.ts (1)
300-300: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
countFileswalks the worktree recursively, including.git.On a large repository this is a full recursive stat walk purely to produce a count for the transition UI, and it runs on the ingestion path before analysis starts. Consider counting only tracked paths (you already have
treePaths) or dropping the recursion.Also applies to: 537-541
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/workspace/Workspaces.ts` at line 300, Update the ingestion flow around countFiles and the related transition UI handling to avoid recursively walking the worktree, including .git, solely for a file count. Reuse the existing treePaths collection to derive the count of tracked paths, or remove the count if it is not required, while preserving the transition behavior.apps/server/src/github/GhRunner.ts (1)
175-192: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
parseStatus's\((\d{3})\)alternative is broad enough to misclassify prose.Any parenthesised three-digit number in
gh's output (a count, a line number, a duration) becomes an HTTP status;(404)-style text is the intent, but(200)in an unrelated message would silently fall through to the transient branch and(4xx)would turn a retryable failure into a permanent one. Consider anchoring the alternative to a nearbyHTTP/statustoken.Also note the OpenGrep
command-injection.exec-jsfindings on Lines 176, 183, and 189 are false positives — these areRegExp.prototype.exec, notchild_process.exec.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/github/GhRunner.ts` around lines 175 - 192, Restrict the parenthesized-number alternative in parseStatus to cases associated with a nearby HTTP or status token, rather than matching any standalone three-digit parenthesized number. Preserve the existing HTTP and status formats and null behavior, while preventing unrelated prose such as counts, line numbers, or durations from being interpreted as status codes.Source: Linters/SAST tools
apps/server/src/workspace/paths.ts (1)
75-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHand-rolled
/joining diverges from thePathservice used by callers.
Workspaces.tscomposes these results withpath.join(...), producing mixed separators on Windows. Node's fs tolerates it, butgit worktree addpaths and any later path comparison (e.g.normalizePathin ingestion) will not. Consider taking the separator from the platform or documenting POSIX-only support.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/workspace/paths.ts` around lines 75 - 77, The join function’s hard-coded “/” separator conflicts with platform-aware path handling and produces inconsistent Windows paths. Update join to use the platform-aware Path service or separator already used by callers, preserving normalized path output for Workspaces.ts, git worktree commands, and normalizePath comparisons.apps/server/src/process/run.ts (1)
49-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
runProcessandrunProcessBytesduplicate the whole spawn/timeout/error pipeline.The two bodies differ only in how stdout is collected. Extracting the
ChildProcess.makeoptions and thetimeout/catchTagtail into one helper keeps the two documented invariants (concurrent drain, scoped spawn) in a single place so they cannot drift.As per coding guidelines, "extract reusable logic into separate modules, avoid duplicating logic across files".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/process/run.ts` around lines 49 - 137, The process execution pipeline is duplicated between runProcess and runProcessBytes, allowing spawn configuration and timeout/error handling to diverge. Extract shared ChildProcess.make configuration and the scoped concurrent spawn, timeout, and catchTag handling into a reusable helper, then have both functions supply only their stdout collection strategy while preserving scoped spawning and concurrent stdout/stderr draining.Source: Coding guidelines
apps/server/src/ws.ts (1)
75-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a bounded buffer for these whole-value streams.
Each event carries the complete current value (whole PR list, whole job array, whole read state), so a slow or stalled WebSocket subscriber accumulates redundant snapshots in an unbounded
Queue— memory grows with lag, and the client eventually replays a backlog it does not care about. A sliding buffer of small capacity keeps only the newest value and gives the same observable result.♻️ Suggested change
- const buffer = yield* Queue.unbounded<Source>(); + // Whole values: the newest one supersedes anything still queued. + const buffer = yield* Queue.sliding<Source>(16);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/ws.ts` around lines 75 - 97, Update snapshotThenLive to use a small bounded sliding Queue instead of Queue.unbounded for whole-value stream updates, retaining only the newest value when a subscriber lags. Preserve the existing live producer, snapshot-first ordering, sequencing, and event conversion behavior.apps/server/src/analysis/Ingestion.ts (1)
882-884: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse Effect’s typed
Causepredicates for cancellation handling.
catchCausealready receives aCause, andString(cause).includes("Interrupt")both misclassifies defects whose message contains "Interrupt" and depends on string rendering. Replace this withCause.hasInterruptsOnly(cause)for pure cancellations, orCause.hasInterrupts(cause)if mixed errors should also map to cancelled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/analysis/Ingestion.ts` around lines 882 - 884, Update isInterrupt to use Effect’s typed Cause predicate instead of string matching: call Cause.hasInterruptsOnly(cause) for cancellation-only handling, or Cause.hasInterrupts(cause) if mixed causes should also be treated as cancelled. Preserve the existing boolean return contract and ensure catchCause passes the Cause directly.packages/journey/src/coverage.ts (1)
345-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard placement reads oddly.
Lines 347–349 read
lines[0]twice before the undefined guard; hoisting the empty check to the top of the function (if (lines.length === 0) return "no lines";) is clearer and lets the locals be plainnumber.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/src/coverage.ts` around lines 345 - 361, Update formatLineList to check lines.length === 0 and return "no lines" before initializing locals, then assign lines[0] to start and previous as plain numbers without redundant undefined checks.packages/journey/tests/evidence.test.ts (1)
116-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for prefix-colliding ids.
Add a case where
tl:hunk/h1is unresolvable andtl:hunk/h12resolves; today's implementation rewrites both (seepackages/journey/src/evidence.tsLines 92-99).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/tests/evidence.test.ts` around lines 116 - 142, Add a test in the downgradeUnresolvableLinks suite where tl:hunk/h1 is rejected while tl:hunk/h12 is accepted, and assert only the h1 link is downgraded and reported. Use distinct occurrences in the markdown so the test catches prefix-based replacement affecting the resolving link.packages/journey/tests/coverage.test.ts (1)
37-649: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA few violation codes are never exercised.
evidence-unresolved-file,cluster-file-order-duplicate,resurfaced-duplicate, and the duplicate-buildsOnpath have no test. Since these codes are fed verbatim to the repair turn, a regression in any of them would be silent. Worth adding one assertion each.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/tests/coverage.test.ts` around lines 37 - 649, Add focused negative tests covering the missing violation paths: unresolved file evidence in validateEvidence, duplicate entries in a cluster’s fileOrder for validateClusters, duplicate resurfaced hunk entries in validateResurfacing, and duplicate cluster IDs in buildsOn for validateClusters. Assert each case emits its exact code: evidence-unresolved-file, cluster-file-order-duplicate, resurfaced-duplicate, and the existing duplicate-buildsOn violation code.packages/journey/src/plan.ts (1)
690-694: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
AssemblyOutcometype.
AssemblyOutcomeis declared inpackages/journey/src/plan.ts:690-694but has no references elsewhere, so drop the speculative convenience interface.
[lower_remove_unused_type]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/journey/src/plan.ts` around lines 690 - 694, Remove the unused AssemblyOutcome interface from plan.ts, including its violations and fallbacks fields, without changing other plan types or behavior.apps/web/src/ui/Markdown.tsx (1)
41-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the lex pass.
marked.lexerruns on every render of everyMarkdowninstance, andClusterScreenre-renders on scroll (visiblePaths) — so a cluster narrative plus every resurfaced note is re-lexed per frame.useMemoonmarkdownmakes it once per string.♻️ Proposed refactor
- const tokens = marked.lexer(markdown.trim()); + const tokens = useMemo(() => marked.lexer(markdown.trim()), [markdown]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/ui/Markdown.tsx` at line 41, Memoize the tokenization in the Markdown component so marked.lexer runs only when the markdown input changes. Wrap the existing trimmed-markdown lex pass in useMemo with markdown as its dependency, preserving the current tokens value for rendering.apps/web/src/features/journey/JourneyLayout.tsx (1)
144-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe two branches render identical trees.
Lines 145-154 and 156-165 differ only in their condition; the JSX is byte-identical. Collapse to one guard.
♻️ Proposed simplification
const running = job !== null && isLivePhase(job.phase); - if (running || (context === null && !AsyncResult.isSuccess(result))) { - return ( - <IngestionTransition - ref={ref} - job={job} - onBack={() => void navigate({ to: "/" })} - onOpen={() => void navigate({ to: "/pr/$owner/$repo/$number", params })} - /> - ); - } - - if (context === null) { + if (running || context === null) { return ( <IngestionTransition ref={ref} job={job} onBack={() => void navigate({ to: "/" })} onOpen={() => void navigate({ to: "/pr/$owner/$repo/$number", params })} /> ); }Note this drops the
AsyncResult.isSuccess(result)read entirely — ifresultis only consulted to distinguish "still loading" from "loaded, no journey", that distinction currently has no visible effect, so either use it (e.g. a different transition state) or stop subscribing to it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/features/journey/JourneyLayout.tsx` around lines 144 - 165, Collapse the duplicate guards in the JourneyLayout render flow into a single `context === null || running` check that returns the existing `IngestionTransition` tree. Remove the now-unneeded `AsyncResult.isSuccess(result)` read and any resulting unused `result` subscription, while preserving the existing navigation handlers and transition props.apps/web/src/app/router.tsx (1)
102-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid a second source of truth for routes.
routesis not consumed outsiderouter.tsx; navigation uses repeated string route keys. Export shared route/path helpers instead, or consume the exported route objects directly, so route changes cannot silently drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/router.tsx` around lines 102 - 109, Update the route definitions around the exported routes object to eliminate the duplicate route-key source of truth. Replace repeated string route keys with shared route/path helpers or direct consumption of the existing route objects, ensuring navigation remains synchronized when route definitions change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/analysis/Ingestion.ts`:
- Around line 802-814: Update pinnedTree so its fileContains callback lazily
loads each referenced path’s contents from the prepared worktree at the head
revision before checking symbol inclusion. Cache successful reads in contents,
handle unreadable or missing files consistently as unresolved evidence, and
preserve the existing literal text.includes(symbol) validation.
- Around line 250-260: Update the job-start flow around Effect.forkIn and the
jobs SynchronizedRef update to insert the job record first with fiber: null,
then fork runJob, and finally patch that existing record with the created fiber.
Preserve the existing key, initial state, and publish behavior while ensuring
early state transitions from runJob are not dropped.
In `@apps/server/src/github/GhRunner.ts`:
- Around line 85-94: Update the run callback’s runProcess error handling in
GhRunner so spawn failures, including ENOENT and timeouts, pass their available
error detail through classify instead of always becoming GhTransientError.
Preserve the existing classify handling for nonzero exit results and ensure
missing or un runnable gh errors reach the classifier’s “enoent”/“command not
found” branch.
In `@apps/server/src/github/GitHub.ts`:
- Around line 386-403: Serialize the refresh window in refreshPrs so concurrent
callers cannot both pass the lastRefreshAt check and invalidate prsCache. Add or
reuse a dedicated single-permit semaphore around the interval check and cache
invalidate/get sequence, while preserving the parked-rate-limit behavior and
existing no-op result when the refresh floor has not elapsed.
In `@apps/server/src/harness/claude.ts`:
- Around line 100-119: Update the zero-token probe around the `query` session
and `Effect.timeoutOption` so interruption always invokes `session.close()`,
even when `initializationResult()` never settles. Hold the created session
outside the interruptible promise or manage it with `Effect.acquireRelease`, and
attach interruption cleanup via `Effect.onInterrupt` while preserving the
existing account result and timeout behavior.
In `@apps/server/src/journeys/JourneyStore.ts`:
- Around line 368-393: Wrap the read–modify–write sequences in markFile and
setDisplayMode with sql.withTransaction (or the store’s per-journey Semaphore),
ensuring readState and writeReadState execute atomically. Apply the same
protection to setPrFlag and updateSettings in
apps/server/src/journeys/JourneyStore.ts lines 397-430; preserve each mutation’s
existing update behavior while preventing concurrent writes from overwriting one
another.
In `@apps/server/src/prs/PrList.ts`:
- Around line 196-207: Debounce or throttle the ingestion change stream before
its `publish("ingestion-changed")` callback so frequent `Ingestion` activity
ticks coalesce into fewer welcome snapshot rebuilds. Keep durable store changes
on their existing immediate publish path, and ensure at least one ingestion
update is published after activity settles.
In `@apps/server/src/workspace/Workspaces.ts`:
- Around line 680-705: Update evictIfNeeded to exclude repositories with active
worktrees from the doomed eviction list. Reuse the existing worktree tracking or
lifecycle state established by prepare and releaseWorktree, ensuring a
repository remains protected for the entire active operation and is eligible
only after its worktree is released.
- Around line 129-139: The prepare contract in Workspaces.prepare claims scoped
cleanup, but cleanup is not registered and callers must remember
releaseWorktree. Make prepare genuinely scope-bound by requiring the appropriate
Scope context and registering worktree cleanup with acquireRelease/addFinalizer,
preserving worktrees on failure as documented; otherwise revise the comment to
describe the actual manual cleanup contract and update affected callers
consistently.
In `@apps/server/tests/github/GitHub.test.ts`:
- Around line 54-83: Replace the inline ServerConfig.make setup in the test’s
configLayer with the shared configLayer from
apps/server/tests/support/config.ts. Import and reuse that shared server-test
configuration builder, removing the local DateTime.now and ServerConfig
construction while preserving the test’s existing configuration wiring.
In `@apps/server/vite.config.ts`:
- Around line 29-33: The external-runtime package list is duplicated between the
server Vite configuration and desktop artifact staging. Extract the contract
into a shared module, update EXTERNAL_RUNTIME_PACKAGES in
apps/server/vite.config.ts to consume it, and update
scripts/build-desktop-artifact.ts to reuse the same list for staged dependencies
and asarUnpack; preserve the current package entries and behavior at both sites.
In `@apps/web/src/features/ingestion/IngestionTransition.tsx`:
- Around line 92-107: In the useEffect handling phase transitions, compute
finished before applying the handledPhase.current === phase deduplication guard
so a watched job changing to null still triggers refreshJourney() and onOpen().
Preserve the existing live-phase tracking and reset behavior, while ensuring
only already-handled terminal phases are skipped.
In `@apps/web/src/features/journey/JourneyLayout.tsx`:
- Around line 89-97: Validate the pull-request number before constructing the
ref in JourneyLayout, so non-numeric params do not produce NaN or reach refKey;
render the existing not-found state for invalid values. Alternatively, add
numeric parameter parsing and validation to prRoute in router.tsx, ensuring
invalid URLs are rejected before the layout and valid numeric refs retain the
current flow.
In `@apps/web/src/journey/CodeSurface.tsx`:
- Around line 280-284: Update the cacheKey values in CodeSurface’s diff, new,
and binary file descriptors to include the journey identity used by bundleKey(),
rather than relying on file.path alone. Preserve the existing suffixes and
ensure each key changes when journeyId changes, preventing `@pierre/diffs` from
reusing cached results across journeys.
In `@apps/web/src/ui/Markdown.tsx`:
- Around line 75-83: Update the list rendering in the Markdown component to
render each list item’s block-level tokens through the block renderer component,
such as Tokens, instead of renderInline. Apply this to both the item mapping in
the "list" case and the "list_item" case, preserving ordered/unordered list
selection and existing token fallbacks so nested lists and loose content render
correctly.
In `@packages/client-runtime/src/rpc/client.ts`:
- Around line 100-116: Bound the session-waiting flow in requestWhenConnected so
it cannot remain pending indefinitely when the supervisor is blocked or never
connects. Add a timeout around the
SubscriptionRef.changes(...).pipe(Stream.filter(Option.isSome), Stream.take(1),
Stream.runDrain) wait and convert timeout expiry into the existing
RpcUnavailableError, while preserving the immediate request path once a session
becomes available.
In `@packages/journey/src/evidence.ts`:
- Around line 39-83: The bare-link scanning logic is duplicated between
extractEvidenceLinks and Markdown.tsx’s withBareEvidence. Export a reusable
scanner from evidence.ts that applies EVIDENCE_URI and TRAILING_PUNCTUATION and
returns each parsed link with its start and end offsets, then update
withBareEvidence to use that scanner instead of its local BARE_EVIDENCE regex
and trimming logic.
- Around line 92-99: Update downgradeEvidenceLink to replace only complete raw
evidence URIs, not prefixes of longer URIs. Replace the split(raw).join(...)
logic with a delimiter-aware match that terminates using the same delimiter set
as EVIDENCE_URI, preserving longer links such as tl:hunk/h12 when downgrading
tl:hunk/h1.
---
Minor comments:
In `@apps/server/src/analysis/Ingestion.ts`:
- Around line 713-758: Implement the documented “regenerate once” rung in the
ingestion flow rather than removing it from the module documentation: after
narration exhausts validation and correction rounds in runWithRepair, discard
the failed result and rerun narration through a fresh HarnessSession once before
using the fallback. Preserve existing validation and correction behavior, and
ensure the regeneration is limited to one fresh-session attempt.
In `@apps/server/src/analysis/prompts.ts`:
- Line 141: Update the fence() helper used by the PR-body prompt interpolation
to choose a backtick delimiter longer than the longest consecutive backtick run
in the text, while retaining a minimum length of three. Apply this consistently
to both opening and closing delimiters so embedded code fences cannot terminate
the quoted author content early.
In `@apps/server/src/github/GitHub.ts`:
- Around line 199-265: Add test coverage for the request flow around
discoverResetAt and the API concurrency gate: occupy the second permit with a
rate-limited call waiting for reset, then start another rate-limited request and
verify its reset discovery completes rather than hanging. Assert the rate-limit
handling resolves while preserving the existing parking behavior.
In `@apps/server/src/github/queries.ts`:
- Around line 240-248: Update the shorthand parsing branch in the pull-reference
parser to require the parsed pull request number to be at least 1, matching the
validation used by the URL branch. Reuse the existing numeric guard or shared
validation logic rather than allowing `number: 0` to proceed; leave the
RegExp.prototype.exec call unchanged.
In `@apps/server/src/journeys/JourneyReader.ts`:
- Around line 39-52: The svg entry in IMAGE_MEDIA_TYPES is unreachable because
dataUrl only receives binary artifact bytes, while SVG files are handled as
text. Remove the svg mapping from IMAGE_MEDIA_TYPES, or update the
text-rendering path in JourneyReader to serve SVG content as a data URL; keep
the existing behavior for other image types unchanged.
In `@apps/server/src/prs/PrList.ts`:
- Around line 216-223: Update the refresh Effect in the refresh method so
Ref.set(refreshing, false) runs in an Effect.ensuring finalizer, covering typed
failures, defects, and interruption. Keep the existing refreshing/refreshed
publication and return behavior, but remove the vulnerable inline reset.
In `@apps/web/src/features/journey/FileScreen.tsx`:
- Around line 298-307: Update the regions useMemo in FileScreen so anchors are
ordered by the surface’s render order, keeping old-side and new-side regions
from being interleaved solely by line number. Preserve ascending line order
within each side and use old-before-new for deletion-only files, otherwise
new-before-old, so n/p navigation remains monotonic.
In `@apps/web/src/index.css`:
- Around line 16-18: Update the Stylelint configuration, not
apps/web/src/index.css: allow Tailwind v4 at-rules including theme, source,
utility, and apply in the scss/at-rule-no-unknown ignore list, and configure
value-keyword-case to ignore the valid font family names Menlo and Consolas plus
optimizeLegibility. Run pnpm check and pnpm test to verify the configuration.
In `@apps/web/src/journey/CodeSurface.tsx`:
- Around line 258-260: Update the version hash in the item-building logic around
`version` to include a stable signature of the current `ClusterFileHeader`
state, including the header inputs that change with `cursorPath`, `displayMode`,
and `read`. Ensure cursor movement changes the version so `CodeView`
resynchronizes the record and moves the `R` badge correctly.
In `@apps/web/src/journey/emphasis.ts`:
- Around line 96-99: Align the documentation and behavior around isChangedRow
and the changed classification in applyEmphasis: either document that context
and context-expanded rows within diff hunk ranges are intentionally emphasized,
moving the just-the-code rationale to isChangedRow, or restrict the context
branch to plain-file item kinds so only changed rows are marked. Ensure the
final comment accurately describes the implemented behavior.
In `@apps/web/src/ui/Markdown.tsx`:
- Around line 150-160: Update the link-rendering logic in the Markdown component
to validate link.href with the existing or introduced isSafeHref allowlist
before rendering the external anchor. For unsafe data:, blob:, unknown, or other
disallowed schemes, render only the label as plain text; preserve the current
anchor attributes and styling for safe hrefs.
In `@packages/journey/src/coverage.ts`:
- Around line 435-441: Add a dedicated duplicate-dependency value such as
cluster-builds-on-duplicate to the ViolationCode union, then update the
violation created in the buildsOn duplicate-detection branch of coverage
validation to use it instead of cluster-builds-on-unknown; leave the message and
detection logic unchanged.
- Around line 594-620: Update validateHints around the existing hint-anchor
range validation to reject anchors whose startLine is less than 1, alongside the
current inverted and upper-bound checks. Emit the existing
hint-anchor-out-of-range violation with the same contextual message and path,
while preserving valid anchor handling and the separate unknown-file behavior.
In `@packages/journey/src/hunks.ts`:
- Around line 309-316: Update the hunk parsing logic around current.ranges.push
so current.additions and current.deletions count only “+” and “-” body lines
rather than using newLines and oldLines from the header, preserving accurate
totals for any unified context width.
- Around line 363-364: Update the comparator used in indexDiff’s ordered sort to
return 0 when left.path and right.path are equal, while preserving ascending
path ordering for unequal values. Use a proper three-way comparison so duplicate
paths receive deterministic ordering and ID assignment.
In `@packages/journey/src/plan.ts`:
- Around line 277-283: Update the tie-breaker in the orderedHunks sort to
compare split-part indices numerically rather than using lexicographic hunk IDs,
so parts such as h1.2 precede h1.10. If IDs do not expose a usable numeric
index, fall back to oldStart/newStart while preserving the existing seedOrder
grouping.
In `@README.md`:
- Around line 52-56: Update the development storage description in README.md to
state that artifacts are split across two locations: the repository’s
<repo>/.logs directory and its sibling data directory. Keep the packaged
<app-data>/throughline location and the existing explanation of what leaves the
machine unchanged.
---
Nitpick comments:
In `@apps/server/src/analysis/Ingestion.ts`:
- Around line 882-884: Update isInterrupt to use Effect’s typed Cause predicate
instead of string matching: call Cause.hasInterruptsOnly(cause) for
cancellation-only handling, or Cause.hasInterrupts(cause) if mixed causes should
also be treated as cancelled. Preserve the existing boolean return contract and
ensure catchCause passes the Cause directly.
In `@apps/server/src/github/GhRunner.ts`:
- Around line 175-192: Restrict the parenthesized-number alternative in
parseStatus to cases associated with a nearby HTTP or status token, rather than
matching any standalone three-digit parenthesized number. Preserve the existing
HTTP and status formats and null behavior, while preventing unrelated prose such
as counts, line numbers, or durations from being interpreted as status codes.
In `@apps/server/src/journeys/JourneyReader.ts`:
- Around line 182-190: Update the server-side toBase64 function to use
Buffer.from(bytes).toString("base64") directly, removing the chunking loop and
intermediate binary string.
In `@apps/server/src/process/run.ts`:
- Around line 49-137: The process execution pipeline is duplicated between
runProcess and runProcessBytes, allowing spawn configuration and timeout/error
handling to diverge. Extract shared ChildProcess.make configuration and the
scoped concurrent spawn, timeout, and catchTag handling into a reusable helper,
then have both functions supply only their stdout collection strategy while
preserving scoped spawning and concurrent stdout/stderr draining.
In `@apps/server/src/workspace/paths.ts`:
- Around line 75-77: The join function’s hard-coded “/” separator conflicts with
platform-aware path handling and produces inconsistent Windows paths. Update
join to use the platform-aware Path service or separator already used by
callers, preserving normalized path output for Workspaces.ts, git worktree
commands, and normalizePath comparisons.
In `@apps/server/src/workspace/Workspaces.ts`:
- Line 300: Update the ingestion flow around countFiles and the related
transition UI handling to avoid recursively walking the worktree, including
.git, solely for a file count. Reuse the existing treePaths collection to derive
the count of tracked paths, or remove the count if it is not required, while
preserving the transition behavior.
In `@apps/server/src/ws.ts`:
- Around line 75-97: Update snapshotThenLive to use a small bounded sliding
Queue instead of Queue.unbounded for whole-value stream updates, retaining only
the newest value when a subscriber lags. Preserve the existing live producer,
snapshot-first ordering, sequencing, and event conversion behavior.
In `@apps/web/src/app/router.tsx`:
- Around line 102-109: Update the route definitions around the exported routes
object to eliminate the duplicate route-key source of truth. Replace repeated
string route keys with shared route/path helpers or direct consumption of the
existing route objects, ensuring navigation remains synchronized when route
definitions change.
In `@apps/web/src/features/journey/JourneyLayout.tsx`:
- Around line 144-165: Collapse the duplicate guards in the JourneyLayout render
flow into a single `context === null || running` check that returns the existing
`IngestionTransition` tree. Remove the now-unneeded
`AsyncResult.isSuccess(result)` read and any resulting unused `result`
subscription, while preserving the existing navigation handlers and transition
props.
In `@apps/web/src/ui/Markdown.tsx`:
- Line 41: Memoize the tokenization in the Markdown component so marked.lexer
runs only when the markdown input changes. Wrap the existing trimmed-markdown
lex pass in useMemo with markdown as its dependency, preserving the current
tokens value for rendering.
In `@packages/journey/src/coverage.ts`:
- Around line 345-361: Update formatLineList to check lines.length === 0 and
return "no lines" before initializing locals, then assign lines[0] to start and
previous as plain numbers without redundant undefined checks.
In `@packages/journey/src/plan.ts`:
- Around line 690-694: Remove the unused AssemblyOutcome interface from plan.ts,
including its violations and fallbacks fields, without changing other plan types
or behavior.
In `@packages/journey/tests/coverage.test.ts`:
- Around line 37-649: Add focused negative tests covering the missing violation
paths: unresolved file evidence in validateEvidence, duplicate entries in a
cluster’s fileOrder for validateClusters, duplicate resurfaced hunk entries in
validateResurfacing, and duplicate cluster IDs in buildsOn for validateClusters.
Assert each case emits its exact code: evidence-unresolved-file,
cluster-file-order-duplicate, resurfaced-duplicate, and the existing
duplicate-buildsOn violation code.
In `@packages/journey/tests/evidence.test.ts`:
- Around line 116-142: Add a test in the downgradeUnresolvableLinks suite where
tl:hunk/h1 is rejected while tl:hunk/h12 is accepted, and assert only the h1
link is downgraded and reported. Use distinct occurrences in the markdown so the
test catches prefix-based replacement affecting the resolving link.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| const fiber = yield* Effect.forkIn( | ||
| Effect.scoped(runJob({ ref, pr: checked.pr, harness: checked.harness })), | ||
| jobScope, | ||
| ); | ||
|
|
||
| yield* SynchronizedRef.update(jobs, (map) => { | ||
| const next = new Map(map); | ||
| next.set(key, { state: initial, fiber }); | ||
| return next; | ||
| }); | ||
| yield* publish; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The job fiber is forked before its record exists, so early state transitions are dropped.
patch no-ops when the map has no entry for the key (Line 138). Here the fiber is forked at Line 250 and the record is only inserted at Line 255; the forked fiber can reach setPhase(ref, "resolving", …) and the queuePosition: null patch before that insert lands, in which case those updates vanish and the job can stay visibly queued.
Insert the record first (with fiber: null), then fork and patch the fiber in.
🔧 Proposed reordering
- const fiber = yield* Effect.forkIn(
- Effect.scoped(runJob({ ref, pr: checked.pr, harness: checked.harness })),
- jobScope,
- );
-
yield* SynchronizedRef.update(jobs, (map) => {
const next = new Map(map);
- next.set(key, { state: initial, fiber });
+ next.set(key, { state: initial, fiber: null });
return next;
});
+
+ const fiber = yield* Effect.forkIn(
+ Effect.scoped(runJob({ ref, pr: checked.pr, harness: checked.harness })),
+ jobScope,
+ );
+ yield* SynchronizedRef.update(jobs, (map) => {
+ const existingRecord = map.get(key);
+ if (existingRecord === undefined) return map;
+ const next = new Map(map);
+ next.set(key, { ...existingRecord, fiber });
+ return next;
+ });
yield* publish;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fiber = yield* Effect.forkIn( | |
| Effect.scoped(runJob({ ref, pr: checked.pr, harness: checked.harness })), | |
| jobScope, | |
| ); | |
| yield* SynchronizedRef.update(jobs, (map) => { | |
| const next = new Map(map); | |
| next.set(key, { state: initial, fiber }); | |
| return next; | |
| }); | |
| yield* publish; | |
| yield* SynchronizedRef.update(jobs, (map) => { | |
| const next = new Map(map); | |
| next.set(key, { state: initial, fiber: null }); | |
| return next; | |
| }); | |
| const fiber = yield* Effect.forkIn( | |
| Effect.scoped(runJob({ ref, pr: checked.pr, harness: checked.harness })), | |
| jobScope, | |
| ); | |
| yield* SynchronizedRef.update(jobs, (map) => { | |
| const existingRecord = map.get(key); | |
| if (existingRecord === undefined) return map; | |
| const next = new Map(map); | |
| next.set(key, { ...existingRecord, fiber }); | |
| return next; | |
| }); | |
| yield* publish; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/analysis/Ingestion.ts` around lines 250 - 260, Update the
job-start flow around Effect.forkIn and the jobs SynchronizedRef update to
insert the job record first with fiber: null, then fork runJob, and finally
patch that existing record with the created fiber. Preserve the existing key,
initial state, and publish behavior while ensuring early state transitions from
runJob are not dropped.
| function pinnedTree(prepared: PreparedRun): PinnedTree { | ||
| const contents = new Map<string, string>(); | ||
| return { | ||
| paths: new Set(prepared.treePaths), | ||
| lineCounts: prepared.lineCounts, | ||
| // Symbol resolution is a substring check against the head revision, on | ||
| // purpose: no language tooling keeps the guarantee cheap and unambiguous. | ||
| fileContains: (path, symbol) => { | ||
| const text = contents.get(path); | ||
| return text === undefined ? true : text.includes(symbol); | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
fileContains always returns true — symbol evidence is never actually verified.
contents is created empty and never populated, so text === undefined on every lookup and the function short-circuits to true. validateEvidence in packages/journey/src/coverage.ts therefore accepts every tl:symbol/... link, contradicting the promise made to the agent in apps/server/src/analysis/prompts.ts (Line 275: a symbol link resolves only if the identifier literally occurs in the file at head). Broken symbol links will ship into narratives instead of being stripped.
Populate contents from the prepared worktree (lazily reading the head revision of the referenced path) rather than leaving the map empty.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/analysis/Ingestion.ts` around lines 802 - 814, Update
pinnedTree so its fileContains callback lazily loads each referenced path’s
contents from the prepared worktree at the head revision before checking symbol
inclusion. Cache successful reads in contents, handle unreadable or missing
files consistently as unresolved evidence, and preserve the existing literal
text.includes(symbol) validation.
| run: (args, options) => | ||
| runProcess("gh", args, { env: GH_ENV, ...options }).pipe( | ||
| Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), | ||
| Effect.mapError((cause) => new GhTransientError({ detail: cause.detail }) as GhError), | ||
| Effect.flatMap((result) => | ||
| result.exitCode === 0 | ||
| ? Effect.succeed(result.stdout) | ||
| : Effect.fail(classify(result.exitCode, result.stderr, result.stdout)), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Spawn failures bypass classify, so a missing/unrunnable gh is reported as transient.
runProcess fails with ProcessFailedError when the spawn itself fails (ENOENT) or times out. Line 88 maps every such cause straight to GhTransientError, so classify's "enoent" / "command not found" branch is unreachable for the case it was written for. Downstream in GitHub.ts that becomes a retried GitHubFailedError rather than GitHubUnavailableError{reason:"missing"}, so callers other than probeIdentity never surface the "install gh" instruction.
🐛 Route spawn failures through the same classifier
- Effect.mapError((cause) => new GhTransientError({ detail: cause.detail }) as GhError),
+ Effect.mapError((cause) => classify(-1, cause.detail, "")),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: (args, options) => | |
| runProcess("gh", args, { env: GH_ENV, ...options }).pipe( | |
| Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), | |
| Effect.mapError((cause) => new GhTransientError({ detail: cause.detail }) as GhError), | |
| Effect.flatMap((result) => | |
| result.exitCode === 0 | |
| ? Effect.succeed(result.stdout) | |
| : Effect.fail(classify(result.exitCode, result.stderr, result.stdout)), | |
| ), | |
| ), | |
| run: (args, options) => | |
| runProcess("gh", args, { env: GH_ENV, ...options }).pipe( | |
| Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), | |
| Effect.mapError((cause) => classify(-1, cause.detail, "")), | |
| Effect.flatMap((result) => | |
| result.exitCode === 0 | |
| ? Effect.succeed(result.stdout) | |
| : Effect.fail(classify(result.exitCode, result.stderr, result.stdout)), | |
| ), | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/github/GhRunner.ts` around lines 85 - 94, Update the run
callback’s runProcess error handling in GhRunner so spawn failures, including
ENOENT and timeouts, pass their available error detail through classify instead
of always becoming GhTransientError. Preserve the existing classify handling for
nonzero exit results and ensure missing or un runnable gh errors reach the
classifier’s “enoent”/“command not found” branch.
| refreshPrs: Effect.gen(function* () { | ||
| const until = yield* parkRemaining; | ||
| if (Option.isSome(until)) { | ||
| return yield* new GitHubParkedError({ | ||
| resetAtMillis: until.value, | ||
| detail: "GitHub's rate limit is exhausted; Throughline is waiting it out.", | ||
| }); | ||
| } | ||
| const now = yield* Clock.currentTimeMillis; | ||
| const last = yield* Ref.get(lastRefreshAt); | ||
| if (now - last < Duration.toMillis(MIN_REFRESH_INTERVAL)) { | ||
| // Honest no-op: the reviewer asked, but the floor says not yet. | ||
| return false; | ||
| } | ||
| yield* Cache.invalidate(prsCache, "prs"); | ||
| yield* Cache.get(prsCache, "prs"); | ||
| return true; | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
refreshPrs can send two requests for a concurrent stampede, contradicting the module's "exactly one request" promise.
The floor check reads lastRefreshAt (only written on success, Line 338) and then invalidates — nothing serializes the window between them. Two callers arriving together both pass the interval check, both Cache.invalidate, and the second invalidation can drop the first's in-flight entry, defeating Cache's single-flighting. A dedicated single-permit semaphore (or setting lastRefreshAt before the fetch) closes it.
🔒️ Serialize the refresh window
+ const refreshGate = yield* Semaphore.make(1);
...
- refreshPrs: Effect.gen(function* () {
+ refreshPrs: refreshGate.withPermits(1)(Effect.gen(function* () {
const until = yield* parkRemaining;
...
yield* Cache.invalidate(prsCache, "prs");
yield* Cache.get(prsCache, "prs");
return true;
- }),
+ })),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| refreshPrs: Effect.gen(function* () { | |
| const until = yield* parkRemaining; | |
| if (Option.isSome(until)) { | |
| return yield* new GitHubParkedError({ | |
| resetAtMillis: until.value, | |
| detail: "GitHub's rate limit is exhausted; Throughline is waiting it out.", | |
| }); | |
| } | |
| const now = yield* Clock.currentTimeMillis; | |
| const last = yield* Ref.get(lastRefreshAt); | |
| if (now - last < Duration.toMillis(MIN_REFRESH_INTERVAL)) { | |
| // Honest no-op: the reviewer asked, but the floor says not yet. | |
| return false; | |
| } | |
| yield* Cache.invalidate(prsCache, "prs"); | |
| yield* Cache.get(prsCache, "prs"); | |
| return true; | |
| }), | |
| refreshPrs: refreshGate.withPermits(1)(Effect.gen(function* () { | |
| const until = yield* parkRemaining; | |
| if (Option.isSome(until)) { | |
| return yield* new GitHubParkedError({ | |
| resetAtMillis: until.value, | |
| detail: "GitHub's rate limit is exhausted; Throughline is waiting it out.", | |
| }); | |
| } | |
| const now = yield* Clock.currentTimeMillis; | |
| const last = yield* Ref.get(lastRefreshAt); | |
| if (now - last < Duration.toMillis(MIN_REFRESH_INTERVAL)) { | |
| // Honest no-op: the reviewer asked, but the floor says not yet. | |
| return false; | |
| } | |
| yield* Cache.invalidate(prsCache, "prs"); | |
| yield* Cache.get(prsCache, "prs"); | |
| return true; | |
| })), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/github/GitHub.ts` around lines 386 - 403, Serialize the
refresh window in refreshPrs so concurrent callers cannot both pass the
lastRefreshAt check and invalidate prsCache. Add or reuse a dedicated
single-permit semaphore around the interval check and cache invalidate/get
sequence, while preserving the parked-rate-limit behavior and existing no-op
result when the refresh floor has not elapsed.
| // A zero-token probe: open a session whose prompt never yields, ask for the | ||
| // initialization result, and close it. No model turn is spent. | ||
| const account = yield* Effect.tryPromise({ | ||
| try: async () => { | ||
| const never = (async function* (): AsyncGenerator<SDKUserMessage> { | ||
| await new Promise<never>(() => {}); | ||
| })(); | ||
| const session = query({ | ||
| prompt: never, | ||
| options: { settingSources: [], strictMcpConfig: true, stderr: () => {} }, | ||
| }); | ||
| try { | ||
| const init = await session.initializationResult(); | ||
| return init.account ?? null; | ||
| } finally { | ||
| session.close(); | ||
| } | ||
| }, | ||
| catch: (cause) => cause, | ||
| }).pipe(Effect.timeoutOption("25 seconds"), Effect.option); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The probe session can outlive the timeout.
Effect.timeoutOption interrupts the Effect, but the underlying promise keeps running: if initializationResult() never settles, the finally that calls session.close() never runs and the spawned Claude process lingers for the life of the server. Since the settings page probes on every visit, these accumulate.
Tie the close to interruption as well — e.g. create the session outside the promise (or hold it in a box) and add Effect.onInterrupt/Effect.acquireRelease so cancellation closes it.
🔧 Sketch
- const account = yield* Effect.tryPromise({
- try: async () => {
- const never = (async function* (): AsyncGenerator<SDKUserMessage> {
- await new Promise<never>(() => {});
- })();
- const session = query({
- prompt: never,
- options: { settingSources: [], strictMcpConfig: true, stderr: () => {} },
- });
- try {
- const init = await session.initializationResult();
- return init.account ?? null;
- } finally {
- session.close();
- }
- },
- catch: (cause) => cause,
- }).pipe(Effect.timeoutOption("25 seconds"), Effect.option);
+ const box: { session: Query | null } = { session: null };
+ const account = yield* Effect.tryPromise({
+ try: async () => {
+ const never = (async function* (): AsyncGenerator<SDKUserMessage> {
+ await new Promise<never>(() => {});
+ })();
+ const session = query({
+ prompt: never,
+ options: { settingSources: [], strictMcpConfig: true, stderr: () => {} },
+ });
+ box.session = session;
+ const init = await session.initializationResult();
+ return init.account ?? null;
+ },
+ catch: (cause) => cause,
+ }).pipe(
+ Effect.timeoutOption("25 seconds"),
+ Effect.option,
+ Effect.ensuring(
+ Effect.sync(() => {
+ try {
+ box.session?.close();
+ } catch {
+ // already closed
+ }
+ }),
+ ),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // A zero-token probe: open a session whose prompt never yields, ask for the | |
| // initialization result, and close it. No model turn is spent. | |
| const account = yield* Effect.tryPromise({ | |
| try: async () => { | |
| const never = (async function* (): AsyncGenerator<SDKUserMessage> { | |
| await new Promise<never>(() => {}); | |
| })(); | |
| const session = query({ | |
| prompt: never, | |
| options: { settingSources: [], strictMcpConfig: true, stderr: () => {} }, | |
| }); | |
| try { | |
| const init = await session.initializationResult(); | |
| return init.account ?? null; | |
| } finally { | |
| session.close(); | |
| } | |
| }, | |
| catch: (cause) => cause, | |
| }).pipe(Effect.timeoutOption("25 seconds"), Effect.option); | |
| // A zero-token probe: open a session whose prompt never yields, ask for the | |
| // initialization result, and close it. No model turn is spent. | |
| const box: { session: Query | null } = { session: null }; | |
| const account = yield* Effect.tryPromise({ | |
| try: async () => { | |
| const never = (async function* (): AsyncGenerator<SDKUserMessage> { | |
| await new Promise<never>(() => {}); | |
| })(); | |
| const session = query({ | |
| prompt: never, | |
| options: { settingSources: [], strictMcpConfig: true, stderr: () => {} }, | |
| }); | |
| box.session = session; | |
| const init = await session.initializationResult(); | |
| return init.account ?? null; | |
| }, | |
| catch: (cause) => cause, | |
| }).pipe( | |
| Effect.timeoutOption("25 seconds"), | |
| Effect.option, | |
| Effect.ensuring( | |
| Effect.sync(() => { | |
| try { | |
| box.session?.close(); | |
| } catch { | |
| // already closed | |
| } | |
| }), | |
| ), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/harness/claude.ts` around lines 100 - 119, Update the
zero-token probe around the `query` session and `Effect.timeoutOption` so
interruption always invokes `session.close()`, even when
`initializationResult()` never settles. Hold the created session outside the
interruptible promise or manage it with `Effect.acquireRelease`, and attach
interruption cleanup via `Effect.onInterrupt` while preserving the existing
account result and timeout behavior.
| file: { | ||
| name: file.path, | ||
| contents: describeBinary(bundle), | ||
| cacheKey: `${file.path}:binary`, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@pierre/diffs processFile cacheKey caching behavior
💡 Result:
In the @pierre/diffs library, the cacheKey property is used to enable and manage a render cache, which helps avoid redundant highlighting work by offloading syntax highlighting to a worker pool [1][2]. Caching Behavior and Usage: 1. Enabling Caching: Caching is automatically enabled per-file or per-diff when a cacheKey is provided [1][2]. Items without a cacheKey are not cached [1][2]. 2. Automatic Cache Key Generation: - When using processFile, you can provide an optional cacheKeyPrefix to generate keys for individual files within a patch automatically (format: prefix-patchIndex-fileIndex) [1][3]. - When using parseDiffFromFile (which processes two file contents), if both oldFile and newFile have a cacheKey, the resulting diff is automatically assigned a combined cacheKey in the format oldKey:newKey [1][4]. 3. Render Cache Validation: The cache is not based solely on the key; it also validates against render options. If options such as the theme or line diff type change, the library skips the cached result and re-renders [1]. 4. Comparison: The library uses an internal function, areDiffTargetsEqual, to determine if diff targets are the same. It checks for strict equality or, if that fails, compares the cacheKey properties if they exist [5]. Best Practices: - Ensure the cacheKey is unique and stable, and change it whenever the underlying content changes to avoid serving stale data [2]. - Avoid using file contents directly as keys to prevent memory waste; instead, use stable identifiers like commit SHAs, file IDs, or version numbers [2].
Citations:
- 1: https://diffs.com/docs
- 2: https://context7.com/pierrecomputer/pierre/llms.txt?tokens=10000
- 3: https://app.unpkg.com/@pierre/diffs@1.2.11/files/dist/utils/parsePatchFiles.d.ts.map
- 4: https://app.unpkg.com/@pierre/diffs@1.2.11/files/dist/utils/parseDiffFromFile.js.map
- 5: https://app.unpkg.com/@pierre/diffs@1.2.11/files/dist/utils/areDiffTargetsEqual.js.map
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate CodeSurface.tsx =="
git ls-files | grep 'apps/web/src/journey/CodeSurface\.tsx$' || true
echo "== relevant CodeSurface sections =="
cat -n apps/web/src/journey/CodeSurface.tsx | sed -n '1,180p'
echo "== lines 180-360 =="
cat -n apps/web/src/journey/CodeSurface.tsx | sed -n '180,360p'
echo "== lines 360-430 =="
cat -n apps/web/src/journey/CodeSurface.tsx | sed -n '360,430p'
echo "== package version hints =="
if [ -f package.json ]; then jq '.dependencies["`@pierre/diffs`"], .devDependencies["`@pierre/diffs`"]' package.json 2>/dev/null || true; fi
for f in pnpm-lock.yaml package-lock.json yarn.lock; do
if [ -f "$f" ]; then
echo "-- $f --"
rg -n '`@pierre/diffs`' "$f" | head -20 || true
fi
doneRepository: AdiRishi/throughline
Length of output: 18233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lockfile `@pierre/diffs` 1.2.12 entry =="
sed -n '880,905p' pnpm-lock.yaml
echo "== installed package cache availability =="
if [ -d "$PWD/node_modules/@pierre/diffs" ]; then
echo "node_modules presence: YES"
find node_modules/@pierre/diffs -maxdepth 3 -type f | sed -n '1,80p'
else
echo "node_modules presence: NO"
fi
echo "== repo occurrences of use Atom cache keys =="
rg -n "clusterFilesAtom|bundleKey|processFile|cacheKey" apps apps/web 2>/dev/null | sed -n '1,160p' || true
echo "== candidate journeyState outline/file =="
git ls-files | grep 'state/journeyState'
cat -n apps/web/src/state/journeyState.ts | sed -n '1,260p'Repository: AdiRishi/throughline
Length of output: 14096
🌐 Web query:
@pierre/diffs 1.2.12 cacheKey areDiffTargetsEqual processFile source
💡 Result:
In the @pierre/diffs library (v1.2.12), the concepts of cacheKey, areDiffTargetsEqual, and processFile are integral to managing efficient rendering and caching of diff data [1]. 1. cacheKey: This is a string property used to enable caching of rendered diff results in the library's worker pool [1][2]. When a file or diff is assigned a cacheKey, the system can skip redundant processing if the same key is encountered again [1]. For example, in parseDiffFromFile, if both the old and new files provide a cacheKey, the library generates a combined key in the format oldKey:newKey [3]. Similarly, when processing patch files, an optional cacheKeyPrefix can be provided to automatically generate unique keys for each file (e.g., prefix-patchIndex-fileIndex) [1][2]. 2. areDiffTargetsEqual: This is a utility function used to compare two FileDiffMetadata objects [4]. Because FileDiffMetadata is a structurally deep object, the function performs a shallow equality check or, if available, compares their cacheKey properties to determine if they are equivalent [4]. Its implementation is: return (diffA === diffB || (diffA?.cacheKey!= null && diffA.cacheKey === diffB?.cacheKey)); [4] 3. processFile: This function is responsible for parsing a file diff string into a structured FileDiffMetadata object [2]. It accepts a configuration object that can include a cacheKey, along with file contents and processing options [2]. It is commonly used within higher-level parsing utilities, such as parseDiffFromFile, to ensure the resulting diff metadata is properly initialized and cached [3]. These utilities are part of the internal infrastructure that allows @pierre/diffs to optimize performance by avoiding expensive re-renders for unchanged files or previously processed diffs [1].
Citations:
- 1: https://diffs.com/docs
- 2: https://app.unpkg.com/@pierre/diffs@1.2.11/files/dist/utils/parsePatchFiles.d.ts.map
- 3: https://app.unpkg.com/@pierre/diffs@1.2.11/files/dist/utils/parseDiffFromFile.js.map
- 4: https://app.unpkg.com/@pierre/diffs@1.2.11/files/dist/utils/areDiffTargetsEqual.js.map
🌐 Web query:
@pierre/diffs 1.2.12 render cache are diff targets equal cache rendering
💡 Result:
In the @pierre/diffs library (version 1.2.12), the "render cache" is an experimental feature designed to improve performance by avoiding redundant syntax highlighting and rendering work for files or diffs [1]. Regarding whether diff targets are equal for cache rendering: The rendering cache functions based on a cacheKey property [1]. - Automatic Cache Keys: When providing oldFile and newFile data, the library automatically generates a combined cache key in the format oldKey:newKey if both files possess their own individual cacheKey [1]. - Cache Validation: The cache does not merely rely on the key; it actively validates against render options [1]. If configuration options such as the theme or the type of line diff change, the library skips the cached result and triggers a re-render [1]. - Granularity: Caching is handled on a per-file or per-diff basis [1]. Files or diffs that lack a cacheKey property will not be cached [1]. If a cacheKey is provided, the library will return the cached result for subsequent requests with the same key immediately, bypassing the worker pool processing [1]. You are generally encouraged to use the WorkerPoolContextProvider (in React) or pass the pool instance via the workerPool option (in Vanilla JS) to manage this behavior rather than interacting with the cache methods directly [1].
Citations:
🌐 Web query:
"@pierre/diffs" "return the cached result for subsequent requests" cacheKey
💡 Result:
In the @pierre/diffs library, the cacheKey property is used to enable and manage caching of rendered AST (Abstract Syntax Tree) results within the library's worker pool [1][2]. When a file or diff object is provided with a valid cacheKey, the library automatically returns the cached result for subsequent requests that use the same cacheKey, thereby avoiding redundant syntax highlighting and processing work [1][2]. Key implementation details include: 1. Automatic Caching: Caching is only active if a cacheKey is explicitly provided [1][2]. Objects without a cacheKey will not be cached [1][2]. 2. Validation: The library validates the cache against render options (e.g., theme, line diff type); if these change, the cached result is invalidated and re-rendered [1]. 3. Best Practices: To ensure cache effectiveness and correctness, users should provide stable, unique identifiers (such as commit SHAs or version numbers) [2]. It is critical that the cacheKey changes whenever the underlying content changes; otherwise, the library may serve stale, mismatched data [3][2]. 4. Combined Keys: When generating diffs from two file contents, if both files have a cacheKey, the library automatically generates a combined cacheKey in the format oldKey:newKey [4][1]. Developers should avoid using raw file contents as keys to prevent memory waste and must ensure that keys remain stable across re-renders (e.g., by using external state management or timestamps like React Query's dataUpdatedAt) to prevent unnecessary cache misses or stale hits [3][2].
Citations:
- 1: https://diffs.com/docs
- 2: https://context7.com/pierrecomputer/pierre/llms.txt
- 3: coder/coder@803cfeb
- 4: https://app.unpkg.com/@pierre/diffs@1.2.11/files/dist/utils/parseDiffFromFile.js.map
🌐 Web query:
@pierre/diffs 1.2.12 worker pool cache returns cache key same inputs
💡 Result:
In the @pierre/diffs library (version 1.2.12 and surrounding versions), the worker pool cache is designed to cache rendered AST results based on a provided cacheKey [1]. When a file or diff is associated with a cacheKey, subsequent requests with the same key will return cached results immediately from the worker pool instead of re-processing [1]. If the cache returns results for the same inputs, it is because the library treats the cacheKey as the unique identifier for the rendering task [1][2]. The worker pool does not automatically derive a cache key from the diff content itself [2]. Therefore, if the diff content or rendering parameters change but the cacheKey remains identical, the worker pool will serve the stale cached result associated with that key [2]. To prevent this issue, ensure the following practices: 1. Cache Key Uniqueness: The cacheKey must change whenever the underlying data or rendering conditions (such as the theme or diff content) change [1][3][2]. 2. Stable Identifiers: Avoid using file contents directly as keys to prevent memory issues [3]. Instead, use stable, composite identifiers that incorporate versioning or data update timestamps (e.g., ${prefix}-${dataUpdatedAt}) [3][2]. 3. Rendering Options: Be aware that the cache also validates against render options (like theme or line diff type) [1]. If these options change, the cached result is automatically skipped and re-rendered, even if the cacheKey is the same [1]. If you are experiencing issues where the UI does not update despite changing content, you must ensure that your cacheKey is properly invalidated by updating it to reflect the new state of the diff [2].
Citations:
- 1: https://diffs.com/docs
- 2: coder/coder@803cfeb
- 3: https://context7.com/pierrecomputer/pierre/llms.txt?tokens=10000
Include journey identity in the @pierre/diffs cache keys.
@pierre/diffs caches by cacheKey, and the path-only values (${file.path}:diff, ${file.path}:new, ${file.path}:binary) are stable across CodeSurface renders. Since bundleKey() already separates artifacts by journeyId, use the same identity in these keys; otherwise a reanalyzed journey or another PR that repeats a file path can receive the cached parse from the previous render instead of the current bundle contents.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/journey/CodeSurface.tsx` around lines 280 - 284, Update the
cacheKey values in CodeSurface’s diff, new, and binary file descriptors to
include the journey identity used by bundleKey(), rather than relying on
file.path alone. Preserve the existing suffixes and ensure each key changes when
journeyId changes, preventing `@pierre/diffs` from reusing cached results across
journeys.
| case "list": { | ||
| const list = token as Tokens.List; | ||
| const items = list.items.map((item, index) => ( | ||
| <li key={index}>{renderInline(item.tokens ?? [], handlers)}</li> | ||
| )); | ||
| return list.ordered ? <ol>{items}</ol> : <ul>{items}</ul>; | ||
| } | ||
| case "list_item": | ||
| return <li>{renderInline((token as Tokens.ListItem).tokens ?? [], handlers)}</li>; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
marked lexer list item tokens loose paragraph nested list
💡 Result:
In the CommonMark specification, a list is classified as either loose or tight, a distinction that determines whether the content of its list items is wrapped in HTML
tags [1][2][3]. A list is considered loose if any of its constituent list items are separated by blank lines, or if any of its list items directly contain two or more block-level elements separated by a blank line [1][2][4]. Conversely, a list is tight if neither of these conditions is met [1]. Key technical aspects include: 1. List-Level Property: The loose or tight status applies to the entire list, not individual items [3]. A list cannot be partially loose and partially tight; if a single item within the list triggers the "loose" criteria, the entire list is rendered as loose [4][3]. 2. Impact on Tokens: In parsers like markdown-it, this distinction is often handled during the transformation from block-level tokens to HTML [2]. Because the property is list-wide, some implementations use a hidden or internal property on the list's child elements (such as paragraphs) to determine if they should be wrapped in
tags, rather than exposing an explicit tight/loose flag on the list token itself [2]. 3. Nested Lists: Ambiguity sometimes arises with nested lists regarding whether blank lines within a sub-list force the outer list to become loose [1]. The prevailing interpretation is that "directly contain" refers to immediate children of the list item; therefore, block elements contained within a nested list are generally not considered direct children of the parent list item, preventing the nested structure from automatically making the outer list loose based on the sub-list's internal spacing [1]. Because CommonMark prioritizes consistent typographic output across different formats (such as HTML and LaTeX), the specification mandates this global classification to avoid mixed spacing, which is often considered typographically undesirable [4][3].
Citations:
- 1: Clarify whether loose lists are transitive commonmark/commonmark-spec#803
- 2: Is there an easy way to determine if a list if tight or loose markdown-it/markdown-it#678
- 3: https://talk.commonmark.org/t/inconsistent-wrapping-of-list-item-contents-inside-p-elements/2243
- 4: https://talk.commonmark.org/t/list-line-spacing-is-crazy/2114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Package manifests mentioning marked:"
rg -n '"marked"|marked@' package.json pnpm-lock.yaml yarn.lock package-lock.json apps 2>/dev/null | head -n 80
echo
echo "Locate Markdown.tsx and inspect relevant sections:"
fd -a 'Markdown.tsx' .
file="$(fd 'Markdown.tsx' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,230p' "$file" | cat -n
fi
echo
echo "Search for list usage/tests:"
rg -n "Markdown|renderInlineToken|renderInline|Tokens|list_item|list_item|loose|\n\n|nested|\\\\n\\\\n" apps tests spec docs . -g '!pnpm-lock.yaml' -g '!package-lock.json' 2>/dev/null | head -n 200Repository: AdiRishi/throughline
Length of output: 412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate Markdown.tsx and inspect relevant sections:"
file="$(fd 'Markdown.tsx' . | head -n 1)"
echo "file=$file"
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,230p' "$file" | cat -n
fi
echo
echo "Search for Markdown usages/tests involving lists:"
rg -n "Markdown\\(|<Markdown|renderInlineToken|renderInline|Tokens|list_item|loose|nested|\\*\\*|1\\.\\s.*\n\n" apps tests 2>/dev/null | head -n 250 || true
echo
echo "Inspect installed marked package metadata if present in sandbox:"
if [ -d node_modules/marked ]; then
cat node_modules/marked/package.json | jq '{version}'
rg -n "ListItem|function token|Parser|Renderer|Lexer" node_modules/marked/lib -m 20 || true
else
echo "node_modules/marked not present"
fiRepository: AdiRishi/throughline
Length of output: 9333
🌐 Web query:
marked 18.0.7 list_item tokens block Token ListItem.tokens
💡 Result:
The term marked 18.0.7 refers to a specific release of the Markdown parser library markedjs/marked, which was released on July 21, 2026 [1][2]. The release notes for version 18.0.7 do not explicitly mention a tokens block or a property named ListItem.tokens [1][2]. However, the phrase list_item tokens commonly appears in the context of Markdown parsing libraries, such as mistletoe, which defines a ListItem block token that contains children tokens (the content of the list item) [3]. In libraries like marked, the internal architecture processes Markdown into an array of tokens, and nested structures (like lists) often contain a list_item token type, which in turn may hold further tokens representing the item's contents. If you are encountering references to ListItem.tokens in a codebase using marked 18.0.7, it is likely part of the library's internal AST (Abstract Syntax Tree) representation or a custom extension, as the library structures parsed content into hierarchical token objects [1]. Given the recent release, ensure you are referencing the official repository documentation for the most accurate details on the current AST schema [1].
Citations:
- 1: https://github.com/markedjs/marked/releases
- 2: https://github.com/markedjs/marked/releases/tag/v18.0.7
- 3: https://github.com/miyuchina/mistletoe/blob/master/mistletoe/block_token.py
🌐 Web query:
site:github.com/markedjs/marked ListItem types.ts Token ListItem tokens
💡 Result:
In the markedjs/marked library, ListItem and List tokens are fundamental components of the library's abstract syntax tree (AST) used to represent Markdown lists [1]. These types are defined within the library's TypeScript source code, typically found in src/Tokens.ts [1][2]. The ListItem token represents an individual item within a list and is structured as follows [1]: interface ListItem { type: 'list_item'; raw: string; task: boolean; checked?: boolean | undefined; loose: boolean; text: string; tokens?: Token[]; } Key properties of the ListItem token include: - type: Always set to 'list_item' [1]. - task: A boolean indicating if the list item is a task list item (e.g., [ ] or [x]) [1][3]. - checked: An optional boolean that specifies if a task is checked [1][3]. - loose: A boolean that indicates whether the list is loose (items are separated by blank lines) or tight [1][3]. - tokens: An optional array of nested Token objects that represent the content within the list item [1]. The List token acts as a container for these items [1]: interface List { type: 'list'; raw: string; ordered: boolean; start: number | ''; loose: boolean; items: ListItem[]; } These tokens are processed by the library's Parser and Renderer, which use the nested tokens array to recursively parse the content of each list item [4][3]. When implementing custom renderers or using the walkTokens method, developers interact with these structures to manipulate or transform the parsed Markdown AST [5][6][7].
Citations:
- 1: https://github.com/markedjs/marked/blob/master/src/Tokens.ts
- 2: markedjs/marked@cb54906
- 3: https://github.com/markedjs/marked/blob/0ccf4769/src/Renderer.ts
- 4: https://github.com/markedjs/marked/blob/master/src/Renderer.ts
- 5: add walkTokens and fix highlight async markedjs/marked#1664
- 6: https://github.com/markedjs/marked/blob/master/docs/USING_PRO.md
- 7: https://github.com/markedjs/marked/blob/0ccf4769/test/types/marked.ts
🌐 Web query:
markedjs marked v18.0.7 source ListItem tokens parsing paragraph text
💡 Result:
In markedjs marked v18.0.7, the parsing of list items and their internal text is handled through a structured tokenization process involving the Lexer, Tokenizer, and Parser modules [1][2][3][4]. Structure and Tokenization: - The List token contains an array of ListItem tokens [1][5]. - Each ListItem token is defined by the following interface [5]: interface ListItem { type: 'list_item'; raw: string; task: boolean; checked?: boolean; loose: boolean; text: string; tokens: Token[]; } - The text property holds the raw string content of the list item, while the tokens property contains the parsed inline tokens (such as text, emphasis, strong, or links) derived from that content [1][5]. Parsing Logic: 1. Block Level: The Lexer identifies a list structure using regex rules defined in rules.ts (e.g., listItem regex) [6]. It uses the Tokenizer.list method to isolate the list block [3][4]. 2. List Item Isolation: Within the Tokenizer, the code calculates the indentation and extracts item content. It normalizes tabs (typically to 4 spaces) to ensure consistent parsing, regardless of whether tabs or spaces were used after the list marker [7][8]. 3. Paragraph vs. Text: The tokens array within a ListItem is populated by parsing the item's content. If the list is considered "loose" (containing blank lines between items), the items may contain block-level elements like paragraphs [1][5]. Otherwise, they often contain inline-level tokens [1][9]. 4. Tokenization of Content: The text within the ListItem is processed to produce the tokens array, which allows the renderer to convert the markdown into HTML correctly [1][9]. Recent refinements in version 18.x have focused on edge cases, such as correctly handling empty list items with trailing spaces and normalizing whitespace to prevent items from being incorrectly identified as indented code blocks [7][10].
Citations:
- 1: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 2: https://github.com/markedjs/marked/blob/0ccf4769/src/Lexer.ts
- 3: https://github.com/markedjs/marked/blob/0ccf4769/src/Tokenizer.ts
- 4: https://github.com/markedjs/marked/blob/0a2cd54c/src/Lexer.ts
- 5: https://github.com/markedjs/marked/blob/master/src/Tokens.ts
- 6: https://github.com/markedjs/marked/blob/0a2cd54c/src/rules.ts
- 7: bug: Marked treats tabs and spaces differently in lists markedjs/marked#3784
- 8: https://github.com/markedjs/marked/blob/master/src/Tokenizer.ts
- 9: https://marked.js.org/using_advanced
- 10: fix: parse empty list item with trailing space markedjs/marked#3984
🌐 Web query:
markedjs marked 18.0.7 list_item token raw text in Renderer
💡 Result:
In marked.js version 18.0.7, the Renderer.listitem method receives a Tokens.ListItem object as its argument rather than a raw text string [1][2]. The Tokens.ListItem interface is structured as follows [1][3]: interface ListItem { type: "list_item"; raw: string; task: boolean; checked?: boolean; loose: boolean; text: string; tokens: Token[]; } Key aspects of working with listitem in the Renderer include: 1. Method Signature: The Renderer.listitem method is defined as listitem(item: Tokens.ListItem): RendererOutput [1]. 2. Processing Content: Since modern marked versions utilize a token-based architecture, the content of the list item is contained within the item.tokens array [3][4]. To render the internal content of the list item, you should typically use this.parser.parse(item.tokens,!!item.loose) within your custom renderer method [5]. 3. Accessing Raw Text: If you specifically need the raw text representation of the list item, it is available via the item.raw property [1]. However, for standard rendering purposes, iterating over and parsing the nested tokens is the expected approach to ensure correct handling of inline elements like links, emphasis, and other nested structures [5][6]. If you are overriding listitem to customize the output, you can access the item's properties (such as item.task or item.checked) to conditionally modify the resulting HTML [5].
Citations:
- 1: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 2: https://www.jsdocs.io/package/marked
- 3: https://github.com/markedjs/marked/blob/master/src/Tokens.ts
- 4: Inline tokens markedjs/marked#1627
- 5: https://github.com/markedjs/marked/blob/0ccf4769/src/Renderer.ts
- 6: https://github.com/markedjs/marked/blob/0a2cd54c/docs/USING_PRO.md
🌐 Web query:
markedjs marked 18.0.7 Token ListItem has text property raw tokens
💡 Result:
In marked version 18.0.7, the ListItem token interface explicitly includes both a text property and a tokens property [1]. According to the library's official type definitions for version 18.0.7, the ListItem interface is defined as follows: interface ListItem { type: "list_item"; raw: string; task: boolean; checked?: boolean; loose: boolean; text: string; tokens: Token[]; } The text property contains the raw text content of the list item, while the tokens property holds an array of child tokens (such as text, strong, em, etc.) that represent the parsed structure of that content [1][2]. This structure allows you to access both the unparsed text and the hierarchical token representation [1][3].
Citations:
- 1: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 2: https://github.com/markedjs/marked/blob/master/src/Tokens.ts
- 3: https://github.com/markedjs/marked/blob/0ccf4769/test/types/marked.ts
🌐 Web query:
marked 18.0.7 listItem parser block tokens Text type Text.tokens undefined
💡 Result:
The issue you are encountering, involving marked.js version 18.0.7 and potentially related to Text.tokens being undefined during listItem parsing, is likely a result of how the library handles token structures, particularly in newer versions that have undergone significant refactoring of the block/list tokenization logic [1][2][3]. There is no widespread bug report specifically stating "Text.tokens undefined" in version 18.0.7 itself, but several recent issues in the marked repository (around versions 15-18) involve the complexity of list and list item tokens, custom renderers, and type narrowing [4][5]. Key points to consider: 1. Token Structure and Type Narrowing: Since v7.0.2, and continuing into the v18.x series, the internal handling of Token types has become more specific [4]. If you are implementing a custom renderer or interacting with the tokens manually, you may find that properties like tokens (which contain inline tokens for paragraphs or list items) are not always present if the underlying content is simple or empty [6][4][7]. 2. Compatibility and Environment: If you are seeing errors related to methods like Array.prototype.at or undefined properties when accessing token lists, ensure your environment supports ES2022 or that you have necessary polyfills [8]. 3. List Item Parsing: Recent versions (including 18.0.7) have included significant fixes for list parsing, specifically addressing empty list items and list item indentation [1][9][10]. If you are manually manipulating tokens or using an outdated custom renderer, the structure expected by the parser might have shifted [5]. Recommendations: - Check your Token: If you are accessing token.tokens, add an optional chaining check (token.tokens?.length or similar) to ensure the property exists before usage, as Tokens.Text objects do not always require a tokens array [6][4]. - Validate Custom Renderers: If you are using a custom renderer, ensure it correctly handles the structure of list items defined in the current version of marked [5]. - Update/Verify Environment: Ensure that your build process (e.g., Vite, esbuild, or Webpack) is correctly handling the modern JavaScript features used in marked v18.x, as discrepancies in polyfills can sometimes surface as "undefined" errors [8]. If this persists, I recommend creating a minimal reproduction and opening an issue on the official marked GitHub repository, as the maintainers are actively addressing list-related parsing edge cases [1][5][2].
Citations:
- 1: ⬆️ bump marked from 18.0.6 to 18.0.7 notthebestdev/markdown-to-docx#142
- 2: https://github.com/chjj/marked/releases/tag/v18.0.7
- 3: fix: fix empty list item markedjs/marked#3890
- 4: Cannot narrow Token type since v7.0.2 markedjs/marked#2938
- 5: list renderer bug markedjs/marked#3870
- 6: https://app.unpkg.com/marked@11.1.0/files/lib/marked.d.ts
- 7: How to convert a token's type markedjs/marked#3476
- 8: [bug]
tokens.atis not a function markedjs/marked#3546 - 9: fix: parse empty list item with trailing space markedjs/marked#3984
- 10: fix: fix list item wrong indent markedjs/marked#3889
🌐 Web query:
marked 18.0.7 src Tokenizer.js list item tokens paragraph text tokens list_item
💡 Result:
In the Marked library (version 18.0.7), the handling of list items, paragraph tokens, and text tokens is managed primarily within the Tokenizer class and the Lexer's orchestration of block tokens [1][2][3]. Key aspects of how these tokens are processed include: 1. List Items (list_item): When the Tokenizer encounters a list, it identifies individual items [1]. These are stored as ListItem objects, which contain properties such as type ('list_item'), raw text, task status, loose list status, and a tokens array [2][4]. The Tokenizer is responsible for parsing these items and populating their corresponding tokens, often recursively invoking the Lexer to process the content within each list item [1]. 2. Paragraph and Text Tokens: Inside a list item, content is typically tokenized into paragraph or text tokens depending on the list's "loose" or "tight" status [5]. In loose lists, list items often contain paragraph tokens to properly handle spacing and block-level styling [5]. These tokens have a tokens property that contains the parsed inline elements [1][2][4]. 3. Recent Changes: Significant updates, such as those found in recent versions leading up to 18.0.7 (e.g., related to PR #3755), have refined this process [5]. Changes include: - Consolidating token creation to occur strictly within the tokenizer methods [5]. - Adjusting how loose list text tokens are represented (often promoting them to paragraph type within the list tokenizer) [5]. - Improving the consistency of how checkboxes and inline content are handled within list items [1][5]. The Lexer acts as the engine that drives this process by iterating through the input source and calling the appropriate Tokenizer methods (e.g., list, paragraph) to produce the structured token list [6][3][7].
Citations:
- 1: https://github.com/markedjs/marked/blob/master/src/Tokenizer.ts
- 2: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 3: https://readmex.com/en-US/markedjs/marked/page-9ad87468c-c086-4c79-a7d3-5566499c2ef2
- 4: https://github.com/markedjs/marked/blob/0ccf4769/src/Tokens.ts
- 5: markedjs/marked@7b19231
- 6: https://github.com/markedjs/marked/blob/0ccf4769/src/Lexer.ts
- 7: https://github.com/markedjs/marked/blob/0a2cd54c/src/Lexer.ts
🌐 Web query:
marked @18.0.7 lib marked.d.ts ListItem tokens Text tokens
💡 Result:
In marked version 18.0.7, the relationship between ListItem tokens and Text tokens is defined by the Token structure used by the parser [1][2]. A ListItem token (Tokens.ListItem) is part of a List token and contains a 'tokens' property, which is an array of child tokens (Token[]) [1][3]. These child tokens represent the parsed content of the list item [4]. A Text token (Tokens.Text) is one of the possible types within the Token[] array [1][3]. The structure of a ListItem is as follows: interface ListItem { type: 'list_item'; raw: string; task: boolean; checked?: boolean; loose: boolean; text: string; // The raw text content of the list item tokens: Token[]; // The parsed child tokens (including Text, Paragraph, List, etc.) } When marked parses a list item, it populates the 'tokens' array with the block-level elements found within that item, such as Paragraph or Text tokens [5][6]. If the content of the list item is simple text, it will often be contained within a Text token inside this 'tokens' array [5][6]. Additionally, the 'text' property on the ListItem interface contains the unparsed, raw text content of the list item [1][3]. For custom rendering or processing, you can access these child tokens via the 'tokens' array in the listitem renderer method: listitem(item: Tokens.ListItem): RendererOutput { return this.parser.parse(item.tokens); } This ensures that any nested elements (like inline text, bold, or links) are properly processed through the parser's token hierarchy [4].
Citations:
- 1: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 2: https://github.com/markedjs/marked/releases/tag/v18.0.7
- 3: https://github.com/markedjs/marked/blob/0ccf4769/src/Tokens.ts
- 4: https://github.com/markedjs/marked/blob/master/src/Renderer.ts
- 5: https://github.com/markedjs/marked/blob/0a2cd54c/test/types/marked.ts
- 6: https://github.com/markedjs/marked/blob/0ccf4769/test/types/marked.ts
🌐 Web query:
unpkg marked@18.0.7 lib marked.js listItem Parser.parse list_item tokens
💡 Result:
In the marked library (version 18.0.7), the processing of list items is handled through a coordinated effort between the Lexer, Tokenizer, and Parser components. The library defines a list item structure in its type definitions as follows [1][2]: interface ListItem { type: 'list_item'; raw: string; task: boolean; checked?: boolean; loose: boolean; text: string; tokens: Token[]; } The lifecycle of a list item token is as follows: 1. Lexing/Tokenization: The Tokenizer (often invoked via the Lexer) identifies list structures in the Markdown source [3][4]. When a list is found, it parses the individual items and creates ListItem tokens, which contain the text and a nested array of tokens (tokens: Token[]) representing the content of that item [2][3]. 2. Parsing: The Parser, specifically its listitem method defined in the Renderer, is responsible for transforming these ListItem tokens into HTML [5]. The relevant part of the Parser workflow involves iterating through the items of a list token and invoking the listitem renderer [5]: listitem(item: Tokens.ListItem): RendererOutput { return <li>${this.parser.parse(item.tokens)}</li> \n as RendererOutput; } Note that in the internal implementation, the Parser's static parse method (or instance parse method) is used to recursively process the nested tokens stored within the list item [1][5]. This recursive call ensures that any block-level or inline Markdown inside the list item is correctly parsed into HTML [5]. For developers using the library, this is largely handled automatically when calling marked.parse on a string containing list syntax [6][7]. Accessing or manipulating these tokens directly is typically done through advanced customization or extension APIs, such as walkTokens [1][8].
Citations:
- 1: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 2: https://github.com/markedjs/marked/blob/0ccf4769/src/Tokens.ts
- 3: https://github.com/markedjs/marked/blob/master/src/Tokenizer.ts
- 4: https://github.com/markedjs/marked/blob/0a2cd54c/src/Lexer.ts
- 5: https://github.com/markedjs/marked/blob/master/src/Renderer.ts
- 6: https://www.npmjs.com/package/marked
- 7: https://marked.js.org/
- 8: https://marked.js.org/using_advanced
🌐 Web query:
raw unparsed markdown Text Token marked 18.0.7
💡 Result:
The term 18.0.7 typically refers to a specific version number of a software package or framework, rather than a specific type of markdown token [1][2][3]. In the context of the marked library—a widely used JavaScript markdown parser—version 18.0.7 was released on July 21, 2026 [1][4]. Within the marked library, a "token" (such as MarkedToken or simply Token) represents a discrete element parsed from markdown source (e.g., blockquotes, code spans, lists) [5]. However, "18.0.7" itself is not a specific markdown token, but rather the version of the software that handles such tokens [1][4]. Other software projects also utilize version 18.0.7, such as Angular and Dolibarr, which maintain their own release histories under this version number [2][6][3]. If you encountered "18.0.7" in a technical document or error message, it is most likely identifying the version of the tool you are using [1][4].
Citations:
- 1: https://github.com/markedjs/marked/releases/tag/v18.0.7
- 2: https://github.com/angular/angular-cli/releases/tag/18.0.7
- 3: https://github.com/Dolibarr/dolibarr/releases/tag/18.0.7
- 4: https://www.npmjs.com/package/marked
- 5: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 6: angular/angular@18.0.6...18.0.7
🌐 Web query:
marked@18.0.7 ListItem text string raw body ListItem tokens Parser.parse item.tokens
💡 Result:
In marked@18.0.7, the ListItem interface defines the structure of a list item token [1][2]. The relevant properties are as follows: raw: A string containing the exact Markdown source text that was consumed to create this list item [1][2]. text: A string containing the text content of the list item [1][2]. tokens: An array of Token objects representing the parsed content within the list item [1][2]. When the marked parser encounters a list item, it populates these properties during the tokenization phase [3]. The Parser component then handles these items during the rendering process [4]. Specifically, when the Renderer processes a list (and subsequently its items via the listitem method), it calls parser.parse(item.tokens) to recursively parse and generate HTML for the content held within the tokens array of that list item [5][6]. This architecture allows marked to support nested block-level elements within list items [7].
Citations:
- 1: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 2: https://github.com/markedjs/marked/blob/master/src/Tokens.ts
- 3: https://github.com/markedjs/marked/blob/master/src/Tokenizer.ts
- 4: https://deepwiki.com/markedjs/marked/2.2-parser-and-rendering-system
- 5: https://github.com/markedjs/marked/blob/0ccf4769/src/Renderer.ts
- 6: https://github.com/markedjs/marked/blob/master/src/Renderer.ts
- 7: Inline tokens markedjs/marked#1627
🌐 Web query:
marked@18.0.7 ListItem tokens loose item.text no tokens or paragraph ListItem text property
💡 Result:
In marked v18.0.7, the ListItem interface includes both a text property (string) and a tokens property (Token[]) [1][2]. The loose property is a boolean that indicates whether the list item is part of a loose list (i.e., whether it contains block-level elements like paragraphs) [1][2][3]. For a ListItem, the tokens property contains the parsed tokens representing the content within that item [4]. If the list item is tight, the tokens array typically contains a single Text token (or other inline tokens). If the list item is loose, the content is parsed as block-level elements, and the tokens array will contain Paragraph tokens (or other block tokens), which themselves contain their own text and tokens properties [1][2]. In marked's architecture, the text property is a string representation of the raw content of the list item, while the tokens property provides the structured, hierarchical representation of that content [1][2]. If you are working with these tokens, you should generally traverse the tokens array for structured data rather than relying solely on the text property, as the tokens property accurately reflects the nested structure (e.g., paragraphs within list items) that distinguishes loose from tight lists [1][4].
Citations:
- 1: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
- 2: https://github.com/markedjs/marked/blob/0ccf4769/src/Tokens.ts
- 3: loose lists markedjs/marked#1304
- 4: Inline tokens markedjs/marked#1627
🌐 Web query:
marked 18.0.7 Text token raw unparsed markdown ListItem text property
💡 Result:
In version 18.0.7 of the marked library, the ListItem token structure is defined within the Tokens namespace as follows: interface ListItem { type: "list_item"; raw: string; task: boolean; checked?: boolean; loose: boolean; text: string; tokens: Token[]; } The text property within this interface contains the raw, unparsed markdown text of the list item [1][2][3]. While marked parses content into a hierarchical structure of tokens (accessible via the tokens property), the text property itself preserves the original string representation for that list item [1][3]. There is no specific property named raw unparsed markdown listed in the official TypeScript definitions; instead, the raw property at the root of the ListItem object represents the full raw markdown string for the entire list item, and the text property provides the extracted text content [1][3].
Citations:
- 1: https://app.unpkg.com/marked@12.0.2/files/lib/marked.d.ts
- 2: https://app.unpkg.com/marked@15.0.7/files/lib/marked.d.ts
- 3: https://fastly.jsdelivr.net/npm/marked@18.0.7/lib/marked.d.ts
Render list-item children through the block renderer.
ListItem.tokens contains block-level tokens for list content, including nested list tokens. The inline renderer does not handle list, and List has no text property, so nested list content returns null and disappears. Route these children through <Tokens/>; loose list content is already handled by the block token chain.
🐛 Proposed fix
case "list": {
const list = token as Tokens.List;
const items = list.items.map((item, index) => (
- <li key={index}>{renderInline(item.tokens ?? [], handlers)}</li>
+ <li key={index}>
+ <Tokens tokens={item.tokens ?? []} handlers={handlers} />
+ </li>
));
return list.ordered ? <ol>{items}</ol> : <ul>{items}</ul>;
}
case "list_item":
- return <li>{renderInline((token as Tokens.ListItem).tokens ?? [], handlers)}</li>;
+ return (
+ <li>
+ <Tokens tokens={(token as Tokens.ListItem).tokens ?? []} handlers={handlers} />
+ </li>
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "list": { | |
| const list = token as Tokens.List; | |
| const items = list.items.map((item, index) => ( | |
| <li key={index}>{renderInline(item.tokens ?? [], handlers)}</li> | |
| )); | |
| return list.ordered ? <ol>{items}</ol> : <ul>{items}</ul>; | |
| } | |
| case "list_item": | |
| return <li>{renderInline((token as Tokens.ListItem).tokens ?? [], handlers)}</li>; | |
| case "list": { | |
| const list = token as Tokens.List; | |
| const items = list.items.map((item, index) => ( | |
| <li key={index}> | |
| <Tokens tokens={item.tokens ?? []} handlers={handlers} /> | |
| </li> | |
| )); | |
| return list.ordered ? <ol>{items}</ol> : <ul>{items}</ul>; | |
| } | |
| case "list_item": | |
| return ( | |
| <li> | |
| <Tokens tokens={(token as Tokens.ListItem).tokens ?? []} handlers={handlers} /> | |
| </li> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/ui/Markdown.tsx` around lines 75 - 83, Update the list rendering
in the Markdown component to render each list item’s block-level tokens through
the block renderer component, such as Tokens, instead of renderInline. Apply
this to both the item mapping in the "list" case and the "list_item" case,
preserving ordered/unordered list selection and existing token fallbacks so
nested lists and loose content render correctly.
| export const requestWhenConnected = <TTag extends UnaryRpcTag>( | ||
| tag: TTag, | ||
| input: RpcInput<TTag>, | ||
| ): Effect.Effect<RpcSuccess<TTag>, RpcFailure<TTag> | RpcUnavailableError, ConnectionSupervisor> => | ||
| Effect.gen(function* () { | ||
| const supervisor = yield* ConnectionSupervisor; | ||
| yield* SubscriptionRef.changes(supervisor.session).pipe( | ||
| Stream.filter(Option.isSome), | ||
| Stream.take(1), | ||
| Stream.runDrain, | ||
| ); | ||
| return yield* request(tag, input); | ||
| }).pipe( | ||
| Effect.withSpan("clientRuntime.rpc.requestWhenConnected", { | ||
| attributes: { "rpc.method": tag }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The wait for a session is unbounded — a never-connecting socket leaves fetch-on-mount surfaces spinning forever.
Stream.take(1) only completes when session becomes Some. If the supervisor is blocked (bad credentials, server down), every atom built on this helper (journeyAtom, treeAtom, harnessReportAtom, settingsAtom) stays pending with no error to render, which is worse than the fail-fast request it wraps. A bound that falls back to RpcUnavailableError keeps the deep-link improvement while preserving a visible failure.
🛡️ Bound the wait
yield* SubscriptionRef.changes(supervisor.session).pipe(
Stream.filter(Option.isSome),
Stream.take(1),
Stream.runDrain,
+ Effect.timeoutFail({
+ duration: "15 seconds",
+ onTimeout: () => new RpcUnavailableError({ tag }),
+ }),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const requestWhenConnected = <TTag extends UnaryRpcTag>( | |
| tag: TTag, | |
| input: RpcInput<TTag>, | |
| ): Effect.Effect<RpcSuccess<TTag>, RpcFailure<TTag> | RpcUnavailableError, ConnectionSupervisor> => | |
| Effect.gen(function* () { | |
| const supervisor = yield* ConnectionSupervisor; | |
| yield* SubscriptionRef.changes(supervisor.session).pipe( | |
| Stream.filter(Option.isSome), | |
| Stream.take(1), | |
| Stream.runDrain, | |
| ); | |
| return yield* request(tag, input); | |
| }).pipe( | |
| Effect.withSpan("clientRuntime.rpc.requestWhenConnected", { | |
| attributes: { "rpc.method": tag }, | |
| }), | |
| ); | |
| export const requestWhenConnected = <TTag extends UnaryRpcTag>( | |
| tag: TTag, | |
| input: RpcInput<TTag>, | |
| ): Effect.Effect<RpcSuccess<TTag>, RpcFailure<TTag> | RpcUnavailableError, ConnectionSupervisor> => | |
| Effect.gen(function* () { | |
| const supervisor = yield* ConnectionSupervisor; | |
| yield* SubscriptionRef.changes(supervisor.session).pipe( | |
| Stream.filter(Option.isSome), | |
| Stream.take(1), | |
| Stream.runDrain, | |
| Effect.timeoutFail({ | |
| duration: "15 seconds", | |
| onTimeout: () => new RpcUnavailableError({ tag }), | |
| }), | |
| ); | |
| return yield* request(tag, input); | |
| }).pipe( | |
| Effect.withSpan("clientRuntime.rpc.requestWhenConnected", { | |
| attributes: { "rpc.method": tag }, | |
| }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-runtime/src/rpc/client.ts` around lines 100 - 116, Bound the
session-waiting flow in requestWhenConnected so it cannot remain pending
indefinitely when the supervisor is blocked or never connects. Add a timeout
around the SubscriptionRef.changes(...).pipe(Stream.filter(Option.isSome),
Stream.take(1), Stream.runDrain) wait and convert timeout expiry into the
existing RpcUnavailableError, while preserving the immediate request path once a
session becomes available.
| const EVIDENCE_URI = /tl:(hunk|file|symbol)\/([^\s)\]<>"'`]+)/g; | ||
|
|
||
| /** | ||
| * A bare link at the end of a sentence — "…is added in tl:hunk/h1." — must not | ||
| * swallow the punctuation that follows it. Trailing sentence marks and slashes | ||
| * are never part of a hunk id, a path, or a symbol name, so they are trimmed | ||
| * back off the match. Internal dots (`tl:file/src/a.ts`) are untouched. | ||
| */ | ||
| const TRAILING_PUNCTUATION = /[.,;:!?/]+$/; | ||
|
|
||
| /** Parse one `tl:` URI. Returns null for anything that is not one. */ | ||
| export function parseEvidenceUri(uri: string): EvidenceLink | null { | ||
| const match = /^tl:(hunk|file|symbol)\/([^\s]+)$/.exec(uri.trim()); | ||
| if (match === null) return null; | ||
| const [, kind, rest] = match; | ||
| if (kind === undefined || rest === undefined || rest.length === 0) return null; | ||
|
|
||
| if (kind === "hunk") { | ||
| return { kind: "hunk", hunkId: rest, raw: uri.trim() }; | ||
| } | ||
| if (kind === "file") { | ||
| return { kind: "file", path: rest, raw: uri.trim() }; | ||
| } | ||
| // A symbol URI is `path#symbol`; the last `#` separates them so paths | ||
| // containing `#` still resolve. | ||
| const separator = rest.lastIndexOf("#"); | ||
| if (separator <= 0 || separator === rest.length - 1) return null; | ||
| return { | ||
| kind: "symbol", | ||
| path: rest.slice(0, separator), | ||
| symbol: rest.slice(separator + 1), | ||
| raw: uri.trim(), | ||
| }; | ||
| } | ||
|
|
||
| /** Every resolvable-shaped `tl:` link in a markdown string, in order. */ | ||
| export function extractEvidenceLinks(markdown: string): ReadonlyArray<EvidenceLink> { | ||
| const links: EvidenceLink[] = []; | ||
| for (const match of markdown.matchAll(EVIDENCE_URI)) { | ||
| const trimmed = match[0].replace(TRAILING_PUNCTUATION, ""); | ||
| const parsed = parseEvidenceUri(trimmed); | ||
| if (parsed !== null) links.push(parsed); | ||
| } | ||
| return links; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Bare-link scanning rules are duplicated in the web app.
apps/web/src/ui/Markdown.tsx (withBareEvidence, lines 170-190) re-implements this module's URI pattern and the /[.,;:!?/]+$/ trim with its own BARE_EVIDENCE regex. The two will drift, and they are the same rule. Consider exporting a scanner from here (returning { link, start, end }) and having Markdown.tsx render over it.
As per coding guidelines, "extract reusable logic into separate modules, avoid duplicating logic across files".
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 51-51: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/journey/src/evidence.ts` around lines 39 - 83, The bare-link
scanning logic is duplicated between extractEvidenceLinks and Markdown.tsx’s
withBareEvidence. Export a reusable scanner from evidence.ts that applies
EVIDENCE_URI and TRAILING_PUNCTUATION and returns each parsed link with its
start and end offsets, then update withBareEvidence to use that scanner instead
of its local BARE_EVIDENCE regex and trimming logic.
Source: Coding guidelines
| export function downgradeEvidenceLink(markdown: string, raw: string): string { | ||
| const escaped = raw.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const asMarkdownLink = new RegExp(`\\[([^\\]]*)\\]\\(\\s*${escaped}\\s*\\)`, "g"); | ||
| const withoutLinks = markdown.replace(asMarkdownLink, (_match, text: string) => | ||
| text.length > 0 ? text : readableTail(raw), | ||
| ); | ||
| return withoutLinks.split(raw).join(readableTail(raw)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bare-link downgrade is prefix-unsafe and will mangle sibling links.
split(raw).join(...) on Line 98 matches any occurrence of raw, including one that is the prefix of a longer URI. Ids are dense (h1, h2, … h12), so downgrading tl:hunk/h1 also rewrites tl:hunk/h12 into plain text, silently destroying a link that still resolves — and, in downgradeUnresolvableLinks, a later downgrade of tl:hunk/h12 then finds nothing and reports a loss that no longer matches the prose.
Terminate the match at the same delimiter set EVIDENCE_URI uses.
🐛 Proposed fix
export function downgradeEvidenceLink(markdown: string, raw: string): string {
const escaped = raw.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
const asMarkdownLink = new RegExp(`\\[([^\\]]*)\\]\\(\\s*${escaped}\\s*\\)`, "g");
const withoutLinks = markdown.replace(asMarkdownLink, (_match, text: string) =>
text.length > 0 ? text : readableTail(raw),
);
- return withoutLinks.split(raw).join(readableTail(raw));
+ // Only a *whole* `tl:` URI may be downgraded: `tl:hunk/h1` must not match
+ // inside `tl:hunk/h12`.
+ const asBareLink = new RegExp(`${escaped}(?![^\\s)\\]<>"'\`])`, "g");
+ return withoutLinks.replace(asBareLink, readableTail(raw));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function downgradeEvidenceLink(markdown: string, raw: string): string { | |
| const escaped = raw.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | |
| const asMarkdownLink = new RegExp(`\\[([^\\]]*)\\]\\(\\s*${escaped}\\s*\\)`, "g"); | |
| const withoutLinks = markdown.replace(asMarkdownLink, (_match, text: string) => | |
| text.length > 0 ? text : readableTail(raw), | |
| ); | |
| return withoutLinks.split(raw).join(readableTail(raw)); | |
| } | |
| export function downgradeEvidenceLink(markdown: string, raw: string): string { | |
| const escaped = raw.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | |
| const asMarkdownLink = new RegExp(`\\[([^\\]]*)\\]\\(\\s*${escaped}\\s*\\)`, "g"); | |
| const withoutLinks = markdown.replace(asMarkdownLink, (_match, text: string) => | |
| text.length > 0 ? text : readableTail(raw), | |
| ); | |
| // Only a *whole* `tl:` URI may be downgraded: `tl:hunk/h1` must not match | |
| // inside `tl:hunk/h12`. | |
| const asBareLink = new RegExp(`${escaped}(?![^\\s)\\]<>\"'\`])`, "g"); | |
| return withoutLinks.replace(asBareLink, readableTail(raw)); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 93-93: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\[([^\\]]*)\\]\\(\\s*${escaped}\\s*\\), "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/journey/src/evidence.ts` around lines 92 - 99, Update
downgradeEvidenceLink to replace only complete raw evidence URIs, not prefixes
of longer URIs. Replace the split(raw).join(...) logic with a delimiter-aware
match that terminates using the same delimiter set as EVIDENCE_URI, preserving
longer links such as tl:hunk/h12 when downgrading tl:hunk/h1.
AdiRishi
changed the title
1 participant
Builds the Throughline product on top of the Effect v4 starter: a pull request goes in, and an ordered journey of clusters comes out, where every changed line has exactly one home and finishing the last cluster means the whole change was seen.
Four commits: the product, then three rounds of fixes for defects that only running it could find, then the documentation reconciled against what actually got built.
What works end to end
Verified against a real private GitHub PR (16 files, +329 −7, including a binary PNG, a chmod, and a generated file). A second run produced 8 clusters with zero fallbacks — 23 hunks, 25 hints, 8 resurfaced, every
tl:evidence link resolving. The agent's decomposition placed tests and docs beside the module they describe rather than in a bucket of their own, which is a better reading order than the fixture's own commit structure.gh-only GitHub access behind one module with structural rate discipline: a width-2 semaphore no path goes around, per-kind cache TTLs with single-flighting, a global park on 429 that even user-initiated refresh cannot bypass, and no mutating call anywhere in the codebase..git/config.packages/journeyderives seed hunks deterministically and the agent may only refine them, so it structurally cannot break coverage — it can only fail to assign, which the validator catches. The server refuses to persist a journey that fails it.The defects worth reading about
Each was invisible to the type checker and to unit tests, and each broke a documented promise.
A database from an older build made the app inert. A migrator answers one question — what is the highest id this file has run? — so an edited-in-place migration produces a file that reports itself migrated while every statement fails (
no such column: pr_key): total, silent, and worded to the user as though retrying would help.schemaGuardnow asks the question the migrator does not, and moves an unreadable database aside — never deletes it — so migrations run onto a clean one. Resetting is right here because nothing in this database is a source of truth; the alternative is an app that opens and does nothing forever.Harness failures logged as a blank line.
Data.TaggedErrorleavesmessageempty unless a field is namedmessage, and every generic rendering readsmessage— so three errors carried a perfectly gooddetailand showed none of it. That cost real diagnosis time.The per-file header was hidden, because the emphasis stylesheet hid the very element
renderCustomHeaderportals into — taking Mark read off the screen. A hard reload of a deep journey URL failed, because a fetch-on-mount raced the opening socket. Staleness stopped updating once a journey was open, since the envelope answers as of fetch time and the artifact is cached forever; it is now derived at display time, which is what "never stored" has to mean.Documentation
The repo is documentation-first, so the docs are normally right. Building the product inverted that in 57 verified places. Most was drift — an interface whose members don't exist, a
Journeysketch missing four required fields. Three were honesty problems: counters specified as "symbols traced" that cannot be derived from anything a harness reports, hints promised to bind in a mode where their lines are not on screen, and a single-accent rule written too narrowly for code that also accents the reviewer's current position. Five ADRs record what a future maintainer would otherwise re-litigate.Known gap
In a packaged build the harness fails and the journey falls to the deterministic floor. The journey is still complete, coverage-valid, and honestly labelled as generated — the reliability guarantee holds — but the prose is not the harness's. The build is exonerated: every harness file and both vendored binaries are correctly marked
unpackedin the asar header, and the SDK runs fine from the packaged app's ownnode_modulesunder both plain Node and Electron's Node. The remaining suspect is the adapter's invocation path. Development and browser use are unaffected.Checks
pnpm checkclean. 349 tests across nine packages. The packaged app builds, signs, installs, and runs with its own icon and identity.https://claude.ai/code/session_01MWkaeWcpTEcbBPdGSDkUKL
Summary by CodeRabbit