Batuhan/pi

[batuhan]

No description provided.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔄 Running review...

📝 Walkthrough

Walkthrough

Bridge moves to appservice-centric init and provisioning with transaction forwarding and portal initial state. Native/core add appservice init, transaction apply, and crypto reset. New PicklePi package adds agent, CLI, rooms/spaces/media/registry, stream publisher/mapping, approval/notice, queue, tests, and configs.

Changes

Bridge appservice refactor + PicklePi + runtime extensions

Layer / File(s)	Summary
Native core and client runtime extensions packages/pickle/native/internal/core/*, packages/pickle/src/*	Adds appservice init/transactions and crypto reset; exposes appservice/deviceId; generated ops/types updated; client adds applyTransaction.
Bridge refactor, provisioning, websocket, build packages/bridge/src/*, packages/bridge/package.json, packages/bridge/tsdown.config.ts	Appservice-first bridge init; provisioning HTTP proxy module; websocket forwards transactions; portal creation initialState; beeper registration normalization; build/export updates; tests.
PicklePi package: agent, utilities, storage, streaming, tests packages/pi/**/*	Introduces PicklePi agent/CLI; stream publisher/mappers; rooms/spaces; media store; registry; approval/notice parsers; queue; matrix client; docs and tests.
Beeper stream sender concurrency/normalization packages/pickle/src/streams/beeper.ts	Concurrent part publishing, rich chunk normalization, and tool timing propagation.
Workspace and tsconfig updates pnpm-workspace.yaml, tsconfig.base.json	Adds path alias for pi and removes bridge/node alias; adjusts workspace packages.

Sequence Diagram(s)

sequenceDiagram
  participant Matrix HS as Matrix Homeserver
  participant Bridge WS as Appservice WebSocket
  participant Core as Native Core
  Matrix HS->>Bridge WS: PUT /_matrix/app/v1/transactions/{txn_id}
  Bridge WS->>Core: appserviceApplyTransaction(transaction)
  Core-->>Bridge WS: processed
  Bridge WS-->>Matrix HS: 200 OK

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch batuhan/pi

[coderabbitai]

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 win

Keep owner and bot identities separate after appservice boot.

createBeeperBridge() now defaults the client token to registration.asToken, so whoami.userId becomes the appservice bot. Assigning that to #ownerUserId breaks implicit management-room detection, default remote sender attribution, and provisioning logins for the real Beeper owner account. Initialize #ownerUserId from this.#beeperOptions?.ownerUserId ?? whoami.userId, and reserve #ownUserId for 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 matrix object computes appservice, device ID, homeserver, store, and token, but RuntimeBridge uses the supplied client directly and does not consume those resolved values. Callers expecting this helper to behave like createBeeperBridge() 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 win

Dynamic import bypasses static analysis and bundler optimization.

Using new Function to 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 tradeoff

Consider 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

stripUndefined mutates input object.

The function mutates the input object by deleting keys with undefined values. 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

randomID uses non-cryptographic randomness for login IDs.

The function generates login IDs using Date.now() and Math.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 win

Silent fallback to first login may be unexpected.

When a login_id query parameter is provided but doesn't match any login, or when no login_id is 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_id is 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 value

Defensive chmod after writeFile with mode.

The file is written with mode: 0o600 and then immediately followed by chmod(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 value

Consider 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 win

Extract the duplicated DecryptErrorCallback assignment.

The DecryptErrorCallback closure 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 value

String-based error detection is fragile.

isMissingServerKeysError relies on matching error message text, which may change in upstream mautrix releases. Consider adding a comment documenting this fragility, or check if mautrix exposes 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 value

Default-allow behavior when allowedUserIds is empty.

When allowedUserIds is 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

📥 Commits

Reviewing files that changed from the base of the PR and between e39c4f7 and 37e1feb.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (49)

• packages/bridge/src/bridge.ts
• packages/bridge/src/index.ts
• packages/bridge/src/node.ts
• packages/bridge/src/provisioning.test.ts
• packages/bridge/src/provisioning.ts
• packages/bridge/src/store.ts
• packages/bridge/src/types.ts
• packages/pi/@beeper-pickle-pi.TODO.md
• packages/pi/README.md
• packages/pi/package.json
• packages/pi/src/approval.test.ts
• packages/pi/src/approval.ts
• packages/pi/src/appservice.ts
• packages/pi/src/beeper-stream.test.ts
• packages/pi/src/beeper-stream.ts
• packages/pi/src/cli.ts
• packages/pi/src/config.ts
• packages/pi/src/index.ts
• packages/pi/src/matrix.ts
• packages/pi/src/media-store.test.ts
• packages/pi/src/media-store.ts
• packages/pi/src/pi-beeper-stream.test.ts
• packages/pi/src/pi-beeper-stream.ts
• packages/pi/src/pi-event-map.test.ts
• packages/pi/src/pi-event-map.ts
• packages/pi/src/pi-runtime.ts
• packages/pi/src/queue.test.ts
• packages/pi/src/queue.ts
• packages/pi/src/registration.ts
• packages/pi/src/registry.test.ts
• packages/pi/src/registry.ts
• packages/pi/src/rooms.test.ts
• packages/pi/src/rooms.ts
• packages/pi/src/spaces.test.ts
• packages/pi/src/spaces.ts
• packages/pi/src/stream-map.test.ts
• packages/pi/src/stream-map.ts
• packages/pi/src/types.ts
• packages/pi/tsconfig.json
• packages/pi/tsdown.config.ts
• packages/pickle/native/internal/core/appservice.go
• packages/pickle/native/internal/core/init.go
• packages/pickle/native/internal/core/persistent_crypto_methods.go
• packages/pickle/src/client.ts
• packages/pickle/src/generated-runtime-types.ts
• packages/pickle/src/streams/beeper.ts
• packages/pickle/src/types.ts
• pnpm-workspace.yaml
• tsconfig.base.json

📜 Review details

🔇 Additional comments (58)

pnpm-workspace.yaml (1)

7-7: LGTM!

Adding packages/pi to the workspace is correct for the new Pi bridge package.

packages/pickle/src/generated-runtime-types.ts (2)

78-78: LGTM!

The initialState property is correctly typed as optional MatrixRoomStateInput[], matching the Go struct definition.

---

137-137: LGTM!

The appservice property addition enables appservice-first initialization flow correctly.

packages/pickle/native/internal/core/appservice.go (2)

95-95: LGTM!

The InitialState field is correctly defined with appropriate JSON serialization tag.

---

301-308: LGTM!

The implementation correctly handles initial state events:

• Local copy of stateKey avoids 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 appservice and deviceId options correctly extend MatrixClientOptions to support appservice-based initialization.

packages/pickle/src/client.ts (1)

332-333: LGTM!

The device ID fallback logic correctly prioritizes the explicit deviceId option 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 while loop in waitForPublishes properly handles promises added during iteration
• Error swallowing with console.warn provides resilience without silent failures
• Promise cleanup in finally ensures no memory leaks

---

318-319: LGTM!

Adding startedAtMs timing field enables tool execution timing tracking in the UI.

---

345-346: LGTM!

Adding completedAtMs timing field complements the start timing for complete execution duration tracking.

---

366-407: LGTM!

The normalizeRichStreamChunk function 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 isStreamPart and new isNativeStreamPartRecord functions 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 asynchronous onEvent implementations 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 BridgeConnector and 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 to BridgeMatrixConfig, and initialState to 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, approved as boolean)
• Normalizes hyphenated decision strings (allow-once → allow_once)
• Computes approvedAlways from explicit decision or flag
• Forces decision to "deny" when approved is 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 -p semantics
• 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 ENOENT during 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 matrixInboundTurnPriorityOrder constant 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 updateTextByEventId implementation 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 until dequeue returns 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 randomBytes with 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 win

Verify domain parameter in production deployments.

The function defaults domain to "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 value

Verify hardcoded "pickle-pi_" prefix in aliases namespace.

The aliases namespace regex uses a hardcoded "pickle-pi_" prefix instead of deriving it from config.appserviceId or 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 return undefined, violating the declared string return type.

JSON.stringify(undefined) returns the primitive undefined value, not a string. This can cause type safety violations when the result is used in tool-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-pi alias 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 win

Check if publisher.start() can be called multiple times safely.

Both the public start() method (line 21) and the chunk handler (line 37) invoke publisher.start(). If a client calls bridge.start() followed by a start chunk 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 afterEach ensures 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/finally pattern 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 on initClient implementation.

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 in safeMediaId.

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 on saveMediaBuffer.

Proper use of 0o600 permissions 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's package.json and tsconfig.json enforce this minimum, or re-export types explicitly for broader compatibility.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

packages/pi/src/stream-map.test.ts (1)

53-59: ⚡ Quick win

Avoid strict shape-coupling on optional preliminary in tool output test.

Line 55 makes the test brittle by requiring an explicit preliminary: undefined key with toEqual. 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

📥 Commits

Reviewing files that changed from the base of the PR and between a99fed1 and 82b3c27.

📒 Files selected for processing (11)

