feat(bus): add subscribeOnce helper (strictly decomposed from B-0459)#3775
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa8a424403
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR aims to introduce a new TypeScript helper subscribeOnce under tools/bus/ to process unseen bus envelopes once per surface by persisting “seen” IDs, as a strictly decomposed slice of B-0459.
Changes:
- Adds
tools/bus/subscribe.tsimplementingsubscribeOnce. - Adds
tools/bus/subscribe.test.tswith Bun tests covering “seen” persistence and retry-on-handler-failure behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/bus/subscribe.ts | Introduces subscribeOnce helper that tracks processed envelope IDs via a per-surface JSON file. |
| tools/bus/subscribe.test.ts | Adds Bun tests for the new helper’s seen-state and retry semantics. |
Comments suppressed due to low confidence (4)
tools/bus/subscribe.ts:20
- P1:
surfaceis used directly in the filenameseen-${surface}.json. Ifsurfacecontains/or other path separators, this will create nested paths andwriteFileSyncwill fail (or create unexpected files). Validate/sanitizesurfaceto a safe filename component (e.g., allow[A-Za-z0-9_-]only, or encode).
ensureDir();
const seenFile = join(BUS_DIR, `seen-${surface}.json`);
let seenIds: Set<string>;
tools/bus/subscribe.ts:10
- P2: The docstring says this writes to a “surface-specific seen.json file”, but the implementation writes
seen-${surface}.json. Update the comment to match the actual filename so callers can locate/debug the state file.
/**
* Reads envelopes from the bus matching the given topic and recipient,
* calls the handler for each unseen envelope, and marks them as seen
* in a surface-specific seen.json file.
*/
tools/bus/subscribe.test.ts:4
- P0:
Topicis imported but never used, and themock(async (env) => {})callback parameter is unused. WithnoUnusedLocals/noUnusedParametersenabled intsconfig.json, this will fail thetsc --noEmitgate. Remove the unused import and either remove the unused parameter or rename it to_env.
import { describe, expect, test, mock } from "bun:test";
import { subscribeOnce } from "./subscribe";
import type { MessageEnvelope, Topic } from "./types";
import { rmSync } from "node:fs";
tools/bus/subscribe.test.ts:9
- P2: The describe label references “B-0449 slice 5”, but the PR is for B-0459. If this is copy/pasted, update the string so test output/reporting matches the actual slice/row.
describe("bus subscribeOnce (B-0449 slice 5)", () => {
const seenFile = join(BUS_DIR, "seen-test-surface.json");
AceHack
left a comment
There was a problem hiding this comment.
Lior Antigravity Check: Decomposition verified. Needs Copilot feedback addressed for merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5618091c2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses Copilot/Codex P1 threads on #3775: - Path-traversal: surface interpolated into `seen-${surface}.json` is now guarded by SURFACE_RE = /^[a-z0-9_-]{1,64}$/. Throws on invalid input; defense-in-depth even though current callers pass hard-coded surface ids. - Broad catch: distinguish ENOENT (no seen-file yet) from real errors. Re-throw permission / corruption errors so seen-state is never silently dropped. Validated locally: `bun build` clean; `bun test tools/bus/subscribe.test.ts` passes (2/2).
Addresses Copilot P2 thread on #3775. PR title + description say "B-0459 slice 1" but the describe string still read "B-0449 slice 5".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36b6a35b0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
tools/bus/subscribe.ts:35
- P1: The comment says JSON corruption should re-throw, but a wrong-but-valid JSON shape (e.g.,
{}) will parse successfully and then be treated as an empty seen-set viaArray.isArray(data) ? data : []. That silently drops seen-state and can cause unexpected re-processing; consider throwing (or at least logging loudly) when the file’s JSON isn’t the expected array of ids.
// Single readFileSync + ENOENT-aware catch. Other errors (permission,
// JSON corruption) re-throw so we never silently drop seen-state and
// re-deliver already-handled envelopes.
try {
const data = JSON.parse(readFileSync(seenFile, "utf8"));
seenIds = new Set(Array.isArray(data) ? data : []);
} catch (err) {
tools/bus/subscribe.ts:66
- P1:
writeFileSyncfailures when persisting the seen-set are swallowed (onlyconsole.error). If the seen file can’t be written (permissions/disk full), the function still returns success after running handlers, which will cause repeated deliveries on the next tick. Consider surfacing this failure to the caller (throw/return error) so the poller can fail fast rather than silently degrading semantics.
if (newlySeen) {
try {
writeFileSync(seenFile, JSON.stringify(Array.from(seenIds), null, 2));
} catch (err) {
console.error(`[subscribeOnce] Failed to write seen file:`, err);
}
Addresses Codex P2 thread on #3775. The read-side already re-throws non-ENOENT errors so we "never silently drop seen-state and re-deliver already-handled envelopes" — the write-side was breaking the same invariant in the opposite direction by logging-and-swallowing. Now writeFileSync errors propagate to the caller. Adds a test that pre-creates the seen file read-only so the read succeeds and the write fails with EACCES; subscribeOnce rejects rather than silently continuing. Co-Authored-By: Claude <noreply@anthropic.com>
|
Codex P2 thread (propagate seen-file write failures) addressed in 5b490ba:
Also merged origin/main into the branch (it was branched before tools/bus/bus.ts existed). Tests pass locally (3/3). |
Strict decomposition of B-0459 (#3643). This PR only contains the subscribeOnce bus helper, leaving the subscriber implementation for a subsequent slice.