fix(cli): clean up inbound webhook subscriptions#1244
Merged
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRelayfile bindings now persist cloud webhook subscription IDs and workspace pins. Subscribe and unsubscribe use a durable journal for crash recovery, retries, compatibility checks, and ordered cleanup, with expanded lifecycle tests. Broker option filtering is also simplified. ChangesWebhook binding lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CleanupJournal
participant Relay
participant RelayfileCloud
participant RelayfileBridge
CLI->>CleanupJournal: record lifecycle intent
CLI->>Relay: create relay resources
CLI->>RelayfileCloud: create webhook subscription
CLI->>RelayfileBridge: bind webhookSubscriptionId
CLI->>RelayfileCloud: delete pending or superseded subscription
CleanupJournal-->>CLI: retain failed cleanup for retry
CLI->>RelayfileBridge: unbind after cleanup is recorded
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the agent-relay integration subscribe|unsubscribe commands to persist and retire relayfile-cloud inbound webhook subscriptions during replacement, rollback, and removal. It introduces webhookSubscriptionId to the binding schema, implements API version compatibility checks, and adds corresponding integration tests. The review feedback suggests minor refactoring in integration.ts to extract webhookSubscriptionId into local constants, which improves code readability, avoids duplicate property accesses, and eliminates non-null assertions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a58facb882
ℹ️ 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".
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Replaces the binding-persisted pendingWebhookSubscriptionIds design with a project-local durable cleanup journal (integration-cleanup-journal.ts), closing the five retry/race gaps found in review: - Replacement: the prior binding's cloud-subscription and relay-subscription ids are pre-recorded in one atomic journal write BEFORE bind() overwrites their only other persisted home; successful retirement clears them. - Unsubscribe: failed deletes are journaled before unbind; if even the journal write fails the run aborts with the binding intact. Completed cleanups and not-found (404) responses converge and clear their entries. - Initial rollback: intents are journaled before the cloud create (its server-assigned id cannot be pre-named); the relay event subscription id is recorded before bind; rollback failures keep/record entries. - Relaycast cleanup: relay webhook/subscription deletes are retry-preserving with the same journal, 404-tolerant sweep, and active-binding/keep-set guards. - Concurrent re-subscribe: pending state never lives in the binding, so there is no post-delete re-bind to stale-overwrite — the race is removed, not synchronized. Journal updates are id-keyed and commutative. Crash window: an intent retained from an interrupted run is reconciled via the daemon's new subscription list (relayfile#346) matched on the deterministic (url, glob set) recovery key, excluding ids referenced by active bindings; when the installed client cannot list (published v0.10.20 has no runtime method), a matching re-subscribe ABORTS before any new create instead of double-subscribing. Journal safeguards per review: entries scoped to the connection identity (sha256 digest, never a secret); read-modify-write under an O_EXCL lockfile with stale takeover only for a proven-dead same-host owner; atomic tmp+rename 0600 writes in the git-excluded project data dir; discriminated schema validation with corrupt journals failing closed (nothing swept, subscribe aborts pre-create); journal-managed logs never contain ids or the capability-bearing inbound url. Also: narrow TEMPORARY casts at the bridge boundary so the CLI typechecks against registry @relayfile/client 0.10.20 (removed when v3 publishes), runtime feature-detection for listWebhookSubscriptions, the unreachable post-try guard removed (Cubic P3), Gemini readability extractions, and the stale relaycast-groups bind expectation fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/cli/commands/integration-cleanup-journal.test.ts`:
- Around line 7-11: Remove the unused CleanupJournalError import from the
integration-cleanup-journal test file, leaving only the symbols referenced by
the tests: fileCleanupJournal and PendingCleanupEntry.
🪄 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 Plus
Run ID: 83cca092-4142-4b83-9ca6-e229219c606b
📒 Files selected for processing (7)
CHANGELOG.mdpackages/cli/src/cli/commands/integration-cleanup-journal.test.tspackages/cli/src/cli/commands/integration-cleanup-journal.tspackages/cli/src/cli/commands/integration-relayfile-contract.test.tspackages/cli/src/cli/commands/integration-subscribe.test.tspackages/cli/src/cli/commands/integration.tspackages/cli/src/cli/commands/relaycast-groups.test.ts
💤 Files with no reviewable changes (1)
- packages/cli/src/cli/commands/integration-relayfile-contract.test.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/cli/commands/integration-subscribe.test.ts
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…eterministic crash recovery Hardens the cleanup journal against every interleaving found in adversarial review of e66ba18: - Lifecycle lease: subscribe AND unsubscribe atomically reserve a per-(scope, provider, resource) owner-stamped lease inside one journal RMW (no check-then-write TOCTOU); each aborts while the other is live, leases release on every exit path, and binding reads happen only after the lease — the lease is the linearization point, so a stale unsubscribe can no longer unbind a concurrent replacement's fresh binding. - Attempt records carry deterministic recovery keys for ALL THREE creates: exact inbound webhook name (never prefix — cannot hit another attempt's pre-bind webhook), a per-attempt writeback-url marker (nonsecret query the ingress ignores) uniquely identifying the relay event subscription on shared channels, and the cloud (url, glob set). Broad prefix-orphan deletion is removed entirely: hygiene deletes only attributable ids. - Workspace pinning end-to-end: the pin is obtained BEFORE any cloud create (writeback-secret carries workspaceId on daemons >= relayfile#346, v3 list as fallback, fail closed when list-capable but unpinnable), passed into the create, verified against the create's echo (mismatch aborts into rollback), persisted on the binding (webhookSubscriptionWorkspaceId), and used on every delete/list; cloud 404 converges ONLY when pinned and list reconciliation requires an exact workspace echo. - Journal durability: fsync(temp) + rename + fsync(parent dir); async mutators run under the exclusive lock; strict discriminated validation (owner/pair/operation) fails closed; the entry key uses an escaped NUL delimiter and .gitattributes forces text diff rendering. - CodeQL: the relay connection scope derives via purpose-domain PBKDF2, never a bare hash of the workspace key. Published-0.10.20 note: compatibility casts stay at the bridge boundary and runtime feature-detection guards the list method; interrupted cloud creates on that client are retained FAIL-CLOSED (same-key lifecycles abort) until the v3 client is published and pinned — full recovery activates then. Regressions cover: concurrent reservation (subscribe/subscribe and subscribe/unsubscribe both ways), lease release on abort, crash at each create window, exact-name and exact-url recovery on shared channels, workspace-switch unsubscribe and pinned/unpinned 404 semantics, list-echo fail-closed, journal lock/corruption/async-mutator/permissions behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mechanical Option::filter rewrites in normalize_initial_task and posthog_api_key so the Clippy CI gate (cargo clippy -- -D warnings) is green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The e66ba18 parent blob contained a raw NUL, so range diffs of the (now clean) safety-critical journal file rendered as binary without an explicit diff attribute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/broker/src/telemetry.rs (1)
936-948: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test comment and pattern after
filterrefactor.The test comment at line 941 references "matching the same
and_thenshape," butposthog_api_key()now usesfilter. The inline synthetic verification also still usesand_thenrather thanfilter, making it a replica of the old implementation rather than the current one.Consider updating the comment and inline pattern to use
filterso the test stays aligned with the actual implementation.♻️ Suggested update
// `posthog_api_key()` reads the build-time const, so we can't mutate // it from a test. What we can guarantee is the wrapper's contract: // a `Some("")` from `option_env!` must round-trip to `None` so the - // disabled path takes over. Verify that contract on a synthetic - // `Option<&str>` matching the same `and_then` shape. + // disabled path takes over. Verify that contract on a synthetic + // `Option<&str>` matching the same `filter` shape. let synthetic: Option<&str> = Some(""); - let normalized = synthetic.and_then(|k| if k.is_empty() { None } else { Some(k) }); + let normalized = synthetic.filter(|k| !k.is_empty()); assert!(normalized.is_none()); let synthetic: Option<&str> = Some("phc_abc"); - let normalized = synthetic.and_then(|k| if k.is_empty() { None } else { Some(k) }); + let normalized = synthetic.filter(|k| !k.is_empty()); assert_eq!(normalized, Some("phc_abc"));🤖 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 `@crates/broker/src/telemetry.rs` around lines 936 - 948, Update the test comment in posthog_api_key_treats_empty_as_unset to reference the current filter-based normalization instead of the old and_then shape, and replace both synthetic normalization expressions with filter(|k| !k.is_empty()) so the test matches posthog_api_key()’s implementation.packages/cli/src/cli/commands/integration-cleanup-journal.ts (1)
145-292: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStale-lock takeover can't fire within a single invocation (
LOCK_STALE_MS>LOCK_TIMEOUT_MS).
acquireLockonly breaks a lock once its age exceedsLOCK_STALE_MS(10s), but each call gives up afterLOCK_TIMEOUT_MS(5s). For the takeover branch (Line 265) to run, the loop must observenow - mtime > 10s, which requires the run to have started more than ~5s after the lock was created. So a run started shortly after a crash that left a lock owned by a provably dead local pid will still fail withcode: 'locked'on its first attempt, and the operator must re-run. This is consistent with the fail-closed intent, but the dead-owner reclamation the comment advertises is effectively unreachable in a single command run.Consider whether
LOCK_TIMEOUT_MSshould exceedLOCK_STALE_MS, or whether the reclamation UX is acceptable as-is.🤖 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/cli/src/cli/commands/integration-cleanup-journal.ts` around lines 145 - 292, Ensure acquireLock can observe and reclaim locks once they exceed LOCK_STALE_MS during a single invocation by increasing LOCK_TIMEOUT_MS beyond LOCK_STALE_MS, while preserving the existing dead local owner and fail-closed checks. Update the constants near acquireLock and verify timeout behavior still reports locked only after stale-lock reclamation has had a chance to run.
🤖 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/cli/src/cli/commands/integration.ts`:
- Line 937: Change the declaration of relayResolved in the relevant command
logic from let to const, since it is not reassigned and must satisfy the
prefer-const lint rule.
---
Nitpick comments:
In `@crates/broker/src/telemetry.rs`:
- Around line 936-948: Update the test comment in
posthog_api_key_treats_empty_as_unset to reference the current filter-based
normalization instead of the old and_then shape, and replace both synthetic
normalization expressions with filter(|k| !k.is_empty()) so the test matches
posthog_api_key()’s implementation.
In `@packages/cli/src/cli/commands/integration-cleanup-journal.ts`:
- Around line 145-292: Ensure acquireLock can observe and reclaim locks once
they exceed LOCK_STALE_MS during a single invocation by increasing
LOCK_TIMEOUT_MS beyond LOCK_STALE_MS, while preserving the existing dead local
owner and fail-closed checks. Update the constants near acquireLock and verify
timeout behavior still reports locked only after stale-lock reclamation has had
a chance to run.
🪄 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 Plus
Run ID: 38b20e5c-93fd-40ca-bc6c-dd95e1fe74e1
📒 Files selected for processing (9)
.gitattributesCHANGELOG.mdcrates/broker/src/runtime/session.rscrates/broker/src/telemetry.rspackages/cli/src/cli/commands/integration-cleanup-journal.test.tspackages/cli/src/cli/commands/integration-cleanup-journal.tspackages/cli/src/cli/commands/integration-subscribe.test.tspackages/cli/src/cli/commands/integration.tspackages/cli/src/cli/commands/relaycast-groups.test.ts
✅ Files skipped from review due to trivial changes (2)
- .gitattributes
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/cli/commands/relaycast-groups.test.ts
- packages/cli/src/cli/commands/integration-subscribe.test.ts
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Recovery of a dead subscribe attempt now CLAIMS the record as a live recovery lease (atomic re-own under the journal lock) before any remote list/delete, releases it back to a reclaimable state on partial or thrown reconciliation, and removes it only when fully reconciled. A same-resource lifecycle therefore aborts (live-owner conflict) for the entire recovery window — a new subscribe can no longer create a same-key pre-bind resource that a mid-flight sweeper then deletes. Also consumes relayfile#346's strict semantics: the real-daemon bridge contract binds the (id, workspace) pair and asserts its round-trip through the published v0.10.20 client runtime. Regressions: claim-precedes-remote-calls ordering, deterministic mid-reconcile same-resource abort (stalled recovery + rival run), and partial-claim release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eview polish - The workspace pin is now MANDATORY before every cloud create — with explicit --bridge-url/--bridge-secret overrides the control plane is still consulted for the workspace identity alone (override url/secret win for delivery), the v3 list is the fallback, and a run that cannot obtain a pin aborts before any external create. - Rollback delete + retry record use the create echo's workspaceId with the attempt pin as fallback. - Automatic stale-lock takeover is REMOVED: the stat->read->unlink dance is itself a TOCTOU (CodeQL), so an abandoned lockfile now fails closed with a manual-remediation message. The lock spans only one read-modify-write, so this is availability-only; entry-owner PID reuse is likewise documented as availability-only. - Telemetry test mirrors the Option::filter shape; relayResolved is const; unused import dropped; Prettier clean. Regressions: bridge-url pin-only resolution (override url wins, pin flows into create + binding), bridge-url no-pin abort-before-create, dead-owner lock now fails closed with the lockfile preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… crash-released) Replaces the path-based journal lockfile protocol with a true OS advisory lock via fs-ext-extra-prebuilt@2.2.9 (flock on Unix, LockFileEx on Windows; prebuilt for Node 20+ across macOS/Linux/Windows, SLSA provenance-attested): - The lock file is a STABLE inode — created once, never unlinked or renamed. Mutual exclusion lives on the open file description and the kernel releases it automatically when the holder exits or dies, eliminating the entire path-takeover problem class: no pid/liveness probes, no mtime/age heuristics, no takeover races, and a crash mid-update can never wedge unattended recovery (Cubic P2) while staying CodeQL-safe (no check-then-unlink anywhere). - File CONTENT is a fsync'd v2 format marker only. A crash-truncated marker (strict prefix) heals under the lock; any unrecognized pre-release sentinel fails closed and is preserved for inspection, never adopted or overwritten. - Lock acquisition is nonblocking-exclusive with bounded retry; unlock+close on every release path. - package-lock.json carries ONLY the new dependency entries (the unrelated 9.2.2->9.2.3 workspace version churn from npm install was excluded). Tests: real child-process kernel contention (holder blocks the update, which proceeds after release), crash-release (holder hard-aborts without unlocking; the very next update succeeds), same-process serialization across separate descriptors, legacy-sentinel fail-closed with content preserved, truncated-marker heal, stable-file/mode/marker assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t cleanup 'a+' sets O_APPEND, and on Linux pwrite honors it — the marker heal would append instead of rewriting at offset 0 (masked on macOS, where pwrite ignores O_APPEND). Open with O_RDWR|O_CREAT (0600) and ftruncate before the heal rewrite. The contention test's child listeners/timer now clean up deterministically so a post-'held' exit cannot double-settle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…un-safe) The static fs-ext-extra-prebuilt import crashed the Bun standalone build (native addons are externalized out of the compiled binary), so the journal lock moves into the already-required Rust broker: - New hidden `agent-relay-broker journal-lock --file <path> --timeout-ms <n>` subcommand holds the SAME stable-inode kernel advisory lock via std File::try_lock (flock on Unix, LockFileEx on Windows; stable since Rust 1.89 — zero new crate deps). It owns the marker rules: fsync'd v2 marker, strict-prefix heal, unrecognized pre-release sentinel fails closed (exit 3) with the file preserved. Exit 4 = timed out against a live holder; exit 2 is clap's usage error, which the CLI maps distinctly as broker version skew. No secrets on stdout/stderr/argv. - The CLI journal spawns the helper through the standard broker resolver (Bun standalone already ships and spawns this binary), waits for the `locked` handshake, and releases by closing stdin — the helper exits on EOF, and the kernel drops the lock if EITHER process dies. Release is awaited so the lock is provably gone before update() returns. No native import remains anywhere in the CLI graph. - fs-ext-extra-prebuilt is fully removed (dependency, lockfile entries, nan runtime marking restored). Tests: Rust unit coverage for marker/contention/sentinel; TS suite drives the REAL helper from this checkout's cargo build (env-pinned via BROKER_BINARY_PATH/AGENT_RELAY_BIN so an older installed release can never be substituted) — cross-process contention block-then-proceed, SIGKILL crash-release, same-process serialization, sentinel fail-closed, marker heal — plus a release-style Bun `--version` smoke proving the import graph is addon-free (skips where bun is absent; CI's Standalone Smoke covers it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Per review, lock ownership must provably span the ENTIRE transaction — the parent no longer writes the journal at all: - `journal-lock` gains --journal-file. After the exact framed `locked` handshake, the parent reads/parses/mutates under the helper's held lock, then streams the complete serialized entries to helper stdin and closes. The helper read_to_end()s with errors propagated; an EMPTY payload aborts (release, journal untouched); a nonempty payload is committed BY THE HELPER — temp (0600) + fsync + atomic rename + parent-directory fsync — before exit 0. A failed run can never leave a partially written journal. - The parent's handshake is exact buffered line parsing (no substring matching) with a startup deadline that kills a wedged helper; it awaits stdin-write + exit 0 for the commit verdict and treats helper loss (death before/during commit, incl. the EPIPE mid-write race — stdin error swallowed, end-callback never relied on) as commit failure with the journal left byte-for-byte unmodified. Regressions: SPLIT `loc`/`ked` handshake accepted; invalid handshake line killed+rejected with the journal intact; helper death during an async mutator aborts with original bytes intact; helper stdin-destroy mid-commit (EPIPE race) fails cleanly; mutator-throw releases via empty payload with no write; Rust-side atomic replace + no temp residue. Full suite: Rust 3/3, TS journal 16/16 (real helper + scripted fakes), CLI vitest 531 passed, tsc/Prettier/eslint/cargo fmt/clippy clean, Bun runs the built CLI (--version → 9.2.3, no native addon). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Plain `npm test` jobs (test.yml, node-compat.yml) run without cargo, so the resolver would fall through to the published optional broker package — which predates `journal-lock --journal-file` and fails as version skew. The suite now selects its helper explicitly: the checkout-local cargo build when present (env-pinned), otherwise a scripted fake implementing the same protocol faithfully (mkdir-mutex exclusion, v2 marker/sentinel rules, exact `locked` handshake, empty-payload abort, temp+rename commit). Real kernel semantics (cross-process contention, SIGKILL release) remain gated behind the built binary. Verified in a simulated no-target environment: 14 passed, 2 kernel tests skipped, zero resolver fall-through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ce good contents P0 from adversarial review: a parent dying mid-`stdin.end(payload)` yields PARTIAL bytes + clean EOF (end() is not one atomic PIPE_BUF write), and the helper would have committed the truncated payload over good JSON. The commit is now a framed envelope: ARJL1\n<payload-byte-length>\n<sha256-hex-of-payload>\n<payload> The helper commits ONLY after the exact declared length AND digest validate, with no trailing bytes; empty EOF remains abort; any short/malformed/ trailing frame exits 5 (`frame` on stderr) with the journal byte-for-byte untouched. sha2 was already a broker dependency. The parent builds and sends the frame and awaits exit for the verdict. Coverage: Rust unit tests over parse_frame (truncated body, trailing bytes, digest corruption, bad magic, empty abort) plus a NEW pipe-boundary integration test (crates/broker/tests/journal_lock_cli.rs) that spawns the real binary via CARGO_BIN_EXE — running in the mandatory Rust Tests job — seeding a journal, sending a proper frame prefix + partial body then EOF, asserting exit 5 and the exact original bytes. The broker-less scripted fakes mirror the frame validation precisely (so broad Node CI cannot mask the P0) and a fake-path truncation regression asserts the same. Also fixes the two clippy doc-lazy-continuation diagnostics that were failing CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/cli/commands/integration-cleanup-journal.test.ts">
<violation number="1" location="packages/cli/src/cli/commands/integration-cleanup-journal.test.ts:517">
P3: `spawn` is already imported at the top of the file via `import { execFileSync, spawn } from 'node:child_process'`. The dynamic `await import('node:child_process')` inside the test body to re-import it under the alias `spawnChild` is redundant — the test can use `spawn` directly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1) Bounded owner lease: PendingCleanupOwner gains a strictly validated heartbeatAt; isOwnerLive requires BOTH a passing pid probe AND a fresh heartbeat within OWNER_LEASE_MS (15 min, safely above every remote request timeout). PID reuse — where a dead owner's pid probes alive forever — and unprobeable foreign-host owners now delay lifecycles for at most the lease window, never permanently. Explicit tradeoff documented: a process paused past the window loses its lease. 2) Exclusive temp creation: replace_journal now creates its temp via create_new (no symlink following, no truncation) under an unpredictable pid+nanos+counter suffix with EEXIST retry, and failure paths remove the temp file. 3) Directory-fsync errors propagate: the OPEN always propagates (missing directory errors on every platform); only a sync failure on an OPENED Windows directory handle — narrowed to PermissionDenied/InvalidInput — is tolerated. 4) Abort-protected serialization: the mutator AND JSON/frame serialization run inside the abort-protected block (in update() and inside commit()), so a serialization failure releases the helper instead of holding the lock until process exit. Regressions for each: expired-lease-with-alive-pid recovers (PID-reuse bound) while fresh/foreign/live owners stay fail-closed; BigInt serialization failure aborts, journal unchanged, and the NEXT update succeeds (lock provably released); exclusive-temp uniqueness/0600/create_new symlink-refusal; missing-directory sync errors on every platform, existing directory clean; swap-failure temp cleanup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ounded A finite future heartbeatAt (foreign clock skew or a malformed value like 9e15) made `Date.now() - heartbeatAt > OWNER_LEASE_MS` false forever, resurrecting the unbounded PID/foreign-host wedge. Liveness now also expires any heartbeat further in the FUTURE than a documented skew allowance (OWNER_HEARTBEAT_SKEW_MS, 2 min). Regression: an alive-pid owner with a 9e15-future heartbeat is recovered and the subscribe proceeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… on Windows Windows cannot open a directory via plain File::open (no FILE_FLAG_BACKUP_SEMANTICS), so after making dir-open errors propagate, EVERY commit's directory fsync would have failed there — helper exits nonzero on each normal journal replacement. open_dir() now sets the flag via OpenOptionsExt::custom_flags on Windows; the existing-directory sync test exercises exactly this path on Windows CI while the missing-directory test keeps every platform propagating real errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
khaliqgant
force-pushed
the
codex/webhook-unsubscribe-relay
branch
from
b5d1ca4 to
4e34eef
Compare
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…view polish Cubic P1: owned attempt/reservation records now renew their lease with an unref'd periodic timer (OWNER_LEASE_CONFIG.renewalMs, well below the lease) started only after the reservation is durably acquired and stopped before the record is cleared on EVERY exit path (subscribe pre-create catch, rollback, settle; unsubscribe release). The renewal atomically re-stamps only its own attemptId's entries, LATCHES failure — including the zero-match case where the record vanished (lease lost) — and assertHealthy() checkpoints before every post-await external mutation in BOTH lifecycles (each subscribe create + bind; each unsubscribe delete + unbind) so a lost lease can never race later mutations. A paused event loop past the bound remains the documented tradeoff. Deterministic regressions with tiny test-only windows: a create outliving the lease 2x stays protected (a rival probing mid-await still sees a LIVE reservation and aborts), and a failing renewal latches, aborts before the cloud create, and rolls the webhook back. Cubic P2: replace_journal cleanup is restructured so remove_file(tmp) can only fire on PRE-rename failures; after a successful rename the temp path no longer belongs to us and a post-rename dir-fsync error propagates WITHOUT unlinking (a rival's recreated same-shaped file survives — regression included). Cubic P3: the journal test reuses top-level spawn/createHash imports instead of redundant dynamic imports. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Rust (journal_lock.rs P3): replace_journal gains a deterministic sync seam receiving (dir, consumed_tmp). The regression now injects an ACTUAL post-rename sync failure that first recreates the EXACT consumed temp pathname as a rival file: the error propagates, the renamed journal payload survives, and the rival bytes survive - the old remove_file-on-any- error ordering demonstrably fails this test. TS (integration.ts P3): startLeaseRenewal.stop() latches "stopped" BEFORE clearInterval, and both the timer callback and the async then/catch settlement of an in-flight tick no-op after stop - a tick straddling the lifecycle settle can no longer emit a false lost-lease warning or latch after success. Pre-stop latching behavior (failure + vanished-record) is unchanged. Deterministic held-renewal regression: one tick is gated across the lifecycle completion, released after settle+stop, and must neither warn, nor latch, nor resurrect the removed reservation, nor reject unhandled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
khaliqgant
force-pushed
the
codex/webhook-unsubscribe-relay
branch
from
36d6ed1 to
f558478
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Depends on the relayfile control-plane API v3 change in the companion PR.