• packages/pi/package.json
• packages/pi/src/appservice.ts
• packages/pi/src/beeper-stream.ts
• packages/pi/src/index.ts
• packages/pi/src/pi-event-map.test.ts
• packages/pi/src/pi-event-map.ts
• packages/pi/src/pi-notice.test.ts
• packages/pi/src/pi-notice.ts
• packages/pi/src/stream-map.test.ts
• packages/pi/src/stream-map.ts
• packages/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, and finish behavior for a full assistant message flow.

---

67-74: Monotonic envelope sequence assertion looks solid.

Nice focused check that withStreamEnvelope increments seq while keeping turn_id stable 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

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

packages/bridge/src/appservice-websocket.ts (1)

261-270: ⚡ Quick win

Normalize 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 makes handleTransaction transport-dependent for no real benefit. Consider injecting txn_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

📥 Commits

Reviewing files that changed from the base of the PR and between 82b3c27 and 0224f67.

📒 Files selected for processing (17)

• packages/bridge/src/appservice-websocket.test.ts
• packages/bridge/src/appservice-websocket.ts
• packages/bridge/src/beeper.test.ts
• packages/bridge/src/beeper.ts
• packages/bridge/src/bridge.ts
• packages/bridge/src/index.ts
• packages/bridge/src/node.ts
• packages/pi/src/registration.ts
• packages/pi/src/types.ts
• packages/pickle/native/internal/core/appservice.go
• packages/pickle/native/internal/core/core.go
• packages/pickle/native/internal/core/init.go
• packages/pickle/native/internal/core/operations.go
• packages/pickle/src/client-types.ts
• packages/pickle/src/client.ts
• packages/pickle/src/generated-runtime-operations.ts
• packages/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

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (1)

packages/pi/src/pi-runtime.ts (1)

30-50: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Serialize PICKLE_PI_OWNED_SESSION mutation to avoid cross-session races.

Line 30-Line 50 restores env state per call, but concurrent createHeadlessPiSession() calls can still clobber each other’s restore window and leak wrong state into another session bootstrap.

Suggested fix

