refactor: traces-stream console + drop session_created and orphaned session subscriptions - #231
Merged
Merged
Conversation
- rename traces-live module to traces-stream (+ tests) - drop unused backend/history module and tests - update Traces hooks (useTraceData, useTraceGroups) and page for streaming - adjust session-events-live, backend real/types, and chat types
…tions - remove fanout/sessions-poll, session/inbox, session/config, and turn-orchestrator/session-tree-mirror - rework session-tree (operations, register, store, types) and ui-subscribe - add state-runtime/context-view; simplify state-runtime store/ports and turn wiring - adjust context-compaction handlers/replay and update affected tests
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 14 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughRemoves client-side prior-history snapshots for chat streaming/compaction; replaces polling traces refresh with stream-based trace-rows/trace-spans feeds; migrates session persistence and compaction to session-tree with context-view reconstruction; removes inbox/flat-state surfaces; updates runtime, orchestration, tests, and docs accordingly. ChangesChat Frontend & Backend History Snapshot Removal
Traces View - Replace Polling with Event Streaming
Harness Session & Compaction Refactoring
Estimated code review effort: Possibly Related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
console/web/src/pages/Traces/index.tsx (1)
162-198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale async detail loads from overwriting the current selection.
loadTraceSpans(traceId)can resolve after the user selects a different trace, then writes stale spans/waterfall into current state. This is an order-of-operations race.Suggested fix
+const selectedTraceIdRef = useRef<string | null>(null) +useEffect(() => { + selectedTraceIdRef.current = selectedTraceId +}, [selectedTraceId]) + +const loadSeqRef = useRef(0) const loadTraceSpans = useCallback( async (traceId: string, opts?: { silent?: boolean }) => { + const seq = ++loadSeqRef.current const silent = opts?.silent ?? false if (!silent) { setIsLoadingSpans(true) setSpansError(null) setWaterfallData(null) @@ - detailSpansRef.current = new Map(spans.map((s) => [s.span_id, s])) + if (seq !== loadSeqRef.current || selectedTraceIdRef.current !== traceId) return + detailSpansRef.current = new Map(spans.map((s) => [s.span_id, s])) const wf = rebuildDetail(traceId) if (!wf && !silent) { setSpansError('no span data available for this trace') }Also applies to: 236-247
🤖 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 `@console/web/src/pages/Traces/index.tsx` around lines 162 - 198, loadTraceSpans can race and write stale spans into state; capture the requested trace id at the start (e.g. const requested = traceId) and before mutating state (setting detailSpansRef.current, setWaterfallData, setSpansError, setIsLoadingSpans) verify the component's current selected trace id (or a currentTraceIdRef you maintain when selection changes) still equals requested; if it differs, abort applying the results. Apply the same guard to the other async loader (the similar function around lines 236-247) so resolved async results don't overwrite a newer selection.
🤖 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 `@console/web/src/lib/backend/real.ts`:
- Around line 209-213: The code collapses a missing resp.tokens_before into zero
by default and returns {status: 'empty'} even when tokens_before was omitted;
change the check so only an explicit zero triggers the empty result. Concretely,
in the resp handling for status 'ok' (the resp object and its tokens_before
field), remove the defaulting to 0 and instead test whether resp has an explicit
tokens_before (e.g., 'tokens_before' in resp or typeof resp.tokens_before ===
'number') and only return { status: 'empty' } when that explicit value === 0;
otherwise treat omitted tokens_before as not-empty/unknown.
In `@console/web/src/lib/traces-stream.ts`:
- Around line 151-163: The code registers a listener via client.on (assigned to
off) then calls client.registerTrigger (function_id built from TRACE_ROWS_FN and
client.browserId) without handling registerTrigger failure, which leaks the
listener; wrap the registerTrigger call in a try/catch (and mirror the same
pattern used in startTraceSpansStream) so that if client.registerTrigger(...)
throws you call the previously returned off() to unregister the handler before
rethrowing or returning an error, and ensure any created offTrigger is cleaned
up on later errors as well.
In `@console/web/src/pages/Traces/hooks/useTraceData.ts`:
- Around line 167-217: Wrap the async IIFE used to bootstrap streaming (the call
to getIiiClient(), startTraceListStream, client.addConnectionStateListener, and
visibility listener setup inside useTraceData) with try/catch and handle
rejections: catch errors from getIiiClient() and any setup steps, call the
existing stop cleanup (or ensure offStream/offConn/offVisibility are cleaned up)
and surface/log the error (e.g., via console.error or a logger) so a rejected
promise doesn't become an unhandled rejection and stream updates can be
recovered; apply this around the async IIFE that assigns stop and calls reseed
so all bootstrap failures are caught.
In `@console/web/src/pages/Traces/index.tsx`:
- Around line 210-234: The trace-detail stream currently only appends live
frames and can stay stale after disconnects/backgrounding; add a reseed on
reconnect/visibility by extending the existing useEffect (the subscription
created with getIiiClient + startTraceSpansStream) to also register
visibilitychange and online event handlers that call a new reseed function
(e.g., reseedDetailSpans(selectedTraceId)): this function should getIiiClient(),
fetch the current snapshot of spans for selectedTraceId (or request an initial
batch from the same backend used by startTraceSpansStream) and then restore the
detail state by resetting/appending via appendDetailSpans (ensure you coordinate
with isPausedRef and detailSpansRef so you don't clobber live updates); make
sure to remove those event listeners and stop the stream in the useEffect
cleanup.
In `@harness/docs/workers/session.md`:
- Around line 67-68: The docs state "all 15 `session-tree::*` functions" and
mention `reconcile` but the actual registered functions (see
harness/src/session/tree/register.ts and the exported `FUNCTION_IDS`) list 14
entries and do not include `reconcile`; update the wording in
harness/docs/workers/session.md to match the code by changing "all 15" to "all
14" and remove the lingering reference to `reconcile` (or alternatively add
`reconcile` to `FUNCTION_IDS` in register.ts if it was accidentally omitted) so
the documented function count and names align with the actual `session-tree::*`
registration.
In `@harness/docs/workers/turn-orchestrator.md`:
- Around line 42-43: Update the `turn::steering_check` documentation and any
runtime description strings so they reflect the actual routing logic: it should
continue to `assistant_streaming` not only when `function_results` remain but
also when steering or followup drain paths are active (i.e., allow routing from
steering/followup drains in addition to `function_results`), and otherwise go to
`turn_end` → `stopped` (respect the `max_turns` guard). Modify the doc text that
mentions `turn::steering_check` and any runtime enum/description values used by
the orchestrator and tests (references: turn::steering_check,
assistant_streaming, function_results, steering drain, followup drain) so
operator/debug logs and tests (e.g., steering.test.ts) match the implemented
behavior.
In `@harness/src/context-compaction/handler-sync.ts`:
- Around line 103-110: Before calling iii.trigger to append the auto-continue,
validate that result.compaction_entry_id is present and non-empty and fail fast
if it's missing: in the replay branch (where iii.trigger is invoked with
function_id 'session-tree::append_synthetic') add a guard that throws or returns
an error when result.compaction_entry_id is undefined/null so you don't pass
parent_id: null and create a root/orphan entry; reference
result.compaction_entry_id and the iii.trigger call to locate where to add this
check.
In `@harness/src/turn-orchestrator/assistant-streaming/ports.ts`:
- Around line 122-127: persistAssistantIfNew currently dedupes using the
pre-stream snapshot parameter messages which may be stale; before persisting,
re-fetch the latest tail for session_id (e.g., via the existing session/messages
read helper or a new getLatestMessages/getSessionTail function) and call
isDuplicateAssistant(latestMessages, asst) to verify the assistant is still
unique, or rely on a DB-level unique constraint and catch duplicate insertion
errors; update persistAssistantIfNew to perform that fresh check (or handle the
conflict) using the symbols session_id, asst, messages, isDuplicateAssistant,
and the persist logic so we never append a duplicate created by another worker.
In `@harness/tests/turn-orchestrator/context-view.test.ts`:
- Around line 29-77: The test is flaky because both the test and
buildContextView call buildSummaryMessage() which uses Date.now(); either
freeze/spy Date.now() to a fixed value before calling buildContextView and then
construct the expected summary with that same timestamp, or change the
assertions to avoid comparing the timestamp (e.g. assert summary.content/text
equals the expected summary and that timestamp is a number). Update the tests
that call buildSummaryMessage() (in the buildContextView spec) to use the chosen
approach so comparisons involving buildSummaryMessage() are deterministic.
---
Outside diff comments:
In `@console/web/src/pages/Traces/index.tsx`:
- Around line 162-198: loadTraceSpans can race and write stale spans into state;
capture the requested trace id at the start (e.g. const requested = traceId) and
before mutating state (setting detailSpansRef.current, setWaterfallData,
setSpansError, setIsLoadingSpans) verify the component's current selected trace
id (or a currentTraceIdRef you maintain when selection changes) still equals
requested; if it differs, abort applying the results. Apply the same guard to
the other async loader (the similar function around lines 236-247) so resolved
async results don't overwrite a newer selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b155b5a-65dc-465f-b0d0-a4bed8166dc9
📒 Files selected for processing (65)
console/web/src/components/chat/ChatView.tsxconsole/web/src/lib/backend/history.test.tsconsole/web/src/lib/backend/history.tsconsole/web/src/lib/backend/real.tsconsole/web/src/lib/backend/session-events-live.tsconsole/web/src/lib/backend/types.tsconsole/web/src/lib/traces-live.test.tsconsole/web/src/lib/traces-live.tsconsole/web/src/lib/traces-stream.test.tsconsole/web/src/lib/traces-stream.tsconsole/web/src/pages/Traces/hooks/useTraceData.tsconsole/web/src/pages/Traces/hooks/useTraceGroups.tsconsole/web/src/pages/Traces/index.tsxconsole/web/src/types/chat.tsharness/docs/architecture.mdharness/docs/workers/context-compaction.mdharness/docs/workers/harness.mdharness/docs/workers/session.mdharness/docs/workers/turn-orchestrator.mdharness/src/context-compaction/flat-state.tsharness/src/context-compaction/handler-async.tsharness/src/context-compaction/handler-pipeline.tsharness/src/context-compaction/handler-sync.tsharness/src/context-compaction/replay.tsharness/src/harness/fanout/index.tsharness/src/harness/fanout/sessions-poll.tsharness/src/harness/main.tsharness/src/harness/ui-subscribe.tsharness/src/index.tsharness/src/session/config.tsharness/src/session/inbox/handlers.tsharness/src/session/inbox/key.tsharness/src/session/register.tsharness/src/session/tree/operations.tsharness/src/session/tree/register.tsharness/src/session/tree/store.tsharness/src/session/tree/types.tsharness/src/turn-orchestrator/assistant-streaming/ports.tsharness/src/turn-orchestrator/assistant-streaming/run.tsharness/src/turn-orchestrator/run-start.tsharness/src/turn-orchestrator/run-transition.tsharness/src/turn-orchestrator/session-tree-mirror.tsharness/src/turn-orchestrator/state-runtime/context-view.tsharness/src/turn-orchestrator/state-runtime/ports.tsharness/src/turn-orchestrator/state-runtime/store.tsharness/src/turn-orchestrator/state.tsharness/src/turn-orchestrator/steering-check/process.tsharness/tests/context-compaction/compaction-done-emit.test.tsharness/tests/context-compaction/e2e/full-session.test.tsharness/tests/context-compaction/integration/flow-sync.test.tsharness/tests/context-compaction/replay.test.tsharness/tests/harness/fanout/sessions-poll.test.tsharness/tests/harness/ui-subscribe.test.tsharness/tests/integration/parallel-approval.e2e.test.tsharness/tests/session/inbox.test.tsharness/tests/session/operations.test.tsharness/tests/turn-orchestrator/_helpers/mockTurnStore.tsharness/tests/turn-orchestrator/assistant-streaming.test.tsharness/tests/turn-orchestrator/context-view.test.tsharness/tests/turn-orchestrator/finish.test.tsharness/tests/turn-orchestrator/run-start.test.tsharness/tests/turn-orchestrator/run-transition.test.tsharness/tests/turn-orchestrator/steering-check-layer.test.tsharness/tests/turn-orchestrator/steering.test.tsharness/tests/turn-orchestrator/store.test.ts
💤 Files with no reviewable changes (15)
- harness/src/harness/fanout/index.ts
- console/web/src/lib/traces-live.test.ts
- harness/tests/harness/fanout/sessions-poll.test.ts
- harness/src/session/inbox/key.ts
- harness/tests/session/inbox.test.ts
- harness/src/turn-orchestrator/session-tree-mirror.ts
- harness/src/session/inbox/handlers.ts
- harness/src/session/config.ts
- console/web/src/lib/backend/history.ts
- console/web/src/lib/backend/history.test.ts
- harness/src/session/tree/types.ts
- harness/src/harness/fanout/sessions-poll.ts
- harness/src/context-compaction/replay.ts
- harness/tests/turn-orchestrator/run-transition.test.ts
- console/web/src/lib/traces-live.ts
Comment on lines
209
to
+213
| if (resp?.status === 'ok') { | ||
| const tokensBefore = | ||
| typeof resp.tokens_before === 'number' ? resp.tokens_before : 0 | ||
| // Surface zero-token "ok" as semantic empty. | ||
| if (tokensBefore === 0) return surfaceEmpty() | ||
| if (tokensBefore === 0) return { status: 'empty' } |
There was a problem hiding this comment.
Don't collapse missing tokens_before into empty.
Line 213 treats an omitted tokens_before the same as a real zero. Since this response type still allows tokens_before?: number, an older or skewed engine can return status: 'ok' without that field, and the UI will incorrectly report “session is too small to summarise” even though compaction succeeded.
Suggested fix
if (resp?.status === 'ok') {
- const tokensBefore =
- typeof resp.tokens_before === 'number' ? resp.tokens_before : 0
- // Surface zero-token "ok" as semantic empty.
- if (tokensBefore === 0) return { status: 'empty' }
+ if (typeof resp.tokens_before === 'number' && resp.tokens_before === 0) {
+ return { status: 'empty' }
+ }
+ if (typeof resp.tokens_before !== 'number') {
+ return {
+ status: 'error',
+ message: 'compact_session returned ok without tokens_before',
+ }
+ }
+ const tokensBefore = resp.tokens_before
// Fallback placeholder for engines that predate summary_text on the
// wire; without it the marker has no <conversation-summary> to ship.📝 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.
Suggested change
| if (resp?.status === 'ok') { | |
| const tokensBefore = | |
| typeof resp.tokens_before === 'number' ? resp.tokens_before : 0 | |
| // Surface zero-token "ok" as semantic empty. | |
| if (tokensBefore === 0) return surfaceEmpty() | |
| if (tokensBefore === 0) return { status: 'empty' } | |
| if (resp?.status === 'ok') { | |
| if (typeof resp.tokens_before === 'number' && resp.tokens_before === 0) { | |
| return { status: 'empty' } | |
| } | |
| if (typeof resp.tokens_before !== 'number') { | |
| return { | |
| status: 'error', | |
| message: 'compact_session returned ok without tokens_before', | |
| } | |
| } | |
| const tokensBefore = resp.tokens_before | |
| // Fallback placeholder for engines that predate summary_text on the | |
| // wire; without it the marker has no <conversation-summary> to ship. |
🤖 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 `@console/web/src/lib/backend/real.ts` around lines 209 - 213, The code
collapses a missing resp.tokens_before into zero by default and returns {status:
'empty'} even when tokens_before was omitted; change the check so only an
explicit zero triggers the empty result. Concretely, in the resp handling for
status 'ok' (the resp object and its tokens_before field), remove the defaulting
to 0 and instead test whether resp has an explicit tokens_before (e.g.,
'tokens_before' in resp or typeof resp.tokens_before === 'number') and only
return { status: 'empty' } when that explicit value === 0; otherwise treat
omitted tokens_before as not-empty/unknown.
Comment on lines
+151
to
+163
| const off = client.on(TRACE_ROWS_FN, (frame: unknown) => { | ||
| const spans = extractStreamSpans(frame) | ||
| if (spans.length > 0) onSpans(spans) | ||
| }) | ||
|
|
||
| // `on()` registers under `<fn>::<browserId>`; the trigger must target that id. | ||
| const functionId = `${TRACE_ROWS_FN}::${client.browserId}` | ||
| const offTrigger = client.registerTrigger({ | ||
| type: 'stream', | ||
| function_id: functionId, | ||
| config: { stream_name: TRACE_ROWS_STREAM, group_id: TRACE_ROWS_GROUP }, | ||
| }) | ||
| dlog('trace-rows stream subscribed', { functionId }) |
There was a problem hiding this comment.
Ensure handler cleanup when trigger registration fails.
If registerTrigger(...) throws, the on(...) handler stays registered with no cleanup path, which can leak listeners and duplicate callbacks on subsequent subscriptions (Line 151 and Line 187 flows).
Suggested fix
export function startTraceListStream(
client: Pick<IiiClient, 'browserId' | 'on' | 'registerTrigger'>,
onSpans: (spans: StoredSpan[]) => void,
): () => void {
const off = client.on(TRACE_ROWS_FN, (frame: unknown) => {
const spans = extractStreamSpans(frame)
if (spans.length > 0) onSpans(spans)
})
// `on()` registers under `<fn>::<browserId>`; the trigger must target that id.
const functionId = `${TRACE_ROWS_FN}::${client.browserId}`
- const offTrigger = client.registerTrigger({
- type: 'stream',
- function_id: functionId,
- config: { stream_name: TRACE_ROWS_STREAM, group_id: TRACE_ROWS_GROUP },
- })
+ let offTrigger: () => void
+ try {
+ offTrigger = client.registerTrigger({
+ type: 'stream',
+ function_id: functionId,
+ config: { stream_name: TRACE_ROWS_STREAM, group_id: TRACE_ROWS_GROUP },
+ })
+ } catch (err) {
+ off()
+ throw err
+ }
dlog('trace-rows stream subscribed', { functionId })
return () => {
off()Apply the same pattern in startTraceSpansStream.
Also applies to: 187-200
🤖 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 `@console/web/src/lib/traces-stream.ts` around lines 151 - 163, The code
registers a listener via client.on (assigned to off) then calls
client.registerTrigger (function_id built from TRACE_ROWS_FN and
client.browserId) without handling registerTrigger failure, which leaks the
listener; wrap the registerTrigger call in a try/catch (and mirror the same
pattern used in startTraceSpansStream) so that if client.registerTrigger(...)
throws you call the previously returned off() to unregister the handler before
rethrowing or returning an error, and ensure any created offTrigger is cleaned
up on later errors as well.
Comment on lines
+167
to
+217
| void (async () => { | ||
| const client = await getIiiClient() | ||
| if (disposed) return | ||
|
|
||
| const offStream = startTraceListStream(client, (spans) => { | ||
| if (isPausedRef.current || isHidden()) return | ||
| const { key, unfiltered } = mergeKeyRef.current | ||
| if (unfiltered) { | ||
| qc.setQueryData<TracesResponse>(key, (old) => { | ||
| const merged = mergeTraceListSpans( | ||
| old?.spans ?? [], | ||
| spans, | ||
| DEFAULT_TRACE_LIMIT, | ||
| ) | ||
| return { | ||
| spans: merged, | ||
| total: merged.length, | ||
| offset: 0, | ||
| limit: DEFAULT_TRACE_LIMIT, | ||
| } | ||
| }) | ||
| } else { | ||
| qc.invalidateQueries({ queryKey: ['traces'] }) | ||
| } | ||
| // The group-by aggregate can't be appended; refetch it on activity. | ||
| qc.invalidateQueries({ queryKey: ['traceGroups'] }) | ||
| }) | ||
|
|
||
| const offConn = client.addConnectionStateListener((state) => { | ||
| if (state === 'connected' && !isPausedRef.current) reseed() | ||
| }) | ||
|
|
||
| let offVisibility: (() => void) | undefined | ||
| if (typeof document !== 'undefined') { | ||
| const onVisible = () => { | ||
| if (document.visibilityState === 'visible' && !isPausedRef.current) { | ||
| reseed() | ||
| } | ||
| } | ||
| document.addEventListener('visibilitychange', onVisible) | ||
| offVisibility = () => | ||
| document.removeEventListener('visibilitychange', onVisible) | ||
| } | ||
|
|
||
| stop = () => { | ||
| offStream() | ||
| offConn() | ||
| offVisibility?.() | ||
| } | ||
| })() | ||
|
|
There was a problem hiding this comment.
Catch stream bootstrap failures in the effect.
The async setup has no error handling; a rejected getIiiClient() (or setup error) causes an unhandled promise rejection and silently disables stream updates.
Suggested fix
void (async () => {
- const client = await getIiiClient()
- if (disposed) return
+ try {
+ const client = await getIiiClient()
+ if (disposed) return
- const offStream = startTraceListStream(client, (spans) => {
- if (isPausedRef.current || isHidden()) return
- const { key, unfiltered } = mergeKeyRef.current
- if (unfiltered) {
- qc.setQueryData<TracesResponse>(key, (old) => {
- const merged = mergeTraceListSpans(
- old?.spans ?? [],
- spans,
- DEFAULT_TRACE_LIMIT,
- )
- return {
- spans: merged,
- total: merged.length,
- offset: 0,
- limit: DEFAULT_TRACE_LIMIT,
- }
- })
- } else {
- qc.invalidateQueries({ queryKey: ['traces'] })
- }
- // The group-by aggregate can't be appended; refetch it on activity.
- qc.invalidateQueries({ queryKey: ['traceGroups'] })
- })
+ const offStream = startTraceListStream(client, (spans) => {
+ if (isPausedRef.current || isHidden()) return
+ const { key, unfiltered } = mergeKeyRef.current
+ if (unfiltered) {
+ qc.setQueryData<TracesResponse>(key, (old) => {
+ const merged = mergeTraceListSpans(
+ old?.spans ?? [],
+ spans,
+ DEFAULT_TRACE_LIMIT,
+ )
+ return {
+ spans: merged,
+ total: merged.length,
+ offset: 0,
+ limit: DEFAULT_TRACE_LIMIT,
+ }
+ })
+ } else {
+ qc.invalidateQueries({ queryKey: ['traces'] })
+ }
+ qc.invalidateQueries({ queryKey: ['traceGroups'] })
+ })
- const offConn = client.addConnectionStateListener((state) => {
- if (state === 'connected' && !isPausedRef.current) reseed()
- })
+ const offConn = client.addConnectionStateListener((state) => {
+ if (state === 'connected' && !isPausedRef.current) reseed()
+ })
- let offVisibility: (() => void) | undefined
- if (typeof document !== 'undefined') {
- const onVisible = () => {
- if (document.visibilityState === 'visible' && !isPausedRef.current) {
- reseed()
- }
- }
- document.addEventListener('visibilitychange', onVisible)
- offVisibility = () =>
- document.removeEventListener('visibilitychange', onVisible)
- }
+ let offVisibility: (() => void) | undefined
+ if (typeof document !== 'undefined') {
+ const onVisible = () => {
+ if (document.visibilityState === 'visible' && !isPausedRef.current) {
+ reseed()
+ }
+ }
+ document.addEventListener('visibilitychange', onVisible)
+ offVisibility = () =>
+ document.removeEventListener('visibilitychange', onVisible)
+ }
- stop = () => {
- offStream()
- offConn()
- offVisibility?.()
+ stop = () => {
+ offStream()
+ offConn()
+ offVisibility?.()
+ }
+ } catch {
+ // optional: add dev log / metric
}
})()📝 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.
Suggested change
| void (async () => { | |
| const client = await getIiiClient() | |
| if (disposed) return | |
| const offStream = startTraceListStream(client, (spans) => { | |
| if (isPausedRef.current || isHidden()) return | |
| const { key, unfiltered } = mergeKeyRef.current | |
| if (unfiltered) { | |
| qc.setQueryData<TracesResponse>(key, (old) => { | |
| const merged = mergeTraceListSpans( | |
| old?.spans ?? [], | |
| spans, | |
| DEFAULT_TRACE_LIMIT, | |
| ) | |
| return { | |
| spans: merged, | |
| total: merged.length, | |
| offset: 0, | |
| limit: DEFAULT_TRACE_LIMIT, | |
| } | |
| }) | |
| } else { | |
| qc.invalidateQueries({ queryKey: ['traces'] }) | |
| } | |
| // The group-by aggregate can't be appended; refetch it on activity. | |
| qc.invalidateQueries({ queryKey: ['traceGroups'] }) | |
| }) | |
| const offConn = client.addConnectionStateListener((state) => { | |
| if (state === 'connected' && !isPausedRef.current) reseed() | |
| }) | |
| let offVisibility: (() => void) | undefined | |
| if (typeof document !== 'undefined') { | |
| const onVisible = () => { | |
| if (document.visibilityState === 'visible' && !isPausedRef.current) { | |
| reseed() | |
| } | |
| } | |
| document.addEventListener('visibilitychange', onVisible) | |
| offVisibility = () => | |
| document.removeEventListener('visibilitychange', onVisible) | |
| } | |
| stop = () => { | |
| offStream() | |
| offConn() | |
| offVisibility?.() | |
| } | |
| })() | |
| void (async () => { | |
| try { | |
| const client = await getIiiClient() | |
| if (disposed) return | |
| const offStream = startTraceListStream(client, (spans) => { | |
| if (isPausedRef.current || isHidden()) return | |
| const { key, unfiltered } = mergeKeyRef.current | |
| if (unfiltered) { | |
| qc.setQueryData<TracesResponse>(key, (old) => { | |
| const merged = mergeTraceListSpans( | |
| old?.spans ?? [], | |
| spans, | |
| DEFAULT_TRACE_LIMIT, | |
| ) | |
| return { | |
| spans: merged, | |
| total: merged.length, | |
| offset: 0, | |
| limit: DEFAULT_TRACE_LIMIT, | |
| } | |
| }) | |
| } else { | |
| qc.invalidateQueries({ queryKey: ['traces'] }) | |
| } | |
| // The group-by aggregate can't be appended; refetch it on activity. | |
| qc.invalidateQueries({ queryKey: ['traceGroups'] }) | |
| }) | |
| const offConn = client.addConnectionStateListener((state) => { | |
| if (state === 'connected' && !isPausedRef.current) reseed() | |
| }) | |
| let offVisibility: (() => void) | undefined | |
| if (typeof document !== 'undefined') { | |
| const onVisible = () => { | |
| if (document.visibilityState === 'visible' && !isPausedRef.current) { | |
| reseed() | |
| } | |
| } | |
| document.addEventListener('visibilitychange', onVisible) | |
| offVisibility = () => | |
| document.removeEventListener('visibilitychange', onVisible) | |
| } | |
| stop = () => { | |
| offStream() | |
| offConn() | |
| offVisibility?.() | |
| } | |
| } catch { | |
| // optional: add dev log / metric | |
| } | |
| })() |
🤖 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 `@console/web/src/pages/Traces/hooks/useTraceData.ts` around lines 167 - 217,
Wrap the async IIFE used to bootstrap streaming (the call to getIiiClient(),
startTraceListStream, client.addConnectionStateListener, and visibility listener
setup inside useTraceData) with try/catch and handle rejections: catch errors
from getIiiClient() and any setup steps, call the existing stop cleanup (or
ensure offStream/offConn/offVisibility are cleaned up) and surface/log the error
(e.g., via console.error or a logger) so a rejected promise doesn't become an
unhandled rejection and stream updates can be recovered; apply this around the
async IIFE that assigns stop and calls reseed so all bootstrap failures are
caught.
Comment on lines
+210
to
+234
| // Subscribe the open trace to its scoped `trace-spans` stream: only this | ||
| // trace's span activity arrives, appending without a reselect or refetch. | ||
| // Re-subscribes when the selection changes; frozen while paused. | ||
| // | ||
| // `active` is shared with the handler so a stream frame still in flight when | ||
| // the selection changes is dropped: without it, the unregistered-but-running | ||
| // handler would append the OLD trace's spans into `detailSpansRef` (already | ||
| // reset for the NEW trace) and rebuild the wrong waterfall. | ||
| useEffect(() => { | ||
| if (!selectedTraceId) return | ||
| let stop: (() => void) | undefined | ||
| let active = true | ||
| void (async () => { | ||
| const client = await getIiiClient() | ||
| if (!active) return | ||
| stop = startTraceSpansStream(client, selectedTraceId, (spans) => { | ||
| if (!active || isPausedRef.current) return | ||
| appendDetailSpans(selectedTraceId, spans) | ||
| }) | ||
| })() | ||
| return () => { | ||
| active = false | ||
| stop?.() | ||
| } | ||
| }, [selectedTraceId, appendDetailSpans]) |
There was a problem hiding this comment.
Add detail-stream reseed on reconnect/visibility to self-heal dropped frames.
The selected-trace stream only appends live frames. If frames are missed during disconnect/background periods, the detail view stays stale indefinitely because this path has no reseed trigger.
Suggested fix
useEffect(() => {
if (!selectedTraceId) return
let stop: (() => void) | undefined
let active = true
void (async () => {
const client = await getIiiClient()
if (!active) return
stop = startTraceSpansStream(client, selectedTraceId, (spans) => {
if (!active || isPausedRef.current) return
appendDetailSpans(selectedTraceId, spans)
})
+
+ const offConn = client.addConnectionStateListener((state) => {
+ if (state === 'connected' && !isPausedRef.current) {
+ void loadTraceSpans(selectedTraceId, { silent: true })
+ }
+ })
+
+ let offVisibility: (() => void) | undefined
+ if (typeof document !== 'undefined') {
+ const onVisible = () => {
+ if (document.visibilityState === 'visible' && !isPausedRef.current) {
+ void loadTraceSpans(selectedTraceId, { silent: true })
+ }
+ }
+ document.addEventListener('visibilitychange', onVisible)
+ offVisibility = () =>
+ document.removeEventListener('visibilitychange', onVisible)
+ }
+
+ const prevStop = stop
+ stop = () => {
+ prevStop?.()
+ offConn()
+ offVisibility?.()
+ }
})()🤖 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 `@console/web/src/pages/Traces/index.tsx` around lines 210 - 234, The
trace-detail stream currently only appends live frames and can stay stale after
disconnects/backgrounding; add a reseed on reconnect/visibility by extending the
existing useEffect (the subscription created with getIiiClient +
startTraceSpansStream) to also register visibilitychange and online event
handlers that call a new reseed function (e.g.,
reseedDetailSpans(selectedTraceId)): this function should getIiiClient(), fetch
the current snapshot of spans for selectedTraceId (or request an initial batch
from the same backend used by startTraceSpansStream) and then restore the detail
state by resetting/appending via appendDetailSpans (ensure you coordinate with
isPausedRef and detailSpansRef so you don't clobber live updates); make sure to
remove those event listeners and stop the stream in the useEffect cleanup.
Comment on lines
67
to
68
| | [src/session/tree/register.ts](harness/src/session/tree/register.ts) | Registers all 15 `session-tree::*` functions; exports `FUNCTION_IDS`. | | ||
| | [src/session/tree/operations.ts](harness/src/session/tree/operations.ts) | Pure tree algorithms: create, fork, clone, compact, active path, messages, reconcile, tree, export_html, list. | |
There was a problem hiding this comment.
Fix stale function-count/API wording in Source layout.
Line 67 says “all 15 session-tree::* functions,” but Lines 23-37 list 14. Line 68 still mentions reconcile, which is no longer present in the registered function list. Please align this section to avoid conflicting docs.
📝 Suggested doc fix
-| [src/session/tree/register.ts](harness/src/session/tree/register.ts) | Registers all 15 `session-tree::*` functions; exports `FUNCTION_IDS`. |
-| [src/session/tree/operations.ts](harness/src/session/tree/operations.ts) | Pure tree algorithms: create, fork, clone, compact, active path, messages, reconcile, tree, export_html, list. |
+| [src/session/tree/register.ts](harness/src/session/tree/register.ts) | Registers all `session-tree::*` functions; exports `FUNCTION_IDS`. |
+| [src/session/tree/operations.ts](harness/src/session/tree/operations.ts) | Pure tree algorithms: create, fork, clone, compact, active path, messages, tree, export_html, list. |📝 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.
Suggested change
| | [src/session/tree/register.ts](harness/src/session/tree/register.ts) | Registers all 15 `session-tree::*` functions; exports `FUNCTION_IDS`. | | |
| | [src/session/tree/operations.ts](harness/src/session/tree/operations.ts) | Pure tree algorithms: create, fork, clone, compact, active path, messages, reconcile, tree, export_html, list. | | |
| | [src/session/tree/register.ts](harness/src/session/tree/register.ts) | Registers all `session-tree::*` functions; exports `FUNCTION_IDS`. | | |
| | [src/session/tree/operations.ts](harness/src/session/tree/operations.ts) | Pure tree algorithms: create, fork, clone, compact, active path, messages, tree, export_html, list. | |
🤖 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 `@harness/docs/workers/session.md` around lines 67 - 68, The docs state "all 15
`session-tree::*` functions" and mention `reconcile` but the actual registered
functions (see harness/src/session/tree/register.ts and the exported
`FUNCTION_IDS`) list 14 entries and do not include `reconcile`; update the
wording in harness/docs/workers/session.md to match the code by changing "all
15" to "all 14" and remove the lingering reference to `reconcile` (or
alternatively add `reconcile` to `FUNCTION_IDS` in register.ts if it was
accidentally omitted) so the documented function count and names align with the
actual `session-tree::*` registration.
Comment on lines
103
to
110
| if (replay) { | ||
| lastEntryId = await reinjectReplay(iii, input.session_id, replay, lastEntryId); | ||
| await iii.trigger<unknown, { entry_id?: string }>({ | ||
| function_id: 'session-tree::append_synthetic', | ||
| payload: { | ||
| session_id: input.session_id, | ||
| text: 'Continue if you have next steps, or stop and ask for clarification.', | ||
| metadata: { compaction_continue: true }, | ||
| parent_id: lastEntryId, | ||
| parent_id: result.compaction_entry_id || null, | ||
| }, |
There was a problem hiding this comment.
Fail fast if compaction_entry_id is missing before auto-continue append.
On Line 109, parent_id: result.compaction_entry_id || null silently allows a root append when the compaction id is missing. That can orphan the continue nudge from the compacted path and corrupt reconstructed context order.
Suggested fix
if (replay) {
+ if (!result.compaction_entry_id) {
+ throw new Error('missing compaction_entry_id for sync auto-continue append');
+ }
await iii.trigger<unknown, { entry_id?: string }>({
function_id: 'session-tree::append_synthetic',
payload: {
session_id: input.session_id,
text: 'Continue if you have next steps, or stop and ask for clarification.',
- parent_id: result.compaction_entry_id || null,
+ parent_id: result.compaction_entry_id,
},
timeoutMs: 10_000,
});
}🤖 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 `@harness/src/context-compaction/handler-sync.ts` around lines 103 - 110,
Before calling iii.trigger to append the auto-continue, validate that
result.compaction_entry_id is present and non-empty and fail fast if it's
missing: in the replay branch (where iii.trigger is invoked with function_id
'session-tree::append_synthetic') add a guard that throws or returns an error
when result.compaction_entry_id is undefined/null so you don't pass parent_id:
null and create a root/orphan entry; reference result.compaction_entry_id and
the iii.trigger call to locate where to add this check.
Comment on lines
+122
to
127
| async persistAssistantIfNew(session_id, asst, messages) { | ||
| // Dedup against the window already loaded in prepareStreamContext: nothing | ||
| // is persisted between that load and here within one invocation, and | ||
| // isDuplicateAssistant only inspects the trailing entry — so reusing it is | ||
| // identical to a fresh reload and saves a full session round-trip. | ||
| if (isDuplicateAssistant(messages, asst)) { |
There was a problem hiding this comment.
Re-check assistant dedupe against fresh persisted tail before append.
At Line 122, dedupe uses a pre-stream snapshot (messages) that can be stale by finalize time. If another worker appends the same assistant in between, this path can persist a duplicate assistant row.
Suggested fix
async persistAssistantIfNew(session_id, asst, messages) {
- if (isDuplicateAssistant(messages, asst)) {
+ // Re-read right before append to close stale-window duplicate races.
+ const latestMessages = await base.loadMessages(session_id);
+ if (isDuplicateAssistant(latestMessages, asst)) {
logger.warn('finalizeAssistant: skipping duplicate assistant push (re-entry detected)', {
session_id,
timestamp: asst.timestamp,
});
return;
}
await base.appendMessages(session_id, [asst]);
},🤖 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 `@harness/src/turn-orchestrator/assistant-streaming/ports.ts` around lines 122
- 127, persistAssistantIfNew currently dedupes using the pre-stream snapshot
parameter messages which may be stale; before persisting, re-fetch the latest
tail for session_id (e.g., via the existing session/messages read helper or a
new getLatestMessages/getSessionTail function) and call
isDuplicateAssistant(latestMessages, asst) to verify the assistant is still
unique, or rely on a DB-level unique constraint and catch duplicate insertion
errors; update persistAssistantIfNew to perform that fresh check (or handle the
conflict) using the symbols session_id, asst, messages, isDuplicateAssistant,
and the persist logic so we never append a duplicate created by another worker.
Comment on lines
+29
to
+77
| describe('buildContextView', () => { | ||
| it('returns raw path when there is no compaction', () => { | ||
| const messages = [entry('a', user('one')), entry('b', asst('two'))]; | ||
| expect(buildContextView(messages, [])).toEqual([user('one'), asst('two')]); | ||
| }); | ||
|
|
||
| it('reconstructs summary + tail from tail_start_id', () => { | ||
| const messages = [ | ||
| entry('head', user('old')), | ||
| entry('tail1', asst('keep')), | ||
| entry('last', user('in flight')), | ||
| ]; | ||
| const compactions = [{ summary: 'condensed', tail_start_id: 'tail1', timestamp: 100 }]; | ||
|
|
||
| expect(buildContextView(messages, compactions)).toEqual([ | ||
| buildSummaryMessage('condensed'), | ||
| asst('keep'), | ||
| user('in flight'), | ||
| ]); | ||
| }); | ||
|
|
||
| it('uses the latest compaction when several exist', () => { | ||
| const messages = [ | ||
| entry('h', user('old')), | ||
| entry('t1', asst('early tail')), | ||
| entry('t2', user('recent')), | ||
| ]; | ||
| const compactions = [ | ||
| { summary: 'first', tail_start_id: 'h', timestamp: 10 }, | ||
| { summary: 'latest', tail_start_id: 't2', timestamp: 20 }, | ||
| ]; | ||
|
|
||
| expect(buildContextView(messages, compactions)).toEqual([ | ||
| buildSummaryMessage('latest'), | ||
| user('recent'), | ||
| ]); | ||
| }); | ||
|
|
||
| it('keeps the whole tail when tail_start_id is absent from the path', () => { | ||
| const messages = [entry('a', user('one')), entry('b', asst('two'))]; | ||
| const compactions = [{ summary: 's', tail_start_id: 'gone', timestamp: 1 }]; | ||
|
|
||
| expect(buildContextView(messages, compactions)).toEqual([ | ||
| buildSummaryMessage('s'), | ||
| user('one'), | ||
| asst('two'), | ||
| ]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Stabilize summary timestamp assertions to avoid flaky test failures.
These assertions call buildSummaryMessage() in expected values while the SUT also builds one internally; both use Date.now(), so equality can fail across millisecond boundaries.
✅ Deterministic test-time fix
-import { describe, expect, it } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
@@
describe('buildContextView', () => {
+ beforeEach(() => {
+ vi.spyOn(Date, 'now').mockReturnValue(1_700_000_000_000);
+ });
+
+ afterEach(() => {
+ vi.restoreAllMocks();
+ });
+
it('returns raw path when there is no compaction', () => {🤖 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 `@harness/tests/turn-orchestrator/context-view.test.ts` around lines 29 - 77,
The test is flaky because both the test and buildContextView call
buildSummaryMessage() which uses Date.now(); either freeze/spy Date.now() to a
fixed value before calling buildContextView and then construct the expected
summary with that same timestamp, or change the assertions to avoid comparing
the timestamp (e.g. assert summary.content/text equals the expected summary and
that timestamp is a number). Update the tests that call buildSummaryMessage()
(in the buildContextView spec) to use the chosen approach so comparisons
involving buildSummaryMessage() are deterministic.
The session-inbox subsystem was dropped, but steering_check still called session-inbox::drain — an unregistered function — at runtime. Remove the consumer to match: - drop drainInbox/parseDrainItems from steering-check ports - reduce route() to continue_after_function/end_turn (no steering/followup) - drop the resume_with_inbox outcome and its apply branch - prune obsolete inbox/route tests; keep handleSteering integration tests - clear stale session-inbox::* mentions from session worker descriptions
…ph and WaterfallChart - Change default state for showCriticalPath from true to false in both FlameGraph and WaterfallChart components to display the full tree initially. - Adjust comments to reflect the new behavior regarding engine routing and critical path visibility.
CI runs biome@2.4.10; these files were committed formatted by a stale local biome 1.9.4, which wraps imports/strings differently. Reformat to match the pinned CI version.
…follow-up wake processResolvedApprovals now re-scans the parked batch until a pass resolves nothing new, so a call whose approval was written as a side effect of executing a sibling (parallel approve-all) is drained in the same wake instead of being orphaned when its own wake was dropped. handleAwaitingApproval kicks one fresh wake when it resolved at least one call but left siblings parked, covering a still-pending sibling whose contender wake lost the lease race and exhausted retries. A wake that resolves nothing enqueues nothing, so this cannot storm. Makes the two previously-red parallel-approval e2e tests pass.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@harness/src/turn-orchestrator/function-awaiting-approval/process.ts`:
- Around line 55-57: The call to enqueueAwaitingApprovalWake (invoked when
resolved > 0 && batch.awaiting_approval.length > 0) lacks error handling and can
cause the whole handler to fail if it throws; wrap that call in a try/catch,
catch any error, and log a warning (using the same logger pattern as
handleApprovalStateWrite and enqueueTurnStep) including context (iii,
batch.session_id, resolved, batch.awaiting_approval length) so failures to
enqueue don't make the handler retry or reprocess successfully completed work.
In `@harness/src/turn-orchestrator/function-awaiting-approval/run.ts`:
- Line 82: Replace the non-null assertion on the find result so the code
defensively handles the missing entry: import logger from
"../../runtime/otel.js", assign const current = work.prepared.find((p) =>
p.call.id === callId), check if current is undefined, and if so call
logger.error with context (include callId and work.prepared length or keys) and
throw a clear Error (or return an appropriate error path) instead of letting the
`!` crash; otherwise continue using current as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3edd5ab-9a7e-4bdb-98ff-bba0825982ea
📒 Files selected for processing (7)
harness/src/context-compaction/handler-sync.tsharness/src/index.tsharness/src/session/main.tsharness/src/turn-orchestrator/function-awaiting-approval/process.tsharness/src/turn-orchestrator/function-awaiting-approval/run.tsharness/tests/context-compaction/e2e/full-session.test.tsharness/tests/integration/parallel-approval.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- harness/src/index.ts
- harness/src/session/main.ts
- harness/src/context-compaction/handler-sync.ts
- harness/tests/context-compaction/e2e/full-session.test.ts
- harness/tests/integration/parallel-approval.e2e.test.ts
Comment on lines
+55
to
+57
| if (resolved > 0 && batch.awaiting_approval.length > 0) { | ||
| await enqueueAwaitingApprovalWake(iii, batch.session_id); | ||
| } |
There was a problem hiding this comment.
Missing error handling for follow-up wake enqueue.
If enqueueAwaitingApprovalWake throws here, the entire handler fails despite the core work (processing approvals, routing) already completing successfully. This could cause the current wake to retry unnecessarily and potentially reprocess already-executed calls.
Wrap in try/catch with a warning log, consistent with the pattern in handleApprovalStateWrite (lines 34-39) and enqueueTurnStep in store.ts.
🛡️ Suggested fix
if (resolved > 0 && batch.awaiting_approval.length > 0) {
- await enqueueAwaitingApprovalWake(iii, batch.session_id);
+ try {
+ await enqueueAwaitingApprovalWake(iii, batch.session_id);
+ } catch (err) {
+ logger.warn('follow-up awaiting-approval wake failed', {
+ session_id: batch.session_id,
+ err: String(err),
+ });
+ }
}📝 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.
Suggested change
| if (resolved > 0 && batch.awaiting_approval.length > 0) { | |
| await enqueueAwaitingApprovalWake(iii, batch.session_id); | |
| } | |
| if (resolved > 0 && batch.awaiting_approval.length > 0) { | |
| try { | |
| await enqueueAwaitingApprovalWake(iii, batch.session_id); | |
| } catch (err) { | |
| logger.warn('follow-up awaiting-approval wake failed', { | |
| session_id: batch.session_id, | |
| err: String(err), | |
| }); | |
| } | |
| } |
🤖 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 `@harness/src/turn-orchestrator/function-awaiting-approval/process.ts` around
lines 55 - 57, The call to enqueueAwaitingApprovalWake (invoked when resolved >
0 && batch.awaiting_approval.length > 0) lacks error handling and can cause the
whole handler to fail if it throws; wrap that call in a try/catch, catch any
error, and log a warning (using the same logger pattern as
handleApprovalStateWrite and enqueueTurnStep) including context (iii,
batch.session_id, resolved, batch.awaiting_approval length) so failures to
enqueue don't make the handler retry or reprocess successfully completed work.
| const current = work.prepared.find((p) => p.call.id === callId)!; | ||
| const resolved = applyDecisionToPrepared(current, decision); | ||
| await runOneCall(executePorts, rec.session_id, resolved, executed, { skipStart: true }); | ||
| const current = work.prepared.find((p) => p.call.id === callId)!; |
There was a problem hiding this comment.
Non-null assertion may crash if invariant is violated.
Per the linter hint, this ! assertion will throw if work.prepared doesn't contain an entry for callId. While the invariant should guarantee this, a defensive check would prevent a crash and make debugging easier if the invariant is ever violated upstream.
🛡️ Suggested fix
- const current = work.prepared.find((p) => p.call.id === callId)!;
+ const current = work.prepared.find((p) => p.call.id === callId);
+ if (!current) {
+ logger.warn('awaiting_approval entry missing from prepared', { session_id: rec.session_id, callId });
+ awaiting = awaiting.filter((e) => e.function_call_id !== callId);
+ continue;
+ }
const resolved = applyDecisionToPrepared(current, decision);This requires importing logger from ../../runtime/otel.js.
🧰 Tools
🪛 GitHub Check: harness: node lint + test
[warning] 82-82: lint/style/noNonNullAssertion
Forbidden non-null assertion.
🤖 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 `@harness/src/turn-orchestrator/function-awaiting-approval/run.ts` at line 82,
Replace the non-null assertion on the find result so the code defensively
handles the missing entry: import logger from "../../runtime/otel.js", assign
const current = work.prepared.find((p) => p.call.id === callId), check if
current is undefined, and if so call logger.error with context (include callId
and work.prepared length or keys) and throw a clear Error (or return an
appropriate error path) instead of letting the `!` crash; otherwise continue
using current as before.
Source: Linters/SAST tools
sergiofilhowz
approved these changes
Jun 8, 2026
andersonleal
approved these changes
Jun 8, 2026
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related cleanups:
traces-livemodule with atraces-streamimplementation and drop the unused history backend.session_createdand the orphaned session-subscription machinery, simplifying the session worker to a puresession-tree::*storage surface.Console — traces stream
lib/traces-live→lib/traces-stream(module + tests) with the live-stream refactor.lib/backend/historymodule and its tests.useTraceData,useTraceGroups) for the streaming path.backend/session-events-live,backend/real/types,ChatView, andtypes/chatwiring.Harness — drop
session_createdand orphaned session subscriptionsharness/fanout/sessions-poll,session/inbox/*,session/config, andturn-orchestrator/session-tree-mirror.session/registerto registersession-tree::*onIiiStateSessionStoredirectly (no config-driven setup, no inbox registration).session-tree::reconcilefunction and the state-snapshot mirroring it backed.turn-orchestrator/state-runtime/context-view; simplifystate-runtimestore/ports and turn wiring.context-compactionhandlers/replay andharness/ui-subscribe.Notable behavior change
session.store_backend: 'memory'config option (and the relatedsession.state_scope).InMemoryStoreremains as a test-only backend used directly by unit tests; production always usesIiiStateSessionStore.Test plan
pnpm -C harness typecheck— note: 2 pre-existing errors onmain(index.ts:75,models-catalog/main.ts:8) are unrelated to this PRpnpm -C harness testsession-tree::*Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Tests