feat(cli): typed v2 harness session-event stream (capture + status + consumer + hooks)#230
Conversation
Additive over v1: reuse bucket/scope/harness verbatim, replace only the message+source surface with a typed event. Two axes — role (owner|agent) and kind (user_prompt|agent_message|agent_thinking|tool_call|tool_result| approval_request|status|lifecycle|error|unknown) — so the OpenHuman orchestrator can differentiate tool calls, messages, outputs and status. v1 and v2 coexist; discriminate on envelope_version. SessionEnvelope alias stays v1-only so no current importer changes behavior.
Pure, self-contained mapper (harnessEventsFromLine) that splits transcript content blocks into per-block v2 events: Claude tool_use/tool_result/ thinking/text and Codex function_call/function_call_output/reasoning/ messages. tool_call<->tool_result correlate via call_id; tool names normalize to a tool_kind; output is byte-bounded. No transport/envelope coupling, so it lands free of the open bridge-tui work. 29 unit tests.
Replace the binary claude/codex ternary with a Record<HarnessProvider, HarnessLineMapper> registry. Adding a provider to the union (e.g. gemini) now fails tsc until its mapper is registered, and an unregistered provider returns [] instead of silently falling through to the codex branch. The envelope's harness.provider field remains the per-packet discriminator that lets OpenHuman tell claude/codex/(future) gemini apart.
Pure, caller-driven state machine (reduceStatus + tickStatus) that turns the typed event stream into the InstanceStatus signal OpenHuman's Master Session needs: running_tool with the active call id while a tool runs, waiting_approval when blocked, running while thinking/replying, idle/stopped on lifecycle. The transcript never says 'idle', so tickStatus ages a silent session out and can re-emit a heartbeat. Change-gated: only emits on an actual transition. 13 tests.
Pure builder (buildEventEnvelopeV2) that wraps a parsed event with seq, a stable content-addressed id (dedup on resend), bucket floor/window and the scope/harness context. Mirrors the fields the v1 tailer already assembles, so wiring it in later is a near 1:1 swap of message+source for the typed event. 4 tests.
Wire the v2 pipeline into the tailer, behind TINYPLACE_HARNESS_V2=1 (default off, so v1 output is byte-identical until the OpenHuman receiver understands v2). Per new line the poll loop runs harnessEventsFromLine, publishes a SessionEnvelopeV2 per typed event, and derives+publishes a status transition; an idle/heartbeat tick ages a silent session. The publisher/writeEnvelope now accept either version (id/role via an envelope_version discriminant). Proven by an integration test that drives the real tailer against a fixture Claude session in dry-run and asserts tool_call/tool_result/status with correlated call ids and monotonic seq.
Receiver-side counterpart to the producer: parseSessionEnvelope safely decodes a DM body, and applySessionEnvelope/foldSessionEnvelopes reduce the v2 event stream into per-session Master-Session state — status, currentTask, a tool activity log (call paired to its result by call_id) and a capped transcript feed. Pure and framework-agnostic; dedups by seq, ignores v1. Lets OpenHuman's receiver just parse each DM and apply it. 9 tests.
|
@oxoxDev is attempting to deploy a commit to the Vezures Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds v2 typed harness session envelopes, semantic event parsing, status derivation, consumer folding, wrapper emission, and SDK re-exports. Tests cover line parsing, hook mapping, envelope construction, status transitions, folding behavior, and v2 emission. ChangesV2 Harness Event Pipeline
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant HarnessSessionTailer
participant harnessEventsFromLine
participant buildEventEnvelopeV2
participant SessionEnvelopePublisher
HarnessSessionTailer->>harnessEventsFromLine: parse new transcript line
harnessEventsFromLine-->>HarnessSessionTailer: HarnessSemanticEvent[]
HarnessSessionTailer->>buildEventEnvelopeV2: build v2 envelope(seq, event)
buildEventEnvelopeV2-->>HarnessSessionTailer: SessionEnvelopeV2
HarnessSessionTailer->>SessionEnvelopePublisher: publish(envelope)
SessionEnvelopePublisher-->>HarnessSessionTailer: queued/dry-run result
sequenceDiagram
participant DMBody
participant parseSessionEnvelope
participant foldSessionEnvelopes
participant SessionView
DMBody->>parseSessionEnvelope: raw JSON body
parseSessionEnvelope-->>foldSessionEnvelopes: AnySessionEnvelope
foldSessionEnvelopes->>foldSessionEnvelopes: filter v2 and sort by event.seq
foldSessionEnvelopes->>SessionView: applySessionEnvelope per event
SessionView-->>foldSessionEnvelopes: updated status, feed, tools
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 842dba806d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export { | ||
| claudeHookEventToSemantic, | ||
| claudeHookEventsFromStdin, | ||
| claudeHookStringToSemantic, | ||
| } from "./cli/harness-hooks.js"; |
There was a problem hiding this comment.
This root entrypoint is used by the website's client-side code via @tinyhumansai/tinyplace, so statically re-exporting CLI harness modules here forces the browser bundle to resolve Node-only imports (harness-hooks imports node:process, and the harness-envelope export below imports node:crypto) even when those APIs are unused. In Next/Webpack client builds this can fail module resolution for the package root; move these runtime harness helpers behind a Node/CLI subpath or keep only type exports in the browser-facing root.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 698075a — moved the harness runtime value exports (events / hooks / status / envelope / consumer) to the ./node subpath (@tinyhumansai/tinyplace/node). The browser-facing root now re-exports only their (type-erased) types, so client bundles no longer resolve node:process / node:crypto. Verified dist/node/index.js carries the moved symbols and the website (which imports none of these values) still typechecks.
| function parseTimestamp(value: unknown): Date { | ||
| if (typeof value !== "string") { | ||
| return new Date(0); | ||
| } | ||
| const parsed = new Date(value); | ||
| return Number.isNaN(parsed.getTime()) ? new Date(0) : parsed; |
There was a problem hiding this comment.
When Codex rollout records omit a top-level timestamp (as the new Codex fixtures do), this returns the Unix epoch; buildEventEnvelopeV2 later normalizes that to now for the envelope, but reduceStatus consumes the original semantic timestamp and falls back to the previous lastEventAtMs of 0. As a result, after any active v2 event, tickV2Status immediately sees the session as stale and emits idle, so live Codex status can flip to idle right after a tool call or response. Default missing/invalid timestamps to the receive time before status reduction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5e36a02 — parseTimestamp now defaults missing/invalid timestamps to receive time (new Date()) instead of the epoch, so reduceStatus's activity clock advances and tickStatus no longer idles a live session right after a tool call. Added a timestamp-less Codex function_call regression test asserting the status stays running_tool and doesn't emit idle on the next tick.
| call_id: asString(object.id) ?? "", | ||
| tool_name: toolName, | ||
| tool_kind: normalizeToolKind(toolName), | ||
| display: toolDisplay(toolName, input), | ||
| input, |
There was a problem hiding this comment.
For tools such as Write/Edit/MCP, tool_input can contain full file contents or very large request payloads, but this stores the raw value directly in every v2 tool_call envelope and the publisher forwards that JSON over DM. Outputs are capped to ~4KB, while inputs are neither truncated nor redacted, so one large edit can exceed relay/message limits or expose unintended content despite the type contract saying inputs are bounded before publish.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 70b6436 — raw tool_input is now byte-capped (INPUT_CAP = 4096) via boundToolInput() before it enters the tool_call envelope, across the Claude, Codex and hook mappers; small inputs keep their object structure. Added a test with an oversized Write input asserting the published input is a byte-bounded string.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
sdk/typescript/src/types/harness.ts (1)
201-206: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDrop the unused
source_rolefromSessionEnvelopeV2. The v2 builder never populates it, and nothing else in the repo writes it. If it’s meant for a future producer, leave a brief note; otherwise remove it to keep the wire contract tight.🤖 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 `@sdk/typescript/src/types/harness.ts` around lines 201 - 206, Remove the unused source_role field from SessionEnvelopeV2 in the source type definition, since the v2 builder does not populate it and no other code writes it. Update the SessionEnvelopeV2 shape so the wire contract only includes the fields that are actually produced; if you want to preserve the field for future use, add a brief inline note near the source definition instead of keeping the optional property.sdk/typescript/src/cli/harness-status.ts (2)
41-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing JSDoc on exported
reduceStatus.
initialStatusandtickStatushave JSDoc, butreduceStatus— the primary exported reducer — does not. As per coding guidelines, "Always include JSDoc comments for public functions and exported modules."🤖 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 `@sdk/typescript/src/cli/harness-status.ts` around lines 41 - 63, The exported reduceStatus reducer is missing the required JSDoc comment. Add a brief JSDoc block above reduceStatus in harness-status.ts describing that it reduces a HarnessSemanticEvent into the next SessionStatusState/StatusStep, matching the style used by initialStatus and tickStatus. Keep the documentation on the exported function itself so the public API is covered.Source: Coding guidelines
98-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
deriveFromEventexceeds the 20-line function guideline.This switch spans ~42 lines. As per coding guidelines, "Limit functions to a maximum of 20 lines of code; extract longer logic into separate helper functions." Consider splitting into a per-kind mapper lookup or extracting the larger case bodies (e.g.
tool_call,approval_request,error) into small named helpers.🤖 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 `@sdk/typescript/src/cli/harness-status.ts` around lines 98 - 139, deriveFromEvent is too long and should be broken up to satisfy the 20-line function guideline. Extract the switch logic in harness-status.ts into smaller helpers, such as per-event mappers for tool_call, approval_request, error, and a small dispatcher from deriveFromEvent, while keeping the existing behavior of deriving state, detail, and activeCallId from HarnessSemanticEvent. Use clear helper names tied to the current cases so the logic stays easy to locate and maintain.Source: Coding guidelines
sdk/typescript/src/cli/harness-envelope.ts (1)
37-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
buildEventEnvelopeV2exceeds the 20-line function guideline and lacks JSDoc.The function is ~49 lines with no JSDoc. As per coding guidelines, "Limit functions to a maximum of 20 lines of code; extract longer logic into separate helper functions" and "Always include JSDoc comments for public functions and exported modules." Consider extracting
bucket,scope,harness, andeventobject construction into small named helpers.🤖 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 `@sdk/typescript/src/cli/harness-envelope.ts` around lines 37 - 85, buildEventEnvelopeV2 is too large for the function-length guideline and is missing JSDoc. Add JSDoc to the exported buildEventEnvelopeV2 function, then refactor the envelope assembly into small named helpers for the bucket, scope, harness, and event sections so the main function stays under 20 lines while preserving the current envelope shape and use of EnvelopeContext, HarnessSemanticEvent, and stableEventId.Source: Coding guidelines
sdk/typescript/src/cli/harness-hooks.ts (1)
40-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared truncation helpers instead of duplicating them.
truncate,byteLength,firstLine, andELISIONare copy-pasted identically fromharness-events.ts, which this file already importsnormalizeToolKind/toolDisplayfrom. Moving these into a small shared module would avoid the two copies drifting (as already happened with the truncation bug flagged above).Also applies to: 387-396
🤖 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 `@sdk/typescript/src/cli/harness-hooks.ts` around lines 40 - 44, The truncation utilities are duplicated between harness-hooks.ts and harness-events.ts, so the shared logic should be extracted into a common module and both files updated to use it. Move the copied helpers such as truncate, byteLength, firstLine, and ELISION into a single shared place, then import them from harness-hooks rather than keeping local copies. Keep the existing behavior of the harnessHooks path intact while ensuring both callers use the same implementation.sdk/typescript/src/cli/harness-events.ts (2)
221-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
codexEventsFromLineis ~150 lines; extract per-record-type helpers.Far exceeds the 20-line function guideline. The Claude side already demonstrates the pattern to follow (
claudeUserBlock/claudeAssistantBlock); eachif (type === ...)branch here could become its own small named function (e.g.codexFunctionCall,codexFunctionCallOutput,codexTaskStatus), improving readability and testability without changing behavior.As per coding guidelines, "Limit functions to a maximum of 20 lines of code; extract longer logic into separate helper functions."
🤖 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 `@sdk/typescript/src/cli/harness-events.ts` around lines 221 - 370, `codexEventsFromLine` is too large and should be broken into smaller helpers to match the function-length guideline. Extract each record-type branch into named helpers in the same module, following the Claude-side pattern (for example, helpers for user messages, agent messages, reasoning, tool calls, tool results, MCP begin, and task status), and have `codexEventsFromLine` delegate to them while preserving the current behavior. Use the existing symbols like `codexEventsFromLine`, `codexOutputText`, `codexIsError`, and `toolDisplay` to keep the refactor localized and easy to test.Source: Coding guidelines
52-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSDoc to exported functions.
harnessEventsFromLine,claudeEventsFromLine,codexEventsFromLine, andnormalizeToolKindare exported (and re-exported from the SDK root) but lack JSDoc, unliketoolDisplayand theHarnessSemanticEvent/HarnessLineMapperdeclarations in the same file.As per coding guidelines, "Always include JSDoc comments for public functions and exported modules."
Also applies to: 63-66, 221-224, 417-417
🤖 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 `@sdk/typescript/src/cli/harness-events.ts` around lines 52 - 59, Add JSDoc comments to all exported public functions in this module so they match the SDK guideline and the existing documented symbols like toolDisplay and the type declarations. Update harnessEventsFromLine, claudeEventsFromLine, codexEventsFromLine, and normalizeToolKind with concise summaries and parameter/return descriptions as needed, keeping the docs attached to each function name so they remain correct if lines shift.Source: Coding guidelines
sdk/typescript/tests/harness-events.test.ts (1)
200-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't cover
display/inputforfunction_call.Once the
arguments-parsing fix inharness-events.tslands, consider assertingdisplayandinputhere too (e.g.display: "cargo test",input: { command: "cargo test" }) so this class of regression is caught going 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 `@sdk/typescript/tests/harness-events.test.ts` around lines 200 - 217, The `maps a function_call to a tool_call` test only checks the tool call shape and misses the parsed `display` and `input` fields. Update this spec in `codexEventsFromLine` to assert the `tool_call` payload also includes the expected `display` value and parsed `input` object for the `function_call` arguments, so regressions in `harness-events.ts` parsing are covered.sdk/typescript/src/cli/harness-consumer.ts (2)
39-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing JSDoc on exported functions/types.
isV2(here),initialSessionView(line 91), and theSessionViewLimitsinterface (line 84) are exported without JSDoc comments, unlike the neighboringparseSessionEnvelope/applySessionEnvelope/foldSessionEnvelopes/ToolActivity/FeedEntry/SessionView.As per coding guidelines, "Always include JSDoc comments for public functions and exported modules."
🤖 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 `@sdk/typescript/src/cli/harness-consumer.ts` around lines 39 - 43, Add JSDoc comments for the exported `isV2` type guard, the `SessionViewLimits` interface, and `initialSessionView` so they follow the same public API documentation style as `parseSessionEnvelope`, `applySessionEnvelope`, `foldSessionEnvelopes`, `ToolActivity`, `FeedEntry`, and `SessionView`. Keep the comments brief but descriptive, explaining each symbol’s purpose and exported contract without changing behavior.Source: Coding guidelines
150-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
applyEventexceeds the 20-line function limit.This switch handles 8 event kinds across ~77 lines. Consider extracting each case body into a small named helper (
applyToolCall,applyToolResult,applyStatus, etc.) so each stays under the limit and is independently testable.As per coding guidelines, "Limit functions to a maximum of 20 lines of code; extract longer logic into separate helper functions."
🤖 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 `@sdk/typescript/src/cli/harness-consumer.ts` around lines 150 - 226, `applyEvent` is too large and violates the 20-line function limit. Refactor the switch in `applyEvent` by extracting each event-case body into small named helpers such as `applyToolCall`, `applyToolResult`, `applyStatus`, `applyApprovalRequest`, `applyError`, `applyLifecycle`, and `applyTextEvent`. Keep `applyEvent` as a thin dispatcher that delegates to these helpers while preserving the existing `view`, `envelope`, and `limits` behavior.Source: Coding guidelines
🤖 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 `@sdk/typescript/src/cli/harness-envelope.ts`:
- Around line 45-50: The v2 event ID generation in `harness-envelope.ts` is
using the process-local `seq`, which can change after a restart and break replay
deduplication. Update the `stableEventId` call in the event envelope builder to
use a persisted/content-stable value from `semantic` such as `semantic.line` (or
another stable source key) instead of `seq`, while keeping the rest of the
`ctx.wrapperSessionId`, `semantic.recordType`, and `semantic.event.kind` inputs
unchanged. Make sure the chosen key is deterministic across resend/replay so the
same semantic event always produces the same `event.id`.
In `@sdk/typescript/src/cli/harness-events.ts`:
- Around line 479-488: The truncate helper in harness-events enforces the limit
by byteLength but still uses character-based slicing, so multi-byte text can
exceed OUTPUT_CAP; update truncate() to perform byte-safe truncation using byte
length as the actual cutoff and keep the ELISION suffix within the cap, with
byteLength() remaining the check helper.
- Around line 278-299: The tool call mapping in harness-events should parse
Codex `arguments` into an object before assigning `input`, since `toolDisplay()`
and the downstream `tool_call` shape expect structured data rather than a raw
JSON string. Update the `response_item` handling for `function_call` and
`tool_search_call` to deserialize `payload.arguments` (and apply the same logic
in `mcp_tool_call_begin`) before building the event payload, while keeping
`payload.query`/`payload.input` as-is when already structured.
In `@sdk/typescript/src/cli/harness-hooks.ts`:
- Around line 363-369: `boundRaw()` has the same byte-vs-character truncation
bug as `truncate()`: it checks `byteLength(serialized) <= RAW_CAP` but then
slices `serialized` by character count, which can still exceed the byte limit
for multi-byte text. Update `boundRaw` in `harness-hooks.ts` to use a byte-safe
truncation approach for the `serialized` fallback, matching the fix used in
`truncate()` and any related helper in the same area.
In `@sdk/typescript/src/cli/harness-status.ts`:
- Around line 128-153: The lifecycle mapping in lifecycleStatus() is missing
support for the compact phase, so compaction events are ignored. Update
lifecycleStatus(phase) in harness-status.ts to recognize "compact" and return an
appropriate Derived status instead of falling through to undefined, keeping the
status transition consistent with the other lifecycle phases.
In `@sdk/typescript/tests/harness-wrapper-v2.test.ts`:
- Around line 101-105: The temporary fixture directories created in fixture()
are only tracked in dirs and never removed, so add real cleanup in the
HarnessSessionTailer v2 emission test setup. Update afterEach to iterate over
dirs and delete each mkdtempSync-created directory from disk before clearing the
array, using the existing dirs fixture tracking and fixture helper to locate the
cleanup point.
---
Nitpick comments:
In `@sdk/typescript/src/cli/harness-consumer.ts`:
- Around line 39-43: Add JSDoc comments for the exported `isV2` type guard, the
`SessionViewLimits` interface, and `initialSessionView` so they follow the same
public API documentation style as `parseSessionEnvelope`,
`applySessionEnvelope`, `foldSessionEnvelopes`, `ToolActivity`, `FeedEntry`, and
`SessionView`. Keep the comments brief but descriptive, explaining each symbol’s
purpose and exported contract without changing behavior.
- Around line 150-226: `applyEvent` is too large and violates the 20-line
function limit. Refactor the switch in `applyEvent` by extracting each
event-case body into small named helpers such as `applyToolCall`,
`applyToolResult`, `applyStatus`, `applyApprovalRequest`, `applyError`,
`applyLifecycle`, and `applyTextEvent`. Keep `applyEvent` as a thin dispatcher
that delegates to these helpers while preserving the existing `view`,
`envelope`, and `limits` behavior.
In `@sdk/typescript/src/cli/harness-envelope.ts`:
- Around line 37-85: buildEventEnvelopeV2 is too large for the function-length
guideline and is missing JSDoc. Add JSDoc to the exported buildEventEnvelopeV2
function, then refactor the envelope assembly into small named helpers for the
bucket, scope, harness, and event sections so the main function stays under 20
lines while preserving the current envelope shape and use of EnvelopeContext,
HarnessSemanticEvent, and stableEventId.
In `@sdk/typescript/src/cli/harness-events.ts`:
- Around line 221-370: `codexEventsFromLine` is too large and should be broken
into smaller helpers to match the function-length guideline. Extract each
record-type branch into named helpers in the same module, following the
Claude-side pattern (for example, helpers for user messages, agent messages,
reasoning, tool calls, tool results, MCP begin, and task status), and have
`codexEventsFromLine` delegate to them while preserving the current behavior.
Use the existing symbols like `codexEventsFromLine`, `codexOutputText`,
`codexIsError`, and `toolDisplay` to keep the refactor localized and easy to
test.
- Around line 52-59: Add JSDoc comments to all exported public functions in this
module so they match the SDK guideline and the existing documented symbols like
toolDisplay and the type declarations. Update harnessEventsFromLine,
claudeEventsFromLine, codexEventsFromLine, and normalizeToolKind with concise
summaries and parameter/return descriptions as needed, keeping the docs attached
to each function name so they remain correct if lines shift.
In `@sdk/typescript/src/cli/harness-hooks.ts`:
- Around line 40-44: The truncation utilities are duplicated between
harness-hooks.ts and harness-events.ts, so the shared logic should be extracted
into a common module and both files updated to use it. Move the copied helpers
such as truncate, byteLength, firstLine, and ELISION into a single shared place,
then import them from harness-hooks rather than keeping local copies. Keep the
existing behavior of the harnessHooks path intact while ensuring both callers
use the same implementation.
In `@sdk/typescript/src/cli/harness-status.ts`:
- Around line 41-63: The exported reduceStatus reducer is missing the required
JSDoc comment. Add a brief JSDoc block above reduceStatus in harness-status.ts
describing that it reduces a HarnessSemanticEvent into the next
SessionStatusState/StatusStep, matching the style used by initialStatus and
tickStatus. Keep the documentation on the exported function itself so the public
API is covered.
- Around line 98-139: deriveFromEvent is too long and should be broken up to
satisfy the 20-line function guideline. Extract the switch logic in
harness-status.ts into smaller helpers, such as per-event mappers for tool_call,
approval_request, error, and a small dispatcher from deriveFromEvent, while
keeping the existing behavior of deriving state, detail, and activeCallId from
HarnessSemanticEvent. Use clear helper names tied to the current cases so the
logic stays easy to locate and maintain.
In `@sdk/typescript/src/types/harness.ts`:
- Around line 201-206: Remove the unused source_role field from
SessionEnvelopeV2 in the source type definition, since the v2 builder does not
populate it and no other code writes it. Update the SessionEnvelopeV2 shape so
the wire contract only includes the fields that are actually produced; if you
want to preserve the field for future use, add a brief inline note near the
source definition instead of keeping the optional property.
In `@sdk/typescript/tests/harness-events.test.ts`:
- Around line 200-217: The `maps a function_call to a tool_call` test only
checks the tool call shape and misses the parsed `display` and `input` fields.
Update this spec in `codexEventsFromLine` to assert the `tool_call` payload also
includes the expected `display` value and parsed `input` object for the
`function_call` arguments, so regressions in `harness-events.ts` parsing are
covered.
🪄 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: 2234b449-1174-464e-a10e-2981c0871a43
📒 Files selected for processing (14)
sdk/typescript/src/cli/harness-consumer.tssdk/typescript/src/cli/harness-envelope.tssdk/typescript/src/cli/harness-events.tssdk/typescript/src/cli/harness-hooks.tssdk/typescript/src/cli/harness-status.tssdk/typescript/src/cli/harness-wrapper.tssdk/typescript/src/index.tssdk/typescript/src/types/harness.tssdk/typescript/tests/harness-consumer.test.tssdk/typescript/tests/harness-envelope.test.tssdk/typescript/tests/harness-events.test.tssdk/typescript/tests/harness-hooks.test.tssdk/typescript/tests/harness-status.test.tssdk/typescript/tests/harness-wrapper-v2.test.ts
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Nice, cleanly-layered v2 stream with strong test coverage, and the opt-in gating keeps v1 byte-identical — thank you. Two majors to resolve before merge (inline). Everything else in my review is minor/nits:
-
tool_call.inputis published verbatim — unbounded and unredacted — contradicting its own type comment.tool_result.outputis capped at 4KB but the call-sideinputis not, so aWrite/MultiEditcarries the whole file and aBashheredoc the whole script over the DM, and tool inputs routinely contain secrets (tokens, credential-file writes, MCP keys). Bound it where the payload is built (Claudeharness-events.ts:191, Codex:294/:343, hookharness-hooks.ts:138) mirroring theoutputcap, and redact if that's what the comment intends — or fix the comment to match reality. -
The untrusted-DM decoder crashes on a version-tagged but malformed envelope.
parseSessionEnvelopevalidates onlyenvelope_versionbefore casting, andapplySessionEnvelope/foldSessionEnvelopesthen readevent.sequnguarded — a{"envelope_version":"tinyplace.harness.session.v2"}body (noevent) throws instead of being dropped. Validate the v2 shape before the cast.
Minor/nits (not blocking): truncate slices by UTF-16 code units after a byte check (~4× loose for multibyte); each semantic event also emits a derived status envelope (~2× DM volume); event.id is described as content-addressed but hashes seq, not payload, and v2Seq resets to 0 on restart; Number(env ?? default) yields NaN and silently disables idle on non-numeric env. Happy to expand any of these.
| tool_name: string; // raw harness name, e.g. "Bash" | ||
| tool_kind: HarnessToolKind; | ||
| display: string; // one-line human summary, e.g. "npm test" | ||
| input: unknown; // bounded + redacted by the tailer before publish |
There was a problem hiding this comment.
Major — contract not honored; unbounded + potential secret exposure.
This comment promises input is bounded + redacted by the tailer before publish, but nothing does either. harness-events.ts:191 (Claude tool_use), :294/:343 (Codex function_call/mcp), and harness-hooks.ts:138 (PreToolUse) all pass the raw input/arguments straight through; writeV2 → buildEventEnvelopeV2 spreads ...semantic.event and publishNow sends JSON.stringify(envelope) over the Signal DM with no size guard.
- Size:
tool_result.outputis capped at 4KB (truncate), buttool_call.inputis not — aWrite/MultiEdit/create_filecall carries the entire file, aBashheredoc the entire script, so the call side can dwarf the (carefully bounded) result side and blow past DM limits. - Secrets: tool inputs routinely contain sensitive data (a
Bashcommand thatexports a token, anEditwriting a credentials file, an MCP call with an API key). The redaction promise exists for exactly this reason.
Suggest bounding where the payload is built, mirroring the output cap:
// in harness-events.ts, alongside truncate()
function boundInput(input: unknown): unknown {
const s = safeStringify(input);
return byteLength(s) <= OUTPUT_CAP ? input : `${s.slice(0, OUTPUT_CAP)}${ELISION}`;
}
// ...then at each pass-through site:
- input,
+ input: boundInput(input),If secret-key redaction is intended, it belongs here too. At minimum, correct this comment so downstream consumers don't assume a bound that isn't there.
There was a problem hiding this comment.
Confirmed resolved — boundToolInput/boundRaw byte-cap tool input at every emit site (Claude tool_use, Codex function_call + MCP, hook PreToolUse) and the type comment now matches. Thanks for the catch.
| version === SESSION_ENVELOPE_VERSION_V2 || | ||
| version === SESSION_ENVELOPE_VERSION_V1 | ||
| ) { | ||
| return record as AnySessionEnvelope; |
There was a problem hiding this comment.
Major — untrusted-input decoder throws instead of rejecting.
This module is documented as the reference decoder for DM bodies, and inbound Signal DMs are untrusted. parseSessionEnvelope validates only envelope_version here and then return record as AnySessionEnvelope with no shape check. Downstream, applySessionEnvelope:115 (if (event.seq <= view.lastSeq)) and foldSessionEnvelopes:143 (.sort((a, b) => a.event.seq - b.event.seq)) read event.seq unguarded — so a body like {"envelope_version":"tinyplace.harness.session.v2"} (no event) passes isV2 and then throws TypeError: Cannot read properties of undefined (reading 'seq'), taking down the fold.
Validate the v2 shape before casting:
if (version === SESSION_ENVELOPE_VERSION_V2) {
const ev = (record as { event?: { seq?: unknown; id?: unknown } }).event;
if (!ev || typeof ev.seq !== "number" || typeof ev.id !== "string") return undefined;
return record as AnySessionEnvelope;
}
if (version === SESSION_ENVELOPE_VERSION_V1) return record as AnySessionEnvelope;
return undefined;Worth a test feeding a malformed v2 body through parseSessionEnvelope/foldSessionEnvelopes asserting it's dropped, not thrown.
There was a problem hiding this comment.
Fixed in c93fc44 — see the reply on the re-review thread. Shape-validated before cast + guarded both fold reads, with regression tests.
…export harness-hooks imports node:process and harness-envelope imports node:crypto. Re-exporting their runtime values from the isomorphic root forced browser bundlers (Next/Webpack client builds) to resolve those Node built-ins even when unused. Move the harness runtime value exports to the ./node subpath and keep only the (erased) type re-exports in the root entry. (tinyhumansai#230)
truncate() and boundRaw() guarded on Buffer.byteLength but then sliced with String.slice, which is code-unit indexed. Multi-byte payloads (emoji / CJK / non-ASCII file contents) could stay several times larger than the byte cap the helper exists to enforce for envelope transport. Slice the UTF-8 buffer instead; Buffer.toString drops a trailing partial code point safely. (tinyhumansai#230)
Codex serializes function_call and mcp_tool_call_begin arguments as a JSON string. toolDisplay only reads object fields, so the string fell through to the raw blob and the published input stayed inconsistent with Claude's object shape. Parse the JSON string back to structured data before deriving display and input. (tinyhumansai#230)
tool_input for Write/Edit/MCP can carry full file contents or large request payloads, but it was stored verbatim in every v2 tool_call envelope while only outputs were capped. One large edit could exceed relay/message limits or leak content despite the contract saying input is bounded before publish. Byte-cap the serialized input (INPUT_CAP) across the Claude, Codex and hook tool_call mappers; small inputs keep their structure. (tinyhumansai#230)
Codex rollout records may omit a top-level timestamp. parseTimestamp returned the Unix epoch, and while buildEventEnvelopeV2 normalizes the envelope ts to now, reduceStatus consumes the original semantic timestamp. An epoch value never advances the activity clock, so tickStatus saw the session as instantly stale and flipped a live Codex status to idle right after a tool call. Default missing/invalid timestamps to receive time before status reduction. (tinyhumansai#230)
LifecyclePayload.phase includes "compact" (emitted for a compaction SessionStart), but lifecycleStatus fell through to the default and left the status unchanged. Surface compaction as active work so a live session isn't shown with a stale status while it compacts. (tinyhumansai#230)
The v2 envelope builder never populated source_role and no consumer read it. Remove the dead field; SessionEnvelopeV1 keeps its own source_role, which the wrapper still sets. (tinyhumansai#230)
afterEach cleared the tracked dirs array but never deleted the mkdtemp directories, leaking temp files on every run. rm each tracked dir before clearing. (tinyhumansai#230)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/typescript/tests/harness-events.test.ts (1)
277-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
claudeLinehelper name is misleading here.
claudeLineis reused to stringify a Codex-shaped fixture record forcodexEventsFromLine. It's just a generic JSON-stringify helper, so the Claude-specific name is confusing at call sites testing Codex parsing.♻️ Suggested rename
-const claudeLine = (record: unknown) => JSON.stringify(record); +const jsonLine = (record: unknown) => JSON.stringify(record);(and update call sites accordingly)
🤖 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 `@sdk/typescript/tests/harness-events.test.ts` around lines 277 - 289, The helper name used in the `harness-events.test.ts` fixture is misleading: `claudeLine` is being used as a generic JSON-stringify wrapper for `codexEventsFromLine`, not something Claude-specific. Rename `claudeLine` to a neutral name that reflects its actual behavior, and update the call sites in this test (and any other references) so the Codex parsing fixtures are easier to understand.
🤖 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.
Nitpick comments:
In `@sdk/typescript/tests/harness-events.test.ts`:
- Around line 277-289: The helper name used in the `harness-events.test.ts`
fixture is misleading: `claudeLine` is being used as a generic JSON-stringify
wrapper for `codexEventsFromLine`, not something Claude-specific. Rename
`claudeLine` to a neutral name that reflects its actual behavior, and update the
call sites in this test (and any other references) so the Codex parsing fixtures
are easier to understand.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43a6496f-c04d-4ddc-b62b-55b52a884f3f
📒 Files selected for processing (9)
sdk/typescript/src/cli/harness-events.tssdk/typescript/src/cli/harness-hooks.tssdk/typescript/src/cli/harness-status.tssdk/typescript/src/index.tssdk/typescript/src/node/index.tssdk/typescript/src/types/harness.tssdk/typescript/tests/harness-events.test.tssdk/typescript/tests/harness-status.test.tssdk/typescript/tests/harness-wrapper-v2.test.ts
💤 Files with no reviewable changes (1)
- sdk/typescript/src/types/harness.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- sdk/typescript/tests/harness-wrapper-v2.test.ts
- sdk/typescript/tests/harness-status.test.ts
- sdk/typescript/src/cli/harness-status.ts
- sdk/typescript/src/cli/harness-events.ts
- sdk/typescript/src/cli/harness-hooks.ts
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Re-reviewed at 770288fc — thanks for the fast turnaround. Major #1 (unbounded/unredacted tool_call.input) is resolved: boundToolInput/boundRaw now byte-cap input at every emit site (Claude tool_use, Codex function_call + MCP, hook PreToolUse), structure preserved when small. You also picked up all my minors and then some — byte-indexed truncate in both mappers, parseMaybeJson on Codex arguments so input/toolDisplay match Claude's object shape, receive-time fallback for timestamp-less events (fixes the premature-idle edge), the compact lifecycle phase in the status reducer, and the Node-only /node subpath split to keep browser bundles clean. All covered by new tests. 👍
One item still open — Major #2: the consumer still crashes on a malformed-but-version-tagged DM body. parseSessionEnvelope (harness-consumer.ts:34) validates only envelope_version before casting, and applySessionEnvelope:115 / foldSessionEnvelopes:143 read event.seq unguarded — so {"envelope_version":"tinyplace.harness.session.v2"} still throws instead of being dropped. Since this is the reference decoder for untrusted inbound DMs, worth shape-validating before the cast (fix + suggested test in the inline comment). Once that lands I'm happy to approve.
| version === SESSION_ENVELOPE_VERSION_V2 || | ||
| version === SESSION_ENVELOPE_VERSION_V1 | ||
| ) { | ||
| return record as AnySessionEnvelope; |
There was a problem hiding this comment.
Still open (Major #2). Unchanged since the last review — this casts on envelope_version alone, and applySessionEnvelope/foldSessionEnvelopes then read event.seq unguarded, so a version-tagged body with no event throws TypeError: Cannot read properties of undefined (reading 'seq') rather than being dropped. Given the module decodes untrusted inbound DMs, validate the v2 shape before casting:
if (version === SESSION_ENVELOPE_VERSION_V2) {
const ev = (record as { event?: { seq?: unknown; id?: unknown } }).event;
if (!ev || typeof ev.seq !== "number" || typeof ev.id !== "string") return undefined;
return record as AnySessionEnvelope;
}
if (version === SESSION_ENVELOPE_VERSION_V1) return record as AnySessionEnvelope;
return undefined;Plus a test feeding a malformed v2 body through parseSessionEnvelope/foldSessionEnvelopes asserting it's dropped, not thrown.
There was a problem hiding this comment.
Addressed in c93fc44 — parseSessionEnvelope now shape-checks the v2 event (numeric seq + string id) via a shared hasUsableV2Event guard, wired into both fold paths (applySessionEnvelope + foldSessionEnvelopes .filter) so a malformed-but-version-tagged body is dropped, not thrown — whether it arrives via the parser or pre-parsed. Added 3 tests: parse drops it, apply returns the same view (no throw), fold skips it and still folds the good stream. Full suite 450 pass.
parseSessionEnvelope validated only envelope_version before casting, so a
version-tagged body with a missing/malformed event (e.g. {"envelope_version":
"...v2"}) passed isV2 and then threw TypeError on event.seq in
applySessionEnvelope/foldSessionEnvelopes. Since this decodes untrusted inbound
Signal DMs, shape-check the event (numeric seq + string id) via a shared
hasUsableV2Event guard wired into the parser and both fold paths, so a bad
packet is dropped, not fatal.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Approving — both blocking findings are resolved and CI is green (Test/Typecheck/Lint/Build all pass; only the unrelated Vercel team-auth check is red).
Major #2 (malformed-envelope crash) — ✅ fixed, thoroughly. hasUsableV2Event shape-checks event.seq/event.id, and you gated it in all three paths — parseSessionEnvelope, applySessionEnvelope, and foldSessionEnvelopes — so a poisoned body is dropped whether it arrives raw or pre-parsed. Tests cover missing-event, missing-seq, non-string-id, and the poisoned-stream-doesn't-throw case. That's better than the minimal fix I suggested.
Major #1 (unbounded/unredacted tool_call.input) — ✅ fixed last round (boundToolInput/boundRaw at every emit site).
Recap of everything addressed across the three rounds: input bounding, byte-indexed truncation, Codex parseMaybeJson, receive-time timestamp fallback, compact lifecycle mapping, the /node subpath split to keep Node built-ins out of browser bundles, and now the untrusted-decoder hardening. Nice work — the v2 stream is clean, well-typed, well-tested, and opt-in. LGTM. 🚀
Number(env ?? default) yielded NaN when a status/poll env var was set to a non-numeric value, silently disabling the idle/heartbeat tick. Route all five knobs through numberEnvOr, which falls back to the default on unset OR non-finite input.
The id is deterministic over (wrapperSessionId, recordType, seq, kind), stable for resend-dedup within a run and collision-free across runs because wrapperSessionId rotates per process — so seq resetting to 0 on relaunch is safe.
|
@sanil-23 Major #2 is fixed (
Full suite 450 pass, build clean. Re-requested your review — thanks for the thorough pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sdk/typescript/src/cli/harness-wrapper.ts`:
- Around line 448-489: The env-backed timer fields in harness-wrapper are too
permissive because numberEnvOr accepts blank strings as 0 and allows negative
values, which can break the polling and heartbeat intervals. Update the
parsing/validation used for the timer settings in the harness config block
(including statusHeartbeatMs, statusIdleMs, receivePollMs, sessionPollMs, and
sessionTailGraceMs) so blank, zero, and negative env values are rejected and the
existing defaults are preserved, matching the CLI validation behavior.
🪄 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: 72a7340b-5e08-4a96-a503-0fa6e4bb7c77
📒 Files selected for processing (2)
sdk/typescript/src/cli/harness-envelope.tssdk/typescript/src/cli/harness-wrapper.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/typescript/src/cli/harness-envelope.ts
numberEnvOr used Number(raw) + isFinite, so a blank var parsed to 0 and negatives passed — either disabling the idle/heartbeat tick or spinning a 0 ms poll loop. Require a finite value > 0 (after trim); fall back to the default otherwise.
|
@coderabbitai the one actionable from the last pass — the over-permissive |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n "numberEnvOr" -A 15 sdk/typescript/src/cli/harness-wrapper.ts | head -60Length of output: 2373 Confirmed — All threads from my review are addressed. Approving. [approve] |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/typescript/src/cli/harness-wrapper.ts (1)
972-1008: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a JSDoc comment for the public
publishmethod.
publishis a public method whose contract changed (now acceptsAnySessionEnvelope), but it lacks a JSDoc block documenting the widened v1/v2 envelope input and its fire-and-forget queueing/DM-forwarding behavior.As per coding guidelines: "Always include JSDoc comments for public functions and exported modules".
🤖 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 `@sdk/typescript/src/cli/harness-wrapper.ts` around lines 972 - 1008, Add a JSDoc block to the public harness-wrapper method `publish` documenting that it accepts `AnySessionEnvelope`, supports both v1/v2 envelope inputs, and is fire-and-forget by enqueueing DM forwarding rather than returning a result. Place it directly above `publish(envelope: AnySessionEnvelope): void` and keep the summary aligned with the method’s queueing, skip-on-dry-run/no-recipient, and `publishNow` forwarding behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@sdk/typescript/src/cli/harness-wrapper.ts`:
- Around line 972-1008: Add a JSDoc block to the public harness-wrapper method
`publish` documenting that it accepts `AnySessionEnvelope`, supports both v1/v2
envelope inputs, and is fire-and-forget by enqueueing DM forwarding rather than
returning a result. Place it directly above `publish(envelope:
AnySessionEnvelope): void` and keep the summary aligned with the method’s
queueing, skip-on-dry-run/no-recipient, and `publishNow` forwarding behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6279d84-8caf-4650-a4fe-51b4ce9a4a1a
📒 Files selected for processing (1)
sdk/typescript/src/cli/harness-wrapper.ts
2 participants
Summary
Adds a typed v2 session-event stream (
tinyplace.harness.session.v2) to the harness bridge, so a consumer (OpenHuman's Master Session) can tell apart tool_call · agent_message · tool_result · status · everything else from one field — instead of the v1 text-only stream that only carries "who spoke."Builds on #229 (merged). Emission is opt-in and off by default, so current v1 consumers are byte-identical until they understand v2.
What's here
types/harness.ts) — additive over v1: reusesbucket/scope/harnessverbatim, swaps onlymessage+sourcefor a typedevent. Two axes:role(owner|agent) +kind(user_prompt|agent_message|agent_thinking|approval_request|tool_call|tool_result|status|lifecycle|error|unknown). v1 and v2 coexist; discriminate onenvelope_version.SessionEnvelopealias stays v1-only so no current importer changes.harness-events.ts) — splits Claudetool_use/tool_result/thinking/textand Codexfunction_call/function_call_output/reasoning/messages into per-block typed events.tool_call↔tool_resultcorrelate viacall_id. Registry dispatch so a 3rd provider (e.g. gemini) is a compile-checked add.harness-hooks.ts) — maps Claude Code hook payloads (PreToolUse/PostToolUse/UserPromptSubmit/Stop/SessionStart/…) into the same event shape, for lower latency than JSONL tailing.tool_use_idon both PreToolUse and PostToolUse gives exact call↔result correlation.harness-status.ts) — derivesrunning_tool/waiting_approval/idle+ heartbeat → drives InstanceStatus + currentTask.harness-envelope.ts) — seq + stable content-addressed id + bucket window.harness-consumer.ts) —parseSessionEnvelope+applySessionEnvelope/foldSessionEnvelopesfold the stream into a liveSessionView(status, currentTask, tool log, capped feed). Framework-agnostic reference decoder.harness-wrapper.ts) — emits v2 behindTINYPLACE_HARNESS_V2=1(default off). Publisher/writeEnvelope accept either version via anenvelope_versiondiscriminant.Testing
tscbuild clean; SDK lint (tsc --noEmit) clean.tool_call/tool_result/statuswith correlatedcall_idand monotonic seq.Notes
Summary by CodeRabbit