+let ownedSessionEnvLock: Promise<void> = Promise.resolve();
+
+async function withOwnedSessionEnv<T>(fn: () => Promise<T>): Promise<T> {
+  const prev = ownedSessionEnvLock;
+  let release!: () => void;
+  ownedSessionEnvLock = new Promise<void>((resolve) => {
+    release = resolve;
+  });
+  await prev;
+
+  const previousOwnedSession = process.env.PICKLE_PI_OWNED_SESSION;
+  process.env.PICKLE_PI_OWNED_SESSION = "1";
+  try {
+    return await fn();
+  } finally {
+    if (previousOwnedSession === undefined) delete process.env.PICKLE_PI_OWNED_SESSION;
+    else process.env.PICKLE_PI_OWNED_SESSION = previousOwnedSession;
+    release();
+  }
+}
+
 export async function createHeadlessPiSession(options: HeadlessPiRuntimeOptions): Promise<HeadlessPiSession> {
   const pi = await loadPiCodingAgent();
@@
-  const previousOwnedSession = process.env.PICKLE_PI_OWNED_SESSION;
-  process.env.PICKLE_PI_OWNED_SESSION = "1";
-  let result: { modelFallbackMessage?: string; session: PiAgentSession };
-  try {
-    const sessionManager = pi.SessionManager.open(options.binding.piSessionFile, nativeSessionDir, options.binding.cwd);
-    const resourceLoader = new pi.DefaultResourceLoader({ cwd: options.binding.cwd });
-    await resourceLoader.reload();
-    result = await pi.createAgentSession({
-      cwd: options.binding.cwd,
-      customTools: [],
-      resourceLoader,
-      sessionManager,
-      sessionStartEvent: { reason: "startup", type: "session_start" },
-      tools: pi.createCodingTools(options.binding.cwd),
-    });
-  } finally {
-    if (previousOwnedSession === undefined) {
-      delete process.env.PICKLE_PI_OWNED_SESSION;
-    } else {
-      process.env.PICKLE_PI_OWNED_SESSION = previousOwnedSession;
-    }
-  }
+  const result = await withOwnedSessionEnv(async () => {
+    const sessionManager = pi.SessionManager.open(options.binding.piSessionFile, nativeSessionDir, options.binding.cwd);
+    const resourceLoader = new pi.DefaultResourceLoader({ cwd: options.binding.cwd });
+    await resourceLoader.reload();
+    return pi.createAgentSession({
+      cwd: options.binding.cwd,
+      customTools: [],
+      resourceLoader,
+      sessionManager,
+      sessionStartEvent: { reason: "startup", type: "session_start" },
+      tools: pi.createCodingTools(options.binding.cwd),
+    });
+  });

🤖 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 30 - 50, The env toggle of
process.env.PICKLE_PI_OWNED_SESSION in createHeadlessPiSession() is not
serialized and can be clobbered by concurrent calls; wrap the whole
mutate–session-create–restore block (the code that sets
process.env.PICKLE_PI_OWNED_SESSION, calls pi.createAgentSession and finally
restores the var) inside a simple async mutex so only one invocation runs that
critical section at a time. Implement a lightweight global lock (e.g., a
Promise-based mutex or queue) used by createHeadlessPiSession() to acquire
before setting PICKLE_PI_OWNED_SESSION and release after the finally restore,
ensuring the restore happens in the same critical section and preventing
cross-session races when calling pi.createAgentSession and SessionManager.open.

🤖 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 122-125: The test currently uses a fixed delay after
websocket.start() to assert that no ACK has arrived, which is flaky; replace the
delay(20) with a race between the ACK-watching promise (the promise that would
resolve/set acknowledged) and a short timeout promise (e.g., 20ms) so you can
assert that the ACK promise did not resolve before releaseTransaction() is
called; locate the ACK-related promise/flag (the variable `acknowledged` or the
promise used to detect ACKs) and replace the await delay(20) with an awaited
Promise.race([ackPromise, timeoutPromise]) and assert that the raced result
indicates no ACK before calling `releaseTransaction()` after the race.

In `@packages/pi/src/appservice.ts`:
- Around line 154-167: The `#ensureHeadlessSession` method has a race where
concurrent calls can create duplicate sessions; fix it by adding an in-flight
map (e.g., this.#sessionPromises) keyed by bindingId: check this.#sessions
first, then check and return any existing promise from this.#sessionPromises,
otherwise create and store a promise that runs the createHeadlessPiSession call
(using registry.data.bindings to find binding and invoking
createHeadlessPiSession with onEvent => this.#mirrorPiEvent), await it, store
the resolved HeadlessPiSession in this.#sessions, and finally remove the entry
from this.#sessionPromises; ensure the promise entry is removed on rejection so
future calls can retry.

In `@packages/pi/src/beeper-stream.ts`:
- Around line 275-280: The truncation notice in truncateWithNotice currently
hardcodes "64 KiB" while the project uses the MAX_MATRIX_EVENT_CONTENT_BYTES
constant (set to 60 * 1024); update truncateWithNotice to reference
MAX_MATRIX_EVENT_CONTENT_BYTES (or its human-readable equivalent "60 KiB") in
the notice so the message matches the actual limit—locate truncateWithNotice and
replace the hardcoded string with a formatted message built from
MAX_MATRIX_EVENT_CONTENT_BYTES (or update the constant if the intended limit was
64 KiB) to keep the notice and limit consistent.

In `@packages/pi/src/pi-runtime.ts`:
- Around line 52-54: The subscription callback currently calls
options.onEvent(event) without handling rejections, which can cause unhandled
promise rejections; modify the callback passed to result.session.subscribe so it
handles errors from options.onEvent(event) (for example by awaiting or chaining
a .catch and logging or forwarding the error) and ensure any thrown error is
handled to avoid process instability; update the code around
result.session.subscribe and the unsubscribe handling to safely consume and
log/re-emit errors from options.onEvent.

---

Duplicate comments:
In `@packages/pi/src/pi-runtime.ts`:
- Around line 30-50: The env toggle of process.env.PICKLE_PI_OWNED_SESSION in
createHeadlessPiSession() is not serialized and can be clobbered by concurrent
calls; wrap the whole mutate–session-create–restore block (the code that sets
process.env.PICKLE_PI_OWNED_SESSION, calls pi.createAgentSession and finally
restores the var) inside a simple async mutex so only one invocation runs that
critical section at a time. Implement a lightweight global lock (e.g., a
Promise-based mutex or queue) used by createHeadlessPiSession() to acquire
before setting PICKLE_PI_OWNED_SESSION and release after the finally restore,
ensuring the restore happens in the same critical section and preventing
cross-session races when calling pi.createAgentSession and SessionManager.open.

🪄 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: aa093347-3c1e-4628-944a-75b650c97ae2

📥 Commits

Reviewing files that changed from the base of the PR and between 0224f67 and 2413365.

📒 Files selected for processing (13)

• packages/bridge/src/appservice-websocket.test.ts
• packages/bridge/src/appservice-websocket.ts
• packages/bridge/src/beeper.ts
• packages/pi/src/appservice.ts
• packages/pi/src/beeper-stream.test.ts
• packages/pi/src/beeper-stream.ts
• packages/pi/src/cli.ts
• packages/pi/src/media-store.ts
• packages/pi/src/pi-event-map.ts
• packages/pi/src/pi-notice.ts
• packages/pi/src/pi-runtime.ts
• packages/pickle/native/internal/core/appservice.go
• packages/pickle/native/internal/core/core.go

🚧 Files skipped from review as they are similar to previous changes (8)

• packages/pickle/native/internal/core/core.go
• packages/pi/src/cli.ts
• packages/bridge/src/beeper.ts
• packages/pi/src/pi-notice.ts
• packages/pi/src/media-store.ts
• packages/bridge/src/appservice-websocket.ts
• packages/pi/src/pi-event-map.ts
• packages/pickle/native/internal/core/appservice.go

📜 Review details

🔇 Additional comments (10)

packages/pi/src/beeper-stream.ts (6)

1-55: LGTM!

The interfaces and constructor are well-structured. The type definitions for BeeperStreamPublisherClient, CreateBeeperStreamPublisherOptions, and related types are clear and appropriately scoped.

---

61-96: Previous concern addressed: targetEventId reuse now works correctly.

The start() method now properly handles the case where targetEventId is provided without a cached #descriptor by fetching the existing message and extracting the stream descriptor (lines 65-73). This resolves the earlier issue where the first start() call would always create a new placeholder.

---

98-120: Previous concern addressed: accumulator mutation now happens after publish succeeds.

The publish() method correctly captures seq before the network call (line 102), then only increments #seq (line 118) and applies to the accumulator (line 119) after streams.publish() resolves successfully. This prevents double-appending on retry failures.

---

134-175: LGTM!

The finalize() method correctly handles the terminal flow: guards against double finalization, publishes the finish delta, compacts oversized content, and edits the original message with proper metadata clearing.

---

449-453: Previous concern addressed: errorText() now always returns a string.

The nullish coalescing fallback ?? String(error) ensures that even when JSON.stringify(error) returns undefined (e.g., when error is undefined), the function returns a valid string.

---

282-461: LGTM!

The helper functions are well-implemented with proper type guards, exception handling in parseMaybeJSON, and comprehensive state handling in applyFinalMessagePart for all part types.

packages/pi/src/beeper-stream.test.ts (2)

5-234: LGTM!

Comprehensive test coverage including the critical scenarios: stream creation/registration, target reuse, monotonic sequence progression, failure recovery without state corruption, finalize editing flow, terminal error/abort handling, and oversized content compaction.

---

242-289: LGTM!

The mock client factory is well-structured, providing appropriate stub implementations for all required client methods. The type assertion to MatrixClient is appropriate for test purposes.

packages/bridge/src/appservice-websocket.test.ts (2)

80-85: Good fix: deferred handleTransaction gate is now in place.

This is the right pattern to validate ACK ordering and addresses the prior immediate-resolve gap.

---

147-199: Nice coverage extension for http_proxy transactions.

Asserting handleTransaction receives both events and txn_id closes the forwarding-path regression risk.

[coderabbitai]

Actionable comments posted: 3

🤖 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/beeper-stream.ts`:
- Around line 207-210: The current logic calculates maxTextChars once and
applies it per-part via compactParts/compactAIMessage, allowing multiple text
parts to cumulatively exceed MAX_MATRIX_EVENT_CONTENT_BYTES; update compacting
to enforce a single global byte cap: compute availableBytes =
MAX_MATRIX_EVENT_CONTENT_BYTES - eventContentBytes(toolCallsOnly, "") and pass
that total budget into compactParts/compactAIMessage (or run a cumulative
truncation step after compacting) so that truncateWithNotice/compactParts
distribute or trim text across all parts collectively before finalize() returns,
referencing maxTextChars, compactParts, compactAIMessage, eventContentBytes,
truncateWithNotice, and finalize in your change.
- Around line 373-376: When a late part provides part.toolName, update the
already-created tool part rather than only setting state.toolNameByCallId: after
calling ensureToolPart(state, toolCallId) and retrieving const toolPart =
state.message.parts[index], if part.toolName is a non-empty string and toolPart
exists and still has the fallback label (e.g., name === "tool" or empty), set
toolPart.name (or the appropriate label field) to part.toolName and also keep
state.toolNameByCallId in sync; this ensures the finalized message uses the
correct tool label for the given toolCallId.
- Around line 337-340: The terminal state metadata for "error"/"abort" currently
only stores part.errorText and thus drops abort reasons; update the branch that
sets state.message.metadata/beeper_terminal_state (in beeper-stream.ts where the
switch handles case "error"/"abort") to include the abort reason when type ===
"abort" (e.g., preserve part.reason alongside part.errorText) so the finalized
beeper_terminal_state contains both errorText and reason.

🪄 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: 9abdc8c4-7848-4e04-8ef1-e25ba7bf8c6f

📥 Commits

Reviewing files that changed from the base of the PR and between 0224f67 and 29b91ee.

📒 Files selected for processing (19)

• packages/bridge/package.json
• packages/bridge/src/appservice-websocket.test.ts
• packages/bridge/src/appservice-websocket.ts
• packages/bridge/src/beeper.ts
• packages/bridge/src/node.ts
• packages/bridge/tsdown.config.ts
• packages/pi/package.json
• packages/pi/src/appservice.ts
• packages/pi/src/beeper-stream.test.ts
• packages/pi/src/beeper-stream.ts
• packages/pi/src/cli.ts
• packages/pi/src/media-store.ts
• packages/pi/src/pi-event-map.ts
• packages/pi/src/pi-notice.ts
• packages/pi/src/pi-runtime.ts
• packages/pi/tsdown.config.ts
• packages/pickle/native/internal/core/appservice.go
• packages/pickle/native/internal/core/core.go
• tsconfig.base.json

💤 Files with no reviewable changes (2)

• packages/bridge/package.json
• packages/bridge/src/node.ts

✅ Files skipped from review due to trivial changes (1)

• packages/bridge/tsdown.config.ts

🚧 Files skipped from review as they are similar to previous changes (10)

• packages/pi/src/pi-runtime.ts
• packages/pickle/native/internal/core/core.go
• packages/bridge/src/appservice-websocket.test.ts
• packages/pickle/native/internal/core/appservice.go
• packages/pi/src/cli.ts
• packages/bridge/src/appservice-websocket.ts
• packages/bridge/src/beeper.ts
• packages/pi/src/beeper-stream.test.ts
• packages/pi/src/appservice.ts
• packages/pi/src/media-store.ts

📜 Review details

🔇 Additional comments (1)

tsconfig.base.json (1)

16-16: @beeper/pickle-pi path alias addition looks good.

Line 16 cleanly maps the new package alias to the intended appservice entrypoint, and this file shows no config-level issues.

[batuhan]

@coderabbitai can you re-review

[coderabbitai]

@batuhan Sure, I'll re-review the PR now!

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

packages/pi/src/beeper-stream.ts (2)

393-396: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the existing tool part when toolName arrives late.

If the first chunk creates the fallback "tool" part, later chunks only update toolNameByCallId. The already-created part keeps the fallback label, so the finalized message can show the wrong tool name.

Suggested fix

   if (typeof part.toolName === "string" && part.toolName.trim()) state.toolNameByCallId.set(toolCallId, part.toolName);
   const index = ensureToolPart(state, toolCallId);
   const toolPart = state.message.parts[index];
   if (!toolPart) return;
+  if (typeof part.toolName === "string" && part.toolName.trim()) {
+    toolPart.toolName = part.toolName;
+  }
   if (type === "tool-input-start") {
     toolPart.state = "input-streaming";
     return;
   }

Also applies to: 424-430

🤖 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 393 - 396, The code creates a
fallback "tool" part early (via ensureToolPart) but when a later chunk provides
part.toolName the existing part isn't updated; change the logic after calling
ensureToolPart by, if part.toolName is a non-empty string, setting the existing
tool part's toolName (e.g., assign to toolPart.toolName or the part's display
label) and also update toolNameByCallId; apply the same fix in the other
occurrence referenced (around the 424-430 block) so late-arriving toolName
values overwrite the placeholder part.

---

227-230: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Enforce the byte budget across all text parts.

maxTextChars is calculated once here, but compactParts() applies that full allowance to every text part independently. A response with multiple text parts can still exceed MAX_MATRIX_EVENT_CONTENT_BYTES, so finalize() can still try to send an oversized edit after this compaction pass. Track a shared remaining budget while compacting, or add a cumulative trim step before returning.

Also applies to: 267-275

🤖 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 227 - 230, The current
compaction computes maxTextChars once and then lets
compactParts/compactAIMessage use that allowance per part, which can still
produce an oversized event; modify the compaction to enforce a shared byte/char
budget across all text parts (or add a cumulative trim pass) so the sum of parts
never exceeds MAX_MATRIX_EVENT_CONTENT_BYTES: compute the total remaining budget
from MAX_MATRIX_EVENT_CONTENT_BYTES - eventContentBytes(...), then when calling
compactAIMessage, compactParts and truncateWithNotice decrement that shared
remaining budget (or after per-part compaction run a final cumulative trim over
aiMessage and body to shave bytes until eventContentBytes(...) <=
MAX_MATRIX_EVENT_CONTENT_BYTES). Ensure changes touch the logic around
maxTextChars, compactAIMessage, compactParts, truncateWithNotice and the
finalize return path so edits cannot exceed the byte limit.

🧹 Nitpick comments (8)

packages/pi/src/pi-notice.test.ts (1)

31-36: 💤 Low value

Consider adding a test for attempt = 0.

The auto-retry tests use attempt: 2, but adding a case with attempt: 0 would verify the fix for the previous truthy-check issue and guard against regression.

expect(piEventNoticeText({ type: "auto_retry_start", attempt: 0, maxAttempts: 3 })).toBe(
  "Retry 0 of 3 started."
);

🤖 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.test.ts` around lines 31 - 36, Add a test case to
cover attempt = 0 to prevent regressions from truthy-check bugs: in
packages/pi/src/pi-notice.test.ts add an assertion calling piEventNoticeText
with { type: "auto_retry_start", attempt: 0, maxAttempts: 3 } and expect "Retry
0 of 3 started." so the piEventNoticeText function is validated for zero
attempts (this complements existing tests for attempt: 2).

packages/pi/src/stream-map.ts (1)

91-117: 💤 Low value

Minor asymmetry in preliminary field handling.

In the error branch (Lines 104-105), preliminary is conditionally spread only when defined. In the success branch (Line 111), preliminary is always included, potentially adding preliminary: undefined to the output. Consider using conditional spread consistently if undefined values should be omitted.

♻️ Consistent conditional spread

   return {
     output: event.output,
-    preliminary: event.preliminary,
     toolCallId: event.toolCallId,
     toolName: event.toolName,
     ...(event.completedAtMs !== undefined ? { completedAtMs: event.completedAtMs } : {}),
+    ...(event.preliminary !== undefined ? { preliminary: event.preliminary } : {}),
     type: "tool-output-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/pi/src/stream-map.ts` around lines 91 - 117, The mapPiToolOutput
function emits preliminary inconsistently: in the error branch it conditionally
spreads preliminary but in the success branch it always includes preliminary
(possibly as undefined); update the success branch to use the same conditional
spread pattern (like in the error branch) so preliminary is only present when
defined, keeping the BeeperUIMessageChunk shape consistent across both
"tool-output-error" and "tool-output-available" branches.

packages/pickle/src/client-types.ts (1)

121-121: ⚡ Quick win

Use the shared transaction options type to avoid drift

MatrixAppservice.applyTransaction currently inlines the payload shape while the runtime surface already defines MatrixAppserviceTransactionOptions. Reusing that type keeps API contracts synchronized across layers.

Proposed diff

 import type {
   MatrixAppserviceBatchSendOptions,
   MatrixAppserviceBatchSendResult,
   MatrixAppserviceCreateManagementRoomOptions,
   MatrixAppserviceCreatePortalRoomOptions,
   MatrixAppserviceCreateRoomOptions,
   MatrixAppserviceInfo,
   MatrixAppserviceInitOptions,
   MatrixAppserviceRoomUserOptions,
   MatrixAppserviceSendMessageOptions,
+  MatrixAppserviceTransactionOptions,
   MatrixAppserviceUserOptions,
 } from "./runtime-types";
@@
-  applyTransaction(options: { transaction: Record<string, unknown> }): Promise<void>;
+  applyTransaction(options: MatrixAppserviceTransactionOptions): Promise<void>;

🤖 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/src/client-types.ts` at line 121, The applyTransaction
declaration currently inlines the payload type; update its signature to reuse
the existing MatrixAppserviceTransactionOptions type instead of Record<string,
unknown> so the public API matches the runtime surface. Locate the
applyTransaction method declaration and replace the options parameter type with
MatrixAppserviceTransactionOptions (ensure the type is imported/available where
this interface is defined) so both compile-time and runtime types remain
synchronized.

packages/pi/src/config.ts (1)

47-51: ⚡ Quick win

Redundant chmod call after writeFile with mode.

The writeFile on line 49 already sets the file mode to 0o600. The subsequent chmod call on line 50 is redundant and can be removed.

♻️ Remove redundant chmod

 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 - 51, The writeConfig function
calls writeFile with mode 0o600 and then redundantly calls chmod; remove the
unnecessary await chmod(path, 0o600) line and rely on writeFile(path, ..., {
mode: 0o600 }) to set permissions; keep mkdir(dirname(path), { recursive: true
}) and the defaultConfigPath logic intact so the function still creates the
directory and writes the file with the intended mode.

packages/pi/src/spaces.ts (1)

41-43: 💤 Low value

localhost default may not suit all deployments.

The default domain of "localhost" works for local development but callers must remember to provide the actual homeserver domain in production. Consider whether this default could lead to subtle bugs if forgotten.

🤖 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/spaces.ts` around lines 41 - 43, The function
serviceBotUserId currently defaults domain to "localhost", which is unsafe for
production; change it to either require the caller to pass the domain (remove
the "localhost" default) or derive the domain from the provided PicklePiConfig
(e.g., use a config.homeserver/config.domain property and throw or log if
missing). Update the serviceBotUserId signature and its callers to ensure a real
homeserver domain is supplied and avoid silently returning @...:localhost.

packages/pickle/native/internal/core/init.go (2)

315-345: ⚡ Quick win

Duplicated DecryptErrorCallback assignment.

The DecryptErrorCallback closure (lines 322-338) is identical to the one assigned earlier (lines 298-314). Consider extracting this callback into a named function or variable to avoid duplication.

♻️ Suggested refactor to extract callback

+	decryptErrorCallback := 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,
+		})
+	}
-	helper.DecryptErrorCallback = 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,
-		})
-	}
+	helper.DecryptErrorCallback = decryptErrorCallback
 	if err := helper.Init(ctx); err != nil {
 		if req.Appservice != nil && isMissingServerKeysError(err) {
 			// ... reset logic ...
 			helper, err = cryptohelper.NewCryptoHelper(cli, c.pickleKey, c.cryptoStore)
 			if err == nil {
-				helper.DecryptErrorCallback = func(evt *event.Event, err error) {
-					// ... duplicate callback ...
-				}
+				helper.DecryptErrorCallback = decryptErrorCallback
 				err = helper.Init(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 315 - 345, The
DecryptErrorCallback closure is duplicated when initializing the crypto helper;
extract the closure into a single reusable function or variable (e.g., a
function named handleDecryptError or a var decryptErrorCallback) and assign that
function to helper.DecryptErrorCallback in both places (the initial helper setup
and after NewCryptoHelper in the reset branch). The extracted handler should
call c.rememberPendingDecryption(ctx, evt), use
c.retryPendingDecryptionEvent(ctx, evt) to short-circuit, build the same
OutboundEvent map with evt.ID/RoomID/Sender when evt != nil, and call
c.emit(...) with the same "decryption_error" payload so behavior remains
identical.

---

382-392: 💤 Low value

isMissingServerKeysError uses substring matching.

The error detection relies on a specific error message substring. This is fragile if the upstream library changes its error messages. Consider adding a comment noting this dependency on the mautrix library's error format.

🤖 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 382 - 392, The
isMissingServerKeysError function relies on substring matching of the upstream
mautrix error message, which is fragile; update the code by adding a clear
comment above isMissingServerKeysError explaining that this function depends on
the exact error text produced by the mautrix library (and may break if that
message changes), mention why we use substring matching here and note TODO to
replace with a stable sentinel/error type or errors.Is when upstream exposes
one; keep the resetCryptoStore logic unchanged but reference
isMissingServerKeysError in the comment for context.

packages/pi/src/rooms.ts (1)

57-59: 💤 Low value

Consider validating ghostLocalpart is defined.

If config.ghostLocalpart is undefined or empty, this will produce an invalid MXID like @:localhost. Consider adding validation or a fallback.

♻️ Proposed defensive check

 export function piGhostUserId(config: PicklePiConfig, domain = "localhost"): string {
+  if (!config.ghostLocalpart) throw new Error("ghostLocalpart is required in config");
   return `@${config.ghostLocalpart}:${domain}`;
 }

🤖 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/rooms.ts` around lines 57 - 59, piGhostUserId currently
interpolates config.ghostLocalpart without validation which can yield invalid
MXIDs like "@:localhost"; update the function (piGhostUserId) to check that
config.ghostLocalpart is a non-empty string and either throw a clear error or
use a safe fallback (e.g. a default localpart) when it's missing/empty, and
ensure the returned value is always a valid MXID format.

🤖 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/approval.ts`:
- Around line 106-122: The switch in approvalDecisionValue currently normalizes
hyphenated inputs for several decisions but misses "allow-always"; add a case
for "allow-always" that returns "allow_always" so the function maps the
hyphenated form to the underscore variant (keep the function signature and other
cases intact and return the ApprovalDecision string as with the other mappings).

In `@packages/pi/src/media-store.ts`:
- Around line 97-99: mediaIdFromUrl currently calls safeMediaId and can let it
throw for malformed inputs (e.g., "media://local/" or when the last path segment
sanitizes to an invalid ID) despite returning string | undefined; update
mediaIdFromUrl to treat any malformed or invalid last-segment as undefined by
first extracting the last segment, returning undefined if it's empty, and then
calling safeMediaId inside a try/catch (or checking its validation result) so
that any error or invalid output from safeMediaId results in undefined rather
than bubbling an exception; reference mediaIdFromUrl and safeMediaId when making
the change.
- Around line 127-133: The path containment check in assertInside uses a
hardcoded '/' which fails on Windows; import sep from 'node:path' and replace
the startsWith check to use `${resolvedRoot}${sep}` (i.e., test resolvedTarget
!== resolvedRoot && !resolvedTarget.startsWith(`${resolvedRoot}${sep}`)) so the
function uses the platform-specific path separator when comparing resolvedRoot
and resolvedTarget; keep returning resolvedTarget and preserve the existing
error message when the check fails.

---

Duplicate comments:
In `@packages/pi/src/beeper-stream.ts`:
- Around line 393-396: The code creates a fallback "tool" part early (via
ensureToolPart) but when a later chunk provides part.toolName the existing part
isn't updated; change the logic after calling ensureToolPart by, if
part.toolName is a non-empty string, setting the existing tool part's toolName
(e.g., assign to toolPart.toolName or the part's display label) and also update
toolNameByCallId; apply the same fix in the other occurrence referenced (around
the 424-430 block) so late-arriving toolName values overwrite the placeholder
part.
- Around line 227-230: The current compaction computes maxTextChars once and
then lets compactParts/compactAIMessage use that allowance per part, which can
still produce an oversized event; modify the compaction to enforce a shared
byte/char budget across all text parts (or add a cumulative trim pass) so the
sum of parts never exceeds MAX_MATRIX_EVENT_CONTENT_BYTES: compute the total
remaining budget from MAX_MATRIX_EVENT_CONTENT_BYTES - eventContentBytes(...),
then when calling compactAIMessage, compactParts and truncateWithNotice
decrement that shared remaining budget (or after per-part compaction run a final
cumulative trim over aiMessage and body to shave bytes until
eventContentBytes(...) <= MAX_MATRIX_EVENT_CONTENT_BYTES). Ensure changes touch
the logic around maxTextChars, compactAIMessage, compactParts,
truncateWithNotice and the finalize return path so edits cannot exceed the byte
limit.

---

Nitpick comments:
In `@packages/pi/src/config.ts`:
- Around line 47-51: The writeConfig function calls writeFile with mode 0o600
and then redundantly calls chmod; remove the unnecessary await chmod(path,
0o600) line and rely on writeFile(path, ..., { mode: 0o600 }) to set
permissions; keep mkdir(dirname(path), { recursive: true }) and the
defaultConfigPath logic intact so the function still creates the directory and
writes the file with the intended mode.

In `@packages/pi/src/pi-notice.test.ts`:
- Around line 31-36: Add a test case to cover attempt = 0 to prevent regressions
from truthy-check bugs: in packages/pi/src/pi-notice.test.ts add an assertion
calling piEventNoticeText with { type: "auto_retry_start", attempt: 0,
maxAttempts: 3 } and expect "Retry 0 of 3 started." so the piEventNoticeText
function is validated for zero attempts (this complements existing tests for
attempt: 2).

In `@packages/pi/src/rooms.ts`:
- Around line 57-59: piGhostUserId currently interpolates config.ghostLocalpart
without validation which can yield invalid MXIDs like "@:localhost"; update the
function (piGhostUserId) to check that config.ghostLocalpart is a non-empty
string and either throw a clear error or use a safe fallback (e.g. a default
localpart) when it's missing/empty, and ensure the returned value is always a
valid MXID format.

In `@packages/pi/src/spaces.ts`:
- Around line 41-43: The function serviceBotUserId currently defaults domain to
"localhost", which is unsafe for production; change it to either require the
caller to pass the domain (remove the "localhost" default) or derive the domain
from the provided PicklePiConfig (e.g., use a config.homeserver/config.domain
property and throw or log if missing). Update the serviceBotUserId signature and
its callers to ensure a real homeserver domain is supplied and avoid silently
returning @...:localhost.

In `@packages/pi/src/stream-map.ts`:
- Around line 91-117: The mapPiToolOutput function emits preliminary
inconsistently: in the error branch it conditionally spreads preliminary but in
the success branch it always includes preliminary (possibly as undefined);
update the success branch to use the same conditional spread pattern (like in
the error branch) so preliminary is only present when defined, keeping the
BeeperUIMessageChunk shape consistent across both "tool-output-error" and
"tool-output-available" branches.

In `@packages/pickle/native/internal/core/init.go`:
- Around line 315-345: The DecryptErrorCallback closure is duplicated when
initializing the crypto helper; extract the closure into a single reusable
function or variable (e.g., a function named handleDecryptError or a var
decryptErrorCallback) and assign that function to helper.DecryptErrorCallback in
both places (the initial helper setup and after NewCryptoHelper in the reset
branch). The extracted handler should call c.rememberPendingDecryption(ctx,
evt), use c.retryPendingDecryptionEvent(ctx, evt) to short-circuit, build the
same OutboundEvent map with evt.ID/RoomID/Sender when evt != nil, and call
c.emit(...) with the same "decryption_error" payload so behavior remains
identical.
- Around line 382-392: The isMissingServerKeysError function relies on substring
matching of the upstream mautrix error message, which is fragile; update the
code by adding a clear comment above isMissingServerKeysError explaining that
this function depends on the exact error text produced by the mautrix library
(and may break if that message changes), mention why we use substring matching
here and note TODO to replace with a stable sentinel/error type or errors.Is
when upstream exposes one; keep the resetCryptoStore logic unchanged but
reference isMissingServerKeysError in the comment for context.

In `@packages/pickle/src/client-types.ts`:
- Line 121: The applyTransaction declaration currently inlines the payload type;
update its signature to reuse the existing MatrixAppserviceTransactionOptions
type instead of Record<string, unknown> so the public API matches the runtime
surface. Locate the applyTransaction method declaration and replace the options
parameter type with MatrixAppserviceTransactionOptions (ensure the type is
imported/available where this interface is defined) so both compile-time and
runtime types remain synchronized.

🪄 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: 71e6f42a-eb04-4c03-b785-37e1227ba350

📥 Commits

Reviewing files that changed from the base of the PR and between e39c4f7 and fc75bd6.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (60)

• packages/bridge/package.json
• packages/bridge/src/appservice-websocket.test.ts
• packages/bridge/src/appservice-websocket.ts
• packages/bridge/src/beeper.test.ts
• packages/bridge/src/beeper.ts
• packages/bridge/src/bridge.ts
• packages/bridge/src/index.ts
• packages/bridge/src/node.ts
• packages/bridge/src/provisioning.test.ts
• packages/bridge/src/provisioning.ts
• packages/bridge/src/store.ts
• packages/bridge/src/types.ts
• packages/bridge/tsdown.config.ts
• packages/pi/@beeper-pickle-pi.TODO.md
• packages/pi/README.md
• packages/pi/package.json
• packages/pi/src/approval.test.ts
• packages/pi/src/approval.ts
• packages/pi/src/appservice.test.ts
• packages/pi/src/appservice.ts
• packages/pi/src/beeper-stream.test.ts
• packages/pi/src/beeper-stream.ts
• packages/pi/src/cli.ts
• packages/pi/src/config.ts
• packages/pi/src/matrix.ts
• packages/pi/src/media-store.test.ts
• packages/pi/src/media-store.ts
• packages/pi/src/pi-event-map.test.ts
• packages/pi/src/pi-event-map.ts
• packages/pi/src/pi-notice.test.ts
• packages/pi/src/pi-notice.ts
• packages/pi/src/pi-runtime.ts
• packages/pi/src/queue.test.ts
• packages/pi/src/queue.ts
• packages/pi/src/registration.ts
• packages/pi/src/registry.test.ts
• packages/pi/src/registry.ts
• packages/pi/src/rooms.test.ts
• packages/pi/src/rooms.ts
• packages/pi/src/serial.ts
• packages/pi/src/spaces.test.ts
• packages/pi/src/spaces.ts
• packages/pi/src/stream-map.test.ts
• packages/pi/src/stream-map.ts
• packages/pi/src/types.ts
• packages/pi/tsconfig.json
• packages/pi/tsdown.config.ts
• packages/pickle/native/internal/core/appservice.go
• packages/pickle/native/internal/core/core.go
• packages/pickle/native/internal/core/init.go
• packages/pickle/native/internal/core/operations.go
• packages/pickle/native/internal/core/persistent_crypto_methods.go
• packages/pickle/src/client-types.ts
• packages/pickle/src/client.ts
• packages/pickle/src/generated-runtime-operations.ts
• packages/pickle/src/generated-runtime-types.ts
• packages/pickle/src/streams/beeper.ts
• packages/pickle/src/types.ts
• pnpm-workspace.yaml
• tsconfig.base.json

💤 Files with no reviewable changes (2)

• packages/bridge/src/node.ts
• packages/bridge/package.json

📜 Review details

🧰 Additional context used

🪛 LanguageTool

packages/pi/README.md

[grammar] ~5-~5: Ensure spelling is correct
Context: ...bile. The day-one target is a headless appservice agent: - one Matrix/Beeper room per Pi...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

packages/pi/@beeper-pickle-pi.TODO.md

[grammar] ~5-~5: Ensure spelling is correct
Context: ...bile. The day-one product is a headless appservice agent with a Pi ghost/puppet that auto-...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~187-~187: Ensure spelling is correct
Context: ...er/src/streaming/homeserver.ts` Bridge/appservice references, possibly useful later for b...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~187-~187: Ensure spelling is correct
Context: ...s, possibly useful later for bridge bot/appservice mode: - `/Users/batuhan/Projects/labs/...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~215-~215: The adverb ‘always’ is usually put before the verb ‘allow’.
Context: ...mmon/ai-common.ts:56approval reaction allow always -/Users/batuhan/projects/texts/beeper...

(ADVERB_WORD_ORDER)

---

[grammar] ~238-~238: Ensure spelling is correct
Context: ...enied` ## Best architecture Implement appservice-first architecture with a headless brid...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~238-~238: Ensure spelling is correct
Context: ...d code must live in a core layer so the appservice agent and future Pi extension use the s...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~259-~259: Ensure spelling is correct
Context: .../src/thread-session.ts. - Use a bridge/appservice package entry such as @beeper/pickle-p...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~264-~264: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...d })thenresourceLoader.reload(). - Use createAgentSession({ cwd, sessionManag...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[grammar] ~271-~271: Ensure spelling is correct
Context: ...extension This is not the MVP, but the appservice data model must support it from day one...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~278-~278: Ensure spelling is correct
Context: ...rrored to Beeper. - Ensure terminal and appservice do not concurrently write the same Pi s...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~293-~293: Ensure spelling is correct
Context: ...trix appservice registration. - Receive appservice transactions or websocket appservice ev...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~293-~293: Ensure spelling is correct
Context: ...ve appservice transactions or websocket appservice events. - Manage ghost user intent for ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~409-~409: Ensure spelling is correct
Context: ...er namespace. - Final architecture uses appservice transaction delivery/dedupe, not a user...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~417-~417: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...eper. - Terminal /resume is synced. - Terminal /new creates or links a Beeper room. ...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~418-~418: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../newcreates or links a Beeper room. - Terminal/forkand/tree` navigation produce ...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~419-~419: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...entually branch/session-room mapping. - Terminal model/thinking changes appear in Beeper...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[grammar] ~423-~423: Ensure spelling is correct
Context: ...future terminal-extension work, not the appservice MVP. - On session_start, create or lo...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~426-~426: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ate a Beeper room and import history. - On `session_before_switch(reason: 'resume'...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~427-~427: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...esume'), record targetSessionFile. - On session_start(reason: 'resume')`, swit...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~428-~428: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...continue mirroring in the bound room. - On session_before_fork/session_tree, c...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[grammar] ~436-~436: Ensure spelling is correct
Context: ...ssion. - Many Beeper rooms can run many appservice-owned Pi sessions. - Appservice-owned s...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~453-~453: Ensure spelling is correct
Context: ...headless AgentSession mode for Beeper/appservice-owned sessions. ### 6. Inbound Beeper ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~516-~516: The adverb ‘always’ is usually put before the verb ‘approve’.
Context: ...eeper Desktop. - Let user approve once, approve always, or deny from Beeper Desktop. Implemen...

(ADVERB_WORD_ORDER)

---

[style] ~543-~543: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e and attach it to the project Space. - On session_tree, post notice with `oldLe...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~624-~624: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...com.beeper.ai messages if possible. - Serialize tool calls/results into `com.beeper.ai....

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[grammar] ~668-~668: Ensure spelling is correct
Context: ...e approval. Implementation details: - Config allow-list by Matrix user ID and room I...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~679-~679: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Unit tests for registry persistence. - Unit tests for queue dispatch ordering. - Un...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~680-~680: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...it tests for queue dispatch ordering. - Unit tests for Desktop-compatible stream seq...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~681-~681: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...-compatible stream sequence ordering. - Unit tests for approval request/response flo...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[grammar] ~731-~731: Ensure spelling is correct
Context: ...ile. ### Phase 1: package skeleton and appservice registration - Create package split or...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~742-~742: Ensure spelling is correct
Context: ...t, - pickle-pi-agent status`. - Add appservice registration generation with Pi ghost n...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[style] ~743-~743: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...n generation with Pi ghost namespace. - Add src/config.ts, src/registry.ts, `sr...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[grammar] ~744-~744: Ensure spelling is correct
Context: ...stream-map.ts`. - Add README for Beeper appservice setup. ### Phase 2: appservice/headles...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~748-~748: Ensure spelling is correct
Context: ...ice/headless session MVP - Boot proper appservice. - Create/join/send as Pi ghost. - Auto...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~769-~769: Ensure spelling is correct
Context: ...a for full tree support later. - Ensure appservice restart resumes existing room/session b...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

---

[grammar] ~815-~815: Ensure spelling is correct
Context: ...**. - Design package around a bridge/appservice-style identity rather than the user's p...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🪛 OpenGrep (1.20.0)

packages/bridge/src/provisioning.ts

[ERROR] 202-202: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.

(coderabbit.command-injection.exec-js)

🔇 Additional comments (65)

packages/bridge/tsdown.config.ts (1)

4-4: Build entry update is consistent with the package surface change.

Removing src/node.ts from entry looks correct given the node subpath export removal, and the rest of the build config remains coherent.

pnpm-workspace.yaml (1)

7-7: AI summary is incorrect: packages/pi was added, not removed.

The summary claims the workspace entry was removed, but the ~ annotation on line 7 indicates it was added. This is consistent with the PR introducing the new packages/pi package.

packages/pickle/native/internal/core/persistent_crypto_methods.go (1)

25-34: LGTM!

The reset method correctly acquires the lock, replaces the in-memory crypto store with a fresh instance, reinitializes the auxiliary maps, and cleans up persisted state. Using context.Background() in the save callback is appropriate since that callback may be invoked outside the reset operation's lifecycle.

packages/pi/src/serial.ts (1)

1-9: LGTM!

Clean serial queue implementation. The pattern of running the operation on both resolve and reject paths ensures proper sequencing, while swallowing errors on the tail preserves queue continuity. Returning next before the error-swallowing transform correctly preserves the operation's result for callers.

packages/bridge/src/store.ts (1)

153-160: LGTM!

The helper correctly persists and retrieves the device ID. The use of Math.random() is acceptable for device ID generation since cryptographic randomness isn't required—just sufficient uniqueness. The theoretical TOCTOU between get and set is acceptable given this runs at startup by a single process.

packages/pi/README.md (1)

1-37: LGTM!

Clear and well-structured README covering the CLI workflow, configuration, and current phase-1 scope. The environment variable documentation will help automation scenarios.

packages/pi/src/pi-notice.ts (2)

37-50: Previous review concern addressed.

The retry label logic now correctly uses !== undefined checks (lines 40 and 46), preserving valid 0 values for attempt and maxAttempts.

---

1-85: LGTM!

The notice text generation is well-structured with proper type guards, clean helper functions, and comprehensive coverage of Pi event types. The stringValue helper's empty-string filtering (line 79) is intentional for the notice use case.

packages/pi/@beeper-pickle-pi.TODO.md (1)

1-856: LGTM as a design/planning document.

Comprehensive specification covering architecture, data models, feature requirements, and phased implementation. The local filesystem paths (e.g., /Users/batuhan/...) are developer-specific references that are fine for internal planning but should be removed or generalized if this document is retained long-term.

packages/pickle/src/streams/beeper.ts (4)

41-54: Concurrent publishing with fire-and-forget error handling.

The publish mechanism silently logs and swallows failures without propagating them. If multiple publishes fail, the final message edit will still proceed, potentially leaving the stream in an inconsistent state where some parts were never delivered. Consider whether failed publishes should abort the stream or be tracked for error reporting.

---

366-407: LGTM!

The normalizeRichStreamChunk function cleanly handles multiple input formats (assistant-message-event, agent-message-update, agent-message-end, tool-call, tool-result, tool-execution-*) and correctly delegates to specialized mapping functions. The early return for native stream parts prevents double-processing.

---

318-319: LGTM!

The timing field propagation (startedAtMs for tool input, completedAtMs for tool output) is correctly implemented with conditional assignment, preserving the fields only when present in the source part.

Also applies to: 345-346

---

680-689: LGTM!

The isStreamPart refactor correctly delegates to isNativeStreamPartRecord for type classification, ensuring consistent behavior between the streaming loop's raw part handling and the new rich chunk normalization path.

packages/pi/src/pi-runtime.ts (3)

59-78: LGTM!

The withOwnedSessionEnv helper correctly serializes environment variable mutations using a promise-based lock pattern. The previous value is properly saved and restored in the finally block, handling both the undefined case (delete) and existing value case (restore).

---

45-49: LGTM!

The event subscription properly handles rejected promises by catching errors and logging them with context (bindingId), preventing unhandled promise rejections from destabilizing the process.

---

86-88: Dynamic import via new Function bypasses static analysis.

This pattern intentionally prevents bundlers from resolving @earendil-works/pi-coding-agent at build time. While functional, it may trigger security linter warnings about new Function usage. Document this intent if not already covered elsewhere.

packages/pi/src/types.ts (1)

1-101: LGTM!

The type definitions are well-structured with appropriate use of optional fields, literal types for discriminants (schemaVersion: 1, priority union), and clear separation between binding metadata, registry data, configuration, and registration shapes.

packages/pi/src/approval.ts (1)

33-74: LGTM!

The approval parsing logic correctly handles the various input shapes and normalizes them to a consistent ParsedApprovalResponse. The defensive override of decision to "deny" when approved is false (Line 67) ensures consistent state even with malformed input.

packages/pi/src/approval.test.ts (1)

1-128: LGTM!

The test suite provides good coverage for approval parsing, including reaction key mapping, Matrix content extraction, tool approval chunk normalization, stream-like delta parsing, and malformed input rejection. The test at Lines 83-91 correctly verifies that approved: false overrides approvedAlways: true to produce decision: "deny".

packages/pi/src/queue.ts (1)

18-127: LGTM!

The MatrixInboundTurnQueue implementation correctly maintains priority ordering (control → priority → default) with FIFO semantics within each priority level. The dispatch gating via canDispatch predicate only checks the current head, enabling the intended "head-stop" behavior where the queue blocks until the head becomes dispatchable or is cancelled.

packages/pi/src/queue.test.ts (1)

1-99: LGTM!

The test suite thoroughly validates the queue's priority ordering, FIFO semantics, cancellation, text updates, and the critical head-stop dispatch gating behavior. The test at Lines 75-85 correctly verifies that drainDispatchable stops when encountering a non-dispatchable head even when subsequent items would pass the predicate.

packages/pi/src/stream-map.ts (2)

34-42: LGTM!

The mapPiMessageDelta implementation correctly addresses the previous review concern about duplicate *-start emissions. The openTextPart/openReasoningPart helpers return an empty array when the part is already open, ensuring start chunks are emitted only once per part lifecycle.

---

119-132: LGTM!

The approval mapping functions correctly track the toolCallId → approvalId relationship in the stream state and produce well-formed request/response chunks with all required fields.

Also applies to: 134-147

packages/pickle/native/internal/core/operations.go (1)

36-37: Operation constant wiring looks correct

The new appservice_apply_transaction operation declaration is consistent with the existing operation annotation pattern.

packages/bridge/src/beeper.test.ts (1)

44-44: Good fixture update for ephemeral support

Including receive_ephemeral in both mocked registration flows keeps these tests aligned with the updated appservice registration payloads.

Also applies to: 97-97

packages/pickle/src/generated-runtime-operations.ts (1)

15-15: Generated operation plumbing is consistent

The new appserviceApplyTransaction method is correctly added to both the interface and operation caller with the matching operation key.

Also applies to: 107-107, 227-229

packages/pi/tsdown.config.ts (1)

1-8: Build config looks good

The config is clear and complete for ESM + declaration output with explicit entrypoints and clean builds.

packages/pickle/src/client.ts (1)

82-82: Client wiring for appservice transaction + init options is correct

Delegation to core.appserviceApplyTransaction and forwarding appservice/deviceId through init are integrated cleanly.

Also applies to: 333-334

packages/pickle/native/internal/core/core.go (1)

103-104: Dispatcher + cleanup updates look solid

Adding the apply-transaction dispatch case and clearing appserviceProcessor during close keeps the new appservice path lifecycle-safe.

Also applies to: 259-259

packages/pickle/src/types.ts (1)

1-1: Type surface extension is coherent

The new MatrixClientOptions fields line up with the client boot path and appservice initialization flow.

Also applies to: 16-16, 19-19

packages/pi/src/matrix.ts (1)

6-17: LGTM!

The factory function correctly validates required configuration, creates file-based storage, and conditionally forwards optional encryption keys. The error messages provide helpful context for missing configuration.

packages/bridge/src/appservice-websocket.test.ts (2)

74-142: LGTM!

The test effectively validates that the websocket ACK is blocked until handleTransaction completes by using a deferred promise pattern. The Promise.race approach avoids flaky timing issues while still asserting proper sequencing.

---

144-204: LGTM!

The test correctly validates that HTTP-proxied appservice transactions are forwarded to the handleTransaction callback alongside the dispatch path.

packages/bridge/src/provisioning.test.ts (1)

1-70: LGTM!

The test suite provides solid coverage of the provisioning HTTP proxy handler, validating endpoint responses and the identifier resolution flow with appropriate mocks and assertions.

packages/pi/src/cli.ts (2)

7-35: LGTM!

The CLI command routing is clean and correctly handles all cases. The exit code for unknown commands has been properly addressed.

---

37-40: LGTM!

The top-level error handler appropriately catches rejections and signals failure.

packages/pi/src/spaces.test.ts (1)

1-90: LGTM!

The test suite provides excellent coverage of space helper utilities with appropriate use of fake timers, comprehensive assertions, and clean test structure.

packages/pi/src/appservice.test.ts (1)

1-134: LGTM!

The test effectively validates the PicklePiAgent streaming integration by mocking the Pi runtime, simulating the message lifecycle, and asserting that stream events are published in the correct order before the final edit.

packages/pi/src/registry.test.ts (1)

1-82: LGTM!

The test suite thoroughly validates registry persistence and hierarchical binding relationships with proper isolation using temporary directories.

packages/pi/src/rooms.test.ts (1)

1-114: LGTM!

The test suite provides comprehensive coverage of room helper utilities with proper use of fake timers, detailed assertions validating all derived fields, and appropriate test structure.

packages/pickle/src/generated-runtime-types.ts (1)

1-149: LGTM!

The type changes are consistent with the broader PR objectives: adding initialState for portal room creation, introducing MatrixAppserviceTransactionOptions for transaction forwarding, and extending MatrixCoreInitOptions with optional appservice configuration.

packages/bridge/src/index.ts (1)

21-54: LGTM!

The refactored createBeeperBridge correctly initializes the appservice, assembles the matrix config with proper fallbacks for homeserver and token, and constructs the RuntimeBridge with the appropriate beeper metadata. The getOrCreateAppserviceDeviceId call is properly awaited.

packages/bridge/src/appservice-websocket.ts (1)

349-360: Index signature placement note.

The index signature [key: string]: unknown is placed before the specific properties. While TypeScript allows this, the conventional style places it after explicit properties for readability. This is a minor style preference and doesn't affect functionality.

packages/pi/src/spaces.ts (1)

4-6: Consider documenting the project key encoding scheme.

The base64url encoding of the full cwd path produces a stable, filesystem-safe key. However, very long paths could produce unwieldy keys. This is likely acceptable for the intended use case, but a brief comment documenting this design choice would help future maintainers.

packages/pi/src/registration.ts (1)

13-30: LGTM!

The registration generation correctly builds namespaces with properly escaped regex patterns. The domainSuffix fallback to .* when no domain is specified allows flexible local development, and writeRegistration uses appropriate permissions (0o600) for the sensitive token file.

packages/bridge/src/types.ts (1)

86-92: LGTM!

The type additions are well-structured:

• HTTPProxyHandlingBridgeConnector correctly extends BridgeConnector with an optional HTTP proxy hook
• getOwnUserId() returning UserID | null handles the uninitialized state appropriately
• Extended BridgeMatrixConfig and BridgeCreatePortalRoomOptions align with the appservice initialization changes

Also applies to: 527-527, 576-576, 649-649

packages/pickle/native/internal/core/init.go (1)

118-166: LGTM!

The initClient refactor cleanly separates appservice and access-token login paths. The appservice flow properly validates login flow support before attempting login, and the access-token path correctly optimizes by skipping Whoami when credentials are already provided.

packages/pi/src/config.ts (1)

19-40: LGTM!

The config factory correctly implements a layered configuration approach: explicit overrides take precedence over environment variables, which take precedence over defaults. The optional fields (homeserver, tokens, allowlists) are only set when values are provided.

packages/bridge/src/provisioning.ts (3)

201-207: Static analysis false positive - ignore.

The OpenGrep hint incorrectly flags this line for command injection, but this is regex.exec() for URL path matching, not child_process.exec(). No action needed.

---

30-81: LGTM on provisioning HTTP handler routing.

The handler correctly routes Matrix Provisioning v3 endpoints with proper method/path matching, URL parameter extraction via the match helper, and appropriate error responses for missing logins.

---

93-119: LGTM on login step submission.

The step handler correctly:

• Validates login existence and step ID/type match
• Dispatches to the appropriate process method based on step type
• Handles completion by cleaning up state and loading the user login
• Updates nextStep for multi-step flows

packages/bridge/src/bridge.ts (3)

91-109: LGTM on bridge initialization refactoring.

The createBeeperBridge function correctly ensures an initialized appservice when not supplied, derives the Matrix runtime config from the appservice token and deviceId, and constructs RuntimeBridge using the shared helper. The null-coalescing chains properly handle optional config values.

---

784-786: LGTM on transaction forwarding.

The #handleAppserviceTransaction correctly forwards appservice transactions to the Matrix client's applyTransaction method. This integrates well with the websocket handler wiring at line 777.

---

788-805: LGTM on HTTP proxy handling refactor.

The delegation pattern is clean: first check if the connector handles the request, normalize the response, then fall back to the shared provisioning handler. The callbacks properly wire capabilities, login flows, and identifier resolution.

packages/pickle/native/internal/core/appservice.go (3)

157-176: LGTM on the event processor implementation.

The beeperStreamEventProcessor correctly implements a simple registry pattern with On for handler registration and Dispatch for event delivery. The nil guards in Dispatch are appropriate.

---

209-227: Addressed: Now correctly returns error when processor unavailable.

The handler properly returns an error "appservice transaction pipeline unavailable" when c.appserviceProcessor == nil, preventing silent acknowledgment of unprocessed transactions. The transaction parsing and dispatch logic is sound.

---

360-367: LGTM on initial state forwarding.

The loop correctly converts each MatrixRoomStateInput entry into a *event.Event with the appropriate type, state key, and raw content, then appends it to the room creation request's InitialState before adding the bridge-related state events.

packages/pi/package.json (1)

1-80: LGTM on package manifest.

The package configuration is well-structured with proper ESM setup, correctly defined subpath exports with both types and import conditions, appropriate workspace dependencies, and consistent engine requirements.

packages/pi/src/rooms.ts (1)

14-55: LGTM on session room creation.

The createSessionRoom function correctly:

• Creates a private appservice room with appropriate defaults
• Derives a deterministic binding ID from the room ID
• Constructs a complete PicklePiBinding with all required fields
• Conditionally adds optional metadata fields

packages/pi/src/appservice.ts (4)

45-55: LGTM on agent startup.

The start() method correctly:

• Loads the registry
• Lazily imports and creates the Matrix client if not provided
• Boots the client and subscribes to message/reaction events

---

86-97: Addressed: Error handling for prompt() failures.

The try/catch block properly catches prompt errors, logs with context (binding ID, error, text), and sends a user-facing notice. This prevents event handler crashes.

---

164-186: Addressed: Race condition guard with in-flight map.

The #ensureHeadlessSession now correctly uses #sessionPromises to prevent duplicate session creation. Concurrent calls for the same binding will await the same promise, and the finally block ensures cleanup on both success and failure.

---

224-283: LGTM on PiStreamRun implementation.

The class correctly:

• Uses a SerialQueue to serialize chunk handling
• Manages stream lifecycle with #closed flag
• Handles all terminal chunk types (finish, error, abort) appropriately
• Cleans up state when the stream closes

packages/pi/src/registry.ts (2)

35-40: LGTM on atomic save implementation.

The save method correctly uses the PID-suffixed temp file pattern with atomic rename to prevent corruption from concurrent writes or crashes. The 0o600 mode appropriately restricts file permissions.

---

110-119: LGTM on registry normalization.

The normalizeRegistry function safely handles malformed input by providing sensible defaults for each field, ensuring the registry always has a valid schema shape regardless of persisted content.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Missing hyphenated "allow-always" normalization.

The approvalDecisionValue function normalizes "allow-once", "allow-session", and "allow-room" to their underscore equivalents, but "allow-always" is not mapped. If the hyphenated form can appear in input, it will return undefined instead of "allow_always".

🔧 Proposed fix

     case "allow-room":
       return "allow_room";
+    case "allow-always":
+      return "allow_always";
     default:
       return undefined;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	function approvalDecisionValue(value: unknown): ApprovalDecision | undefined {
	switch (value) {
	case "allow_once":
	case "allow_always":
	case "allow_session":
	case "allow_room":
	case "deny":
	return value;
	case "allow-once":
	return "allow_once";
	case "allow-session":
	return "allow_session";
	case "allow-room":
	return "allow_room";
	default:
	return undefined;
	}
	function approvalDecisionValue(value: unknown): ApprovalDecision | undefined {
	switch (value) {
	case "allow_once":
	case "allow_always":
	case "allow_session":
	case "allow_room":
	case "deny":
	return value;
	case "allow-once":
	return "allow_once";
	case "allow-session":
	return "allow_session";
	case "allow-room":
	return "allow_room";
	case "allow-always":
	return "allow_always";
	default:
	return 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/pi/src/approval.ts` around lines 106 - 122, The switch in
approvalDecisionValue currently normalizes hyphenated inputs for several
decisions but misses "allow-always"; add a case for "allow-always" that returns
"allow_always" so the function maps the hyphenated form to the underscore
variant (keep the function signature and other cases intact and return the
ApprovalDecision string as with the other mappings).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Return undefined for malformed URLs instead of throwing.

Lines 98-99 promise a parse-style helper, but media://local/ or any URL whose last segment sanitizes to an invalid ID still throws via safeMediaId. That is a surprising runtime edge case for an API typed as string | undefined.

Suggested fix

 export function mediaIdFromUrl(value: string): string | undefined {
   const index = value.lastIndexOf("/");
-  return index === -1 ? undefined : safeMediaId(value.slice(index + 1));
+  if (index === -1) return undefined;
+  const id = value.slice(index + 1);
+  if (!id) return undefined;
+  try {
+    return safeMediaId(id);
+  } catch {
+    return undefined;
+  }
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	export function mediaIdFromUrl(value: string): string | undefined {
	const index = value.lastIndexOf("/");
	return index === -1 ? undefined : safeMediaId(value.slice(index + 1));
	export function mediaIdFromUrl(value: string): string | undefined {
	const index = value.lastIndexOf("/");
	if (index === -1) return undefined;
	const id = value.slice(index + 1);
	if (!id) return undefined;
	try {
	return safeMediaId(id);
	} catch {
	return 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/pi/src/media-store.ts` around lines 97 - 99, mediaIdFromUrl
currently calls safeMediaId and can let it throw for malformed inputs (e.g.,
"media://local/" or when the last path segment sanitizes to an invalid ID)
despite returning string | undefined; update mediaIdFromUrl to treat any
malformed or invalid last-segment as undefined by first extracting the last
segment, returning undefined if it's empty, and then calling safeMediaId inside
a try/catch (or checking its validation result) so that any error or invalid
output from safeMediaId results in undefined rather than bubbling an exception;
reference mediaIdFromUrl and safeMediaId when making the change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm the current implementation hardcodes a POSIX separator, then
# demonstrate why that condition fails for Windows-style resolved paths.
rg -n -C2 'resolvedTarget\.startsWith\(`\$\{resolvedRoot\}/`\)' packages/pi/src/media-store.ts

python - <<'PY'
root = r"C:\media"
target = r"C:\media\file"
print("condition using '/':", target.startswith(f"{root}/"))
print("expected child path:", target)
PY

Repository: beeper/pickle

Length of output: 377

---

🏁 Script executed:

# Check if 'sep' is already imported in media-store.ts
rg -n "import.*sep" packages/pi/src/media-store.ts

# Check all imports in the file
head -20 packages/pi/src/media-store.ts

# Look for test files related to media-store
fd -e test.ts -e spec.ts -e test.js | grep -i media

# Check if there are any Windows path tests
rg -l "windows|backslash|sep" --glob="*test*" --glob="*spec*" packages/pi/

Repository: beeper/pickle

Length of output: 667

---

🏁 Script executed:

# Read the test file to check for assertInside tests
cat -n packages/pi/src/media-store.test.ts | head -100

Repository: beeper/pickle

Length of output: 2337

---

Use the platform-specific path separator in assertInside.

Line 130 hardcodes /, causing path containment checks to fail on Windows. Since path.resolve() returns backslash-separated paths on Windows, the condition resolvedTarget.startsWith(${resolvedRoot}/) will always be false for valid child paths, breaking all media reads and writes on Windows systems.

Import sep from node:path and use ${resolvedRoot}${sep} instead:

Suggested fix

-import { basename, resolve } from "node:path";
+import { basename, resolve, sep } from "node:path";
@@
 function assertInside(root: string, target: string): string {
   const resolvedRoot = resolve(root);
   const resolvedTarget = resolve(target);
-  if (resolvedTarget !== resolvedRoot && !resolvedTarget.startsWith(`${resolvedRoot}/`)) {
+  if (resolvedTarget !== resolvedRoot && !resolvedTarget.startsWith(`${resolvedRoot}${sep}`)) {
     throw new Error("Resolved media path escapes media root");
   }
   return resolvedTarget;
 }

🤖 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 path containment
check in assertInside uses a hardcoded '/' which fails on Windows; import sep
from 'node:path' and replace the startsWith check to use `${resolvedRoot}${sep}`
(i.e., test resolvedTarget !== resolvedRoot &&
!resolvedTarget.startsWith(`${resolvedRoot}${sep}`)) so the function uses the
platform-specific path separator when comparing resolvedRoot and resolvedTarget;
keep returning resolvedTarget and preserve the existing error message when the
check fails.