fix(intake): make Notion claims workspace-durable - #217
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNotion intake now requires immutable, workspace-global Agent Relay claims before GitHub issue creation or workspace-agent dispatch. The fleet CLI manages claim-store lifecycle. Documentation and schema validation separate Notion intake from lifecycle issue sources. ChangesNotion claim durability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NotionIntake
participant ClaimStore
participant AgentRelay
participant GitHub
participant WorkspaceAgent
NotionIntake->>ClaimStore: Acquire digest-bound claim
ClaimStore->>AgentRelay: Create or join workspace claim channel
AgentRelay-->>ClaimStore: Return claim status
ClaimStore-->>NotionIntake: New or existing claim
NotionIntake->>GitHub: Create or reconcile issue
NotionIntake->>WorkspaceAgent: Locate or spawn exact-path agent
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/intake/notion.test.ts (2)
23-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the claim/source digest mismatch branch.
The shared fake returns the stored claim on conflict and never returns a different digest.
claimNotionTaskinsrc/intake/notion.tsthrowsdurable Notion claim digest does not match the mounted specwhen the returned claim carries another digest, andobserveNotionClaimthrows the same message fromget. The PR objectives list claim/source digest mismatch as fail-closed behavior. No test in the provided ranges drives a store that returns a mismatched digest.Add one test with a store whose
getorclaimreturns a claim for the samesourceKeywith a differentdigest, and assert the blocked result.🤖 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 23 - 37, Add a test in the Notion intake test suite using a claim store whose get or claim returns the same sourceKey with a different digest, then assert the operation produces the blocked result and the expected durable Notion claim digest mismatch error. Keep the existing shared claims fixture unchanged and target the claimNotionTask or observeNotionClaim path as appropriate.
657-688: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the recovered-agent branch.
findreturnsundefinedhere, so the test exercises only the blocked path.src/intake/notion.tslines 621-629 build a reconstructed workspace receipt fromrunning.agent,running.node, andclaim.claim.claimedAtwhenfindreturns an agent. That branch writes local state and reportsalready-dispatched. No test in the provided ranges drives it.Add a case where the second run's
findresolves to a running agent, then assert thealready-dispatchedstatus, the reported agent, and thatdispatchis still called only once.🤖 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 657 - 688, The existing concurrency test only covers the blocked path because WorkspaceTaskDispatcher.find returns undefined. Add a test case in the same suite where the second run’s find resolves to a running agent, exercising runNotionIntake’s reconstructed workspace receipt path; assert the second result is already-dispatched with the expected agent and workspace.dispatch is called exactly once.src/cli/fleet.ts (1)
466-505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider flattening the nested shutdown steps.
The
finallyblock now nests five levels oftry/finallyto guarantee that each disposal runs after the previous one fails. The behavior is correct. The nesting makes the ordering hard to verify by reading.A sequential loop over guarded steps preserves the order and the independent-failure property with less nesting.
♻️ Proposed refactor
} finally { - try { - try { - await notionClaims?.dispose?.() - } catch { - err.write('[factory] warning: Notion claim store failed during shutdown\n') - } - } finally { - try { - try { - await notionContracts?.dispose?.() - } catch { - err.write('[factory] warning: Notion contract publisher failed during shutdown\n') - } - } finally { - try { - await mount?.dispose?.() - } finally { - try { - await fleet?.dispose() - } finally { - if (reporter) { - try { - await reporter.report(createFactoryCloudEventV1({ - type: 'instance.stopping', - attributes: { component: 'cli', operation: 'stop' }, - })) - await reporter.report(createFactoryCloudEventV1({ - type: 'instance.stopped', - attributes: { component: 'cli', operation: 'stop' }, - })) - await reporter.close?.({ deadlineMs: 2_000 }) - } catch { - err.write('[factory] warning: Cloud progress reporter failed during shutdown\n') - } - } - } - } - } - } + const shutdownSteps: readonly { run: () => Promise<unknown>; warning: string }[] = [ + { + run: async () => await notionClaims?.dispose?.(), + warning: '[factory] warning: Notion claim store failed during shutdown\n', + }, + { + run: async () => await notionContracts?.dispose?.(), + warning: '[factory] warning: Notion contract publisher failed during shutdown\n', + }, + { + run: async () => await mount?.dispose?.(), + warning: '[factory] warning: Relayfile mount failed during shutdown\n', + }, + { + run: async () => await fleet?.dispose(), + warning: '[factory] warning: fleet client failed during shutdown\n', + }, + { + run: async () => { + if (!reporter) return + await reporter.report(createFactoryCloudEventV1({ + type: 'instance.stopping', + attributes: { component: 'cli', operation: 'stop' }, + })) + await reporter.report(createFactoryCloudEventV1({ + type: 'instance.stopped', + attributes: { component: 'cli', operation: 'stop' }, + })) + await reporter.close?.({ deadlineMs: 2_000 }) + }, + warning: '[factory] warning: Cloud progress reporter failed during shutdown\n', + }, + ] + for (const step of shutdownSteps) { + try { + await step.run() + } catch { + err.write(step.warning) + } + } }Note one behavior change in the proposal:
mountandfleetdisposal failures are currently propagated, and the proposal converts them to warnings. Keep them propagating if the current behavior is intentional.🤖 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 466 - 505, Flatten the shutdown sequence currently anchored by the outer try/finally chain into sequential guarded disposal steps, preserving the order: notionClaims, notionContracts, mount, fleet, then reporter shutdown. Keep independent cleanup running after earlier failures; retain the existing warning handling for Notion and reporter failures, while allowing mount and fleet disposal failures to propagate as they do now.src/intake/notion-relay-claim.ts (1)
61-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd or document how operators clear a stranded claim channel.
If
channels.createsucceeds butrelay.messages.sendfails, the channel exists with no claim record. Laterclaim/getcalls throwhas 0 immutable claim records; refusing dispatchand block the source key.RelayChannelNotionClaimStoreonly exposesclaim,get, anddispose; add arelease/cleanupmethod or document the operatoragent-relaychannel-deletion step.🤖 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-claim.ts` around lines 61 - 86, The claim flow around RelayChannelNotionClaimStore must provide a way to clear a channel created before relay.messages.send fails. Add a release/cleanup operation that deletes the stranded channel, or document the exact agent-relay channel-deletion procedure for operators; ensure it is accessible alongside claim, get, and dispose and targets the source key’s durable Notion claim channel.
🤖 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`:
- Around line 226-234: Update the README paragraph around workerMountRoot to
explicitly name workerMountTransport as the field that defaults to `{ "kind":
"local" }`, while preserving the existing behavior and explanation of omitted
manifests.
In `@src/intake/notion.ts`:
- Around line 611-620: Update the existing-claim branch around workspace.find so
it distinguishes an unavailable find capability from a completed lookup with no
running agent. Preserve the current fail-closed blocked result, but use a reason
stating that the dispatcher lacks lookup support when find is absent and the
existing no-running-agent reason only when find was invoked and returned no
agent.
---
Nitpick comments:
In `@src/cli/fleet.ts`:
- Around line 466-505: Flatten the shutdown sequence currently anchored by the
outer try/finally chain into sequential guarded disposal steps, preserving the
order: notionClaims, notionContracts, mount, fleet, then reporter shutdown. Keep
independent cleanup running after earlier failures; retain the existing warning
handling for Notion and reporter failures, while allowing mount and fleet
disposal failures to propagate as they do now.
In `@src/intake/notion-relay-claim.ts`:
- Around line 61-86: The claim flow around RelayChannelNotionClaimStore must
provide a way to clear a channel created before relay.messages.send fails. Add a
release/cleanup operation that deletes the stranded channel, or document the
exact agent-relay channel-deletion procedure for operators; ensure it is
accessible alongside claim, get, and dispose and targets the source key’s
durable Notion claim channel.
In `@src/intake/notion.test.ts`:
- Around line 23-37: Add a test in the Notion intake test suite using a claim
store whose get or claim returns the same sourceKey with a different digest,
then assert the operation produces the blocked result and the expected durable
Notion claim digest mismatch error. Keep the existing shared claims fixture
unchanged and target the claimNotionTask or observeNotionClaim path as
appropriate.
- Around line 657-688: The existing concurrency test only covers the blocked
path because WorkspaceTaskDispatcher.find returns undefined. Add a test case in
the same suite where the second run’s find resolves to a running agent,
exercising runNotionIntake’s reconstructed workspace receipt path; assert the
second result is already-dispatched with the expected agent and
workspace.dispatch is called exactly 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: 8d5869ee-8f4f-4043-a4d9-ed36f8f7ea87
📒 Files selected for processing (12)
README.mddocs/notion-ticket-feeder-assessment.mdsrc/__tests__/dist-entrypoints.test.tssrc/cli/fleet.test.tssrc/cli/fleet.tssrc/config/schema.test.tssrc/config/schema.tssrc/intake/index.tssrc/intake/notion-relay-claim.test.tssrc/intake/notion-relay-claim.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.
2 issues found across 12 files
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="src/intake/notion.ts">
<violation number="1" location="src/intake/notion.ts:523">
P1: A transient failure after this claim succeeds can permanently dead-letter the task: no receipt is written, and every retry sees the existing claim and refuses to create the issue or spawn the agent. A durable completion/retry reconciliation path or an explicit operator-removable claim is needed for failures after claim acquisition.</violation>
</file>
<file name="src/intake/notion-relay-claim.test.ts">
<violation number="1" location="src/intake/notion-relay-claim.test.ts:53">
P3: This test gives logical-contract coverage, not cross-machine coverage. Both dispatchers share a single in-process Map whose create-then-join and immediately-visible-message semantics are hand-coded to always produce one claim, so it cannot detect a real cross-host regression such as non-atomic channel creation across processes or delayed message visibility (which would yield 2 records or a 0-record race). Consider renaming the intent as 'workspace-uniqueness contract' and adding a separate two-process/e2e test against the real relay if cross-machine durability is a stated goal.</violation>
</file>
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 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
3 participants
Summary
issueSourceremains the lifecycle adapter selector (linear | github)Assessment
issueSource: notion.issueSourceselects discovery plus lifecycle writeback. The separate intake command normalizes Notion tasks into GitHub lifecycle or exact-path work.Full assessment:
docs/notion-ticket-feeder-assessment.md.Fail-closed behavior
statePathcannot create a second issue or spawn a second agentVerification
npm run builddist/intake/notion-relay-claim.jsand its declarationsLive proof / current limitation
Chief already has a live Notion-to-agent receipt for
notion-9a84f582-8fc3bc47onkjg-laptop, with a durable fleet invocation record and portable Relay delivery. Mounted repository pages also produced labeled lifecycle issues (Cloud #2935 and Relay #1433).I did not create another production task: every page in Chief's active manifest already has a receipt, so that would duplicate real work. Current Relayfile health reports Notion as lagging with no sync cursor/watermark, and the Chief snapshots are dated August 5. The remote page is readable and the checked page digest matches the local snapshot, but a fresh-page ingress proof remains blocked until a new Notion sync watermark is observable.
No merge or deployment performed.
Summary by cubic
Makes Notion intake claims workspace-durable to stop duplicate issue creation and agent spawns across machines. Adds strict portable-mount migration safeguards that block uncertain reconciliation and require an acknowledged shared claim before any external action.
Bug Fixes
RelayChannelNotionClaimStore(via@agent-relay/sdk) and require a claim ACK before any issue, body edit, worker spawn, or portable-mount redispatch.<sourceKey>:portable-mount) to prevent double redispatch during portable mount upgrades.RelayChannelNotionContractPublisherwhenworkerMountTransport.kindisrelay-channel.issueSourcetolinear | github(schema rejects'notion').Migration
relay-channelworker delivery still also needs the key.factory-notion-claim-<sha256(sourceKey)>). Verify no matching issue/agent exists before deleting the channel to unblock; for portable upgrades use<sourceKey>:portable-mount.Written for commit 5c13be5. Summary will update on new commits.