feat(v2-arch): strip local dispute tables, add cc_node_leases, align docs#130
feat(v2-arch): strip local dispute tables, add cc_node_leases, align docs#130chitcommit wants to merge 6 commits into
Conversation
…micity and idempotency
- DROP cc_disputes, cc_dispute_correspondence (truth → ChittyCases service) - ADD cc_node_leases: cluster leader election + heartbeat (migration 0020) - schema.ts: remove local dispute tables, linked_dispute_id → text ref - docs/ARCHITECTURE_V2.md: principled refactor spec (ChittyCommand = cockpit) - CHARTER.md / CHITTY.md / AGENTS.md: embed v2 core principles + routing rules - tests/transactions.test.ts: fix TS typecheck errors (Hono Variables type) Fixes: command.chitty.cc health probe degraded (cc_node_leases missing) Ref: docs/ARCHITECTURE_V2.md — ChittyCommand does not own truth
|
|
To use Codex here, create a Codex account and connect to github. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
chittycommand | edb51e6 | Jul 26 2026, 05:55 PM |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - name: Install 1Password CLI | ||
| uses: 1password/install-cli-action@v1 | ||
| - name: Install chittysecrets CLI | ||
| uses: chittysecrets/install-cli-action@v1 |
📝 WalkthroughWalkthroughThe PR updates ChittyCommand’s architecture and governance documentation, migrates secret-management terminology to chittysecrets, removes local dispute storage, adds node leases, broadens SQL compatibility, and introduces authenticated sync status and transactional triage intent flows with idempotency tests. ChangesCredential management terminology
Architecture and governance model
Canonical data model and execution compatibility
Transactional intent and authenticated API flows
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: edb51e64f1
ℹ️ 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".
| DROP TABLE IF EXISTS cc_dispute_correspondence CASCADE; | ||
| DROP TABLE IF EXISTS cc_disputes CASCADE; |
There was a problem hiding this comment.
Applying this migration immediately breaks the still-mounted dispute functionality: src/routes/disputes.ts queries both dropped tables, src/lib/triage.ts reads cc_disputes, and phase 10 of src/lib/cron.ts still inserts and updates it through reconcileNotionDisputes. The dashboard, legal, recommendation, MCP, and agent paths contain additional live references, so production requests and scheduled work will start failing with relation does not exist; rewire or remove these consumers before dropping the tables, or retain a compatibility projection until that work is complete.
Useful? React with 👍 / 👎.
| linkedObligationId: uuid('linked_obligation_id').references(() => ccObligations.id), | ||
| linkedAccountId: uuid('linked_account_id').references(() => ccAccounts.id), | ||
| linkedDisputeId: uuid('linked_dispute_id').references(() => ccDisputes.id), | ||
| linkedDisputeId: text('linked_dispute_id'), // Migrating away from local FK to ChittyCases reference |
There was a problem hiding this comment.
Changing the Drizzle declaration does not alter an existing database column. On databases created by migrations 0002/0003, cc_documents.linked_dispute_id and cc_recommendations.dispute_id remain UUID columns after migration 0020 because DROP ... CASCADE removes their foreign-key constraints but does not change their types. Consequently, the newly advertised non-UUID ChittyCases canonical IDs will fail on insert; migration 0020 needs explicit ALTER COLUMN ... TYPE text USING ...::text operations for both columns.
Useful? React with 👍 / 👎.
| FROM cc_intents | ||
| WHERE intent_type = ${intent_type} | ||
| AND metadata->>'idempotency_key' = ${idempotencyKey} | ||
| LIMIT 1 |
There was a problem hiding this comment.
When two retries with the same Idempotency-Key and intent_type execute concurrently, both statements can observe an empty existing_intent snapshot and each insert a separate goal, plan, and pending intent. Because there is no unique index for this key and the insert has no ON CONFLICT handling, the endpoint's idempotency guarantee can produce duplicate executable actions; enforce the key with a database uniqueness constraint and use the conflict result to fetch the winner.
Useful? React with 👍 / 👎.
| FROM cc_intents | ||
| WHERE intent_type = ${intent_type} | ||
| AND metadata->>'idempotency_key' = ${idempotencyKey} | ||
| LIMIT 1 |
There was a problem hiding this comment.
For multiple elevated triage users or service principals, this lookup is global across the entire table and ignores userId/goal ownership. If two callers reuse a common key for the same intent type, the second caller receives the first caller's intent, including its payload and identifiers, instead of creating its own work; include the authenticated owner in the stored idempotency identity and lookup.
Useful? React with 👍 / 👎.
| INSERT INTO cc_intents (plan_id, goal_id, intent_type, payload, status, priority, privilege, space, metadata) | ||
| SELECT id, goal_id, ${intent_type}, ${JSON.stringify(payload)}::jsonb, 'pending', ${finalPriority}, ${parsedPrivilege ?? 'public'}, ${parsedSpace ?? 'business'}, ${JSON.stringify({idempotency_key: idempotencyKey})}::jsonb | ||
| FROM new_plan |
There was a problem hiding this comment.
Intents created through this endpoint have no sovereignty snapshot, but their metadata contains only the idempotency key (or {} in the no-key branch). When the daemon executes one without a global actorChittyId override, meta/executors/dispatch.ts can only recover the actor from metadata.actorChittyId or metadata.ownerChittyId, so it records a sovereignty refusal and fails the intent even though userId was stored on the related goal; persist that actor in the intent metadata or create a fresh sovereignty assessment here.
Useful? React with 👍 / 👎.
| const scopes = c.get("scopes") || []; | ||
| const userId = c.get("userId") || "system"; | ||
| const hasAuth = scopes.some((s) => s === 'chittytriage:write' || s === 'admin' || s === '*'); | ||
| if (!hasAuth) { | ||
| return c.json({ error: 'Insufficient scope' }, 403); |
There was a problem hiding this comment.
The outer requireTriageScope middleware explicitly recognizes chittytriage:admin, so a principal carrying that canonical elevated scope is admitted to /api/triage/*, but this endpoint's second authorization check omits it and returns 403. Include chittytriage:admin here or rely on the already-applied middleware so triage administrators can create intents.
Useful? React with 👍 / 👎.
| const p = Number(priority); | ||
| const finalPriority = Number.isFinite(p) ? Math.floor(p) : 5; |
There was a problem hiding this comment.
This coercion accepts values outside the repository's normal 1–10 priority range, including null or an empty string as priority 0 and arbitrary negative integers. Because pending intents are claimed with ORDER BY priority ASC, malformed requests can permanently jump ahead of legitimate critical work or distort queue ordering; validate the body with the existing Zod conventions and reject priorities outside the supported range.
Useful? React with 👍 / 👎.
| const [log] = await sql` | ||
| SELECT id, source, sync_type, status, records_synced, error_message, started_at, completed_at | ||
| FROM cc_sync_log WHERE id = ${syncId} | ||
| `; |
There was a problem hiding this comment.
For a request such as /api/sync/status/not-a-uuid, PostgreSQL attempts to compare the UUID cc_sync_log.id column with the malformed parameter and raises invalid input syntax for type uuid, producing a 500 through the global error handler rather than a client error. Validate sync_id as a UUID before issuing the query and return 400 for malformed route parameters.
Useful? React with 👍 / 👎.
| env: IntentEnv & Record<string, unknown>, | ||
| intentId: string, | ||
| options: { actorChittyId?: string; freshnessMs?: number } = {}, | ||
| tx?: NeonQueryFunction<false, false> | ||
| ): Promise<import('./executors/types').ExecutorResult> { | ||
| // Lazy import to avoid forcing the executor registry on every meta/intent | ||
| // consumer (and to keep the existing module's surface stable). | ||
| const { dispatch } = await import('./executors'); | ||
|
|
||
| const sql = getSql(env); | ||
| const sql = tx ?? getSql(env); |
There was a problem hiding this comment.
When a caller uses the newly added tx argument, only the direct state updates use it: the initial and retry reads call getIntent without tx, dispatch opens its own SQL connection, and completeIntent/failIntent are also called without tx. An intent created or claimed earlier in the transaction may therefore appear missing to those operations, and an external action can complete while the transactional row remains claimed or later rolls back; either propagate the transaction through every database collaborator or do not expose transactional execution for this function.
Useful? React with 👍 / 👎.
| // Determine if it's a terminal state | ||
| const isTerminal = ['completed', 'error', 'skipped'].includes(log.status); |
There was a problem hiding this comment.
The existing bridge scrape routes write cc_sync_log.status = 'success' when a scrape finishes successfully, but this polling endpoint recognizes only completed, error, and skipped as terminal. Polling one of those completed scrape executions therefore returns is_terminal: false and instructs the client to continue polling forever; include success in the terminal-state set or normalize all producers to the same status vocabulary.
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
meta/intent.ts (2)
549-631: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
executeIntentdoesn't forwardtxto its own nested calls.
executeIntentaccepts an optionaltxand uses it for the directsql\...`claim/update statements (Lines 559, 574, 617), butgetIntent(env, intentId)(Lines 562, 582),completeIntent(env, intentId)(Line 621), andfailIntent(env, intentId, ...)(Line 625) are all called without passingtxthrough — so they fall back to a brand-newgetSql(env)connection instead. If a caller passestx` expecting the whole read-claim-complete/fail flow to participate in one transaction, part of it silently runs outside that transaction, which can produce inconsistent reads or lost updates under concurrent execution.🔧 Proposed fix — forward `tx` to nested calls
- const current = await getIntent(env, intentId); + const current = await getIntent(env, intentId, tx); @@ - const fresh = await getIntent(env, intentId); + const fresh = await getIntent(env, intentId, tx); @@ - await completeIntent(env, intentId); + await completeIntent(env, intentId, undefined, tx); @@ - await failIntent(env, intentId, result.error ?? 'unknown error').catch( + await failIntent(env, intentId, result.error ?? 'unknown error', undefined, tx).catch( () => null, );🤖 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 `@meta/intent.ts` around lines 549 - 631, Forward the optional tx transaction through every nested intent operation in executeIntent: pass it to getIntent for both initial and reload reads, and to completeIntent and failIntent for status updates. Keep the existing sql selection and dispatch behavior unchanged so the entire read-claim-complete/fail flow uses the caller-provided transaction when present.
100-106: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t expect shared
NeonQueryFunction<F,F>values to coordinate statements.
getSqlreturns plainneon(conn), and callers currently only pass plain query functions through thistxparameter. With that shape, separatesqlcalls undercreateGoal→createPlan→createIntentdo not share a Postgres transaction, so atomic-create workflows must wrap them with Neon Serverlesssql.transaction([...])rather than forwarding a separate one-shot query function.🤖 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 `@meta/intent.ts` around lines 100 - 106, Update getSql and the createGoal → createPlan → createIntent workflow so atomic creates use Neon Serverless sql.transaction([...]) for all statements instead of forwarding separate one-shot NeonQueryFunction values through tx. Preserve the existing connection resolution and ensure each nested operation participates in the same transaction.src/routes/sync.ts (1)
11-18: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGuard
GET /statuswith the same per-source sync scopes.
GET /statusreturns the latest row for everycc_sync_log.source, but unlikeGET /status/:sync_idandPOST /trigger/:source, it never scopes bychittycommand:sync:${source}orchittycommand:sync:*. A caller with only partial sync permissions can still read restricted sources such asmercury,plaid, andchittyfinancethrough the list endpoint. Add source-level filtering/authorization, or at least exclude non-scoped sources before returning the list.🤖 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/routes/sync.ts` around lines 11 - 18, Update the GET /status handler in syncRoutes to enforce the same per-source authorization as the other sync endpoints, permitting rows only when the caller has chittycommand:sync:${source} or chittycommand:sync:*; filter unauthorized sources before returning statuses, including restricted sources such as mercury, plaid, and chittyfinance.
🧹 Nitpick comments (2)
tests/transactions.test.ts (1)
54-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest names/comments claim more than they verify.
The rollback test (Lines 54-72) only checks
res.status === 500and a single mock call — it can't actually confirm "goals=0, plans=0, intents=0" sincesqlis fully mocked; that's a property of the real Postgres CTE, not something a mocked unit test exercises. Similarly, the idempotency test (Lines 74-100) only replays a single sequential call and can't catch theWHERE NOT EXISTSrace condition flagged insrc/routes/triage.ts(Lines 298-330). Consider an integration test against a real/ephemeral Postgres (or at least firing two concurrent requests against the mock) to actually validate the atomicity/idempotency claims, or soften the comments to reflect what's truly being tested.Also applies to: 74-100
🤖 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 `@tests/transactions.test.ts` around lines 54 - 72, The transaction and idempotency tests overstate what mocked sequential requests verify. Update the tests around the rollback case and the idempotency test to either use real/ephemeral Postgres for database guarantees or add concurrent requests against the mock to exercise the race; otherwise, soften test names and comments to describe only the observed status, call count, and sequential behavior.scratch_update_triage.js (1)
1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove this scratch/dev-tooling script before merge.
This is a one-off patch script (hardcoded to
projects/worktrees/migration-chittycommand/...) apparently used to generate thetriage.tsedits, not application code. It shouldn't ship in the repo.On the static analysis hint:
fs.readFileSync(path, ...)/fs.writeFileSync(path, ...)flagged for non-literal path —pathis a fixed constant here, not user-controlled, so this is a false positive rather than a real traversal risk; it doesn't change the recommendation to delete the file.🤖 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 `@scratch_update_triage.js` around lines 1 - 22, Delete the scratch patch script containing the hardcoded worktree path and fs.readFileSync/fs.writeFileSync calls; it is development tooling and must not remain in the repository.Source: Linters/SAST tools
🤖 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 @.github/copilot-instructions.md:
- Line 110: Define one canonical local chittysecrets invocation consistently in
.github/copilot-instructions.md (line 110), AGENTS.md (line 94), and CLAUDE.md
(line 121), explicitly stating whether direct op run or op run -- chittysecrets
run is authoritative. In docs/registration/SUBMISSION_RUNBOOK.md (line 37),
preserve and clarify the chittysecrets cold-source to op run runtime handoff if
it remains two-step. In scripts/onepassword-rotation-audit.sh (line 8) and
templates/governance-baseline/scripts/onepassword-rotation-audit.sh (line 8),
remove the ambiguous “chittysecrets CLI (op)” wording or document that
chittysecrets is supplied through the 1Password CLI.
In @.github/workflows/onepassword-rotation-audit.yml:
- Line 1: Complete the legacy chittysecrets/onepassword naming migration
consistently: update the workflow name and issue/report identifiers in
.github/workflows/onepassword-rotation-audit.yml lines 1 and 49, then mirror
those decisions in
templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml
lines 1 and 49. Clarify the compatibility rationale for chittysecrets
terminology alongside onepassword-named files in
docs/security/ACCESS_BROKER_AND_1PASSWORD_RUNBOOK.md lines 1 and 25.
- Around line 26-27: Pin the Install chittysecrets CLI action to a verified
immutable commit SHA in both .github/workflows/onepassword-rotation-audit.yml
(lines 26-27) and
templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml
(lines 26-27), or replace it in both workflow files with the established
1Password/install-cli-action using an immutable tag.
In `@migrations/0020_v2_arch_strip.sql`:
- Around line 23-24: Complete the ChittyCases cutover across
migrations/0020_v2_arch_strip.sql:23-24, src/agents/tools/actions.ts:134-146,
src/lib/triage.ts:64, and src/routes/disputes.ts:22-36. In the migration, drop
foreign keys, convert cc_documents.linked_dispute_id and
cc_recommendations.dispute_id to text while preserving values, then drop local
dispute tables. Replace local dispute/correspondence queries and recommendation
generation with canonical ChittyCases data, and migrate the disputes route to
the ChittyCases-backed flow before removing those tables.
In `@src/db/schema.ts`:
- Line 135: Add a composite database index for linked_dispute_id and created_at
in the schema definition near linkedDisputeId, then add the corresponding CREATE
INDEX statement to migration 0020_v2_arch_strip.sql. Ensure the index column
order supports the disputes route’s filtering by linked_dispute_id followed by
ordering by created_at.
In `@src/routes/sync.ts`:
- Around line 61-67: Update the authorization logic in the sync route so every
source in chittyFinanceAliases receives the same exact requiredScope check as
mercury, plaid, and chittyfinance, preventing admin, wildcard, or general sync
scopes from bypassing financial restrictions. Move or define
chittyFinanceAliases before the authorization check, and preserve the existing
broader authorization behavior for non-financial sources.
In `@src/routes/triage.ts`:
- Around line 252-299: The POST /intents handler currently parses and validates
request data manually. Define a Zod schema for intent_type, privilege, space,
priority, source_reference, document_chitty_id, classification_scores, and
reason_codes, including z.string().uuid() for document_chitty_id and the
required date/enum constraints, then apply it with zValidator('json', schema).
Use the validated payload in the handler and remove the body cast and redundant
manual field validation.
- Around line 25-27: Update the /intents endpoint to use the imported
createIntent flow instead of hand-rolled raw SQL inserts, preserving
sovereigntyAssessment persistence and initialStatus derivation, including
blocked_human or failed outcomes. Remove or replace the direct goal/plan/intent
insertion logic in the endpoint so every intent creation passes through the
Attorney Decision Queue and human-approval gate enforced by createIntent.
- Around line 298-330: The idempotency flow in the triage endpoint is not
concurrency-safe because its CTE existence check lacks a database-enforced
uniqueness conflict. Update the `cc_intents` schema/migration to add the
equivalent partial unique index for `(intent_type,
metadata->>'idempotency_key')`, then change the `inserted_intent` insertion to
use `ON CONFLICT (...) DO NOTHING` and preserve returning the existing row when
a duplicate key is encountered, following `createRouxIngestIntentIdempotent` and
`createContextualIngestIntentIdempotent` as references.
---
Outside diff comments:
In `@meta/intent.ts`:
- Around line 549-631: Forward the optional tx transaction through every nested
intent operation in executeIntent: pass it to getIntent for both initial and
reload reads, and to completeIntent and failIntent for status updates. Keep the
existing sql selection and dispatch behavior unchanged so the entire
read-claim-complete/fail flow uses the caller-provided transaction when present.
- Around line 100-106: Update getSql and the createGoal → createPlan →
createIntent workflow so atomic creates use Neon Serverless
sql.transaction([...]) for all statements instead of forwarding separate
one-shot NeonQueryFunction values through tx. Preserve the existing connection
resolution and ensure each nested operation participates in the same
transaction.
In `@src/routes/sync.ts`:
- Around line 11-18: Update the GET /status handler in syncRoutes to enforce the
same per-source authorization as the other sync endpoints, permitting rows only
when the caller has chittycommand:sync:${source} or chittycommand:sync:*; filter
unauthorized sources before returning statuses, including restricted sources
such as mercury, plaid, and chittyfinance.
---
Nitpick comments:
In `@scratch_update_triage.js`:
- Around line 1-22: Delete the scratch patch script containing the hardcoded
worktree path and fs.readFileSync/fs.writeFileSync calls; it is development
tooling and must not remain in the repository.
In `@tests/transactions.test.ts`:
- Around line 54-72: The transaction and idempotency tests overstate what mocked
sequential requests verify. Update the tests around the rollback case and the
idempotency test to either use real/ephemeral Postgres for database guarantees
or add concurrent requests against the mock to exercise the race; otherwise,
soften test names and comments to describe only the observed status, call count,
and sequential behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1abd26da-d3c1-42fb-b78f-43bad69ee0d5
📒 Files selected for processing (38)
.github/copilot-instructions.md.github/workflows/onepassword-rotation-audit.ymlAGENTS.mdCHARTER.mdCHITTY.mdCLAUDE.mddaemon/supervisor.mddocs/ARCHITECTURE_V2.mddocs/plans/2026-02-23-mercury-chittybooks-plan.mddocs/plans/2026-02-23-mercury-live-data-design.mddocs/plans/2026-02-23-scrapers-design.mddocs/plans/2026-02-23-scrapers-implementation-plan.mddocs/plans/2026-02-24-ingestion-enhancement-design.mddocs/plans/2026-02-24-ingestion-phase1-plan.mddocs/plans/2026-02-25-email-connections-design.mddocs/plans/2026-02-26-ai-sidebar-design.mddocs/plans/2026-02-26-ai-sidebar-implementation.mddocs/plans/2026-02-26-chittyrouter-dynamic-email-design.mddocs/plans/2026-02-26-chittyrouter-dynamic-email-plan.mddocs/registration/SUBMISSION_RUNBOOK.mddocs/security/ACCESS_BROKER_AND_1PASSWORD_RUNBOOK.mddocs/security/SECRET_ROTATION_CHECKLIST.mdmeta/executors/update-obligation-status.tsmeta/intent.tsmigrations/0019_vendors.sqlmigrations/0020_v2_arch_strip.sqlscratch_update_triage.jsscripts/onepassword-rotation-audit.shsrc/agents/tools/actions.tssrc/db/schema.tssrc/lib/db.tssrc/lib/triage.tssrc/routes/bridge/mercury.tssrc/routes/sync.tssrc/routes/triage.tstemplates/governance-baseline/.github/workflows/onepassword-rotation-audit.ymltemplates/governance-baseline/scripts/onepassword-rotation-audit.shtests/transactions.test.ts
| - KV service tokens: `bridge:service_token`, `mcp:service_token`, `scrape:service_token`. | ||
| - CORS is restricted to approved origins: `app.command.chitty.cc`, `command.mychitty.com`, `chittycommand-ui.pages.dev`, `localhost:5173`. | ||
| - Credentials use 1Password (`op run`) in local development — never expose in terminal output or logs. | ||
| - Credentials use chittysecrets (`chittysecrets run`) in local development — never expose in terminal output or logs. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files of interest:"
for f in \
.github/copilot-instructions.md \
AGENTS.md \
CLAUDE.md \
docs/registration/SUBMISSION_RUNBOOK.md \
scripts/onepassword-rotation-audit.sh \
templates/governance-baseline/scripts/onepassword-rotation-audit.sh
do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
sed -n '1,180p' "$f"
else
echo "MISSING $f"
fi
done
echo
echo "Search for chittysecrets/op commands:"
rg -n "chittysecrets|\"?op run\"?|op run|op run" -S .
echo
echo "Search for dependency checks around chittysecrets/op:"
rg -n -C 3 "chittysecrets CLI|onepassword|op command|command -v op|which op|chittysecrets run|op run" -S .Repository: chittyos/chittycommand
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow/action and runbook mentions:"
rg -n -C 3 "chittysecrets|\"?op run\"?|op run|onepassword-rotation|ACCESS_BROKER_AND_1PASSWORD|1Password" .github docs scripts templates -S || true
echo
echo "Read targeted audit workflow examples:"
for f in \
.github/workflows/onepassword-rotation-audit.yml \
templates/governance-baseline/.github/workflows/onepassword-rotation-audit.sh \
templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml \
docs/security/ACCESS_BROKER_AND_1PASSWORD_RUNBOOK.md
do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f"
else
echo "MISSING $f"
fi
doneRepository: chittyos/chittycommand
Length of output: 34277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Targeted files around referenced lines:"
for f in AGENTS.md CLAUDE.md
do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
sed -n '70,135p' "$f"
fi
done
echo
echo "Audit script implementation around checks/invocations:"
for f in scripts/onepassword-rotation-audit.sh templates/governance-baseline/scripts/onepassword-rotation-audit.sh
do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,30p'
sed -n '100,180p'
sed -n '210,260p'
fi
done
echo
echo "Direct executable invocations/dependencies in audit scripts:"
python3 - <<'PY'
import pathlib, re
for p in ["scripts/onepassword-rotation-audit.sh", "templates/governance-baseline/scripts/onepassword-rotation-audit.sh"]:
text = pathlib.Path(p).read_text()
print(f"--- {p}")
for m in re.finditer(r'(require|command -v|chittysecrets|op |`op )', text):
print(f"{m.start()}: {text[m.start():m.start()+120]}")
PYRepository: chittyos/chittycommand
Length of output: 7713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib, re
for p in ["scripts/onepassword-rotation-audit.sh", "templates/governance-baseline/scripts/onepassword-rotation-audit.sh"]:
text = pathlib.Path(p).read_text()
lines=text.splitlines()
print(f"--- {p} line counts")
for i,line in enumerate(lines,1):
if re.search(r'\b(op|chittysecrets)\b|command -v', line):
print(f"{i}: {line}")
print()
for f in ["AGENTS.md","CLAUDE.md",".github/copilot-instructions.md","docs/registration/SUBMISSION_RUNBOOK.md"]:
if pathlib.Path(f).exists():
print(f"--- {f}")
for i,line in enumerate(pathlib.Path(f).read_text().splitlines(),1):
if "chittysecrets" in line or "op run" in line:
print(f"{i}: {line}")
PYRepository: chittyos/chittycommand
Length of output: 1677
🌐 Web query:
1Password CLI op run chittysecrets command chittysecrets run
💡 Result:
The command sequence you provided, op run chittysecrets command chittysecrets run, is not a standard or documented 1Password CLI command [1][2][3]. In the context of the 1Password CLI, the op run command is used to securely inject secrets from 1Password into the environment of a subprocess [1][4]. The standard syntax for op run is [1]: op run -- For example, to run a script or application, you would use [5][4]: op run --./your-script.sh The term "chittysecrets" appears to be specific to a private project or organization-specific utility rather than a native part of the 1Password CLI [6][7]. Based on public code repositories, "chittysecrets" is referenced in custom commit messages and internal service configuration files (such as those for chittyconnect), suggesting it is a custom wrapper or tool used within that specific environment [6][7]. If you are trying to use 1Password CLI to run an application that relies on this custom "chittysecrets" tool, you would likely use the 1Password CLI to provide the necessary credentials to that tool, for instance: op run -- chittysecrets run This would execute the chittysecrets run command as a subprocess, with any secret references defined in your environment or configuration files automatically resolved and injected by 1Password [1][4].
Citations:
- 1: https://www.1password.dev/cli/reference/commands/run
- 2: https://www.1password.dev/cli/reference
- 3: https://www.1password.dev/cli
- 4: https://www.1password.dev/cli/secret-references
- 5: https://www.1password.dev/cli/secrets-environment-variables
- 6: chittyos/chittyconnect@3c4c8b4
- 7: chittyos/chittyconnect@chittyos:bf47df0...chittyos:aab5a91
Define the canonical chittysecrets invocation style. The docs and scripts use chittysecrets run, standalone op run/op read, and direct op item get without documenting whether these are equivalent, wrapper-specific, or environment-specific flows. Align this consistently.
.github/copilot-instructions.md#L110,AGENTS.md#L94,CLAUDE.md#L121: declare whetherop run -- chittysecrets runor directop runis the canonical local command.docs/registration/SUBMISSION_RUNBOOK.md#L37-L55: keep thechittysecretscold-source →op runruntime handoff explicit if it remains a two-step flow.scripts/onepassword-rotation-audit.sh#L7-L9and template: either remove the confusing “chittysecrets CLI (op)” label or document thatchittysecretsis provided through 1Password CLI.
📍 Affects 6 files
.github/copilot-instructions.md#L110-L110(this comment)AGENTS.md#L94-L94CLAUDE.md#L121-L121docs/registration/SUBMISSION_RUNBOOK.md#L37-L37scripts/onepassword-rotation-audit.sh#L8-L8templates/governance-baseline/scripts/onepassword-rotation-audit.sh#L8-L8
🤖 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 @.github/copilot-instructions.md at line 110, Define one canonical local
chittysecrets invocation consistently in .github/copilot-instructions.md (line
110), AGENTS.md (line 94), and CLAUDE.md (line 121), explicitly stating whether
direct op run or op run -- chittysecrets run is authoritative. In
docs/registration/SUBMISSION_RUNBOOK.md (line 37), preserve and clarify the
chittysecrets cold-source to op run runtime handoff if it remains two-step. In
scripts/onepassword-rotation-audit.sh (line 8) and
templates/governance-baseline/scripts/onepassword-rotation-audit.sh (line 8),
remove the ambiguous “chittysecrets CLI (op)” wording or document that
chittysecrets is supplied through the 1Password CLI.
| @@ -1,4 +1,4 @@ | |||
| name: 1Password Rotation Audit | |||
| name: chittysecrets Rotation Audit | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete or explicitly document the legacy naming migration.
.github/workflows/onepassword-rotation-audit.yml#L1-L1: align the workflow name with the remaining script and artifact identifiers, or document compatibility..github/workflows/onepassword-rotation-audit.yml#L49-L49: align the issue/report naming contract with the remaining legacy identifiers.docs/security/ACCESS_BROKER_AND_1PASSWORD_RUNBOOK.md#L1-L1: explain why the document uses chittysecrets while referencing onepassword-named files.docs/security/ACCESS_BROKER_AND_1PASSWORD_RUNBOOK.md#L25-L25: apply the same compatibility clarification to the section heading.templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml#L1-L1: mirror the primary workflow’s naming decision.templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml#L49-L49: mirror the primary workflow’s issue/report naming decision.
📍 Affects 3 files
.github/workflows/onepassword-rotation-audit.yml#L1-L1(this comment).github/workflows/onepassword-rotation-audit.yml#L49-L49docs/security/ACCESS_BROKER_AND_1PASSWORD_RUNBOOK.md#L1-L1docs/security/ACCESS_BROKER_AND_1PASSWORD_RUNBOOK.md#L25-L25templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml#L1-L1templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml#L49-L49
🤖 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 @.github/workflows/onepassword-rotation-audit.yml at line 1, Complete the
legacy chittysecrets/onepassword naming migration consistently: update the
workflow name and issue/report identifiers in
.github/workflows/onepassword-rotation-audit.yml lines 1 and 49, then mirror
those decisions in
templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml
lines 1 and 49. Clarify the compatibility rationale for chittysecrets
terminology alongside onepassword-named files in
docs/security/ACCESS_BROKER_AND_1PASSWORD_RUNBOOK.md lines 1 and 25.
| - name: Install chittysecrets CLI | ||
| uses: chittysecrets/install-cli-action@v1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
fd -a 'onepassword-rotation-audit\.ya?ml$' . | sed 's#^\./##'
echo
echo "== relevant lines =="
for f in $(fd 'onepassword-rotation-audit\.ya?ml$' .); do
echo "--- $f"
sed -n '1,60p' "$f" | cat -n
done
echo
echo "== all chittysecrets action refs in repo =="
rg -n "chittysecrets/install-cli-action|uses: .*`@v`[0-9]|uses: [^@]+@[0-9a-fA-F]{40}" . || trueRepository: chittyos/chittycommand
Length of output: 248
🌐 Web query:
chittysecrets install-cli-action v1 GitHub repository releases
💡 Result:
There is no GitHub repository named chittysecrets/install-cli-action. It appears you may be confusing this with 1Password/install-cli-action, an official GitHub Action for installing the 1Password CLI [1][2]. While searching for the term chittysecrets, results indicate it is associated with the organization chittyos (e.g., chittyconnect, chittymcp, chittychronicle) [3][4][5]. Within the repositories of the chittyos organization, there are instances where they utilize 1Password/install-cli-action, but they do not maintain an install-cli-action of their own [4][6]. If you are looking for the releases for the 1Password/install-cli-action repository, they are available on the official GitHub repository page [2]. As of July 26, 2026, the latest release for 1Password/install-cli-action is v4.0.0, which was released on June 11, 2026 [1][2].
Citations:
- 1: https://github.com/1Password/install-cli-action
- 2: https://github.com/1Password/install-cli-action/releases
- 3: chittyos/chittyconnect@3c4c8b4
- 4: chore(deps): bump 1password/install-cli-action from 3 to 4 chittymcp#132
- 5: chittyos/chittychronicle@97dcabf
- 6: chittyos/chittymcp@6093a43
Pin the installed Action to a verified immutable commit.
Replace chittysecrets/install-cli-action@v1 with a full commit SHA in both workflow files, or switch to an established/released Action such as 1Password/install-cli-action with an immutable tag.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 27-27: Unpinned tag for a non-immutable Action in workflow or composite action
Unpinned 3rd party Action 'chittysecrets Rotation Audit' step Uses Step uses 'chittysecrets/install-cli-action' with ref 'v1', not a pinned commit hash
📍 Affects 2 files
.github/workflows/onepassword-rotation-audit.yml#L26-L27(this comment)templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml#L26-L27
🤖 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 @.github/workflows/onepassword-rotation-audit.yml around lines 26 - 27, Pin
the Install chittysecrets CLI action to a verified immutable commit SHA in both
.github/workflows/onepassword-rotation-audit.yml (lines 26-27) and
templates/governance-baseline/.github/workflows/onepassword-rotation-audit.yml
(lines 26-27), or replace it in both workflow files with the established
1Password/install-cli-action using an immutable tag.
Source: Linters/SAST tools
| DROP TABLE IF EXISTS cc_dispute_correspondence CASCADE; | ||
| DROP TABLE IF EXISTS cc_disputes CASCADE; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Complete the ChittyCases cutover before dropping local dispute storage.
CASCADE removes foreign keys but does not convert the existing UUID columns to text. After this migration, cc_documents.linked_dispute_id and cc_recommendations.dispute_id remain UUID in PostgreSQL while the Drizzle schema expects text; meanwhile action and triage flows query tables that no longer exist.
migrations/0020_v2_arch_strip.sql#L23-L24: alter both reference columns totextusing their existing values after dropping the FKs, then remove the tables.src/agents/tools/actions.ts#L134-L146: replace local dispute/correspondence SQL with the ChittyCases-backed draft flow.src/lib/triage.ts#L64-L64: inject canonical dispute data or remove local-dispute recommendation generation; its currentcc_disputesquery will fail.- The supplied
src/routes/disputes.ts:22-36must also be migrated before deploying this table drop.
As per coding guidelines, every schema change requires a SQL migration; the PR objective makes ChittyCases the canonical dispute source.
🧰 Tools
🪛 Squawk (2.59.0)
[warning] 23-23: Dropping a table may break existing clients.
(ban-drop-table)
[warning] 24-24: Dropping a table may break existing clients.
(ban-drop-table)
📍 Affects 3 files
migrations/0020_v2_arch_strip.sql#L23-L24(this comment)src/agents/tools/actions.ts#L134-L146src/lib/triage.ts#L64-L64
🤖 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 `@migrations/0020_v2_arch_strip.sql` around lines 23 - 24, Complete the
ChittyCases cutover across migrations/0020_v2_arch_strip.sql:23-24,
src/agents/tools/actions.ts:134-146, src/lib/triage.ts:64, and
src/routes/disputes.ts:22-36. In the migration, drop foreign keys, convert
cc_documents.linked_dispute_id and cc_recommendations.dispute_id to text while
preserving values, then drop local dispute tables. Replace local
dispute/correspondence queries and recommendation generation with canonical
ChittyCases data, and migrate the disputes route to the ChittyCases-backed flow
before removing those tables.
Source: Coding guidelines
| linkedObligationId: uuid('linked_obligation_id').references(() => ccObligations.id), | ||
| linkedAccountId: uuid('linked_account_id').references(() => ccAccounts.id), | ||
| linkedDisputeId: uuid('linked_dispute_id').references(() => ccDisputes.id), | ||
| linkedDisputeId: text('linked_dispute_id'), // Migrating away from local FK to ChittyCases reference |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Index the external-dispute document lookup.
src/routes/disputes.ts:22-36 filters on linked_dispute_id and orders by created_at, but neither the Drizzle schema nor migration adds a supporting index. Add a composite index and create it in 0020_v2_arch_strip.sql.
As per coding guidelines, add database indexes to columns used in WHERE or ORDER BY clauses.
🤖 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/db/schema.ts` at line 135, Add a composite database index for
linked_dispute_id and created_at in the schema definition near linkedDisputeId,
then add the corresponding CREATE INDEX statement to migration
0020_v2_arch_strip.sql. Ensure the index column order supports the disputes
route’s filtering by linked_dispute_id followed by ordering by created_at.
Source: Coding guidelines
| // Financial sources like Mercury are highly restricted; no general admin bypass. | ||
| let isAuthorized = false; | ||
| if (['mercury', 'plaid', 'chittyfinance'].includes(source)) { | ||
| isAuthorized = scopes.includes(requiredScope); | ||
| } else { | ||
| isAuthorized = scopes.includes(requiredScope) || scopes.includes('chittycommand:sync:*') || scopes.includes('admin') || scopes.includes('*'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Financial-source restriction can be bypassed via aliases.
The restricted check only covers ['mercury', 'plaid', 'chittyfinance'], but chittyFinanceAliases (wave, stripe, turbotenant, chittyrental) all resolve to the exact same syncFinance dispatcher (Lines 112-114). Triggering /trigger/wave (etc.) with just chittycommand:sync:wave, admin, or * therefore invokes the same financial sync that mercury/plaid/chittyfinance intentionally require an exact, non-bypassable scope for — contradicting the comment "Financial sources like Mercury are highly restricted; no general admin bypass."
🔒 Proposed fix — restrict aliases too
- if (['mercury', 'plaid', 'chittyfinance'].includes(source)) {
+ const restrictedFinancialSources = ['mercury', 'plaid', 'chittyfinance', ...chittyFinanceAliases];
+ if (restrictedFinancialSources.includes(source)) {
isAuthorized = scopes.includes(requiredScope);Note: chittyFinanceAliases is declared later in the function (Line 93); it would need to be hoisted above the scope check to reference it here.
Also applies to: 92-96, 98-108
🤖 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/routes/sync.ts` around lines 61 - 67, Update the authorization logic in
the sync route so every source in chittyFinanceAliases receives the same exact
requiredScope check as mercury, plaid, and chittyfinance, preventing admin,
wildcard, or general sync scopes from bypassing financial restrictions. Move or
define chittyFinanceAliases before the authorization check, and preserve the
existing broader authorization behavior for non-financial sources.
| createGoal, | ||
| createPlan, | ||
| createIntent, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
New /intents endpoint bypasses the sovereignty gate that createIntent enforces.
createGoal, createPlan, and createIntent are imported (Lines 25-27) but never used — instead this endpoint hand-rolls the goal/plan/intent inserts via raw SQL. Unlike createIntent in meta/intent.ts, which derives initialStatus (blocked_human/failed/pending) from input.sovereigntyAssessment?.decision and persists the assessment, this endpoint always inserts status = 'pending' and never writes sovereignty_assessment at all (it's absent from both INSERT column lists, Lines 322-325 and 343-346). Any intent created through this route skips the sovereignty/human-approval gate entirely.
Based on learnings: "Do not bundle governance or ruleset changes with unrelated application logic, and do not weaken authentication, CORS, or governance controls." As per path instructions: "Agents must not bypass the Attorney Decision Queue or human approval gate for filings, strategy shifts, or irreversible ecosystem mutations."
Also applies to: 300-348, 371-373
🤖 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/routes/triage.ts` around lines 25 - 27, Update the /intents endpoint to
use the imported createIntent flow instead of hand-rolled raw SQL inserts,
preserving sovereigntyAssessment persistence and initialStatus derivation,
including blocked_human or failed outcomes. Remove or replace the direct
goal/plan/intent insertion logic in the endpoint so every intent creation passes
through the Attorney Decision Queue and human-approval gate enforced by
createIntent.
Sources: Path instructions, Learnings
| triageRoutes.post('/intents', async (c) => { | ||
| // Authorization check | ||
| const scopes = c.get("scopes") || []; | ||
| const userId = c.get("userId") || "system"; | ||
| const hasAuth = scopes.some((s) => s === 'chittytriage:write' || s === 'admin' || s === '*'); | ||
| if (!hasAuth) { | ||
| return c.json({ error: 'Insufficient scope' }, 403); | ||
| } | ||
|
|
||
| const body = await c.req.json().catch(() => ({})); | ||
| const { | ||
| intent_type, | ||
| privilege, | ||
| space, | ||
| priority, | ||
| source_reference, | ||
| document_chitty_id, | ||
| classification_scores, | ||
| reason_codes, | ||
| } = body as Record<string, any>; | ||
|
|
||
| if (!intent_type) { | ||
| return c.json({ error: 'intent_type is required' }, 400); | ||
| } | ||
|
|
||
| // Validate privilege and space | ||
| const parsedPrivilege = parsePrivilege(privilege); | ||
| if (privilege !== undefined && privilege !== null && parsedPrivilege === null) { | ||
| return c.json({ error: `Invalid privilege; must be one of ${[...VALID_PRIVILEGE].join(',')}` }, 400); | ||
| } | ||
|
|
||
| const parsedSpace = parseSpace(space); | ||
| if (space !== undefined && space !== null && parsedSpace === null) { | ||
| return c.json({ error: `Invalid space; must be one of ${[...VALID_SPACE].join(',')}` }, 400); | ||
| } | ||
|
|
||
| const p = Number(priority); | ||
| const finalPriority = Number.isFinite(p) ? Math.floor(p) : 5; | ||
|
|
||
| const payload = { | ||
| source_reference, | ||
| document_chitty_id, | ||
| classification_scores, | ||
| reason_codes, | ||
| }; | ||
|
|
||
| const idempotencyKey = c.req.header('Idempotency-Key'); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Manual body parsing/validation instead of zValidator.
intent_type, privilege, space, priority, document_chitty_id, etc. are parsed by hand from body as Record<string, any> rather than validated with @hono/zod-validator/zValidator('json', schema), and document_chitty_id isn't checked against z.string().uuid().
As per coding guidelines: "Validate JSON request bodies with @hono/zod-validator and zValidator('json', schema)... Validate all user input with Zod before use... Validate date strings with /^\d{4}-\d{2}-\d{2}$/ and UUID fields with z.string().uuid()."
🤖 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/routes/triage.ts` around lines 252 - 299, The POST /intents handler
currently parses and validates request data manually. Define a Zod schema for
intent_type, privilege, space, priority, source_reference, document_chitty_id,
classification_scores, and reason_codes, including z.string().uuid() for
document_chitty_id and the required date/enum constraints, then apply it with
zValidator('json', schema). Use the validated payload in the handler and remove
the body cast and redundant manual field validation.
Source: Coding guidelines
| const idempotencyKey = c.req.header('Idempotency-Key'); | ||
|
|
||
| // Atomic CTE to prevent orphaned goals/plans if intent insertion fails. | ||
| const sql = getDb(c.env); | ||
| let rows; | ||
| if (idempotencyKey) { | ||
| rows = await sql` | ||
| WITH existing_intent AS ( | ||
| SELECT id, plan_id, goal_id, intent_type, target_channel, payload, status, priority, sovereignty_assessment, human_gate_reason, dispatched_task_id, scheduled_for, completed_at, error_message, privilege, space, metadata, created_at, updated_at | ||
| FROM cc_intents | ||
| WHERE intent_type = ${intent_type} | ||
| AND metadata->>'idempotency_key' = ${idempotencyKey} | ||
| LIMIT 1 | ||
| ), new_goal AS ( | ||
| INSERT INTO cc_goals (owner_chitty_id, title, description, priority, status, metadata) | ||
| SELECT ${userId}, ${'Triage: ' + intent_type}, 'Triage queue intent for review', ${finalPriority}, 'open', '{}'::jsonb | ||
| WHERE NOT EXISTS (SELECT 1 FROM existing_intent) | ||
| RETURNING id, title | ||
| ), new_plan AS ( | ||
| INSERT INTO cc_plans (goal_id, title, status, authored_by, metadata) | ||
| SELECT id, 'Plan for ' || title, 'draft', ${userId}, '{}'::jsonb | ||
| FROM new_goal | ||
| RETURNING id, goal_id | ||
| ), inserted_intent AS ( | ||
| INSERT INTO cc_intents (plan_id, goal_id, intent_type, payload, status, priority, privilege, space, metadata) | ||
| SELECT id, goal_id, ${intent_type}, ${JSON.stringify(payload)}::jsonb, 'pending', ${finalPriority}, ${parsedPrivilege ?? 'public'}, ${parsedSpace ?? 'business'}, ${JSON.stringify({idempotency_key: idempotencyKey})}::jsonb | ||
| FROM new_plan | ||
| RETURNING id, plan_id, goal_id, intent_type, target_channel, payload, status, priority, sovereignty_assessment, human_gate_reason, dispatched_task_id, scheduled_for, completed_at, error_message, privilege, space, metadata, created_at, updated_at | ||
| ) | ||
| SELECT *, false as is_existing FROM inserted_intent | ||
| UNION ALL | ||
| SELECT *, true as is_existing FROM existing_intent; | ||
| `; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Idempotency check is a TOCTOU race, not an atomic guarantee.
existing_intent is evaluated against the snapshot at query start; new_goal's WHERE NOT EXISTS (SELECT 1 FROM existing_intent) doesn't prevent two concurrent requests with the same Idempotency-Key from both seeing "no existing row" and both inserting — producing duplicate goals/plans/intents. meta/intent.ts's createRouxIngestIntentIdempotent/createContextualIngestIntentIdempotent correctly avoid this by using ON CONFLICT (...) DO NOTHING against a partial unique index. This endpoint has no equivalent unique constraint on cc_intents (intent_type, metadata->>'idempotency_key'), so the guarantee the "Repeated-request/idempotency" test exercises doesn't hold under real concurrency (the test only mocks a single sequential call).
🤖 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/routes/triage.ts` around lines 298 - 330, The idempotency flow in the
triage endpoint is not concurrency-safe because its CTE existence check lacks a
database-enforced uniqueness conflict. Update the `cc_intents` schema/migration
to add the equivalent partial unique index for `(intent_type,
metadata->>'idempotency_key')`, then change the `inserted_intent` insertion to
use `ON CONFLICT (...) DO NOTHING` and preserve returning the existing row when
a duplicate key is encountered, following `createRouxIngestIntentIdempotent` and
`createContextualIngestIntentIdempotent` as references.
2 participants
Summary
ChittyCommand v2 Architecture Phase 1 — enforce the principle that ChittyCommand does not own truth.
Changes
Schema strip (truth delegation):
DROP cc_disputes— truth moves to ChittyCases serviceDROP cc_dispute_correspondence— truth moves to ChittyCases servicelinked_dispute_idfields →text(canonical ID reference, not FK)New table:
cc_node_leases(migration 0020):UPDATE ... RETURNINGmeta-orchestrator-leaderrole rowcommand.chitty.cc/healthdegraded probe (cc_node_leases does not exist)Docs:
docs/ARCHITECTURE_V2.md— principled refactor spec (cockpit/engine model)CHARTER.md/CHITTY.md/AGENTS.md— embed v2 principles + routing rulesApply to prod
Health before/after
daemon: {status: "not_provisioned", error: "relation cc_node_leases does not exist"}Ref:
docs/ARCHITECTURE_V2.mdSummary by CodeRabbit
New Features
Documentation
Tests