feat: integrate disputes with Notion Task Triager and TriageAgent#26
feat: integrate disputes with Notion Task Triager and TriageAgent#26chitcommit wants to merge 3 commits intomainfrom
Conversation
…ter TriageAgent Connect ChittyCommand's dispute system bidirectionally with Notion and add AI-powered dispute scoring via ChittyRouter's TriageAgent. - Add dispute-sync.ts coordinator with 3 public entry points: fireDisputeSideEffects, reconcileNotionDisputes, pushUnlinkedDisputesToNotion - Add notionClient() to integrations.ts for writing task pages to Notion - Add classifyDispute() to routerClient for TriageAgent scoring - Replace direct ledgerClient fire-and-forget with unified side effects (Notion + TriageAgent + Ledger in parallel via Promise.allSettled) - Add bridge route POST /api/bridge/disputes/sync-notion for manual sync - Add cron Phase 10: auto-create disputes from legal-type Notion tasks - Add migration 0012 with metadata indexes for loop-guard lookups - 3-layer loop prevention: metadata flags on both sides + Notion upsert dedup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
📝 WalkthroughWalkthroughAdds dispute synchronization: DB partial indexes, a new dispute-sync module (Notion, TriageAgent, Ledger), Notion/Triage integrations, cron Phase 10 to reconcile legal tasks into disputes, a bridge API to trigger syncs, and route changes to dispatch side effects asynchronously. Changes
Sequence Diagram(s)sequenceDiagram
actor Cron
participant DB as Database
participant Reconciler as reconcileNotionDisputes
participant Notion
participant SideEffects as fireDisputeSideEffects
participant Triage as TriageAgent
participant Ledger
Cron->>Reconciler: Phase 10 trigger
Reconciler->>DB: Query cc_tasks (type=legal, no dispute_id) limit 50
DB-->>Reconciler: Task rows
loop per task
Reconciler->>Notion: validate notion origin / notion_task_id
alt existing dispute by notion_task_id
Reconciler->>DB: link task -> dispute
else create dispute
Reconciler->>DB: insert cc_disputes (set metadata.notion_task_id)
DB-->>Reconciler: new dispute id
Reconciler->>DB: link task -> dispute
Reconciler->>SideEffects: invoke side effects (async)
par side effects
SideEffects->>Notion: linkDisputeToNotion()
SideEffects->>Triage: scoreDisputeWithTriage()
SideEffects->>Ledger: linkDisputeToLedger()
and
Notion-->>SideEffects: result
Triage-->>SideEffects: result
Ledger-->>SideEffects: result
end
SideEffects->>DB: update dispute metadata
end
end
Reconciler-->>Cron: return created count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/lib/integrations.ts (1)
706-710: Include the standard source header on the Notion write path.This new outbound call is the only one in
src/lib/integrations.tsthat skipsX-Source-Service, so it loses the tracing/audit convention the rest of the clients follow.💡 Suggested fix
const res = await fetch('https://api.notion.com/v1/pages', { method: 'POST', headers: { 'Authorization': `Bearer ${creds.token}`, 'Notion-Version': '2022-06-28', 'Content-Type': 'application/json', + 'X-Source-Service': 'chittycommand', },As per coding guidelines "Include
X-Source-Service: chittycommandheader on all outbound service calls".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/integrations.ts` around lines 706 - 710, The Notion write request's headers block (the headers object that currently sets 'Authorization', 'Notion-Version', and 'Content-Type') is missing the required audit/tracing header; add "X-Source-Service": "chittycommand" to that headers object so the Notion write path follows the same outbound convention as other clients in src/lib/integrations.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migrations/0012_dispute_sync_indexes.sql`:
- Around line 5-7: Add a uniqueness constraint on the Notion page id to prevent
duplicate disputes by replacing the non-unique index
idx_cc_disputes_notion_task_id with a UNIQUE index on cc_disputes
((metadata->>'notion_task_id')) WHERE metadata->>'notion_task_id' IS NOT NULL,
and update the reconcileNotionDisputes code to handle duplicate-key races (use
INSERT ... ON CONFLICT DO NOTHING or catch unique-violation errors and proceed)
so concurrent runs cannot create two disputes for the same Notion page.
In `@src/lib/dispute-sync.ts`:
- Around line 192-195: The loop increments pushed for every call to
linkDisputeToNotion even when that function swallows errors and returns void;
change linkDisputeToNotion (or createTask inside it) to return a success
indicator (boolean or created Notion id) or rethrow on failure, then update the
for-loop that calls linkDisputeToNotion(dispute.id, ...) to only increment
pushed when the returned value signals a successful Notion link (e.g., true or
non-null), ensuring pushed accurately counts only actual pushes.
- Around line 71-89: The SELECT that populates legalTasks currently fetches up
to 50 legal/unlinked rows then skips those with no notion_page_id, which can
waste the batch; update the SQL in the legalTasks query to add AND
notion_page_id IS NOT NULL (or (notion_page_id IS NOT NULL)) so only
Notion-origin tasks are counted before ORDER/LIMIT, and also update the index
predicate for idx_cc_tasks_legal_unlinked to include notion_page_id IS NOT NULL
so the index matches the new query predicate.
- Around line 259-265: The triage payload omits zero-dollar disputes because
amount is set with a falsy check; update the ternary used in the
router.classifyDispute call inside src/lib/dispute-sync.ts (the amount property
in the object passed to router.classifyDispute) to explicitly check for
null/undefined (e.g., use != null or !== undefined) so that 0 is preserved and
converted with Number(dispute.amount_at_stake), otherwise set undefined.
In `@src/routes/bridge/disputes.ts`:
- Around line 49-52: The branch handling direction === 'from_notion' only runs
reconcileNotionDisputes(c.env, sql) which operates on the local cc_tasks cache
and thus misses pages not yet synced; update the flow so that when direction is
'from_notion' (or 'both') you first run a sync to pull pages from Notion into
cc_tasks (e.g., call syncNotionTasks(c.env, sql) or the existing sync function
used elsewhere) and only then call reconcileNotionDisputes(c.env, sql);
alternatively add an option/flag to reconcileNotionDisputes to fetch directly
from Notion and use that when direction is 'from_notion'—ensure you reference
reconcileNotionDisputes and syncNotionTasks in the change so the route will
reconcile newly created Notion pages immediately.
- Around line 29-31: The request currently swallows JSON parse errors by using
c.req.json().catch(() => ({})), which lets syncDirectionSchema default direction
to 'both' and trigger unwanted full syncs; change the logic in the block around
parsed = syncDirectionSchema.safeParse(...) so that JSON parse failures are
detected and rejected: explicitly await c.req.json() inside a try/catch (or
remove the .catch), and on any parse error return c.json({ error: 'Malformed
JSON' }, 400) before calling syncDirectionSchema.safeParse; update the return
path where parsed.success is false to keep the existing 400 response with
details from parsed.error.flatten() so malformed JSON never falls through to
start both sync directions.
---
Nitpick comments:
In `@src/lib/integrations.ts`:
- Around line 706-710: The Notion write request's headers block (the headers
object that currently sets 'Authorization', 'Notion-Version', and
'Content-Type') is missing the required audit/tracing header; add
"X-Source-Service": "chittycommand" to that headers object so the Notion write
path follows the same outbound convention as other clients in
src/lib/integrations.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a09dd09f-380a-49a7-9b9f-5719acab66b1
📒 Files selected for processing (7)
migrations/0012_dispute_sync_indexes.sqlsrc/lib/cron.tssrc/lib/dispute-sync.tssrc/lib/integrations.tssrc/routes/bridge/disputes.tssrc/routes/bridge/index.tssrc/routes/disputes.ts
| CREATE INDEX IF NOT EXISTS idx_cc_disputes_notion_task_id | ||
| ON cc_disputes ((metadata->>'notion_task_id')) | ||
| WHERE metadata->>'notion_task_id' IS NOT NULL; |
There was a problem hiding this comment.
Enforce one dispute per Notion page.
The reconciliation flow does an existence check and then inserts. Without a uniqueness guarantee on metadata->>'notion_task_id', concurrent cron/manual runs can materialize two disputes for the same Notion page and fan out duplicate triage/ledger side effects.
💡 Suggested migration tweak
-CREATE INDEX IF NOT EXISTS idx_cc_disputes_notion_task_id
+CREATE UNIQUE INDEX IF NOT EXISTS idx_cc_disputes_notion_task_id
ON cc_disputes ((metadata->>'notion_task_id'))
WHERE metadata->>'notion_task_id' IS NOT NULL;Please pair this with duplicate-key handling in reconcileNotionDisputes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE INDEX IF NOT EXISTS idx_cc_disputes_notion_task_id | |
| ON cc_disputes ((metadata->>'notion_task_id')) | |
| WHERE metadata->>'notion_task_id' IS NOT NULL; | |
| CREATE UNIQUE INDEX IF NOT EXISTS idx_cc_disputes_notion_task_id | |
| ON cc_disputes ((metadata->>'notion_task_id')) | |
| WHERE metadata->>'notion_task_id' IS NOT NULL; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrations/0012_dispute_sync_indexes.sql` around lines 5 - 7, Add a
uniqueness constraint on the Notion page id to prevent duplicate disputes by
replacing the non-unique index idx_cc_disputes_notion_task_id with a UNIQUE
index on cc_disputes ((metadata->>'notion_task_id')) WHERE
metadata->>'notion_task_id' IS NOT NULL, and update the reconcileNotionDisputes
code to handle duplicate-key races (use INSERT ... ON CONFLICT DO NOTHING or
catch unique-violation errors and proceed) so concurrent runs cannot create two
disputes for the same Notion page.
| const legalTasks = await sql` | ||
| SELECT id, title, description, priority, due_date, notion_page_id, metadata | ||
| FROM cc_tasks | ||
| WHERE task_type = 'legal' | ||
| AND backend_status NOT IN ('done', 'verified') | ||
| AND (metadata->>'dispute_id') IS NULL | ||
| ORDER BY priority ASC, created_at ASC | ||
| LIMIT 50 | ||
| `; | ||
|
|
||
| let created = 0; | ||
|
|
||
| for (const task of legalTasks) { | ||
| const taskId = task.id as string; | ||
| const notionPageId = task.notion_page_id as string | null; | ||
|
|
||
| // Skip tasks without a Notion origin — prevents loop where we'd create a | ||
| // Notion page that syncs back as a new task on the next cron run | ||
| if (!notionPageId) continue; |
There was a problem hiding this comment.
Exclude non-Notion tasks before applying LIMIT 50.
Lines 71-79 fetch all legal/unlinked tasks, but Lines 87-89 skip rows without notion_page_id. If enough locally-created legal tasks have notion_page_id = NULL, every run can burn the whole batch on skips and never reach actual Notion-origin tasks.
💡 Suggested fix
const legalTasks = await sql`
SELECT id, title, description, priority, due_date, notion_page_id, metadata
FROM cc_tasks
WHERE task_type = 'legal'
AND backend_status NOT IN ('done', 'verified')
+ AND notion_page_id IS NOT NULL
AND (metadata->>'dispute_id') IS NULL
ORDER BY priority ASC, created_at ASC
LIMIT 50
`;Also mirror the same predicate in idx_cc_tasks_legal_unlinked.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const legalTasks = await sql` | |
| SELECT id, title, description, priority, due_date, notion_page_id, metadata | |
| FROM cc_tasks | |
| WHERE task_type = 'legal' | |
| AND backend_status NOT IN ('done', 'verified') | |
| AND (metadata->>'dispute_id') IS NULL | |
| ORDER BY priority ASC, created_at ASC | |
| LIMIT 50 | |
| `; | |
| let created = 0; | |
| for (const task of legalTasks) { | |
| const taskId = task.id as string; | |
| const notionPageId = task.notion_page_id as string | null; | |
| // Skip tasks without a Notion origin — prevents loop where we'd create a | |
| // Notion page that syncs back as a new task on the next cron run | |
| if (!notionPageId) continue; | |
| const legalTasks = await sql` | |
| SELECT id, title, description, priority, due_date, notion_page_id, metadata | |
| FROM cc_tasks | |
| WHERE task_type = 'legal' | |
| AND backend_status NOT IN ('done', 'verified') | |
| AND notion_page_id IS NOT NULL | |
| AND (metadata->>'dispute_id') IS NULL | |
| ORDER BY priority ASC, created_at ASC | |
| LIMIT 50 | |
| `; | |
| let created = 0; | |
| for (const task of legalTasks) { | |
| const taskId = task.id as string; | |
| const notionPageId = task.notion_page_id as string | null; | |
| // Skip tasks without a Notion origin — prevents loop where we'd create a | |
| // Notion page that syncs back as a new task on the next cron run | |
| if (!notionPageId) continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/dispute-sync.ts` around lines 71 - 89, The SELECT that populates
legalTasks currently fetches up to 50 legal/unlinked rows then skips those with
no notion_page_id, which can waste the batch; update the SQL in the legalTasks
query to add AND notion_page_id IS NOT NULL (or (notion_page_id IS NOT NULL)) so
only Notion-origin tasks are counted before ORDER/LIMIT, and also update the
index predicate for idx_cc_tasks_legal_unlinked to include notion_page_id IS NOT
NULL so the index matches the new query predicate.
| for (const dispute of unlinked) { | ||
| try { | ||
| await linkDisputeToNotion(dispute.id as string, dispute as unknown as DisputeCore, env, sql); | ||
| pushed++; |
There was a problem hiding this comment.
Only count successful Notion links as pushed.
linkDisputeToNotion() catches its own failures and returns void, so Line 195 increments pushed even when credentials are missing or createTask() returns null. The bridge response can therefore report successful pushes that never happened.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/dispute-sync.ts` around lines 192 - 195, The loop increments pushed
for every call to linkDisputeToNotion even when that function swallows errors
and returns void; change linkDisputeToNotion (or createTask inside it) to return
a success indicator (boolean or created Notion id) or rethrow on failure, then
update the for-loop that calls linkDisputeToNotion(dispute.id, ...) to only
increment pushed when the returned value signals a successful Notion link (e.g.,
true or non-null), ensuring pushed accurately counts only actual pushes.
| const result = await router.classifyDispute({ | ||
| entity_id: disputeId, | ||
| entity_type: 'dispute', | ||
| title: dispute.title, | ||
| dispute_type: dispute.dispute_type, | ||
| amount: dispute.amount_at_stake ? Number(dispute.amount_at_stake) : undefined, | ||
| description: dispute.description || undefined, |
There was a problem hiding this comment.
Preserve 0 when building the triage payload.
Line 264 treats 0 as falsy and omits amount entirely. That changes the TriageAgent input for zero-dollar disputes.
💡 Suggested fix
- amount: dispute.amount_at_stake ? Number(dispute.amount_at_stake) : undefined,
+ amount: dispute.amount_at_stake == null ? undefined : Number(dispute.amount_at_stake),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await router.classifyDispute({ | |
| entity_id: disputeId, | |
| entity_type: 'dispute', | |
| title: dispute.title, | |
| dispute_type: dispute.dispute_type, | |
| amount: dispute.amount_at_stake ? Number(dispute.amount_at_stake) : undefined, | |
| description: dispute.description || undefined, | |
| const result = await router.classifyDispute({ | |
| entity_id: disputeId, | |
| entity_type: 'dispute', | |
| title: dispute.title, | |
| dispute_type: dispute.dispute_type, | |
| amount: dispute.amount_at_stake == null ? undefined : Number(dispute.amount_at_stake), | |
| description: dispute.description || undefined, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/dispute-sync.ts` around lines 259 - 265, The triage payload omits
zero-dollar disputes because amount is set with a falsy check; update the
ternary used in the router.classifyDispute call inside src/lib/dispute-sync.ts
(the amount property in the object passed to router.classifyDispute) to
explicitly check for null/undefined (e.g., use != null or !== undefined) so that
0 is preserved and converted with Number(dispute.amount_at_stake), otherwise set
undefined.
| const parsed = syncDirectionSchema.safeParse(await c.req.json().catch(() => ({}))); | ||
| if (!parsed.success) { | ||
| return c.json({ error: 'Invalid request', details: parsed.error.flatten() }, 400); |
There was a problem hiding this comment.
Reject malformed JSON instead of defaulting to a full sync.
Line 29 turns any JSON parse failure into {}, and the schema default then resolves direction to 'both'. A bad request body can therefore kick off both sync directions instead of returning 400.
💡 Suggested fix
-disputesBridgeRoutes.post('/sync-notion', async (c) => {
- const parsed = syncDirectionSchema.safeParse(await c.req.json().catch(() => ({})));
+disputesBridgeRoutes.post('/sync-notion', async (c) => {
+ const rawBody = await c.req.text();
+ let raw: unknown = {};
+
+ if (rawBody.trim()) {
+ try {
+ raw = JSON.parse(rawBody);
+ } catch {
+ return c.json({ error: 'Invalid JSON body' }, 400);
+ }
+ }
+
+ const parsed = syncDirectionSchema.safeParse(raw);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parsed = syncDirectionSchema.safeParse(await c.req.json().catch(() => ({}))); | |
| if (!parsed.success) { | |
| return c.json({ error: 'Invalid request', details: parsed.error.flatten() }, 400); | |
| disputesBridgeRoutes.post('/sync-notion', async (c) => { | |
| const rawBody = await c.req.text(); | |
| let raw: unknown = {}; | |
| if (rawBody.trim()) { | |
| try { | |
| raw = JSON.parse(rawBody); | |
| } catch { | |
| return c.json({ error: 'Invalid JSON body' }, 400); | |
| } | |
| } | |
| const parsed = syncDirectionSchema.safeParse(raw); | |
| if (!parsed.success) { | |
| return c.json({ error: 'Invalid request', details: parsed.error.flatten() }, 400); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/bridge/disputes.ts` around lines 29 - 31, The request currently
swallows JSON parse errors by using c.req.json().catch(() => ({})), which lets
syncDirectionSchema default direction to 'both' and trigger unwanted full syncs;
change the logic in the block around parsed = syncDirectionSchema.safeParse(...)
so that JSON parse failures are detected and rejected: explicitly await
c.req.json() inside a try/catch (or remove the .catch), and on any parse error
return c.json({ error: 'Malformed JSON' }, 400) before calling
syncDirectionSchema.safeParse; update the return path where parsed.success is
false to keep the existing 400 response with details from parsed.error.flatten()
so malformed JSON never falls through to start both sync directions.
| if (direction === 'from_notion' || direction === 'both') { | ||
| try { | ||
| reconciled = await reconcileNotionDisputes(c.env, sql); | ||
| } catch (err) { |
There was a problem hiding this comment.
from_notion only reconciles the local cache.
reconcileNotionDisputes() reads cc_tasks, so this branch will not see pages created in Notion until a separate syncNotionTasks() run has already populated the table. For a manual /sync-notion endpoint, that makes direction: 'from_notion' incomplete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/bridge/disputes.ts` around lines 49 - 52, The branch handling
direction === 'from_notion' only runs reconcileNotionDisputes(c.env, sql) which
operates on the local cc_tasks cache and thus misses pages not yet synced;
update the flow so that when direction is 'from_notion' (or 'both') you first
run a sync to pull pages from Notion into cc_tasks (e.g., call
syncNotionTasks(c.env, sql) or the existing sync function used elsewhere) and
only then call reconcileNotionDisputes(c.env, sql); alternatively add an
option/flag to reconcileNotionDisputes to fetch directly from Notion and use
that when direction is 'from_notion'—ensure you reference
reconcileNotionDisputes and syncNotionTasks in the change so the route will
reconcile newly created Notion pages immediately.
- Fix notionClient to write 'Priority 1' matching actual Notion DB schema - Add disputes.chitty.cc as custom domain alias on the Worker - Add Priority 1 fallback in cron Phase 9 Notion task sync Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 83022
🔴 Critical / Merge Blockers1. Bridge Auth Escalation — Any Authenticated User Gets
|
| # | Severity | Finding | File |
|---|---|---|---|
| 1 | 🔴 Critical | Any user token gains admin scopes on all bridge routes |
src/middleware/auth.ts |
| 2 | 🔴 Critical | TOCTOU race → duplicate disputes; no UNIQUE constraint | migrations/0012_dispute_sync_indexes.sql |
| 3 | 🔴 Critical | Unvalidated external priority written directly to DB | src/lib/dispute-sync.ts |
| 4 | 🟠 High | PATCH clears loop-guard flags → unlimited duplicate side effects | src/routes/disputes.ts |
| 5 | 🟠 High | User-supplied metadata silently suppresses Notion/Ledger linkage | src/routes/disputes.ts |
| 6 | 🟡 Medium | No rate limiting on bridge sync endpoint | src/routes/bridge/disputes.ts |
| 7 | 🟡 Medium | LIMIT 50 with no cursor → backlog drain requires manual repeated calls | src/lib/dispute-sync.ts |
| 8 | 🟡 Medium | counterparty: 'Unknown' hardcoded in auto-created disputes |
src/lib/dispute-sync.ts |
| 9 | 🔵 Info | Notion API error body logged verbatim | src/lib/integrations.ts |
Findings #1, #2, and #3 are merge-blocking per the repository's must-fix policy. Findings #4 and #5 together create a reliable privilege abuse path once #1 is exploited.
🧠 Learnings used
Learnt from: CR
Repo: chittyos/chittycommand PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-04T04:42:57.380Z
Learning: Resolve must-fix review comments (security, correctness, compliance, merge blockers) before merge
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/cron.ts (1)
653-653: Prefer nullish fallback for numeric priority.Line 653 uses
||, so an explicit numeric0would be treated as missing and replaced by the fallback.??keeps the same behavior for absent fields without changing valid numeric values.♻️ Suggested change
- const priority = extractNotionNumber(props['Priority 1']) || extractNotionNumber(props['Priority']) || 5; + const priority = + extractNotionNumber(props['Priority 1']) ?? + extractNotionNumber(props['Priority']) ?? + 5;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/cron.ts` at line 653, The priority assignment uses || which treats 0 as falsy; update the priority computation so it uses nullish coalescing (??) instead: call extractNotionNumber(props['Priority 1']) ?? extractNotionNumber(props['Priority']) ?? 5, preserving valid numeric 0 while still falling back when values are null/undefined; change the expression where the variable priority is declared to use ?? and keep references to extractNotionNumber, props['Priority 1'], and props['Priority'] unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/cron.ts`:
- Around line 125-135: reconcileNotionDisputes can race and create duplicate
cc_disputes because there is no DB-enforced uniqueness; add a DB-level dedupe by
creating a UNIQUE expression index (or constraint) on the notion task id
extracted from metadata (e.g. UNIQUE ((metadata->>'notion_task_id'))) for the
cc_disputes table via a new migration (or update
migrations/0012_dispute_sync_indexes.sql), and then update
reconcileNotionDisputes to use an idempotent insert (INSERT ... ON CONFLICT DO
NOTHING or handle unique-violation error) so the cron path and bridge route
cannot both create duplicates; reference reconcileNotionDisputes, cc_disputes,
migrations/0012_dispute_sync_indexes.sql and migrations/0002_command_legal.sql
when making these changes.
---
Nitpick comments:
In `@src/lib/cron.ts`:
- Line 653: The priority assignment uses || which treats 0 as falsy; update the
priority computation so it uses nullish coalescing (??) instead: call
extractNotionNumber(props['Priority 1']) ??
extractNotionNumber(props['Priority']) ?? 5, preserving valid numeric 0 while
still falling back when values are null/undefined; change the expression where
the variable priority is declared to use ?? and keep references to
extractNotionNumber, props['Priority 1'], and props['Priority'] unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c605eaf9-1390-453e-a275-2be2cdc1bf91
📒 Files selected for processing (3)
src/lib/cron.tssrc/lib/integrations.tswrangler.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/integrations.ts
| // Phase 10: Dispute-Notion reconciliation | ||
| // Auto-creates cc_disputes from legal tasks not yet linked. | ||
| try { | ||
| const disputesSynced = await reconcileNotionDisputes(env, sql); | ||
| if (disputesSynced > 0) { | ||
| recordsSynced += disputesSynced; | ||
| console.log(`[cron:dispute_reconcile] created ${disputesSynced} disputes from Notion legal tasks`); | ||
| } | ||
| } catch (err) { | ||
| console.error('[cron:dispute_reconcile] failed:', err); | ||
| } |
There was a problem hiding this comment.
Add a DB-enforced dedupe guard before running this on cron.
reconcileNotionDisputes() in src/lib/dispute-sync.ts:67-171 currently does a SELECT on metadata->>'notion_task_id' and then an INSERT, but the schema only has a non-unique index for that key in migrations/0012_dispute_sync_indexes.sql:1-10, and cc_disputes has no unique constraint in migrations/0002_command_legal.sql:28-44. Now that this path also runs on the daily cron, it can race with the bridge route or another worker and create duplicate disputes plus duplicate downstream side effects for the same Notion task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/cron.ts` around lines 125 - 135, reconcileNotionDisputes can race and
create duplicate cc_disputes because there is no DB-enforced uniqueness; add a
DB-level dedupe by creating a UNIQUE expression index (or constraint) on the
notion task id extracted from metadata (e.g. UNIQUE
((metadata->>'notion_task_id'))) for the cc_disputes table via a new migration
(or update migrations/0012_dispute_sync_indexes.sql), and then update
reconcileNotionDisputes to use an idempotent insert (INSERT ... ON CONFLICT DO
NOTHING or handle unique-violation error) so the cron path and bridge route
cannot both create duplicates; reference reconcileNotionDisputes, cc_disputes,
migrations/0012_dispute_sync_indexes.sql and migrations/0002_command_legal.sql
when making these changes.
Instructions for configuring the Notion Task Triager agent to classify legal/dispute emails into the Business Task Tracker with properties that auto-sync into ChittyCommand disputes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 90655
✅ Confirmation of Prior Findings + New Issues🔴 Confirmed Critical / Merge BlockersPrior Finding
|
| # | Severity | Status | Finding |
|---|---|---|---|
| 1 | 🔴 Critical | ✅ Confirmed + Expanded | Any user/ChittyAuth token gets unchecked bridge access |
| 2 | 🔴 Critical | ✅ Confirmed | TOCTOU → duplicate disputes; no UNIQUE constraint |
| 3 | 🔴 Critical | ✅ Confirmed + Expanded | No schema validation on TriageAgent response; raw values written to DB |
| N | 🔴 Critical | 🆕 New | disputes.chitty.cc exposes full Worker, not a 307 redirect |
| 4 | 🟠 High | ✅ Confirmed | PATCH metadata: {} wipes loop-guard flags |
| 5 | 🟠 High | ✅ Confirmed | User-supplied metadata silently suppresses Notion/Ledger linkage |
| 6 | 🟡 Medium | ✅ Confirmed | No rate limiting on bridge sync endpoint |
| 7 | 🟡 Medium | ✅ Confirmed | LIMIT 50, no cursor — backlog drains slowly |
| 8 | 🟡 Medium | ✅ Confirmed | counterparty: 'Unknown' hardcoded, no enrichment path |
| 9 | 🔵 Info | ✅ Confirmed | Notion error body logged verbatim |
| R | 🔵 Info | 🆕 New | scoreDisputeWithTriage lacks idempotency guard |
| S | 🔵 Info | 🆕 New | Infra IDs committed in wrangler.toml |
Findings #1, #2, #3, and #N are merge-blocking per the repository's must-fix policy. Findings #4 and #5 together create a reliable side-effect manipulation path once #1 is exploited.
🧠 Learnings used
Learnt from: CR
Repo: chittyos/chittycommand PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-04T04:42:57.380Z
Learning: Resolve must-fix review comments (security, correctness, compliance, merge blockers) before merge
Learnt from: CR
Repo: chittyos/chittycommand PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-04T04:42:57.380Z
Learning: One concern area per PR: scope, risk/blast radius, test evidence, rollback plan, and migration impact
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/notion-task-triager-instructions.md (3)
58-68: Clarify the best-effort nature of the automated workflow.The workflow description implies that all steps (cron sync, TriageAgent scoring, ChittyLedger case creation) always succeed together. However, context snippet 2 (src/lib/dispute-sync.ts:118-162) and the PR test plan indicate that TriageAgent and Ledger operations are non-blocking—if they fail, the dispute still syncs successfully. Users should understand that:
- The dispute will always be created in ChittyCommand (guaranteed)
- TriageAgent scoring and ChittyLedger case creation are best-effort (may fail without blocking the sync)
- Failures in these side effects won't prevent the dispute from appearing in the dashboard
Consider adding a note to manage user expectations about partial failures.
📝 Suggested clarification
Once you create a legal task in the Business Task Tracker: 1. **ChittyCommand's daily cron** (6 AM CT) syncs new legal tasks into `cc_disputes` -2. **TriageAgent** automatically scores the dispute for severity and priority -3. **ChittyLedger** creates a case record for chain-of-custody tracking +2. **TriageAgent** attempts to score the dispute for severity and priority (best-effort) +3. **ChittyLedger** attempts to create a case record for chain-of-custody tracking (best-effort) 4. The dispute appears in the ChittyCommand dashboard for action tracking -You do NOT need to create anything in ChittyCommand directly — the sync is automatic. +You do NOT need to create anything in ChittyCommand directly — the sync is automatic. If TriageAgent or Ledger fail, the dispute will still be created and visible in the dashboard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/notion-task-triager-instructions.md` around lines 58 - 68, Update the "What Happens After Creation" section to state that the cron sync into cc_disputes (ChittyCommand) is guaranteed but the subsequent TriageAgent scoring and ChittyLedger case creation are best-effort and non-blocking: mention that TriageAgent (e.g., TriageAgent.score/triage call in src/lib/dispute-sync.ts) and ChittyLedger.createCase may fail without preventing the dispute from being created/synced and appearing in the ChittyCommand dashboard, and advise users that partial failures of these side effects will not stop the dispute from being visible in the dashboard.
34-47: Consider documenting tag validation behavior.The tag guidelines specify exact tag values (e.g.,
Insurance-claim,Court-filing), but context snippet 3 (src/lib/cron.ts:645-675) shows that tags are extracted and stored without validation against this list. Users might create typos (e.g., "Insuranceclaim") or custom tags, which could break downstream filtering or reporting. Consider either:
- Adding a note that these are recommended tags and custom tags are permitted (if that's the intent), or
- Implementing tag validation during Notion sync to ensure only these values are accepted
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/notion-task-triager-instructions.md` around lines 34 - 47, The docs list a fixed set of tags but the Notion sync code in src/lib/cron.ts (tag extraction block around lines 645-675) currently accepts any tag; update behavior by either: (A) editing docs/notion-task-triager-instructions.md to explicitly state these are recommended tags and that custom tags are permitted, or (B) adding validation in the tag-extraction logic in src/lib/cron.ts to normalize (trim/case) incoming tags, check them against the canonical list (Dispute, Insurance-claim, Court-filing, Property-issue, Vendor-dispute, Legal-deadline, Payment, Tax, Utility), and then either reject/replace unknown tags (e.g., map to "Custom" or skip) while logging a warning so downstream filters remain consistent. Ensure the implementation uses the existing tag-extraction function/block so behavior is centralized and add a concise doc note if choosing validation.
48-57: Consider noting priority bounds are advisory.The priority guidance suggests a 1-10 scale, but the code does not enforce these bounds during Notion sync (context snippet 3, line 654 accepts any number with a fallback to 5 if missing). While this is primarily user guidance, you might consider adding a note that values outside 1-10 are technically permitted but may behave unexpectedly in ChittyCommand's priority sorting or filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/notion-task-triager-instructions.md` around lines 48 - 57, Update the "Priority Guidance" section to explicitly state that the 1-10 bounds are advisory: note that the Notion sync parsing logic accepts any numeric priority (with a fallback to 5 when missing) so values outside 1-10 are technically permitted but may behave unexpectedly in ChittyCommand's priority sorting/filtering; reference the priority field parsing in the Notion sync implementation (the code that reads/returns the priority value) so maintainers know why the advisory is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/notion-task-triager-instructions.md`:
- Around line 20-33: The docs fail to state that the counterparty property is
auto-populated as "Unknown" for disputes created from Notion; add a new row or a
short note directly below the Required Properties table stating: "counterparty —
defaults to 'Unknown' for Notion-imported disputes; update manually in
ChittyCommand if known." Reference the code that enforces this default (the
assignment setting counterparty = "Unknown" in the dispute-sync logic) so
readers know why the field may need manual correction after sync.
---
Nitpick comments:
In `@docs/notion-task-triager-instructions.md`:
- Around line 58-68: Update the "What Happens After Creation" section to state
that the cron sync into cc_disputes (ChittyCommand) is guaranteed but the
subsequent TriageAgent scoring and ChittyLedger case creation are best-effort
and non-blocking: mention that TriageAgent (e.g., TriageAgent.score/triage call
in src/lib/dispute-sync.ts) and ChittyLedger.createCase may fail without
preventing the dispute from being created/synced and appearing in the
ChittyCommand dashboard, and advise users that partial failures of these side
effects will not stop the dispute from being visible in the dashboard.
- Around line 34-47: The docs list a fixed set of tags but the Notion sync code
in src/lib/cron.ts (tag extraction block around lines 645-675) currently accepts
any tag; update behavior by either: (A) editing
docs/notion-task-triager-instructions.md to explicitly state these are
recommended tags and that custom tags are permitted, or (B) adding validation in
the tag-extraction logic in src/lib/cron.ts to normalize (trim/case) incoming
tags, check them against the canonical list (Dispute, Insurance-claim,
Court-filing, Property-issue, Vendor-dispute, Legal-deadline, Payment, Tax,
Utility), and then either reject/replace unknown tags (e.g., map to "Custom" or
skip) while logging a warning so downstream filters remain consistent. Ensure
the implementation uses the existing tag-extraction function/block so behavior
is centralized and add a concise doc note if choosing validation.
- Around line 48-57: Update the "Priority Guidance" section to explicitly state
that the 1-10 bounds are advisory: note that the Notion sync parsing logic
accepts any numeric priority (with a fallback to 5 when missing) so values
outside 1-10 are technically permitted but may behave unexpectedly in
ChittyCommand's priority sorting/filtering; reference the priority field parsing
in the Notion sync implementation (the code that reads/returns the priority
value) so maintainers know why the advisory is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b1c22c3-1939-46af-bc3b-dfbc7a8b40cf
📒 Files selected for processing (1)
docs/notion-task-triager-instructions.md
| ### Required Properties for Legal Tasks | ||
|
|
||
| When creating a task page in the **Business Task Tracker** database for dispute/legal items: | ||
|
|
||
| | Property | Value | Notes | | ||
| |----------|-------|-------| | ||
| | **Title** | Clear, descriptive title | e.g. "Water damage claim — 123 Main St — Allstate denial" | | ||
| | **Type** | `Legal` | MUST be "Legal" for ChittyCommand to pick it up as a dispute | | ||
| | **Source** | `Email` | Use "Email" for email-ingested items, "Mention" for @-mentions | | ||
| | **Priority 1** | 1-10 (number) | 1 = most urgent. Use 1-3 for court deadlines, 4-6 for active disputes, 7-10 for monitoring | | ||
| | **Tags** | One or more from the list below | Helps categorize the dispute type | | ||
| | **Description** | Summarize the key facts | Include: who, what, amounts, dates, and any deadlines mentioned | | ||
| | **Due Date** | Set if there's an explicit deadline | Court dates, response deadlines, filing windows | | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document the counterparty limitation.
The required properties table does not mention that the counterparty field will be auto-populated as 'Unknown' when disputes are created from Notion legal tasks. Users may expect this to be extracted from Notion task properties (e.g., from the Description or a dedicated field), but context snippet 2 (src/lib/dispute-sync.ts:123) shows it is hardcoded. Consider adding a row or note explaining this limitation so users understand they may need to update the counterparty field manually in ChittyCommand after sync.
📝 Suggested addition to the table
Add a note below the table:
| **Due Date** | Set if there's an explicit deadline | Court dates, response deadlines, filing windows |
+
+> **Note**: The `Counterparty` field is automatically set to "Unknown" during sync. You can update it manually in ChittyCommand after the dispute is created.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Required Properties for Legal Tasks | |
| When creating a task page in the **Business Task Tracker** database for dispute/legal items: | |
| | Property | Value | Notes | | |
| |----------|-------|-------| | |
| | **Title** | Clear, descriptive title | e.g. "Water damage claim — 123 Main St — Allstate denial" | | |
| | **Type** | `Legal` | MUST be "Legal" for ChittyCommand to pick it up as a dispute | | |
| | **Source** | `Email` | Use "Email" for email-ingested items, "Mention" for @-mentions | | |
| | **Priority 1** | 1-10 (number) | 1 = most urgent. Use 1-3 for court deadlines, 4-6 for active disputes, 7-10 for monitoring | | |
| | **Tags** | One or more from the list below | Helps categorize the dispute type | | |
| | **Description** | Summarize the key facts | Include: who, what, amounts, dates, and any deadlines mentioned | | |
| | **Due Date** | Set if there's an explicit deadline | Court dates, response deadlines, filing windows | | |
| ### Required Properties for Legal Tasks | |
| When creating a task page in the **Business Task Tracker** database for dispute/legal items: | |
| | Property | Value | Notes | | |
| |----------|-------|-------| | |
| | **Title** | Clear, descriptive title | e.g. "Water damage claim — 123 Main St — Allstate denial" | | |
| | **Type** | `Legal` | MUST be "Legal" for ChittyCommand to pick it up as a dispute | | |
| | **Source** | `Email` | Use "Email" for email-ingested items, "Mention" for `@-mentions` | | |
| | **Priority 1** | 1-10 (number) | 1 = most urgent. Use 1-3 for court deadlines, 4-6 for active disputes, 7-10 for monitoring | | |
| | **Tags** | One or more from the list below | Helps categorize the dispute type | | |
| | **Description** | Summarize the key facts | Include: who, what, amounts, dates, and any deadlines mentioned | | |
| | **Due Date** | Set if there's an explicit deadline | Court dates, response deadlines, filing windows | | |
| > **Note**: The `Counterparty` field is automatically set to "Unknown" during sync. You can update it manually in ChittyCommand after the dispute is created. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/notion-task-triager-instructions.md` around lines 20 - 33, The docs fail
to state that the counterparty property is auto-populated as "Unknown" for
disputes created from Notion; add a new row or a short note directly below the
Required Properties table stating: "counterparty — defaults to 'Unknown' for
Notion-imported disputes; update manually in ChittyCommand if known." Reference
the code that enforces this default (the assignment setting counterparty =
"Unknown" in the dispute-sync logic) so readers know why the field may need
manual correction after sync.
1 participant
Summary
/agents/triage/classifyfor AI-powered severity/prioritydispute-sync.tscoordinator running Notion + TriageAgent + Ledger in parallelPOST /api/bridge/disputes/sync-notionfor manual bidirectional reconciliationNew files
src/lib/dispute-sync.ts— Core sync coordinatorsrc/routes/bridge/disputes.ts— Manual sync bridge routemigrations/0012_dispute_sync_indexes.sql— Metadata indexes for loop guardsModified files
src/lib/integrations.ts— AddednotionClient()androuterClient.classifyDispute()src/routes/disputes.ts— WiredfireDisputeSideEffectsviawaitUntil()src/routes/bridge/index.ts— Registered disputes bridgesrc/lib/cron.ts— Added Phase 10 reconciliationPre-deploy setup required
disputes.chitty.cc→command.chitty.cc/api/disputes(307)notion:dispute_database_idto Business Task Tracker DB IDnpm run db:migratefor index creationTest plan
POST /api/disputesreturns 201, logs show Notion task + TriageAgent + Ledger side effectsPOST /api/bridge/disputes/sync-notion { direction: "both" }returns{ pushed, reconciled }notionClientreturns null gracefully when KV keys missing🤖 Generated with Claude Code
Summary by CodeRabbit