Conversation
Greptile SummaryThis PR makes the Slack bridge the authoritative writer of outbound-reply proofs: both Key points:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant Bridge as bridge.mjs / broker-bridge.mjs
participant Slack
participant Ledger as slack-reply-log.jsonl
participant Heartbeat as heartbeat.ts
Agent->>Bridge: POST /send (with thread_ts)
Bridge->>Slack: chat.postMessage
Slack-->>Bridge: ok
Bridge->>Ledger: appendReplyLedgerEntry({channel, thread_ts, route:"/send"})
Bridge->>Bridge: resolveAckReaction(channel, thread_ts)
Agent->>Bridge: POST /reply (thread_id)
Bridge->>Slack: chat.postMessage
Slack-->>Bridge: ok
Bridge->>Ledger: appendReplyLedgerEntry({channel, thread_ts, route:"/reply"})
Bridge->>Bridge: resolveAckReaction(channel, thread_ts)
Note over Heartbeat: Heartbeat cycle fires
Heartbeat->>Ledger: readFileSync (Path 1 — primary)
alt thread_ts found in ledger
Ledger-->>Heartbeat: matched → hasReplied = true
else ledger miss / file absent
Heartbeat->>Heartbeat: scan SESSION_DIR subdirs (Path 2 — fallback)<br/>checks curl /send commands only
end
Last reviewed commit: c41b4dd |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| mode: 0o600, | ||
| }); | ||
| } catch (err) { | ||
| console.warn(`⚠️ failed to append reply ledger entry: ${err.message}`); |
There was a problem hiding this comment.
Inconsistent error serialisation vs broker-bridge.mjs
broker-bridge.mjs uses err instanceof Error ? err.message : String(err) to safely handle non-Error thrown values, but bridge.mjs accesses err.message directly. If a non-Error is ever thrown (e.g. a plain string), err.message will be undefined and the warning will be useless.
| console.warn(`⚠️ failed to append reply ledger entry: ${err.message}`); | |
| console.warn(`⚠️ failed to append reply ledger entry: ${err instanceof Error ? err.message : String(err)}`); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway-bridge/bridge.mjs
Line: 141
Comment:
**Inconsistent error serialisation vs `broker-bridge.mjs`**
`broker-bridge.mjs` uses `err instanceof Error ? err.message : String(err)` to safely handle non-`Error` thrown values, but `bridge.mjs` accesses `err.message` directly. If a non-Error is ever thrown (e.g. a plain string), `err.message` will be `undefined` and the warning will be useless.
```suggestion
console.warn(`⚠️ failed to append reply ledger entry: ${err instanceof Error ? err.message : String(err)}`);
```
How can I resolve this? If you propose a fix, please make it concise.| const SOCKET_DIR = path.join(homedir(), ".pi", "session-control"); | ||
| const AGENT_TIMEOUT_MS = 120_000; | ||
| const API_PORT = parseInt(process.env.BRIDGE_API_PORT || "7890", 10); | ||
| const REPLY_LEDGER_PATH = path.join(homedir(), ".pi", "agent", "slack-reply-log.jsonl"); |
There was a problem hiding this comment.
Unbounded ledger file growth — no pruning or TTL
slack-reply-log.jsonl is append-only. Every outbound Slack reply adds a new line and nothing ever removes old entries. The heartbeat check in hasRepliedToThread reads the entire file on every invocation, so on an active bot over weeks or months this will cause steadily increasing I/O on each heartbeat cycle.
Consider either:
- Periodically rewriting the file keeping only entries from the last N days (e.g. 7 days), or
- Reading the file in reverse / using a line limit so only recent entries are scanned.
This same concern applies to broker-bridge.mjs (same REPLY_LEDGER_PATH, same append-only strategy).
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway-bridge/bridge.mjs
Line: 50
Comment:
**Unbounded ledger file growth — no pruning or TTL**
`slack-reply-log.jsonl` is append-only. Every outbound Slack reply adds a new line and nothing ever removes old entries. The heartbeat check in `hasRepliedToThread` reads the **entire file** on every invocation, so on an active bot over weeks or months this will cause steadily increasing I/O on each heartbeat cycle.
Consider either:
- Periodically rewriting the file keeping only entries from the last N days (e.g. 7 days), or
- Reading the file in reverse / using a line limit so only recent entries are scanned.
This same concern applies to `broker-bridge.mjs` (same `REPLY_LEDGER_PATH`, same append-only strategy).
How can I resolve this? If you propose a fix, please make it concise.| for (const sessionDir of sessionDirs) { | ||
| let sessionFiles: string[] = []; | ||
| try { | ||
| const content = readFileSync(join(controlAgentSessionDir, file), "utf-8"); | ||
| const lines = content.split("\n"); | ||
|
|
||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) continue; | ||
|
|
||
| let parsed: any; | ||
| try { | ||
| parsed = JSON.parse(trimmed); | ||
| } catch { | ||
| continue; | ||
| } | ||
| sessionFiles = readdirSync(sessionDir) | ||
| .filter((f) => f.endsWith(".jsonl")) | ||
| .sort() | ||
| .reverse() | ||
| .slice(0, 10); | ||
| } catch { | ||
| continue; | ||
| } | ||
|
|
||
| for (const file of sessionFiles) { | ||
| try { | ||
| const content = readFileSync(join(sessionDir, file), "utf-8"); | ||
| const lines = content.split("\n"); | ||
|
|
||
| if (parsed?.type !== "message") continue; | ||
| if (parsed?.message?.role !== "assistant") continue; | ||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) continue; | ||
|
|
||
| const items = parsed?.message?.content; | ||
| if (!Array.isArray(items)) continue; | ||
| let parsed: any; | ||
| try { | ||
| parsed = JSON.parse(trimmed); | ||
| } catch { | ||
| continue; | ||
| } | ||
|
|
||
| for (const item of items) { | ||
| if (item?.type !== "toolCall") continue; | ||
| if (item?.name !== "bash") continue; | ||
| if (parsed?.type !== "message") continue; | ||
| if (parsed?.message?.role !== "assistant") continue; | ||
|
|
||
| const command = typeof item?.arguments?.command === "string" ? item.arguments.command : ""; | ||
| if (!command.includes("curl")) continue; | ||
| if (!command.includes("/send")) continue; | ||
| if (!threadTsPattern.test(command)) continue; | ||
| const items = parsed?.message?.content; | ||
| if (!Array.isArray(items)) continue; | ||
|
|
||
| return true; | ||
| for (const item of items) { | ||
| if (item?.type !== "toolCall") continue; | ||
| if (item?.name !== "bash") continue; | ||
|
|
||
| const command = typeof item?.arguments?.command === "string" ? item.arguments.command : ""; | ||
| if (!command.includes("curl")) continue; | ||
| if (!command.includes("/send")) continue; | ||
| if (!threadTsPattern.test(command)) continue; | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
| } catch { | ||
| // File read error - skip | ||
| } | ||
| } catch { | ||
| // File read error - skip | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback session scan now covers all directories but still only matches /send
The PR expands the fallback (Path 2) from scanning a single --home-baudbot_agent-- directory to scanning every subdirectory of SESSION_DIR, and increases the per-directory file limit from 3 to 10. However, the pattern match still only looks for curl commands that include "/send" (line 498). Replies issued via /reply in older sessions that predate the ledger will remain undetected through this path.
Since the comment explicitly calls this a "fallback for older runs," this is a minor gap, but it's worth noting so future contributors understand the limitation: Path 2 cannot retroactively detect pre-ledger /reply calls.
The expanded scan also has a performance implication on the hot path: if Path 1 misses (e.g. ledger doesn't exist or was rotated), Path 2 now reads up to N_dirs × 10 JSONL session files in full on every heartbeat invocation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pi/extensions/heartbeat.ts
Line: 458-508
Comment:
**Fallback session scan now covers all directories but still only matches `/send`**
The PR expands the fallback (Path 2) from scanning a single `--home-baudbot_agent--` directory to scanning **every** subdirectory of `SESSION_DIR`, and increases the per-directory file limit from 3 to 10. However, the pattern match still only looks for curl commands that include `"/send"` (line 498). Replies issued via `/reply` in older sessions that predate the ledger will remain undetected through this path.
Since the comment explicitly calls this a "fallback for older runs," this is a minor gap, but it's worth noting so future contributors understand the limitation: Path 2 cannot retroactively detect pre-ledger `/reply` calls.
The expanded scan also has a performance implication on the hot path: if Path 1 misses (e.g. ledger doesn't exist or was rotated), Path 2 now reads up to `N_dirs × 10` JSONL session files in full on every heartbeat invocation.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Slack bridge appends to reply ledger.