feat(intake): deliver private Notion specs across fleet nodes - #215
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. |
📝 WalkthroughWalkthroughThe Notion intake adds relay-channel delivery for mounted content. It publishes digest-bound chunks, renders worker reconstruction instructions, persists delivery receipts, reconciles GitHub and workspace dispatches, and manages the publisher through CLI startup and shutdown. ChangesNotion relay delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 81500b6c26
ℹ️ 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".
| const delivery = await prepareContractDelivery(task, input, receipt.delivery) | ||
| if (delivery && !sameContractDelivery(receipt.delivery, delivery)) { | ||
| state.receipts[task.sourceKey] = { ...receipt, delivery } |
There was a problem hiding this comment.
When an exact-path task already has a receipt from the local transport and the manifest is changed to relay-channel, this publishes the snapshot and records its delivery but immediately returns already-dispatched; the previously spawned worker still has only the old local-mount prompt and never receives the channel or message IDs. This is especially harmful when the transport is changed because that worker is on a node without the shared mount: subsequent runs see the new receipt and permanently suppress a dispatch that contains the portable instructions. The migration must update/re-dispatch the worker task or remain blocked rather than persisting an unusable delivery.
Useful? React with 👍 / 👎.
| if (input.manifest.workerMountTransport.kind === 'relay-channel' && | ||
| !receipt.delivery && existing.body !== renderIssueBody(task, summary)) { |
There was a problem hiding this comment.
If the process exits or writeIntakeState fails after updateIssue succeeds but before the delivery is persisted, the next run has a receipt without delivery and an issue body that already contains the generated portable instructions. This guard classifies that exact partially committed state as a manual edit before the idempotent publisher can recover the same delivery, so every retry remains blocked even though the issue update was performed by Factory. Detect and verify the generated contract marker/body so this external-write/local-receipt gap can be reconciled safely.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
src/cli/fleet.ts (1)
189-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the injected publisher before requiring a workspace key.
Line 117 documents
deps.notionContractsas a hermetic publisher for intake tests and alternate runtimes. An injected publisher supplies its own transport and needs no workspace key.Lines 190-193 resolve the key and throw before line 194 consults
deps.notionContracts. A hermetic test must therefore populateRELAY_WORKSPACE_KEYor an equivalent variable purely to reach its own injected dependency. That defeats the stated purpose of the injection point.Resolve the key only when the CLI constructs the real publisher.
♻️ Proposed reordering
if (!globals.dryRun && manifest.workerMountTransport.kind === 'relay-channel') { - const workspaceKey = resolveRelayWorkspaceKey({ env: deps.env ?? process.env }) - if (!workspaceKey) { - throw new Error('relay-channel worker mount transport requires an active Agent Relay workspace') + if (deps.notionContracts) { + notionContracts = deps.notionContracts + } else { + const workspaceKey = resolveRelayWorkspaceKey({ env: deps.env ?? process.env }) + if (!workspaceKey) { + throw new Error('relay-channel worker mount transport requires an active Agent Relay workspace') + } + notionContracts = new RelayChannelNotionContractPublisher({ workspaceKey }) } - notionContracts = deps.notionContracts ?? new RelayChannelNotionContractPublisher({ workspaceKey }) }🤖 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 `@src/cli/fleet.ts` around lines 189 - 195, Update the relay-channel branch around notionContracts so it first reuses deps.notionContracts when provided, without resolving or requiring a workspace key. Only resolveRelayWorkspaceKey and throw for a missing key when constructing the fallback RelayChannelNotionContractPublisher.src/intake/notion.test.ts (3)
443-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
workerMountTransportdefault through the manifest loader.Every fixture sets
workerMountTransportexplicitly, becauseNotionIntakeManifestis the zod output type and the field is required there. No test parses a manifest that omits the field.Backward compatibility for existing manifests depends entirely on
.default({ kind: 'local' })inworkerMountTransportSchema. If that default is removed,loadNotionIntakeManifestrejects every manifest already on disk, and the current suite still passes.Add one test that calls
loadNotionIntakeManifestwith a manifest JSON that omitsworkerMountTransportand asserts the resolved value is{ kind: 'local' }.🤖 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 `@src/intake/notion.test.ts` at line 443, Add a test for loadNotionIntakeManifest that parses manifest JSON without workerMountTransport and asserts the resolved field equals { kind: 'local' }. Keep existing explicit-field fixtures unchanged and ensure the test exercises the schema default rather than constructing a NotionIntakeManifest directly.
230-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the migrated receipt on disk.
This test verifies the GitHub side of the migration through
updateIssue. It does not verify the receipt.The delivery persistence on the migration path is new behavior in this PR:
publishRepoTaskmutates the receipt, andrunNotionIntakeUnlockedwrites the state only because the change detection now compares the full serialized receipt. A regression in either place leaves the issue updated and the receipt without a delivery, which is the exact crash-consistency state that blocks later runs.Read
manifest.statePathand assert the persisteddelivery, as the test at lines 198-204 does for the create path.💚 Proposed assertion
expect(github.updateIssue).toHaveBeenCalledWith(expect.objectContaining({ repo: 'AgentWorkforce/cloud', number: 42, body: expect.stringContaining('factory-notion-e1cff7cf-aabbccddee'), })) + const stored = JSON.parse(await readFile(manifest.statePath, 'utf8')) + expect(stored.receipts[`notion:${pageId}:repo:agentworkforce/cloud`].delivery).toEqual({ + kind: 'relay-channel', + channel: 'factory-notion-e1cff7cf-aabbccddee', + messageIds: ['message-1'], + encoding: 'base64-chunks-v1', + })🤖 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 `@src/intake/notion.test.ts` around lines 230 - 239, Extend the migration-path test around runNotionIntake to read the persisted receipt from manifest.statePath after dispatching, then assert that its delivery matches the expected migrated delivery data, following the existing create-path persistence assertion near lines 198-204. Keep the current GitHub update assertions unchanged.
176-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for
RelayChannelNotionContractPublisher.These tests exercise the intake reconciliation through a hand-written
NotionContractPublisherstub. The real publisher insrc/intake/notion-relay-contract.tscarries the safety-critical logic and appears to have no test file: the digest gate, base64 chunking at the 6,000-character boundary, marker-based reuse of prior messages, thetext !== expectedTextrejection, and the pagination guards inlistAllMessages.The PR objective states that publication is retry-safe across publisher identities and that cross-node byte reconstruction was verified manually. Cover those properties with a fake
AgentRelayso they hold under change.Suggested cases:
- A content and digest mismatch throws before any network call.
- A second publish reuses the prior message ids for identical content.
- A prior message with a matching marker but different payload throws.
- Content longer than 6,000 base64 characters produces multiple ordered chunks.
- A page that returns fewer than 100 messages stops pagination.
Do you want me to generate these tests, or open an issue to track them?
🤖 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 `@src/intake/notion.test.ts` around lines 176 - 183, Add unit tests for RelayChannelNotionContractPublisher using a fake AgentRelay, covering digest/content mismatch before network calls, retry reuse of message IDs for identical content, rejection of marker-matched messages with different text, ordered chunking beyond 6,000 base64 characters, and listAllMessages stopping when a page contains fewer than 100 messages. Keep the tests focused on cross-node reconstruction and retry-safe behavior.src/intake/notion-relay-contract.ts (3)
53-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve the first channel error for diagnosis.
The three-step
join→create→joinsequence discards the first two errors. Only the innermostjoinat line 62 propagates. An authentication failure or a permissions failure on the firstjointherefore surfaces as a second, less informativejoinerror.Capture the first error and attach it when the final attempt also fails.
♻️ Proposed error preservation
try { await relay.channels.join(channel) - } catch { + } catch (joinError) { try { await relay.channels.create({ name: channel, topic: `Read-only Notion contract ${input.pageId}`, }) - } catch { - await relay.channels.join(channel) + } catch (createError) { + try { + await relay.channels.join(channel) + } catch { + throw new Error( + `unable to join or create Notion contract channel ${channel}`, + { cause: joinError ?? createError }, + ) + } } }🤖 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 `@src/intake/notion-relay-contract.ts` around lines 53 - 64, Update the join/create retry logic around relay.channels.join and relay.channels.create to capture the initial join error, then attach or preserve it when the final join attempt fails. Keep the existing three-step sequence and success behavior unchanged while ensuring authentication or permission failures from the first join remain available in the propagated error.
107-121: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMemoize the in-flight registration.
#relayassigns#agentRelayonly afteragents.registerresolves at line 114. Two overlappingpublishcalls therefore both register, both using the same#publisherName, and oneAgentRelaypair is orphaned.disposedeletes the name once, so the orphan is not cleaned up.The current caller in
runNotionIntakeUnlockedawaits each task in sequence, so this cannot happen today. The class is exported fromsrc/intake/index.ts, so an external caller can reach it.Store the promise instead of the resolved value.
🤖 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 `@src/intake/notion-relay-contract.ts` around lines 107 - 121, Update the `#relay` method to memoize the in-flight initialization promise before awaiting agents.register, so overlapping calls share one registration and AgentRelay pair. Store and return that promise while preserving the existing fast path for an initialized `#agentRelay`; ensure `#workspaceRelay` and `#agentRelay` are assigned only once after registration completes.
72-88: 🩺 Stability & Availability | 🔵 TrivialPlan for channel growth across content revisions.
The marker prefix binds
pageIdandcontentDigest. A content change produces entirely new markers, so the previous revision's messages stay in the channel and are never referenced again. Nothing prunes them.
listAllMessagescaps traversal at 100 pages of 100 messages. Once a channel accumulates 10,000 messages, line 154 throws and all publication for thatsourceKeyfails permanently. The error text names the limit but gives the operator no remedy.Consider deleting messages whose marker prefix does not match the current
contentDigestafter a successful publish, or document the manual channel cleanup procedure and include it in the thrown error text.🤖 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 `@src/intake/notion-relay-contract.ts` around lines 72 - 88, Add cleanup to the publication flow around the chunk loop so that, after all current chunks publish successfully, messages with the same page/source marker prefix but a different contentDigest are deleted or otherwise removed from channel history. Preserve current-revision messages and ensure cleanup does not run before successful publication; if cleanup is intentionally not implemented, update the limit error in the surrounding publication logic to include actionable manual channel-cleanup instructions.src/intake/notion.ts (1)
422-440: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated body-edited guard.
Lines 424-429 and lines 434-439 return the same blocked result with the same reason, and both evaluate
existing.body !== renderIssueBody(task, summary). The rendering runs up to twice.The two checks are not redundant in behavior. The first one runs before
prepareContractDeliveryso that an edited body prevents relay publication, which the test at line 270 asserts. Keep that ordering, but compute the comparison once and extract the blocked result.♻️ Proposed consolidation
+ const bodyWasEdited = existing.body !== renderIssueBody(task, summary) + const editedResult = { + ...base, + status: 'blocked' as const, + issue: existing, + reason: 'existing lifecycle issue body was edited; refusing to overwrite it during portable mount migration', + } if (input.manifest.workerMountTransport.kind === 'relay-channel' && - !receipt.delivery && existing.body !== renderIssueBody(task, summary)) { - return { ...base, status: 'blocked', issue: existing, reason: '...' } - } + !receipt.delivery && bodyWasEdited) return editedResult const delivery = await prepareContractDelivery(task, input, receipt.delivery) if (delivery && !issueHasContractDelivery(existing.body, delivery)) { - if (existing.body !== renderIssueBody(task, summary)) { - return { ...base, status: 'blocked', issue: existing, reason: '...' } - } + if (bodyWasEdited) return editedResult🤖 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 `@src/intake/notion.ts` around lines 422 - 440, In the surrounding intake flow, compute the existing-body comparison once before the pre-prepareContractDelivery guard, then reuse that boolean in both checks. Preserve the first guard’s position before prepareContractDelivery and extract or reuse a single blocked-result construction so both paths return the same status, issue, and reason without rendering renderIssueBody more than once.
🤖 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 `@README.md`:
- Line 201: Update the README passage around the workerMountTransport example
and recommendation to state that local is the default whenever the field is
omitted, including in new manifests, and that relay-channel requires explicitly
setting the transport plus a resolvable active Agent Relay workspace key.
Clarify that copying the relay-channel example without resolving the required
workspace key will fail.
In `@src/intake/notion-relay-contract.ts`:
- Around line 44-49: Update the cache lookup and corresponding write path in the
contract publisher to key entries by both sourceKey and contentDigest, ensuring
a repeated sourceKey with a different digest cannot reuse an older delivery.
Preserve the existing digest validation and return behavior for matching key
pairs.
- Around line 100-105: Update RelayChannelNotionContractPublisher.dispose() in
src/intake/notion-relay-contract.ts:100-105 to independently catch failures from
disconnect() and agents.delete(this.#publisherName), ensuring neither rejection
escapes. Also update the notionContracts?.dispose?.() call in
src/cli/fleet.ts:425-427 to independently catch disposal failures so cleanup
cannot change the CLI command result.
In `@src/intake/notion.ts`:
- Around line 665-675: Update prepareContractDelivery to validate the result
returned by input.contracts.publish before writing it into state.receipts.
Reject an empty delivery.messageIds array by throwing a descriptive Error, so
the task is blocked and invalid state is not persisted; leave valid publisher
results unchanged.
- Around line 508-512: Update the already-dispatched workspace path around
prepareContractDelivery and sameContractDelivery so it does not persist a newly
migrated delivery that the running agent was never given. For receipts predating
the transport change, either retain the original delivery without recording the
relay contract or return an explicit blocked/incomplete status instead of
already-dispatched; preserve the existing behavior when the agent already has
the matching delivery.
- Around line 422-449: Update the pre-publication guard in publishRepoTask to
recognize bodies containing a Factory-authored delivery marker as unedited.
Parse the delivery from existing.body and compare it against
renderIssueBody(task, summary, deliveryFromBody(existing.body)), while retaining
the existing edited-body block for genuinely different content and preserving
the delivery update flow.
- Around line 420-421: Update the existing-issue handling around renderIssueBody
to resolve repository visibility only when body rendering is required, avoiding
repositoryVisibility calls for local transport tasks. Before using
target.publicSummary for public repositories, add the same explicit
manifest-value guard used by the create path; preserve the existing summary and
lifecycle behavior when the value is present.
---
Nitpick comments:
In `@src/cli/fleet.ts`:
- Around line 189-195: Update the relay-channel branch around notionContracts so
it first reuses deps.notionContracts when provided, without resolving or
requiring a workspace key. Only resolveRelayWorkspaceKey and throw for a missing
key when constructing the fallback RelayChannelNotionContractPublisher.
In `@src/intake/notion-relay-contract.ts`:
- Around line 53-64: Update the join/create retry logic around
relay.channels.join and relay.channels.create to capture the initial join error,
then attach or preserve it when the final join attempt fails. Keep the existing
three-step sequence and success behavior unchanged while ensuring authentication
or permission failures from the first join remain available in the propagated
error.
- Around line 107-121: Update the `#relay` method to memoize the in-flight
initialization promise before awaiting agents.register, so overlapping calls
share one registration and AgentRelay pair. Store and return that promise while
preserving the existing fast path for an initialized `#agentRelay`; ensure
`#workspaceRelay` and `#agentRelay` are assigned only once after registration
completes.
- Around line 72-88: Add cleanup to the publication flow around the chunk loop
so that, after all current chunks publish successfully, messages with the same
page/source marker prefix but a different contentDigest are deleted or otherwise
removed from channel history. Preserve current-revision messages and ensure
cleanup does not run before successful publication; if cleanup is intentionally
not implemented, update the limit error in the surrounding publication logic to
include actionable manual channel-cleanup instructions.
In `@src/intake/notion.test.ts`:
- Line 443: Add a test for loadNotionIntakeManifest that parses manifest JSON
without workerMountTransport and asserts the resolved field equals { kind:
'local' }. Keep existing explicit-field fixtures unchanged and ensure the test
exercises the schema default rather than constructing a NotionIntakeManifest
directly.
- Around line 230-239: Extend the migration-path test around runNotionIntake to
read the persisted receipt from manifest.statePath after dispatching, then
assert that its delivery matches the expected migrated delivery data, following
the existing create-path persistence assertion near lines 198-204. Keep the
current GitHub update assertions unchanged.
- Around line 176-183: Add unit tests for RelayChannelNotionContractPublisher
using a fake AgentRelay, covering digest/content mismatch before network calls,
retry reuse of message IDs for identical content, rejection of marker-matched
messages with different text, ordered chunking beyond 6,000 base64 characters,
and listAllMessages stopping when a page contains fewer than 100 messages. Keep
the tests focused on cross-node reconstruction and retry-safe behavior.
In `@src/intake/notion.ts`:
- Around line 422-440: In the surrounding intake flow, compute the existing-body
comparison once before the pre-prepareContractDelivery guard, then reuse that
boolean in both checks. Preserve the first guard’s position before
prepareContractDelivery and extract or reuse a single blocked-result
construction so both paths return the same status, issue, and reason without
rendering renderIssueBody more than once.
🪄 Autofix
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: 589b6893-b3a7-45d5-b0bc-2fd6d2bfb34b
📒 Files selected for processing (6)
README.mdsrc/cli/fleet.tssrc/intake/index.tssrc/intake/notion-relay-contract.tssrc/intake/notion.test.tssrc/intake/notion.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed against the latest diff
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 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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
1 participant
Summary
Verification