Conversation
Introduce a new @beeper/pickle-pi package (package.json, README, tsconfig and a large set of src/tests) as the Beeper-first Pi appservice/bridge skeleton. Add an HTTPProxyHandlingBridgeConnector type and wire runtime handling: RuntimeBridge now exposes getOwnUserId() and will delegate provisioning HTTP requests to connector.handleHTTPProxy when present. Update bridge types to include getOwnUserId and the new connector interface, and adjust workspace/tsconfig to include the new package.
Introduce a new media-store module to save/read Matrix media with stable ids and sidecar metadata (saveMediaBuffer, saveMatrixAttachment, readMediaBuffer/readStoredMedia, helpers for mime/kind/id). Add media-store tests and export it from the package index. Extend rooms to support fork and subagent metadata (createForkMetadata, createSubagentMetadata) and accept them when creating session rooms. Expand types with PicklePiForkMetadata, PicklePiSubagentMetadata, and binding kind/subagent/fork fields. Enhance the registry with additional lookup helpers (by id, by cwd, child/subagent bindings), updateBinding and setActiveLeaf methods, and add tests to cover indexing child and subagent bindings.
Initialize the Beeper appservice in bridge entrypoints and construct RuntimeBridge with appservice info (homeserver/token) instead of the previous client helper. This adds createBeeperAppServiceInit usage and wires appservice.registration.asToken and homeserver into matrix config, and passes appservice + beeper metadata into RuntimeBridge (packages/bridge/src/index.ts, node.ts). Also extend approval handling: add session/room approval reaction constants and decision types, normalize hyphenated decision values, and update parsing logic to derive approvedAlways and decision correctly. Tests updated to cover new allow_session/allow_room cases (packages/pi/src/*).
Propagate appservice configuration through the bridge and pickle stacks so bridges can initialize Matrix clients as appservice bots. Key changes: - Bridge: add BridgeMatrixConfig.appservice and set matrix.appservice/homeserver/token from the created appservice when building beeper bridges; introduce createBeeperRuntimeOptions helper and return RuntimeBridge with the assembled runtime options. - Pickle native (Go): extend MatrixCoreInitOptions with Appservice, extract client init logic into initClient which handles both appservice login (verifies flows and logs in as appservice bot) and normal token-based clients. - Pickle JS: pass the appservice option through to core.init and update generated/runtime types and MatrixClientOptions to include appservice. These changes enable using the appservice registration/token for homeserver, token and bot identity instead of relying solely on an account access token.
Expose an initialState option for portal room creation and propagate it through types, runtime, and native appservice code so initial state events are included on room creation. Allow callers to provide an existing appservice via options.matrix?.appservice (prefer it over creating a new one) in bridge factory functions and propagate the resolved appservice into the matrix config. Also include a small provisioning change (query value in test and conservative lookup in provisioningLogin).
Avoid awaiting each publishBeeperStreamPart call serially by introducing a publishPart helper that fires publishes, tracks them in a pendingPublishes set, logs errors, and removes completed promises. A waitForPublishes call ensures all publishes finish before editing the final message. Also set preliminary: false on tool execution end UI chunks to mark tool output as final.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR makes the bridge appservice-first (deviceId persistence, appservice init), centralizes Matrix Provisioning v3 handling, and adds a full new ChangesBridge Initialization & Provisioning
Pi Package – Complete New Implementation
Native Go & Pickle Client Enhancements
Stream Publishing Enhancement
Workspace & Paths
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Add exports for beeper-stream and pi-event-map and update tsdown entries. Apply initialMessageMetadata to outgoing messages and include messageMetadata in finalize; allow a terminalPart override. Extend final-message accumulator to track tool input text and tool names, add ensureToolPart and parseMaybeJSON to assemble tool parts and finalize content. Improve event mapping to handle text/thinking start/end and toolcall_start/delta/end, normalize tool outputs and parse stringified arguments. Update stream mapping to include dynamic flag, timestamps and preliminary/completed fields on tool parts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bridge/src/bridge.ts (2)
200-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep owner and bot identities separate after appservice boot.
createBeeperBridge()now defaults the client token toregistration.asToken, sowhoami.userIdbecomes the appservice bot. Assigning that to#ownerUserIdbreaks implicit management-room detection, default remote sender attribution, and provisioning logins for the real Beeper owner account. Initialize#ownerUserIdfromthis.#beeperOptions?.ownerUserId ?? whoami.userId, and reserve#ownUserIdfor the bot identity.Suggested fix
const whoami = await this.#matrixClient.boot(); - this.#ownerUserId = whoami.userId; - this.#ownUserId = whoami.userId; + this.#ownerUserId = this.#beeperOptions?.ownerUserId ?? whoami.userId; + this.#ownUserId = whoami.userId; defaultLogger("info", "bridge_matrix_booted", { userId: whoami.userId }); if (this.#appserviceOptions) { const result = await this.#matrixClient.appservice.init(this.#appserviceOptions); defaultLogger("info", "bridge_appservice_initialized", { botUserId: appserviceBotUserId(this.#appserviceOptions), @@ }); this.#ownUserId = appserviceBotUserId(this.#appserviceOptions); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridge/src/bridge.ts` around lines 200 - 213, The code currently sets both `#ownerUserId` and `#ownUserId` from whoami.userId which makes the owner become the appservice bot; change initialization so `#ownerUserId` is set to this.#beeperOptions?.ownerUserId ?? whoami.userId (preserving an explicit owner if provided) and keep `#ownUserId` reserved for the bot identity (set from whoami.userId initially and later overwritten by appserviceBotUserId(this.#appserviceOptions) in the appservice init block); update the assignment near the this.#matrixClient.boot() call in bridge.ts to use these distinct sources (`#ownerUserId` from beeperOptions fallback, `#ownUserId` from whoami.userId).
111-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
createBeeperBridgeWithClient()resolves Matrix defaults it never applies.The new
matrixobject computes appservice, device ID, homeserver, store, and token, butRuntimeBridgeuses the suppliedclientdirectly and does not consume those resolved values. Callers expecting this helper to behave likecreateBeeperBridge()will still boot whatever identity/config the prebuilt client was created with.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridge/src/bridge.ts` around lines 111 - 131, createBeeperBridgeWithClient computes resolved Matrix defaults into the local matrix object but then passes the original client to RuntimeBridge, so those defaults are never applied; update createBeeperBridgeWithClient so that after computing matrix it ensures the MatrixClient used by RuntimeBridge matches those resolved values—either by creating a new MatrixClient from the resolved matrix (using your existing client-creation helper) or by applying the resolved token/deviceId/homeserver properties to the supplied client—then pass that updated/constructed client along with createBeeperRuntimeOptions(options, appservice, matrix) into new RuntimeBridge; reference createBeeperBridgeWithClient, matrix, RuntimeBridge, and createBeeperRuntimeOptions to locate where to make the change.
🧹 Nitpick comments (10)
packages/pi/src/pi-runtime.ts (1)
60-68: ⚡ Quick winDynamic import bypasses static analysis and bundler optimization.
Using
new Functionto construct a dynamic import prevents bundlers and static analysis tools from discovering the dependency on@earendil-works/pi-coding-agent. While the descriptive error message is helpful, this approach can make dependency tracking and optimization more difficult.Consider using standard dynamic import syntax (
import()) if the goal is optional dependency loading, or document why the Function constructor approach is necessary.📦 Suggested refactor using standard dynamic import
- const dynamicImport = new Function("specifier", "return import(specifier)") as (specifier: string) => Promise<unknown>; - return (await dynamicImport("@earendil-works/pi-coding-agent")) as Awaited<ReturnType<typeof loadPiCodingAgent>>; + return (await import("@earendil-works/pi-coding-agent")) as Awaited<ReturnType<typeof loadPiCodingAgent>>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi/src/pi-runtime.ts` around lines 60 - 68, The code uses new Function("specifier", "return import(specifier)") to perform a dynamic import (dynamicImport) which bypasses bundler/static analysis; replace that construct with the standard dynamic import expression import("@earendil-works/pi-coding-agent") (await it and cast to Awaited<ReturnType<typeof loadPiCodingAgent>> as before) inside the same try/catch so bundlers can detect the optional dependency while preserving the existing error handling that throws a new Error with the original error as cause.packages/pi/src/registry.ts (1)
14-108: ⚖️ Poor tradeoffConsider documenting concurrency guarantees.
The registry class performs in-memory mutations (
upsertBinding,updateBinding, etc.) without locking. If multiple instances or concurrent operations access the same registry file, the last writer wins, potentially losing updates.Document whether:
- Single-threaded/single-instance usage is expected
- External locking is required for concurrent access
- Or if eventual consistency is acceptable
This is informational; the implementation is correct for single-instance usage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi/src/registry.ts` around lines 14 - 108, Summarize concurrency guarantees for PicklePiRegistry: add documentation (class JSDoc or README) near the PicklePiRegistry declaration explaining that registry mutators (upsertBinding, updateBinding, markDedupe, upsertProjectSpace, setActiveLeaf) operate on in-memory state and that load/save use a temporary-file + rename for atomic writes but do not provide multi-process synchronization; state the expected usage is single-instance/single-threaded, recommend using external locking (e.g., flock/advisory file locks or a coordinating service) if multiple processes may access the same path, or accept last-writer-wins eventual consistency, and point readers at the load() and save() behavior for how to handle concurrent writers.packages/bridge/src/provisioning.ts (3)
233-238: 💤 Low value
stripUndefinedmutates input object.The function mutates the input object by deleting keys with
undefinedvalues. While this may be intentional for efficiency, it could lead to unexpected behavior if callers assume their objects remain unchanged.Consider documenting this behavior or returning a new object if immutability is important in this codebase.
📝 Optional improvement for immutability
function stripUndefined<T extends Record<string, unknown>>(value: T): StripUndefined<T> { + const result = { ...value }; - for (const key of Object.keys(value)) { - if (value[key] === undefined) delete value[key]; + for (const key of Object.keys(result)) { + if (result[key] === undefined) delete result[key]; } - return value as StripUndefined<T>; + return result as StripUndefined<T>; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridge/src/provisioning.ts` around lines 233 - 238, stripUndefined currently mutates its input by deleting keys with undefined values; change it to return a new object instead of mutating the argument (or explicitly document the mutating behavior) — implement by creating a new result object, iterate Object.keys(value) and copy only keys whose value !== undefined into result, then return result cast to StripUndefined<T>; ensure the function signature stays stripUndefined<T extends Record<string, unknown>>(value: T): StripUndefined<T> and update any callers/tests if they relied on mutation.
223-225: 💤 Low value
randomIDuses non-cryptographic randomness for login IDs.The function generates login IDs using
Date.now()andMath.random(), which is not cryptographically secure. However, since these IDs are ephemeral (stored only in memory for the duration of a login flow) and not used for security-sensitive operations, this is likely acceptable.If login IDs need to be unpredictable for security reasons, consider using
crypto.randomBytes()instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridge/src/provisioning.ts` around lines 223 - 225, The current randomID function (randomID) builds login IDs from Date.now() and Math.random(), which is non-cryptographic; if you need unpredictable IDs for security-sensitive flows replace the implementation to use Node's crypto (e.g., crypto.randomBytes) to generate a secure random hex string and combine it with the prefix (instead of Math.random()/Date.now()); if IDs are only ephemeral and not security-critical, leave as-is but add a comment in randomID explaining the choice and potential swap to crypto.randomBytes for stronger randomness.
83-91: ⚡ Quick winSilent fallback to first login may be unexpected.
When a
login_idquery parameter is provided but doesn't match any login, or when nologin_idis provided, the function silently falls back to the first available login. This could lead to operations being performed with an unintended login context, especially in multi-login scenarios.Consider returning an error when an explicit but invalid
login_idis provided.♻️ Suggested improvement
function provisioningLogin(runtime: ProvisioningRuntime, request: HTTPProxyRequest): UserLogin | null { const logins = runtime.listLogins(); const loginId = queryParam(request.query, "login_id"); if (loginId) { const matching = logins.find((login) => login.id === loginId); - if (matching) return matching; + return matching ?? null; } return logins[0] ?? null; }Then in callers, return 404 when login is null:
const login = provisioningLogin(runtime, request); - if (!login) return jsonHTTPResponse(404, matrixError("M_NOT_FOUND", "Login not found")); + if (!login) return jsonHTTPResponse(404, matrixError("M_NOT_FOUND", loginId ? "Login not found" : "No logins available"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridge/src/provisioning.ts` around lines 83 - 91, The function provisioningLogin currently falls back to the first login when a provided login_id doesn't match any login; change provisioningLogin (and its use of ProvisioningRuntime.listLogins, queryParam, HTTPProxyRequest, and UserLogin) so that if a login_id is present but no matching UserLogin is found it returns null immediately, while preserving the existing behavior of returning the first login only when no login_id was provided; update callers to treat a returned null as "not found" and return a 404 response.packages/pi/src/config.ts (1)
47-50: 💤 Low valueDefensive chmod after writeFile with mode.
The file is written with
mode: 0o600and then immediately followed bychmod(path, 0o600). While defensive, the second chmod is likely redundant since the mode option already sets the permissions atomically during creation.♻️ Optional simplification
export async function writeConfig(config: PicklePiConfig, path = defaultConfigPath(config.dataDir)): Promise<void> { await mkdir(dirname(path), { recursive: true }); await writeFile(path, `${JSON.stringify(config, null, 2)}\n`, { mode: 0o600 }); - await chmod(path, 0o600); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi/src/config.ts` around lines 47 - 50, In writeConfig, the file is created with writeFile(..., { mode: 0o600 }) which already sets permissions atomically, so remove the redundant chmod(path, 0o600) call to simplify the function; update the function (export async function writeConfig) to rely on the mode option and delete the extra chmod invocation to avoid unnecessary system calls.packages/pi/src/pi-beeper-stream.ts (1)
47-50: 💤 Low valueConsider preserving error objects for better debugging.
Line 49 converts errors to strings, losing the stack trace and other error properties. For better observability in production, consider passing the full error object to
publisher.error()if it accepts it, or at least including the stack trace in the message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi/src/pi-beeper-stream.ts` around lines 47 - 50, The error branch currently converts chunk.errorText to a string and loses stack/metadata; update the error handling in the block that checks if (chunk.type === "error") to pass the full Error object (or include error.stack) to publisher.error instead of only a string, e.g. use chunk.errorText when it's already an Error or construct an Error with message+stack and send that; ensure this.#closed remains set and that publisher.error is called with the richer error object so stack traces and properties are preserved.packages/pickle/native/internal/core/init.go (2)
313-337: ⚡ Quick winExtract the duplicated
DecryptErrorCallbackassignment.The
DecryptErrorCallbackclosure is duplicated verbatim here and at lines 295-311. Consider extracting it to a helper method to reduce duplication and ensure consistent behavior.♻️ Proposed refactor
+func (c *Core) makeDecryptErrorCallback(ctx context.Context) func(*event.Event, error) { + return func(evt *event.Event, err error) { + c.rememberPendingDecryption(ctx, evt) + if c.retryPendingDecryptionEvent(ctx, evt) { + return + } + eventData := OutboundEvent{} + if evt != nil { + eventData["eventId"] = evt.ID.String() + eventData["roomId"] = evt.RoomID.String() + eventData["sender"] = evt.Sender.String() + } + c.emit(OutboundEvent{ + "type": "decryption_error", + "error": err.Error(), + "event": eventData, + }) + } +}Then use it in both places:
helper.DecryptErrorCallback = c.makeDecryptErrorCallback(ctx)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pickle/native/internal/core/init.go` around lines 313 - 337, The DecryptErrorCallback closure is duplicated; extract it into a method on the same receiver (e.g., c.makeDecryptErrorCallback(ctx) or c.decryptErrorCallback(ctx)) and return a func(*event.Event, error) that calls c.rememberPendingDecryption, c.retryPendingDecryptionEvent, builds the OutboundEvent with eventId/roomId/sender when evt != nil, and calls c.emit with type "decryption_error" and the error string; then replace both inline assignments to helper.DecryptErrorCallback with helper.DecryptErrorCallback = c.makeDecryptErrorCallback(ctx) to remove duplication and ensure consistent behavior across uses (keep existing symbols: DecryptErrorCallback, rememberPendingDecryption, retryPendingDecryptionEvent, emit, OutboundEvent).
379-381: 💤 Low valueString-based error detection is fragile.
isMissingServerKeysErrorrelies on matching error message text, which may change in upstreammautrixreleases. Consider adding a comment documenting this fragility, or check ifmautrixexposes a typed error for this condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pickle/native/internal/core/init.go` around lines 379 - 381, The helper isMissingServerKeysError currently does fragile string matching of err.Error() for "keys seem to have disappeared from the server"; update it to use a typed error check (errors.Is / errors.As) against any specific mautrix error type if available (e.g., a hypothetical mautrix.ErrMissingServerKeys) and fall back to the string match only as a last resort, or if no typed error exists add a clear comment above isMissingServerKeysError explaining the fragility and linking to the upstream mautrix behavior; reference the function name isMissingServerKeysError and the upstream package mautrix when making the change.packages/pi/src/appservice.ts (1)
176-178: 💤 Low valueDefault-allow behavior when
allowedUserIdsis empty.When
allowedUserIdsis undefined or empty, all senders are allowed. If this is intentional for development/testing, consider documenting it. For production, you may want to require explicit configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi/src/appservice.ts` around lines 176 - 178, The current isAllowedSender function implicitly allows all senders when config.allowedUserIds is undefined or an empty array; change it so the default is deny (require explicit configuration) by returning true only when allowedUserIds is a non-empty array and includes the sender (i.e., ensure config.allowedUserIds?.length is truthy before includes). Update any related comments or docs to state that allowedUserIds must be configured for access in production and keep the function name isAllowedSender and the config.allowedUserIds reference unchanged so reviewers can locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pi/src/appservice.ts`:
- Around line 74-76: The call to headless.session.prompt(event.text, { source:
"matrix" }) can throw and should be guarded; wrap that await in a try/catch
inside the code path that calls this.#ensureHeadlessSession(binding.id) and, on
error, log the full error with context (include binding.id and event.text) using
the service logger (e.g., this.logger.error) and send a user-facing failure
notice via the headless session or other user-notification method (e.g.,
headless.session.sendMessage/notify) so the event handler doesn't crash and the
user is informed.
In `@packages/pi/src/beeper-stream.ts`:
- Around line 54-79: The start() method always creates a new placeholder event
because it only reuses this.#targetEventId when this.#descriptor is already set;
fix by short-circuiting when a constructor-provided targetEventId exists: in
start(), if this.#targetEventId is present but this.#descriptor is missing,
fetch the existing message by that event id (e.g. via this.#client.messages.get
/ appropriate API), extract the stream descriptor from
message.content["com.beeper.stream"], assign it to this.#descriptor, and then
return the existing descriptor/eventId/turnId instead of creating a new stream
or sending a placeholder message; ensure you still call any required
registration/publish steps only when creating a fresh stream.
- Around line 82-101: The code currently mutates this.#accumulator by calling
applyFinalMessagePart(this.#accumulator, part) before the network write in
publish; move that mutation until after this.#client.beeper.streams.publish
resolves successfully so failed publishes don't double-append locally and
finalize() won't reference content that never streamed. Concretely, in publish()
(referencing applyFinalMessagePart, this.#client.beeper.streams.publish,
this.#seq, this.#accumulator, and finalize()) call publish first with the part
(and only increment this.#seq on success), then applyFinalMessagePart to
this.#accumulator and update seq/state after the publish promise resolves.
- Around line 316-319: The errorText function can return undefined because
JSON.stringify(undefined) yields undefined; update errorText to always return a
string by guarding that fallback—use a deterministic string conversion (e.g.,
String(error) or JSON.stringify(error) ?? "<no error>") so errorText(error:
unknown): string never returns undefined; update the errorText implementation to
coerce or provide a default when JSON.stringify would be undefined.
In `@packages/pi/src/cli.ts`:
- Around line 7-34: The CLI's main function prints the usage for unknown
commands but doesn't signal failure; update the final/default branch in main to
set process.exitCode = 1 (or call process.exit(1)) when the provided command is
not recognized, keeping the existing console.log("Usage: pickle-pi-agent
<init|register [path]|start|status>") message; ensure this change only affects
the unrecognized-command path in main so the existing successful branches (init,
register, status, start) still return normally.
In `@packages/pi/src/media-store.ts`:
- Around line 127-133: The assertInside function can falsely accept targets that
merely share a prefix with root (e.g., "/mediax")—fix it by resolving both paths
and checking proper prefix containment: call path.resolve on both inputs (use
the existing assertInside function name and its root and target parameters),
then allow the target only if resolvedTarget === resolvedRoot or
resolvedTarget.startsWith(resolvedRoot + path.sep); otherwise throw the same
Error("Resolved media path escapes media root"). This ensures correct directory
containment regardless of string prefixes or separator differences.
In `@packages/pi/src/pi-runtime.ts`:
- Line 30: The code sets a global env var via
process.env.PICKLE_PI_OWNED_SESSION = "1", which can leak across sessions;
change this to a scoped approach by removing the direct global mutation and
instead pass this flag through your session creation/config API (e.g., add a
parameter to the constructor/createSession function used in pi-runtime.ts or a
SessionOptions type) or set/unset it only around the exact operation that needs
it (wrap the operation in set, execute, then restore previous value). Update
references that read PICKLE_PI_OWNED_SESSION to read the new scoped option
(e.g., from the SessionOptions or the createSession call) so no global
process.env mutation is required.
In `@packages/pi/src/stream-map.ts`:
- Around line 36-45: The current delta handling always emits a "text-start" /
"reasoning-start" chunk even when state.textPartId or state.reasoningPartId
already exists; change the logic in the stream-map delta branch so you only
assign state.textPartId (or state.reasoningPartId) and push the corresponding "{
id, type: ...-start }" chunk when the id is not yet set (e.g., if
(!state.textPartId) { state.textPartId = ...; chunks.push({ id:
state.textPartId, type: "text-start" }); }), then always push the delta chunk
(chunks.push({ delta: delta.value, id: state.textPartId, type: "text-delta" }));
apply the same conditional pattern for reasoningPartId/ "reasoning-start" and
"reasoning-delta" to avoid emitting duplicate start chunks.
---
Outside diff comments:
In `@packages/bridge/src/bridge.ts`:
- Around line 200-213: The code currently sets both `#ownerUserId` and `#ownUserId`
from whoami.userId which makes the owner become the appservice bot; change
initialization so `#ownerUserId` is set to this.#beeperOptions?.ownerUserId ??
whoami.userId (preserving an explicit owner if provided) and keep `#ownUserId`
reserved for the bot identity (set from whoami.userId initially and later
overwritten by appserviceBotUserId(this.#appserviceOptions) in the appservice
init block); update the assignment near the this.#matrixClient.boot() call in
bridge.ts to use these distinct sources (`#ownerUserId` from beeperOptions
fallback, `#ownUserId` from whoami.userId).
- Around line 111-131: createBeeperBridgeWithClient computes resolved Matrix
defaults into the local matrix object but then passes the original client to
RuntimeBridge, so those defaults are never applied; update
createBeeperBridgeWithClient so that after computing matrix it ensures the
MatrixClient used by RuntimeBridge matches those resolved values—either by
creating a new MatrixClient from the resolved matrix (using your existing
client-creation helper) or by applying the resolved token/deviceId/homeserver
properties to the supplied client—then pass that updated/constructed client
along with createBeeperRuntimeOptions(options, appservice, matrix) into new
RuntimeBridge; reference createBeeperBridgeWithClient, matrix, RuntimeBridge,
and createBeeperRuntimeOptions to locate where to make the change.
---
Nitpick comments:
In `@packages/bridge/src/provisioning.ts`:
- Around line 233-238: stripUndefined currently mutates its input by deleting
keys with undefined values; change it to return a new object instead of mutating
the argument (or explicitly document the mutating behavior) — implement by
creating a new result object, iterate Object.keys(value) and copy only keys
whose value !== undefined into result, then return result cast to
StripUndefined<T>; ensure the function signature stays stripUndefined<T extends
Record<string, unknown>>(value: T): StripUndefined<T> and update any
callers/tests if they relied on mutation.
- Around line 223-225: The current randomID function (randomID) builds login IDs
from Date.now() and Math.random(), which is non-cryptographic; if you need
unpredictable IDs for security-sensitive flows replace the implementation to use
Node's crypto (e.g., crypto.randomBytes) to generate a secure random hex string
and combine it with the prefix (instead of Math.random()/Date.now()); if IDs are
only ephemeral and not security-critical, leave as-is but add a comment in
randomID explaining the choice and potential swap to crypto.randomBytes for
stronger randomness.
- Around line 83-91: The function provisioningLogin currently falls back to the
first login when a provided login_id doesn't match any login; change
provisioningLogin (and its use of ProvisioningRuntime.listLogins, queryParam,
HTTPProxyRequest, and UserLogin) so that if a login_id is present but no
matching UserLogin is found it returns null immediately, while preserving the
existing behavior of returning the first login only when no login_id was
provided; update callers to treat a returned null as "not found" and return a
404 response.
In `@packages/pi/src/appservice.ts`:
- Around line 176-178: The current isAllowedSender function implicitly allows
all senders when config.allowedUserIds is undefined or an empty array; change it
so the default is deny (require explicit configuration) by returning true only
when allowedUserIds is a non-empty array and includes the sender (i.e., ensure
config.allowedUserIds?.length is truthy before includes). Update any related
comments or docs to state that allowedUserIds must be configured for access in
production and keep the function name isAllowedSender and the
config.allowedUserIds reference unchanged so reviewers can locate the change.
In `@packages/pi/src/config.ts`:
- Around line 47-50: In writeConfig, the file is created with writeFile(..., {
mode: 0o600 }) which already sets permissions atomically, so remove the
redundant chmod(path, 0o600) call to simplify the function; update the function
(export async function writeConfig) to rely on the mode option and delete the
extra chmod invocation to avoid unnecessary system calls.
In `@packages/pi/src/pi-beeper-stream.ts`:
- Around line 47-50: The error branch currently converts chunk.errorText to a
string and loses stack/metadata; update the error handling in the block that
checks if (chunk.type === "error") to pass the full Error object (or include
error.stack) to publisher.error instead of only a string, e.g. use
chunk.errorText when it's already an Error or construct an Error with
message+stack and send that; ensure this.#closed remains set and that
publisher.error is called with the richer error object so stack traces and
properties are preserved.
In `@packages/pi/src/pi-runtime.ts`:
- Around line 60-68: The code uses new Function("specifier", "return
import(specifier)") to perform a dynamic import (dynamicImport) which bypasses
bundler/static analysis; replace that construct with the standard dynamic import
expression import("@earendil-works/pi-coding-agent") (await it and cast to
Awaited<ReturnType<typeof loadPiCodingAgent>> as before) inside the same
try/catch so bundlers can detect the optional dependency while preserving the
existing error handling that throws a new Error with the original error as
cause.
In `@packages/pi/src/registry.ts`:
- Around line 14-108: Summarize concurrency guarantees for PicklePiRegistry: add
documentation (class JSDoc or README) near the PicklePiRegistry declaration
explaining that registry mutators (upsertBinding, updateBinding, markDedupe,
upsertProjectSpace, setActiveLeaf) operate on in-memory state and that load/save
use a temporary-file + rename for atomic writes but do not provide multi-process
synchronization; state the expected usage is single-instance/single-threaded,
recommend using external locking (e.g., flock/advisory file locks or a
coordinating service) if multiple processes may access the same path, or accept
last-writer-wins eventual consistency, and point readers at the load() and
save() behavior for how to handle concurrent writers.
In `@packages/pickle/native/internal/core/init.go`:
- Around line 313-337: The DecryptErrorCallback closure is duplicated; extract
it into a method on the same receiver (e.g., c.makeDecryptErrorCallback(ctx) or
c.decryptErrorCallback(ctx)) and return a func(*event.Event, error) that calls
c.rememberPendingDecryption, c.retryPendingDecryptionEvent, builds the
OutboundEvent with eventId/roomId/sender when evt != nil, and calls c.emit with
type "decryption_error" and the error string; then replace both inline
assignments to helper.DecryptErrorCallback with helper.DecryptErrorCallback =
c.makeDecryptErrorCallback(ctx) to remove duplication and ensure consistent
behavior across uses (keep existing symbols: DecryptErrorCallback,
rememberPendingDecryption, retryPendingDecryptionEvent, emit, OutboundEvent).
- Around line 379-381: The helper isMissingServerKeysError currently does
fragile string matching of err.Error() for "keys seem to have disappeared from
the server"; update it to use a typed error check (errors.Is / errors.As)
against any specific mautrix error type if available (e.g., a hypothetical
mautrix.ErrMissingServerKeys) and fall back to the string match only as a last
resort, or if no typed error exists add a clear comment above
isMissingServerKeysError explaining the fragility and linking to the upstream
mautrix behavior; reference the function name isMissingServerKeysError and the
upstream package mautrix when making the change.
🪄 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: 5e568cc5-e002-492e-bd60-9b8d37c28182
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
packages/bridge/src/bridge.tspackages/bridge/src/index.tspackages/bridge/src/node.tspackages/bridge/src/provisioning.test.tspackages/bridge/src/provisioning.tspackages/bridge/src/store.tspackages/bridge/src/types.tspackages/pi/@beeper-pickle-pi.TODO.mdpackages/pi/README.mdpackages/pi/package.jsonpackages/pi/src/approval.test.tspackages/pi/src/approval.tspackages/pi/src/appservice.tspackages/pi/src/beeper-stream.test.tspackages/pi/src/beeper-stream.tspackages/pi/src/cli.tspackages/pi/src/config.tspackages/pi/src/index.tspackages/pi/src/matrix.tspackages/pi/src/media-store.test.tspackages/pi/src/media-store.tspackages/pi/src/pi-beeper-stream.test.tspackages/pi/src/pi-beeper-stream.tspackages/pi/src/pi-event-map.test.tspackages/pi/src/pi-event-map.tspackages/pi/src/pi-runtime.tspackages/pi/src/queue.test.tspackages/pi/src/queue.tspackages/pi/src/registration.tspackages/pi/src/registry.test.tspackages/pi/src/registry.tspackages/pi/src/rooms.test.tspackages/pi/src/rooms.tspackages/pi/src/spaces.test.tspackages/pi/src/spaces.tspackages/pi/src/stream-map.test.tspackages/pi/src/stream-map.tspackages/pi/src/types.tspackages/pi/tsconfig.jsonpackages/pi/tsdown.config.tspackages/pickle/native/internal/core/appservice.gopackages/pickle/native/internal/core/init.gopackages/pickle/native/internal/core/persistent_crypto_methods.gopackages/pickle/src/client.tspackages/pickle/src/generated-runtime-types.tspackages/pickle/src/streams/beeper.tspackages/pickle/src/types.tspnpm-workspace.yamltsconfig.base.json
📜 Review details
🔇 Additional comments (58)
pnpm-workspace.yaml (1)
7-7: LGTM!Adding
packages/pito the workspace is correct for the new Pi bridge package.packages/pickle/src/generated-runtime-types.ts (2)
78-78: LGTM!The
initialStateproperty is correctly typed as optionalMatrixRoomStateInput[], matching the Go struct definition.
137-137: LGTM!The
appserviceproperty addition enables appservice-first initialization flow correctly.packages/pickle/native/internal/core/appservice.go (2)
95-95: LGTM!The
InitialStatefield is correctly defined with appropriate JSON serialization tag.
301-308: LGTM!The implementation correctly handles initial state events:
- Local copy of
stateKeyavoids pointer aliasing issues when taking its address- User-provided state events are appended before bridge-specific events, which is the correct ordering
packages/pickle/src/types.ts (2)
1-2: LGTM!Import correctly brings in the generated type needed for the appservice option.
14-19: LGTM!The new
appserviceanddeviceIdoptions correctly extendMatrixClientOptionsto support appservice-based initialization.packages/pickle/src/client.ts (1)
332-333: LGTM!The device ID fallback logic correctly prioritizes the explicit
deviceIdoption for appservice mode while preserving backward compatibility with account-based device IDs.packages/bridge/src/store.ts (1)
153-160: LGTM!The device ID generation and persistence logic is well implemented:
- Correctly uses
Pick<MatrixStore, "get" | "set">for minimal interface requirements- Sanitizes bridge name appropriately for ID inclusion
- Returns consistent ID across restarts via storage
packages/pi/@beeper-pickle-pi.TODO.md (1)
1-856: LGTM!This is a comprehensive specification document that clearly outlines the architecture, data models, and implementation phases for the Pi bridge. The references to local paths are understandable for internal development notes.
packages/pickle/src/streams/beeper.ts (6)
41-54: LGTM!The concurrent publish mechanism is correctly implemented:
seq++increments synchronously, preserving ordering guarantees- The
whileloop inwaitForPublishesproperly handles promises added during iteration- Error swallowing with
console.warnprovides resilience without silent failures- Promise cleanup in
finallyensures no memory leaks
318-319: LGTM!Adding
startedAtMstiming field enables tool execution timing tracking in the UI.
345-346: LGTM!Adding
completedAtMstiming field complements the start timing for complete execution duration tracking.
366-407: LGTM!The
normalizeRichStreamChunkfunction handles the various event format variations from Pi (snake_case/camelCase, different wrapper types) with appropriate defensive coding.
460-528: LGTM!The
uiChunkFrom*helper functions correctly map Pi-specific tool events to the standard Beeper stream chunk format, with proper handling of tool call IDs, names, inputs, and outputs.
680-689: LGTM!The refactored
isStreamPartand newisNativeStreamPartRecordfunctions cleanly separate the type guard logic while maintaining correct behavior.packages/pi/src/pi-runtime.ts (1)
42-44: Fire-and-forget promise pattern is appropriate here.The
void Promise.resolve(options.onEvent(event))pattern correctly handles both synchronous and asynchronousonEventimplementations without blocking the Pi session event loop. The intentional void discard prevents unhandled rejection warnings.packages/bridge/src/types.ts (2)
86-92: LGTM! HTTPProxyHandlingBridgeConnector interface is well-designed.The interface properly extends
BridgeConnectorand defines a clear contract for HTTP proxy handling with appropriate request/response types. The nullable return type allows connectors to decline handling specific requests.
527-527: LGTM! Type extensions align with appservice and portal initialization features.The additions of
getOwnUserId(), appservice/deviceId toBridgeMatrixConfig, andinitialStateto portal room options are consistent with the PR's appservice-first initialization objectives.Also applies to: 576-576, 649-649
packages/pi/src/approval.ts (2)
55-74: Decision normalization logic is robust and handles edge cases well.The function correctly:
- Validates required fields (
type,approvedas boolean)- Normalizes hyphenated decision strings (allow-once → allow_once)
- Computes
approvedAlwaysfrom explicit decision or flag- Forces
decisionto "deny" whenapprovedis false regardless of other fields (line 67)- Conditionally copies optional identifiers
The fallback logic on line 63 provides sensible defaults when decision is missing.
106-123: LGTM! Approval decision normalization handles format variations.The function correctly normalizes hyphenated variants (allow-once, allow-session, allow-room) to underscore format, ensuring consistent parsing regardless of input format. The explicit allow_always case at line 109 ensures both formats are accepted.
packages/pi/src/approval.test.ts (1)
1-128: LGTM! Comprehensive test coverage for approval parsing.The test suite thoroughly validates:
- All approval reaction key constants
- Matrix reaction content structure parsing
- Tool approval response chunk normalization (including decision variants)
- Nested delta structure traversal
- Malformed input rejection
The tests cover happy paths, edge cases (allow-room, deny with approvedAlways), and error cases.
packages/pi/src/types.ts (1)
1-115: LGTM! Well-structured type definitions for the PicklePi package.The type definitions provide a clear and comprehensive model:
- Binding metadata with owner/mode/kind discriminators
- Runtime execution state (ActiveRun, MatrixInboundTurn with priority levels)
- Persistence layer types (registry data with schema versioning)
- Configuration and appservice registration structures
The use of optional fields, union types, and schema versioning demonstrates good API design.
packages/pi/src/registry.ts (2)
35-40: LGTM! Atomic file write pattern prevents corruption.The save implementation correctly:
- Creates parent directory with
mkdir -psemantics- Writes to a PID-suffixed temporary file
- Sets restrictive file permissions (0o600, owner-only)
- Uses atomic rename to prevent partial writes
This pattern protects against crashes during write and prevents other processes from reading partial state.
27-32: LGTM! ENOENT handling provides safe initialization.Tolerating
ENOENTduring load allows the registry to initialize gracefully on first run, while re-throwing other errors (permission denied, disk full, etc.) ensures real problems are surfaced.packages/pi/src/registry.test.ts (1)
1-83: LGTM! Test coverage validates persistence and indexing behavior.The tests verify:
- Round-trip persistence of bindings, project spaces, and dedupe state
- Lookup operations by room, id, cwd, and parent relationships
- Child/subagent filtering and relationship tracking
- Active leaf updates with timestamp tracking
The use of temporary directories ensures test isolation.
packages/pi/src/queue.ts (3)
6-6: LGTM! Priority ordering and FIFO semantics are correct.The
matrixInboundTurnPriorityOrderconstant defines the dispatch order (control → priority → default), and the#next()implementation correctly iterates in that order, selecting the first element from the first non-empty queue. This ensures:
- Higher priority turns are dispatched first
- FIFO ordering is preserved within each priority level (array shift semantics)
Also applies to: 108-114
82-94: LGTM! Text update preserves queue position and priority.The
updateTextByEventIdimplementation correctly:
- Searches all priority queues in order
- Updates the turn in-place without changing its position
- Returns the updated turn or undefined if not found
This allows queued messages to be modified (e.g., edit event) without affecting dispatch order.
65-72: Infinite loop implementation is correct.The
for(;;)loop with explicit return inside is an appropriate pattern for draining the queue untildequeuereturns undefined (when no more dispatchable turns exist).packages/pi/src/queue.test.ts (1)
1-99: LGTM!The test suite provides comprehensive coverage of queue behavior including priority ordering, cancellation by ID/event ID, text updates, and dispatch gating. The test structure is clear and the helper function provides good fixture support.
packages/pi/src/config.ts (1)
53-55: LGTM!Cryptographically secure token generation using
randomByteswith appropriate default length.packages/pi/src/matrix.ts (1)
6-17: LGTM!Clear validation of required configuration fields with helpful error messages, and clean conditional parameter passing for optional encryption keys.
packages/pi/src/spaces.ts (4)
4-11: LGTM!Stable project key derivation using base64url encoding and clean path segment extraction for human-readable space names.
13-24: LGTM!Correct space creation using appservice room creation with appropriate parameters (non-direct, private, with invited users).
26-39: LGTM!Correct Matrix space attachment implementation with bidirectional state events and federation routing hints.
41-43: ⚡ Quick winVerify domain parameter in production deployments.
The function defaults
domainto"localhost", which is appropriate for local development but could cause issues in production if not explicitly provided. Ensure callers in production environments pass the correct homeserver domain.packages/pi/src/spaces.test.ts (1)
1-90: LGTM!Comprehensive test coverage of space utilities with proper mocking and assertion patterns.
packages/pi/src/registration.ts (2)
32-43: LGTM!Secure file writing with restrictive permissions and proper regex escaping for namespace patterns.
22-22: 💤 Low valueVerify hardcoded "pickle-pi_" prefix in aliases namespace.
The aliases namespace regex uses a hardcoded
"pickle-pi_"prefix instead of deriving it fromconfig.appserviceIdor another configurable field. This prevents different pickle-pi instances from claiming separate alias namespaces and could cause conflicts if multiple instances operate under the same homeserver.packages/bridge/src/provisioning.test.ts (1)
1-70: LGTM with note on test coverage.The tests verify core provisioning endpoints (capabilities, logins, create DM). Consider adding tests for login flow steps (
start,step) and error cases in future iterations.packages/pi/src/stream-map.ts (1)
135-138: ⚡ Quick win
errorText()can returnundefined, violating the declaredstringreturn type.
JSON.stringify(undefined)returns the primitiveundefinedvalue, not a string. This can cause type safety violations when the result is used intool-output-error.Suggested fix
function errorText(error: unknown): string { if (error instanceof Error) return error.message; if (typeof error === "string") return error; - return JSON.stringify(error); + return JSON.stringify(error) ?? String(error); }packages/pi/src/beeper-stream.test.ts (1)
1-197: LGTM! Comprehensive test coverage.The test suite thoroughly validates the Beeper stream publisher lifecycle, including:
- Stream creation and registration
- Monotonic sequence numbering for deltas
- Proper finalization with message editing
- Terminal error/abort handling without editing
The test utilities are well-designed and the assertions are precise.
packages/pi/src/pi-beeper-stream.test.ts (1)
1-97: LGTM! Well-structured integration tests.The test suite effectively validates:
- End-to-end Pi event mapping and publishing
- Correct sequencing of assistant message parts
- Tool execution callback handling with proper metadata
- Final message editing behavior
tsconfig.base.json (1)
16-16: LGTM! Path alias correctly configured.The
@beeper/pickle-pialias is properly added and follows the existing pattern used by other packages in the monorepo.packages/pi/tsconfig.json (1)
1-9: LGTM! Standard TypeScript configuration.The config properly extends the base configuration and excludes test files from the build output, which is the expected pattern.
packages/pi/tsdown.config.ts (1)
1-9: LGTM! Clean build configuration.The tsdown config properly defines all entry points with ESM output and TypeScript declarations enabled.
packages/pi/src/pi-beeper-stream.ts (1)
35-38: ⚡ Quick winCheck if
publisher.start()can be called multiple times safely.Both the public
start()method (line 21) and the chunk handler (line 37) invokepublisher.start(). If a client callsbridge.start()followed by astartchunk event, the publisher will be initialized twice. Verify that the underlying publisher implementation is idempotent or add guard logic to prevent duplicate initialization.packages/pi/README.md (1)
1-37: LGTM!The documentation is clear, well-structured, and accurately describes the package's purpose, CLI commands, configuration, and current status. The environment variable naming convention is consistent.
packages/pi/src/media-store.test.ts (1)
1-52: LGTM!The test properly covers the happy path for media storage, including metadata persistence, file content verification, and helper function behavior. The temp directory cleanup in
afterEachensures test isolation.packages/pi/src/rooms.test.ts (1)
1-114: LGTM!The test suite thoroughly covers the room helper functions with deterministic timestamp handling using fake timers. The
try/finallypattern ensures timer cleanup even on test failures.packages/pi/package.json (1)
1-72: LGTM!The package manifest is well-structured with proper ESM configuration, appropriate workspace dependencies, and correctly configured subpath exports for the agent and stream-map modules.
packages/pickle/native/internal/core/init.go (1)
117-165: LGTM oninitClientimplementation.The function cleanly handles both standard access-token and appservice login flows, with proper flow validation before attempting appservice authentication.
packages/pi/src/appservice.ts (1)
39-65: LGTM on the agent lifecycle and event handling.The deduplication pattern using the registry is clean, and the subscription setup properly filters for relevant event kinds.
packages/pi/src/rooms.ts (1)
1-100: LGTM!The room helper functions are clean and well-tested. The binding ID derivation using base64url encoding ensures URL-safe identifiers, and the metadata constructors properly handle optional fields.
packages/pi/src/media-store.ts (2)
121-125: Good sanitization insafeMediaId.The ID sanitization correctly prevents directory traversal by replacing path-unsafe characters and rejecting
.and... The 180-character limit is a reasonable safeguard for filesystem compatibility.
28-50: LGTM onsaveMediaBuffer.Proper use of
0o600permissions for sensitive media files and atomic sidecar metadata storage.packages/pi/src/index.ts (2)
1-26: Clean and well-structured barrel exports.The public API surface is clear, and value/type exports are separated appropriately.
27-27: Ensure TypeScript 5.0+ is configured as the minimum supported version (Line 27).
export type *requires TypeScript 5.0 or later (introduced in PR#52217). Verify that the project'spackage.jsonandtsconfig.jsonenforce this minimum, or re-export types explicitly for broader compatibility.
| const headless = await this.#ensureHeadlessSession(binding.id); | ||
| await headless.session.prompt(event.text, { source: "matrix" }); | ||
| } |
There was a problem hiding this comment.
Add error handling for prompt() failures.
If headless.session.prompt() throws, the error will propagate and potentially crash the event handler. Consider wrapping in try/catch and logging/notifying the user.
🛡️ Proposed fix
const binding = this.registry.getBindingByRoom(event.roomId);
if (!binding) return;
const headless = await this.#ensureHeadlessSession(binding.id);
- await headless.session.prompt(event.text, { source: "matrix" });
+ try {
+ await headless.session.prompt(event.text, { source: "matrix" });
+ } catch (err) {
+ console.error("Failed to prompt Pi session:", err);
+ await this.#client?.messages.send({
+ messageType: "m.notice",
+ roomId: event.roomId,
+ text: `Pi session error: ${err instanceof Error ? err.message : "unknown error"}`,
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const headless = await this.#ensureHeadlessSession(binding.id); | |
| await headless.session.prompt(event.text, { source: "matrix" }); | |
| } | |
| const headless = await this.#ensureHeadlessSession(binding.id); | |
| try { | |
| await headless.session.prompt(event.text, { source: "matrix" }); | |
| } catch (err) { | |
| console.error("Failed to prompt Pi session:", err); | |
| await this.#client?.messages.send({ | |
| messageType: "m.notice", | |
| roomId: event.roomId, | |
| text: `Pi session error: ${err instanceof Error ? err.message : "unknown error"}`, | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/appservice.ts` around lines 74 - 76, The call to
headless.session.prompt(event.text, { source: "matrix" }) can throw and should
be guarded; wrap that await in a try/catch inside the code path that calls
this.#ensureHeadlessSession(binding.id) and, on error, log the full error with
context (include binding.id and event.text) using the service logger (e.g.,
this.logger.error) and send a user-facing failure notice via the headless
session or other user-notification method (e.g.,
headless.session.sendMessage/notify) so the event handler doesn't crash and the
user is informed.
| async start(): Promise<BeeperStreamStartResult> { | ||
| if (this.#targetEventId && this.#descriptor) { | ||
| return { descriptor: this.#descriptor, eventId: this.#targetEventId, turnId: this.turnId }; | ||
| } | ||
| const stream = await this.#client.beeper.streams.create({ roomId: this.roomId, streamType: "com.beeper.llm" }); | ||
| this.#descriptor = stream.descriptor; | ||
| const target = await this.#client.messages.send({ | ||
| content: { | ||
| body: "...", | ||
| "com.beeper.ai": { id: this.turnId, metadata: { turn_id: this.turnId }, parts: [], role: "assistant" }, | ||
| "com.beeper.stream": stream.descriptor, | ||
| msgtype: "m.text", | ||
| }, | ||
| messageType: "m.text", | ||
| roomId: this.roomId, | ||
| text: "...", | ||
| ...(this.#threadRoot ? { threadRoot: this.#threadRoot } : {}), | ||
| }); | ||
| this.#targetEventId = target.eventId; | ||
| await this.#client.beeper.streams.register({ | ||
| descriptor: stream.descriptor, | ||
| eventId: target.eventId, | ||
| roomId: this.roomId, | ||
| }); | ||
| await this.publish({ messageId: this.turnId, messageMetadata: { turn_id: this.turnId }, type: "start" }); | ||
| return { descriptor: stream.descriptor, eventId: target.eventId, turnId: this.turnId }; |
There was a problem hiding this comment.
targetEventId is never reused on the first start().
The constructor accepts targetEventId, but start() only takes the reuse path when #descriptor is also already populated. There is no way to seed #descriptor from options, so the first call always creates a new placeholder event and overwrites the configured target.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/beeper-stream.ts` around lines 54 - 79, The start() method
always creates a new placeholder event because it only reuses
this.#targetEventId when this.#descriptor is already set; fix by
short-circuiting when a constructor-provided targetEventId exists: in start(),
if this.#targetEventId is present but this.#descriptor is missing, fetch the
existing message by that event id (e.g. via this.#client.messages.get /
appropriate API), extract the stream descriptor from
message.content["com.beeper.stream"], assign it to this.#descriptor, and then
return the existing descriptor/eventId/turnId instead of creating a new stream
or sending a placeholder message; ensure you still call any required
registration/publish steps only when creating a fresh stream.
| async publish(part: BeeperUIMessageChunk): Promise<void> { | ||
| if (this.#finalized) throw new Error("Cannot publish to finalized Beeper stream"); | ||
| const { eventId: targetEventId } = await this.start(); | ||
| applyFinalMessagePart(this.#accumulator, part); | ||
| const descriptorType = descriptorTypeOf(this.#descriptor); | ||
| await this.#client.beeper.streams.publish({ | ||
| content: { | ||
| [`${descriptorType}.deltas`]: [ | ||
| { | ||
| "m.relates_to": { event_id: targetEventId, rel_type: "m.reference" }, | ||
| part, | ||
| seq: this.#seq++, | ||
| target_event: targetEventId, | ||
| turn_id: this.turnId, | ||
| }, | ||
| ], | ||
| }, | ||
| eventId: targetEventId, | ||
| roomId: this.roomId, | ||
| }); |
There was a problem hiding this comment.
Only mutate the accumulator after streams.publish() succeeds.
applyFinalMessagePart() runs before the network write. If publish fails and the caller retries, text/reasoning deltas are appended twice locally, and finalize() can edit the Matrix message with content that never actually streamed.
Suggested fix
async publish(part: BeeperUIMessageChunk): Promise<void> {
if (this.#finalized) throw new Error("Cannot publish to finalized Beeper stream");
const { eventId: targetEventId } = await this.start();
- applyFinalMessagePart(this.#accumulator, part);
const descriptorType = descriptorTypeOf(this.#descriptor);
await this.#client.beeper.streams.publish({
content: {
[`${descriptorType}.deltas`]: [
{
@@
eventId: targetEventId,
roomId: this.roomId,
});
+ applyFinalMessagePart(this.#accumulator, part);
}📝 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.
| async publish(part: BeeperUIMessageChunk): Promise<void> { | |
| if (this.#finalized) throw new Error("Cannot publish to finalized Beeper stream"); | |
| const { eventId: targetEventId } = await this.start(); | |
| applyFinalMessagePart(this.#accumulator, part); | |
| const descriptorType = descriptorTypeOf(this.#descriptor); | |
| await this.#client.beeper.streams.publish({ | |
| content: { | |
| [`${descriptorType}.deltas`]: [ | |
| { | |
| "m.relates_to": { event_id: targetEventId, rel_type: "m.reference" }, | |
| part, | |
| seq: this.#seq++, | |
| target_event: targetEventId, | |
| turn_id: this.turnId, | |
| }, | |
| ], | |
| }, | |
| eventId: targetEventId, | |
| roomId: this.roomId, | |
| }); | |
| async publish(part: BeeperUIMessageChunk): Promise<void> { | |
| if (this.#finalized) throw new Error("Cannot publish to finalized Beeper stream"); | |
| const { eventId: targetEventId } = await this.start(); | |
| const descriptorType = descriptorTypeOf(this.#descriptor); | |
| await this.#client.beeper.streams.publish({ | |
| content: { | |
| [`${descriptorType}.deltas`]: [ | |
| { | |
| "m.relates_to": { event_id: targetEventId, rel_type: "m.reference" }, | |
| part, | |
| seq: this.#seq++, | |
| target_event: targetEventId, | |
| turn_id: this.turnId, | |
| }, | |
| ], | |
| }, | |
| eventId: targetEventId, | |
| roomId: this.roomId, | |
| }); | |
| applyFinalMessagePart(this.#accumulator, part); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/beeper-stream.ts` around lines 82 - 101, The code currently
mutates this.#accumulator by calling applyFinalMessagePart(this.#accumulator,
part) before the network write in publish; move that mutation until after
this.#client.beeper.streams.publish resolves successfully so failed publishes
don't double-append locally and finalize() won't reference content that never
streamed. Concretely, in publish() (referencing applyFinalMessagePart,
this.#client.beeper.streams.publish, this.#seq, this.#accumulator, and
finalize()) call publish first with the part (and only increment this.#seq on
success), then applyFinalMessagePart to this.#accumulator and update seq/state
after the publish promise resolves.
| function errorText(error: unknown): string { | ||
| if (error instanceof Error) return error.message; | ||
| if (typeof error === "string") return error; | ||
| return JSON.stringify(error); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/pi/src/beeper-stream.ts | sed -n '310,325p'Repository: beeper/pickle
Length of output: 1902
🌐 Web query:
JSON.stringify undefined behavior JavaScript
💡 Result:
There is no “undefined behavior” in the sense of the ECMAScript/JSON specs leaving it unspecified. Instead, JSON.stringify has well-defined special-case behavior for values that have no JSON representation—most notably undefined (and also functions and symbols). 1) For object properties: keys with value undefined are omitted MDN states that undefined is “not valid JSON,” and if such values are encountered during conversion they are “omitted (when found in an object).” It also notes that JSON.stringify “can return undefined” for “pure” values like JSON.stringify(undefined). [1] So: - JSON.stringify({ a: undefined }) produces "{}" (property omitted). [1] 2) For array elements: undefined becomes null MDN similarly states that for array positions, undefined is “changed to null (when found in an array).” [1] So: - JSON.stringify([undefined]) produces "[null]". [1] 3) Top-level (passing a “pure” undefined/function/symbol): JSON.stringify can return undefined (not a string) MDN explicitly describes the return as “a JSON string representing the given value, or undefined,” and says JSON.stringify can return undefined for pure values such as JSON.stringify(undefined) or JSON.stringify( => {}). [1] ECMAScript spec likewise defines JSON.stringify(...) as returning a String in UTF-16 JSON format representing the value, or undefined. [2] 4) How to control dropping: use a replacer MDN documents that returning undefined from the replacer for a property causes that property to be excluded, and provides examples of a replacer that filters out values. [1] In practice, this is how you can avoid “missing fields”: map undefined to something JSON-valid (often null) in the replacer. [1] Bottom line: - “Undefined behavior” is a misconception here: behavior is specified. - undefined is handled deterministically: - object properties: omitted [1] - array elements: null [1] - top-level argument undefined: JSON.stringify returns undefined (not a string) [1][2] - to override, use a replacer to transform undefined into a JSON value [1]
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify
- 2: https://tc39.es/ecma262/2023/multipage/structured-data.html
errorText() can return undefined, violating the declared string return type.
The fallback JSON.stringify(error) can return undefined when passed undefined as the argument, per ECMAScript specification. This breaks the function contract and can leave terminal stream chunks without usable error text.
Suggested fix
function errorText(error: unknown): string {
if (error instanceof Error) return error.message;
if (typeof error === "string") return error;
- return JSON.stringify(error);
+ return JSON.stringify(error) ?? String(error);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/beeper-stream.ts` around lines 316 - 319, The errorText
function can return undefined because JSON.stringify(undefined) yields
undefined; update errorText to always return a string by guarding that
fallback—use a deterministic string conversion (e.g., String(error) or
JSON.stringify(error) ?? "<no error>") so errorText(error: unknown): string
never returns undefined; update the errorText implementation to coerce or
provide a default when JSON.stringify would be undefined.
| async function main(argv: string[]): Promise<void> { | ||
| const command = argv[2] ?? "help"; | ||
| if (command === "init") { | ||
| const config = createDefaultConfig(); | ||
| await writeConfig(config); | ||
| console.log(`Wrote ${defaultConfigPath(config.dataDir)}`); | ||
| return; | ||
| } | ||
| if (command === "register") { | ||
| const config = await readConfig().catch(() => createDefaultConfig()); | ||
| const out = resolve(argv[3] ?? config.dataDir, "registration.json"); | ||
| await writeRegistration(out, generateRegistration(config)); | ||
| console.log(`Wrote ${out}`); | ||
| return; | ||
| } | ||
| if (command === "status") { | ||
| const config = await readConfig().catch(() => createDefaultConfig()); | ||
| console.log(JSON.stringify({ appserviceId: config.appserviceId, dataDir: config.dataDir }, null, 2)); | ||
| return; | ||
| } | ||
| if (command === "start") { | ||
| const agent = await PicklePiAgent.create(); | ||
| await agent.start(); | ||
| console.log("pickle-pi-agent started"); | ||
| return; | ||
| } | ||
| console.log("Usage: pickle-pi-agent <init|register [path]|start|status>"); | ||
| } |
There was a problem hiding this comment.
Missing error exit code for unknown commands.
When an unknown command is provided, line 33 prints the usage message but the function returns normally without setting process.exitCode = 1. This means the CLI will exit with success (code 0) even when given invalid input, which could confuse automation or scripts that rely on exit codes.
🐛 Proposed fix
console.log("Usage: pickle-pi-agent <init|register [path]|start|status>");
+ process.exitCode = 1;
}📝 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.
| async function main(argv: string[]): Promise<void> { | |
| const command = argv[2] ?? "help"; | |
| if (command === "init") { | |
| const config = createDefaultConfig(); | |
| await writeConfig(config); | |
| console.log(`Wrote ${defaultConfigPath(config.dataDir)}`); | |
| return; | |
| } | |
| if (command === "register") { | |
| const config = await readConfig().catch(() => createDefaultConfig()); | |
| const out = resolve(argv[3] ?? config.dataDir, "registration.json"); | |
| await writeRegistration(out, generateRegistration(config)); | |
| console.log(`Wrote ${out}`); | |
| return; | |
| } | |
| if (command === "status") { | |
| const config = await readConfig().catch(() => createDefaultConfig()); | |
| console.log(JSON.stringify({ appserviceId: config.appserviceId, dataDir: config.dataDir }, null, 2)); | |
| return; | |
| } | |
| if (command === "start") { | |
| const agent = await PicklePiAgent.create(); | |
| await agent.start(); | |
| console.log("pickle-pi-agent started"); | |
| return; | |
| } | |
| console.log("Usage: pickle-pi-agent <init|register [path]|start|status>"); | |
| } | |
| async function main(argv: string[]): Promise<void> { | |
| const command = argv[2] ?? "help"; | |
| if (command === "init") { | |
| const config = createDefaultConfig(); | |
| await writeConfig(config); | |
| console.log(`Wrote ${defaultConfigPath(config.dataDir)}`); | |
| return; | |
| } | |
| if (command === "register") { | |
| const config = await readConfig().catch(() => createDefaultConfig()); | |
| const out = resolve(argv[3] ?? config.dataDir, "registration.json"); | |
| await writeRegistration(out, generateRegistration(config)); | |
| console.log(`Wrote ${out}`); | |
| return; | |
| } | |
| if (command === "status") { | |
| const config = await readConfig().catch(() => createDefaultConfig()); | |
| console.log(JSON.stringify({ appserviceId: config.appserviceId, dataDir: config.dataDir }, null, 2)); | |
| return; | |
| } | |
| if (command === "start") { | |
| const agent = await PicklePiAgent.create(); | |
| await agent.start(); | |
| console.log("pickle-pi-agent started"); | |
| return; | |
| } | |
| console.log("Usage: pickle-pi-agent <init|register [path]|start|status>"); | |
| process.exitCode = 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/cli.ts` around lines 7 - 34, The CLI's main function prints
the usage for unknown commands but doesn't signal failure; update the
final/default branch in main to set process.exitCode = 1 (or call
process.exit(1)) when the provided command is not recognized, keeping the
existing console.log("Usage: pickle-pi-agent <init|register
[path]|start|status>") message; ensure this change only affects the
unrecognized-command path in main so the existing successful branches (init,
register, status, start) still return normally.
| function assertInside(root: string, target: string): string { | ||
| const relative = target.slice(root.length); | ||
| if (target !== root && !relative.startsWith("/") && !relative.startsWith("\\")) { | ||
| throw new Error("Resolved media path escapes media root"); | ||
| } | ||
| return target; | ||
| } |
There was a problem hiding this comment.
Path containment check has edge case issues.
The assertInside function has a subtle bug: it checks if the slice after root.length starts with a separator, but this doesn't correctly handle paths that share a common prefix but aren't actually inside root (e.g., /media vs /mediax/file). The safeMediaId sanitization mitigates this in practice, but the defense-in-depth check should be corrected.
🛡️ Proposed fix using proper path prefix checking
function assertInside(root: string, target: string): string {
- const relative = target.slice(root.length);
- if (target !== root && !relative.startsWith("/") && !relative.startsWith("\\")) {
- throw new Error("Resolved media path escapes media root");
+ const normalizedRoot = root.endsWith("/") ? root : root + "/";
+ if (!target.startsWith(normalizedRoot) && target !== root) {
+ throw new Error("Resolved media path escapes media root");
}
return target;
}📝 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.
| function assertInside(root: string, target: string): string { | |
| const relative = target.slice(root.length); | |
| if (target !== root && !relative.startsWith("/") && !relative.startsWith("\\")) { | |
| throw new Error("Resolved media path escapes media root"); | |
| } | |
| return target; | |
| } | |
| function assertInside(root: string, target: string): string { | |
| const normalizedRoot = root.endsWith("/") ? root : root + "/"; | |
| if (!target.startsWith(normalizedRoot) && target !== root) { | |
| throw new Error("Resolved media path escapes media root"); | |
| } | |
| return target; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/media-store.ts` around lines 127 - 133, The assertInside
function can falsely accept targets that merely share a prefix with root (e.g.,
"/mediax")—fix it by resolving both paths and checking proper prefix
containment: call path.resolve on both inputs (use the existing assertInside
function name and its root and target parameters), then allow the target only if
resolvedTarget === resolvedRoot or resolvedTarget.startsWith(resolvedRoot +
path.sep); otherwise throw the same Error("Resolved media path escapes media
root"). This ensures correct directory containment regardless of string prefixes
or separator differences.
| await mkdir(dirname(options.binding.piSessionFile), { recursive: true }); | ||
| await mkdir(nativeSessionDir, { recursive: true }); | ||
|
|
||
| process.env.PICKLE_PI_OWNED_SESSION = "1"; |
There was a problem hiding this comment.
Global environment variable mutation may affect concurrent sessions.
Setting process.env.PICKLE_PI_OWNED_SESSION = "1" mutates the global process environment, which could affect all subsequent sessions created in the same process. If multiple headless sessions are created concurrently or sequentially, they will all share this environment variable.
Consider whether this is intentional, or if the environment variable should be scoped more narrowly (e.g., passed as a configuration option or set/unset around specific operations).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/pi-runtime.ts` at line 30, The code sets a global env var via
process.env.PICKLE_PI_OWNED_SESSION = "1", which can leak across sessions;
change this to a scoped approach by removing the direct global mutation and
instead pass this flag through your session creation/config API (e.g., add a
parameter to the constructor/createSession function used in pi-runtime.ts or a
SessionOptions type) or set/unset it only around the exact operation that needs
it (wrap the operation in set, execute, then restore previous value). Update
references that read PICKLE_PI_OWNED_SESSION to read the new scoped option
(e.g., from the SessionOptions or the createSession call) so no global
process.env mutation is required.
Extract open/close helpers for text and reasoning parts and use them across the stream mapping logic. mapPiMessageDelta and closeOpenMessageParts now delegate to openTextPart/openReasoningPart and closeTextPart/closeReasoningPart to avoid duplicate starts and centralize id management. pi-event-map was updated to use the new helpers for assistant message events. Narrowed BeeperStreamPublisherClient.messages to only include edit/send. Tests updated to reflect the new start/delta/end chunk behavior.
Set room name when events include a generated session title and tidy up notice wording. Add piEventSessionTitle and call it from the appservice to send an m.room.name state event. Adjust piEventNoticeText wording (remove redundant "Pi" prefixes, improve queue phrasing, sentence-case thinking levels, change "aborted"→"canceled", simplify retry labels) and add a sentenceCase helper. Update tests to cover session title extraction and the revised notice texts.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/pi/src/stream-map.test.ts (1)
53-59: ⚡ Quick winAvoid strict shape-coupling on optional
preliminaryin tool output test.Line 55 makes the test brittle by requiring an explicit
preliminary: undefinedkey withtoEqual. Prefer behavior-focused assertions so the test still passes if undefined fields are omitted during serialization/refactors.Proposed test refactor
- expect(mapPiToolOutput({ output: "ok", toolCallId: "call_1", toolName: "bash" })).toEqual({ - output: "ok", - preliminary: undefined, - toolCallId: "call_1", - toolName: "bash", - type: "tool-output-available", - }); + const toolOutput = mapPiToolOutput({ output: "ok", toolCallId: "call_1", toolName: "bash" }); + expect(toolOutput).toMatchObject({ + output: "ok", + toolCallId: "call_1", + toolName: "bash", + type: "tool-output-available", + }); + expect(toolOutput.preliminary).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pi/src/stream-map.test.ts` around lines 53 - 59, The test for mapPiToolOutput is brittle by asserting an exact shape that includes preliminary: undefined; update the assertion to be behavior-focused by using a partial match (e.g., expect(...).toMatchObject or expect(...).objectContaining) against the required keys { output, toolCallId, toolName, type } and, if you need to guarantee preliminary is absent/undefined, add a separate assertion like expect(result).not.toHaveProperty('preliminary') or expect(result.preliminary).toBeUndefined(); change the assertion around mapPiToolOutput(...) accordingly so the test no longer requires an explicit preliminary: undefined property.
🤖 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 `@packages/pi/src/pi-event-map.ts`:
- Around line 141-149: mapToolResultMessage currently only reads
record.toolCallId and record.toolName so messages using aliases like callId, id,
or name get dropped; update the extraction to accept those aliases (e.g., derive
toolCallId from record.toolCallId || record.callId || record.id and toolName
from record.toolName || record.name) before the empty-return check, then pass
the same fields into mapPiToolOutput/normalizeToolOutput as before; use the
existing helpers recordValue, stringValue, mapPiToolOutput, and
normalizeToolOutput to remain consistent with other handlers.
- Around line 106-110: The mapToolExecutionStart function only reads event.args
so tool_execution_start events that provide input as event.input or
event.arguments produce empty inputs; update mapToolExecutionStart to select the
same input aliases used elsewhere (check event.args, event.input, and
event.arguments) and pass the resolved input to mapPiToolInput along with
toolCallId and optional toolName (same symbol references: mapToolExecutionStart,
mapPiToolInput, toolCallId, toolName, event.args/event.input/event.arguments).
In `@packages/pi/src/pi-notice.ts`:
- Around line 38-40: The label logic currently uses truthy checks so attempt = 0
or maxAttempts = 0 gets treated as missing; change the condition to check for
undefined explicitly (e.g., use attempt !== undefined and maxAttempts !==
undefined) when building the retry label computed from
numberValue(record.attempt) and numberValue(record.maxAttempts) (and apply the
same fix to the second occurrence around lines referencing
label/attempt/maxAttempts). This preserves valid zero values while still
treating undefined as missing.
---
Nitpick comments:
In `@packages/pi/src/stream-map.test.ts`:
- Around line 53-59: The test for mapPiToolOutput is brittle by asserting an
exact shape that includes preliminary: undefined; update the assertion to be
behavior-focused by using a partial match (e.g., expect(...).toMatchObject or
expect(...).objectContaining) against the required keys { output, toolCallId,
toolName, type } and, if you need to guarantee preliminary is absent/undefined,
add a separate assertion like expect(result).not.toHaveProperty('preliminary')
or expect(result.preliminary).toBeUndefined(); change the assertion around
mapPiToolOutput(...) accordingly so the test no longer requires an explicit
preliminary: undefined property.
🪄 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: 5c4b066f-4999-4e78-84ec-f274591f57e2
📒 Files selected for processing (11)
packages/pi/package.jsonpackages/pi/src/appservice.tspackages/pi/src/beeper-stream.tspackages/pi/src/index.tspackages/pi/src/pi-event-map.test.tspackages/pi/src/pi-event-map.tspackages/pi/src/pi-notice.test.tspackages/pi/src/pi-notice.tspackages/pi/src/stream-map.test.tspackages/pi/src/stream-map.tspackages/pi/tsdown.config.ts
✅ Files skipped from review due to trivial changes (3)
- packages/pi/src/pi-notice.test.ts
- packages/pi/src/index.ts
- packages/pi/src/pi-event-map.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/pi/tsdown.config.ts
- packages/pi/package.json
- packages/pi/src/appservice.ts
- packages/pi/src/stream-map.ts
- packages/pi/src/beeper-stream.ts
📜 Review details
🔇 Additional comments (3)
packages/pi/src/stream-map.test.ts (2)
15-42: Good coverage of reasoning/text chunk lifecycle.This test cleanly exercises
start, incremental deltas, closure of open parts, andfinishbehavior for a full assistant message flow.
67-74: Monotonic envelope sequence assertion looks solid.Nice focused check that
withStreamEnvelopeincrementsseqwhile keepingturn_idstable for the same run state.packages/pi/src/pi-notice.ts (1)
2-4: Defensive unknown-payload handling looks solid.The runtime extraction pattern here is robust and prevents malformed event payloads from surfacing as runtime errors.
Also applies to: 73-80
| function mapToolExecutionStart(event: Record<string, unknown>): BeeperUIMessageChunk[] { | ||
| const toolCallId = stringValue(event.toolCallId) ?? stringValue(event.callId) ?? stringValue(event.id); | ||
| const toolName = stringValue(event.toolName) ?? stringValue(event.name); | ||
| if (!toolCallId) return []; | ||
| return [mapPiToolInput({ input: event.args, toolCallId, ...(toolName ? { toolName } : {}) })]; |
There was a problem hiding this comment.
Handle the same input aliases here as the other tool-event paths.
This start-event mapper only forwards event.args, so tool_execution_start payloads that use input or arguments will publish an empty tool input even though the rest of this file already supports those shapes. That makes the execution-start path less compatible than tool_call.
Suggested fix
function mapToolExecutionStart(event: Record<string, unknown>): BeeperUIMessageChunk[] {
const toolCallId = stringValue(event.toolCallId) ?? stringValue(event.callId) ?? stringValue(event.id);
const toolName = stringValue(event.toolName) ?? stringValue(event.name);
if (!toolCallId) return [];
- return [mapPiToolInput({ input: event.args, toolCallId, ...(toolName ? { toolName } : {}) })];
+ return [
+ mapPiToolInput({
+ input: event.input ?? event.args ?? parseMaybeJSONValue(event.arguments),
+ toolCallId,
+ ...(toolName ? { toolName } : {}),
+ }),
+ ];
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/pi-event-map.ts` around lines 106 - 110, The
mapToolExecutionStart function only reads event.args so tool_execution_start
events that provide input as event.input or event.arguments produce empty
inputs; update mapToolExecutionStart to select the same input aliases used
elsewhere (check event.args, event.input, and event.arguments) and pass the
resolved input to mapPiToolInput along with toolCallId and optional toolName
(same symbol references: mapToolExecutionStart, mapPiToolInput, toolCallId,
toolName, event.args/event.input/event.arguments).
| function mapToolResultMessage(message: unknown): BeeperUIMessageChunk[] { | ||
| const record = recordValue(message); | ||
| const toolCallId = stringValue(record?.toolCallId); | ||
| const toolName = stringValue(record?.toolName); | ||
| if (!toolCallId) return []; | ||
| if (record?.isError === true) { | ||
| return [mapPiToolOutput({ error: record.content, toolCallId, ...(toolName ? { toolName } : {}) })]; | ||
| } | ||
| return [mapPiToolOutput({ output: normalizeToolOutput(record?.content), toolCallId, ...(toolName ? { toolName } : {}) })]; |
There was a problem hiding this comment.
Broaden wrapped tool-result ID/name extraction to match the other handlers.
Line 143 only checks toolCallId, and Line 144 only checks toolName. If the message_end / toolResult variant carries callId/id or name, this branch returns [] and drops the tool result, even though the top-level tool-result handlers already support those aliases.
Suggested fix
function mapToolResultMessage(message: unknown): BeeperUIMessageChunk[] {
const record = recordValue(message);
- const toolCallId = stringValue(record?.toolCallId);
- const toolName = stringValue(record?.toolName);
+ const toolCallId =
+ stringValue(record?.toolCallId) ??
+ stringValue(record?.callId) ??
+ stringValue(record?.id);
+ const toolName = stringValue(record?.toolName) ?? stringValue(record?.name);
if (!toolCallId) return [];
if (record?.isError === true) {
return [mapPiToolOutput({ error: record.content, toolCallId, ...(toolName ? { toolName } : {}) })];
}
return [mapPiToolOutput({ output: normalizeToolOutput(record?.content), toolCallId, ...(toolName ? { toolName } : {}) })];
}📝 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.
| function mapToolResultMessage(message: unknown): BeeperUIMessageChunk[] { | |
| const record = recordValue(message); | |
| const toolCallId = stringValue(record?.toolCallId); | |
| const toolName = stringValue(record?.toolName); | |
| if (!toolCallId) return []; | |
| if (record?.isError === true) { | |
| return [mapPiToolOutput({ error: record.content, toolCallId, ...(toolName ? { toolName } : {}) })]; | |
| } | |
| return [mapPiToolOutput({ output: normalizeToolOutput(record?.content), toolCallId, ...(toolName ? { toolName } : {}) })]; | |
| function mapToolResultMessage(message: unknown): BeeperUIMessageChunk[] { | |
| const record = recordValue(message); | |
| const toolCallId = | |
| stringValue(record?.toolCallId) ?? | |
| stringValue(record?.callId) ?? | |
| stringValue(record?.id); | |
| const toolName = stringValue(record?.toolName) ?? stringValue(record?.name); | |
| if (!toolCallId) return []; | |
| if (record?.isError === true) { | |
| return [mapPiToolOutput({ error: record.content, toolCallId, ...(toolName ? { toolName } : {}) })]; | |
| } | |
| return [mapPiToolOutput({ output: normalizeToolOutput(record?.content), toolCallId, ...(toolName ? { toolName } : {}) })]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/pi-event-map.ts` around lines 141 - 149, mapToolResultMessage
currently only reads record.toolCallId and record.toolName so messages using
aliases like callId, id, or name get dropped; update the extraction to accept
those aliases (e.g., derive toolCallId from record.toolCallId || record.callId
|| record.id and toolName from record.toolName || record.name) before the
empty-return check, then pass the same fields into
mapPiToolOutput/normalizeToolOutput as before; use the existing helpers
recordValue, stringValue, mapPiToolOutput, and normalizeToolOutput to remain
consistent with other handlers.
| const attempt = numberValue(record.attempt); | ||
| const maxAttempts = numberValue(record.maxAttempts); | ||
| const label = attempt && maxAttempts ? ` ${attempt} of ${maxAttempts}` : ""; |
There was a problem hiding this comment.
Preserve valid 0 attempt values in retry labels.
Line 40 and Line 46 use truthy checks, so attempt = 0 (or maxAttempts = 0) is treated as missing and label text is dropped. Check for undefined instead.
Suggested fix
if (type === "auto_retry_start") {
const attempt = numberValue(record.attempt);
const maxAttempts = numberValue(record.maxAttempts);
- const label = attempt && maxAttempts ? ` ${attempt} of ${maxAttempts}` : "";
+ const label =
+ attempt !== undefined
+ ? maxAttempts !== undefined
+ ? ` ${attempt} of ${maxAttempts}`
+ : ` ${attempt}`
+ : "";
return `Retry${label} started${errorSuffix(record.errorMessage)}.`;
}
if (type === "auto_retry_end") {
const attempt = numberValue(record.attempt);
- const label = attempt ? ` ${attempt}` : "";
+ const label = attempt !== undefined ? ` ${attempt}` : "";
return record.success === true
? `Retry${label} succeeded.`
: `Retry${label} failed${errorSuffix(record.finalError)}.`;
}Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pi/src/pi-notice.ts` around lines 38 - 40, The label logic currently
uses truthy checks so attempt = 0 or maxAttempts = 0 gets treated as missing;
change the condition to check for undefined explicitly (e.g., use attempt !==
undefined and maxAttempts !== undefined) when building the retry label computed
from numberValue(record.attempt) and numberValue(record.maxAttempts) (and apply
the same fix to the second occurrence around lines referencing
label/attempt/maxAttempts). This preserves valid zero values while still
treating undefined as missing.
Add end-to-end support for appservice transactions: a new test ensures transactions are forwarded before acknowledgement. AppserviceWebsocket gains a handleTransaction option and invokes it when transactions arrive. RuntimeBridge forwards transactions to matrixClient.appservice.applyTransaction via a new handler. Native Pickle core adds an appservice_apply_transaction operation, a transaction options/type, a beeperStream event processor for dispatching transaction events, and wiring into initialization. Client and generated runtime types are updated to expose applyTransaction. Also set beeper: true in matrix configs when creating bridges.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/bridge/src/appservice-websocket.ts (1)
261-270: ⚡ Quick winNormalize HTTP-proxy transactions with
txn_id.The websocket path forwards
txn_id, but the HTTP-proxy path drops it because it only exists in the URL. That makeshandleTransactiontransport-dependent for no real benefit. Consider injectingtxn_id: transactionMatch[1]before invoking the callback.Possible normalization
if (method === "PUT" && transactionMatch) { - const transaction = objectValue(request.body) ?? {}; + const transaction = { + ...(objectValue(request.body) ?? {}), + txn_id: transactionMatch[1], + }; const events = Array.isArray(transaction.events) ? transaction.events : []; this.#log("debug", "appservice_websocket_http_transaction", { eventCount: events.length, toDeviceCount: eventCount(transaction.to_device), txnId: transactionMatch[1], }); await this.#handleTransaction?.(transaction);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bridge/src/appservice-websocket.ts` around lines 261 - 270, The HTTP-proxy branch for PUT transactions drops txn_id from the transaction body, making `#handleTransaction` transport-dependent; inject txn_id: transactionMatch[1] into the transaction object before calling this.#handleTransaction. Locate the block that builds const transaction = objectValue(request.body) ?? {}; (and uses transactionMatch) and set transaction.txn_id = transactionMatch[1] (or create a shallow copy with txn_id) prior to the await this.#handleTransaction?.(transaction) call so both websocket and HTTP-proxy paths pass the same normalized payload to `#handleTransaction`.
🤖 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 `@packages/bridge/src/appservice-websocket.test.ts`:
- Around line 80-128: The test currently uses a mocked async function that
resolves immediately, which doesn't confirm that the ACK waits for
handleTransaction to finish. Modify the test to use a deferred promise inside
handleTransaction, causing it to block until explicitly resolved. Then assert
that no websocket response (the ACK) is received before resolving this promise,
ensuring the implementation properly awaits handleTransaction. Use this deferred
approach in the test for the handleTransaction mock and check websocket messages
accordingly.
In `@packages/bridge/src/beeper.ts`:
- Around line 255-257: The booleanField helper currently treats only undefined
as “missing”, so a null alias stops resolution and causes the field to be
dropped; update the alias-selection predicate in booleanField (the
keys.map(...).find(...)) to skip both undefined and null (e.g., treat candidate
== null as missing) so it will continue to the next alias and then return the
first boolean value found (preserve the existing typeof value === "boolean"
check).
In `@packages/pickle/native/internal/core/appservice.go`:
- Around line 209-212: The handler handleAppserviceApplyTransaction should not
return c.empty() when c.appserviceProcessor == nil; instead detect the missing
processor and return a retryable error (the same failure behavior used by other
appservice entrypoints) so the bridge does not acknowledge the transaction;
update the nil-check in handleAppserviceApplyTransaction to return an error
describing "appservice transaction pipeline unavailable" (or similar) rather
than calling c.empty(), referencing the function
handleAppserviceApplyTransaction and the field appserviceProcessor to locate and
change the behavior.
In `@packages/pickle/native/internal/core/core.go`:
- Line 25: The new appserviceProcessor field is not cleared when a session is
closed, which lets old handlers persist and double-dispatch on re-init; modify
handleClose() to explicitly set appserviceProcessor = nil (or call its
Close/Stop method then nil it) alongside existing cleanup of beeperStream and
other runtime state so the previous processor cannot receive events after close;
locate and update the handleClose method in core.go to reset the
appserviceProcessor field (and ensure any needed teardown is invoked before
nil'ing).
---
Nitpick comments:
In `@packages/bridge/src/appservice-websocket.ts`:
- Around line 261-270: The HTTP-proxy branch for PUT transactions drops txn_id
from the transaction body, making `#handleTransaction` transport-dependent; inject
txn_id: transactionMatch[1] into the transaction object before calling
this.#handleTransaction. Locate the block that builds const transaction =
objectValue(request.body) ?? {}; (and uses transactionMatch) and set
transaction.txn_id = transactionMatch[1] (or create a shallow copy with txn_id)
prior to the await this.#handleTransaction?.(transaction) call so both websocket
and HTTP-proxy paths pass the same normalized payload to `#handleTransaction`.
🪄 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: 14ff1aa0-3a70-4ab3-a36d-8cdf7982c4a5
📒 Files selected for processing (17)
packages/bridge/src/appservice-websocket.test.tspackages/bridge/src/appservice-websocket.tspackages/bridge/src/beeper.test.tspackages/bridge/src/beeper.tspackages/bridge/src/bridge.tspackages/bridge/src/index.tspackages/bridge/src/node.tspackages/pi/src/registration.tspackages/pi/src/types.tspackages/pickle/native/internal/core/appservice.gopackages/pickle/native/internal/core/core.gopackages/pickle/native/internal/core/init.gopackages/pickle/native/internal/core/operations.gopackages/pickle/src/client-types.tspackages/pickle/src/client.tspackages/pickle/src/generated-runtime-operations.tspackages/pickle/src/generated-runtime-types.ts
✅ Files skipped from review due to trivial changes (3)
- packages/pickle/native/internal/core/operations.go
- packages/pickle/src/generated-runtime-types.ts
- packages/bridge/src/beeper.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/pi/src/types.ts
- packages/pickle/src/client.ts
- packages/pi/src/registration.ts
- packages/pickle/native/internal/core/init.go
- packages/bridge/src/node.ts
- packages/bridge/src/index.ts
- packages/bridge/src/bridge.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Typecheck, test, build, and package
| const handleTransaction = vi.fn(async () => {}); | ||
| const connected = new Promise<void>((resolve, reject) => { | ||
| wsServer.on("connection", (socket) => { | ||
| socket.once("message", (raw) => { | ||
| try { | ||
| const response = JSON.parse(raw.toString()) as { command: string; data: { txn_id: string }; id: number }; | ||
| expect(response).toEqual({ | ||
| command: "response", | ||
| data: { txn_id: "txn-td" }, | ||
| id: 8, | ||
| }); | ||
| resolve(); | ||
| } catch (error) { | ||
| reject(error); | ||
| } | ||
| }); | ||
| socket.send(JSON.stringify({ | ||
| command: "transaction", | ||
| id: 8, | ||
| to_device: [{ | ||
| content: { device_id: "DESKTOP", event_id: "$event", room_id: "!room:example" }, | ||
| sender: "@alice:example", | ||
| to_device_id: "PICKLE", | ||
| to_user_id: "@bot:example", | ||
| type: "com.beeper.stream.subscribe", | ||
| }], | ||
| txn_id: "txn-td", | ||
| })); | ||
| }); | ||
| }); | ||
| const websocket = createWebsocket(homeserver, { | ||
| handleTransaction, | ||
| log: (() => {}) as BridgeLogger, | ||
| }); | ||
| websockets.push(websocket); | ||
|
|
||
| websocket.start(); | ||
| await connected; | ||
|
|
||
| expect(handleTransaction).toHaveBeenCalledWith(expect.objectContaining({ | ||
| to_device: [expect.objectContaining({ | ||
| content: { device_id: "DESKTOP", event_id: "$event", room_id: "!room:example" }, | ||
| sender: "@alice:example", | ||
| to_device_id: "PICKLE", | ||
| to_user_id: "@bot:example", | ||
| type: "com.beeper.stream.subscribe", | ||
| })], | ||
| txn_id: "txn-td", | ||
| })); |
There was a problem hiding this comment.
Actually block the ACK on handleTransaction.
This test does not prove the ACK waits for handleTransaction to finish. vi.fn(async () => {}) records the call immediately, so the assertion still passes if the implementation stops awaiting the promise.
Use a deferred promise and assert no websocket response is received until you release it.
Suggested test shape
- const handleTransaction = vi.fn(async () => {});
+ let releaseTransaction!: () => void;
+ const transactionGate = new Promise<void>((resolve) => {
+ releaseTransaction = resolve;
+ });
+ const handleTransaction = vi.fn(() => transactionGate);
+ let acknowledged = false;
const connected = new Promise<void>((resolve, reject) => {
wsServer.on("connection", (socket) => {
socket.once("message", (raw) => {
try {
+ acknowledged = true;
const response = JSON.parse(raw.toString()) as { command: string; data: { txn_id: string }; id: number };
expect(response).toEqual({
command: "response",
data: { txn_id: "txn-td" },
id: 8,
});
resolve();
} catch (error) {
reject(error);
}
});
socket.send(JSON.stringify({
command: "transaction",
id: 8,
to_device: [{
content: { device_id: "DESKTOP", event_id: "$event", room_id: "!room:example" },
sender: "@alice:example",
to_device_id: "PICKLE",
to_user_id: "@bot:example",
type: "com.beeper.stream.subscribe",
}],
txn_id: "txn-td",
}));
});
});
websocket.start();
+ await delay(20);
+ expect(acknowledged).toBe(false);
+ releaseTransaction();
await connected;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleTransaction = vi.fn(async () => {}); | |
| const connected = new Promise<void>((resolve, reject) => { | |
| wsServer.on("connection", (socket) => { | |
| socket.once("message", (raw) => { | |
| try { | |
| const response = JSON.parse(raw.toString()) as { command: string; data: { txn_id: string }; id: number }; | |
| expect(response).toEqual({ | |
| command: "response", | |
| data: { txn_id: "txn-td" }, | |
| id: 8, | |
| }); | |
| resolve(); | |
| } catch (error) { | |
| reject(error); | |
| } | |
| }); | |
| socket.send(JSON.stringify({ | |
| command: "transaction", | |
| id: 8, | |
| to_device: [{ | |
| content: { device_id: "DESKTOP", event_id: "$event", room_id: "!room:example" }, | |
| sender: "@alice:example", | |
| to_device_id: "PICKLE", | |
| to_user_id: "@bot:example", | |
| type: "com.beeper.stream.subscribe", | |
| }], | |
| txn_id: "txn-td", | |
| })); | |
| }); | |
| }); | |
| const websocket = createWebsocket(homeserver, { | |
| handleTransaction, | |
| log: (() => {}) as BridgeLogger, | |
| }); | |
| websockets.push(websocket); | |
| websocket.start(); | |
| await connected; | |
| expect(handleTransaction).toHaveBeenCalledWith(expect.objectContaining({ | |
| to_device: [expect.objectContaining({ | |
| content: { device_id: "DESKTOP", event_id: "$event", room_id: "!room:example" }, | |
| sender: "@alice:example", | |
| to_device_id: "PICKLE", | |
| to_user_id: "@bot:example", | |
| type: "com.beeper.stream.subscribe", | |
| })], | |
| txn_id: "txn-td", | |
| })); | |
| let releaseTransaction!: () => void; | |
| const transactionGate = new Promise<void>((resolve) => { | |
| releaseTransaction = resolve; | |
| }); | |
| const handleTransaction = vi.fn(() => transactionGate); | |
| let acknowledged = false; | |
| const connected = new Promise<void>((resolve, reject) => { | |
| wsServer.on("connection", (socket) => { | |
| socket.once("message", (raw) => { | |
| try { | |
| acknowledged = true; | |
| const response = JSON.parse(raw.toString()) as { command: string; data: { txn_id: string }; id: number }; | |
| expect(response).toEqual({ | |
| command: "response", | |
| data: { txn_id: "txn-td" }, | |
| id: 8, | |
| }); | |
| resolve(); | |
| } catch (error) { | |
| reject(error); | |
| } | |
| }); | |
| socket.send(JSON.stringify({ | |
| command: "transaction", | |
| id: 8, | |
| to_device: [{ | |
| content: { device_id: "DESKTOP", event_id: "$event", room_id: "!room:example" }, | |
| sender: "@alice:example", | |
| to_device_id: "PICKLE", | |
| to_user_id: "@bot:example", | |
| type: "com.beeper.stream.subscribe", | |
| }], | |
| txn_id: "txn-td", | |
| })); | |
| }); | |
| }); | |
| const websocket = createWebsocket(homeserver, { | |
| handleTransaction, | |
| log: (() => {}) as BridgeLogger, | |
| }); | |
| websockets.push(websocket); | |
| websocket.start(); | |
| await delay(20); | |
| expect(acknowledged).toBe(false); | |
| releaseTransaction(); | |
| await connected; | |
| expect(handleTransaction).toHaveBeenCalledWith(expect.objectContaining({ | |
| to_device: [expect.objectContaining({ | |
| content: { device_id: "DESKTOP", event_id: "$event", room_id: "!room:example" }, | |
| sender: "@alice:example", | |
| to_device_id: "PICKLE", | |
| to_user_id: "@bot:example", | |
| type: "com.beeper.stream.subscribe", | |
| })], | |
| txn_id: "txn-td", | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bridge/src/appservice-websocket.test.ts` around lines 80 - 128, The
test currently uses a mocked async function that resolves immediately, which
doesn't confirm that the ACK waits for handleTransaction to finish. Modify the
test to use a deferred promise inside handleTransaction, causing it to block
until explicitly resolved. Then assert that no websocket response (the ACK) is
received before resolving this promise, ensuring the implementation properly
awaits handleTransaction. Use this deferred approach in the test for the
handleTransaction mock and check websocket messages accordingly.
| function booleanField(input: Record<string, unknown>, ...keys: string[]): boolean | undefined { | ||
| const value = keys.map((key) => input[key]).find((candidate) => candidate !== undefined); | ||
| return typeof value === "boolean" ? value : undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, what is the behavioral difference between candidate !== undefined and nullish checks (candidate != null/??) when candidatemay benull?
💡 Result:
Behavioral difference (when candidate may be null): 1) candidate!== undefined - This check only filters out the value undefined. - If candidate is null, the expression evaluates to true (because null!== undefined) [1]. So: it treats null as a present value. 2) Nullish checks: candidate!= null / (candidate === null || candidate === undefined) /?? - candidate!= null is a loose comparison that is true for both null and undefined, i.e. it matches “nullish” [2]. - The nullish coalescing operator?? returns the right-hand side only when the left operand is null or undefined; otherwise it returns the left operand (so null is treated as missing) [3]. So: these treat null and undefined equivalently as “no value”. Practical example - if (candidate!== undefined) {... } • runs the block for candidate = null [1]. - if (candidate!= null) {... } // or “if (candidate?? fallback) …” patterns • does not run the block for candidate = null because null is nullish [2][3]. TypeScript angle (important nuance) - TypeScript’s control-flow narrowing with?? / nullish checks aligns with the runtime meaning:?? handles both null and undefined (and can narrow away null/undefined in typical cases) [3]. - However, TypeScript’s narrowing is not always able to prove complex “at least one is non-null” constraints, even when your boolean logic suggests it; there are known cases where nullish coalescing still triggers type errors because the checker can’t model the constraint precisely [4]. Bottom line - candidate!== undefined: excludes only undefined; null passes. - candidate!= null /??: excludes both null and undefined; null is treated as absent/missing.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Strict_equality
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing
- 4: Nullish coalescing operator does not understand cases where at least one operand is guaranteed to not be null, but not which one is not null microsoft/TypeScript#61670
🏁 Script executed:
cat -n packages/bridge/src/beeper.ts | sed -n '250,270p'Repository: beeper/pickle
Length of output: 967
🏁 Script executed:
# Search for calls to booleanField and normalizeRegistration
rg "booleanField|normalizeRegistration" packages/bridge/src/beeper.ts -B 2 -A 5Repository: beeper/pickle
Length of output: 1956
🏁 Script executed:
# Check for any test files related to beeper.ts
fd -e test.ts -e spec.ts -e tests.ts | xargs grep -l "booleanField\|normalizeRegistration" 2>/dev/nullRepository: beeper/pickle
Length of output: 41
Handle null as "missing" in boolean alias resolution.
At Line 256, the first value that is not undefined is chosen. If an earlier alias is null and a later alias is a valid boolean, this path returns undefined and the normalized field gets dropped instead of falling back to the next key.
Suggested fix
function booleanField(input: Record<string, unknown>, ...keys: string[]): boolean | undefined {
- const value = keys.map((key) => input[key]).find((candidate) => candidate !== undefined);
+ const value = keys
+ .map((key) => input[key])
+ .find((candidate) => candidate != null);
return typeof value === "boolean" ? value : undefined;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bridge/src/beeper.ts` around lines 255 - 257, The booleanField
helper currently treats only undefined as “missing”, so a null alias stops
resolution and causes the field to be dropped; update the alias-selection
predicate in booleanField (the keys.map(...).find(...)) to skip both undefined
and null (e.g., treat candidate == null as missing) so it will continue to the
next alias and then return the first boolean value found (preserve the existing
typeof value === "boolean" check).
| func (c *Core) handleAppserviceApplyTransaction(ctx context.Context, payload []byte) ([]byte, error) { | ||
| if c.appserviceProcessor == nil { | ||
| return c.empty() | ||
| } |
There was a problem hiding this comment.
Do not silently succeed when no processor is configured.
Returning {} here causes the bridge layer to acknowledge the transaction even though nothing was applied. On startup-order bugs or post-close() calls, that turns into a silent drop instead of a retryable error. This should fail like the other appservice entrypoints when the appservice transaction pipeline is unavailable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pickle/native/internal/core/appservice.go` around lines 209 - 212,
The handler handleAppserviceApplyTransaction should not return c.empty() when
c.appserviceProcessor == nil; instead detect the missing processor and return a
retryable error (the same failure behavior used by other appservice entrypoints)
so the bridge does not acknowledge the transaction; update the nil-check in
handleAppserviceApplyTransaction to return an error describing "appservice
transaction pipeline unavailable" (or similar) rather than calling c.empty(),
referencing the function handleAppserviceApplyTransaction and the field
appserviceProcessor to locate and change the behavior.
| backupKey *backup.MegolmBackupKey | ||
| backupVersion id.KeyBackupVersion | ||
| beeperStream *beeperstream.Helper | ||
| appserviceProcessor *beeperStreamEventProcessor |
There was a problem hiding this comment.
Reset appserviceProcessor in handleClose().
This new field outlives the session right now. handleClose() clears beeperStream and the rest of the runtime state, but not appserviceProcessor, so close() followed by re-init can keep old handlers alive and double-dispatch later transactions into stale state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pickle/native/internal/core/core.go` at line 25, The new
appserviceProcessor field is not cleared when a session is closed, which lets
old handlers persist and double-dispatch on re-init; modify handleClose() to
explicitly set appserviceProcessor = nil (or call its Close/Stop method then nil
it) alongside existing cleanup of beeperStream and other runtime state so the
previous processor cannot receive events after close; locate and update the
handleClose method in core.go to reset the appserviceProcessor field (and ensure
any needed teardown is invoked before nil'ing).
1 participant
No description provided.