Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,30 @@ jobs:
test/skill-workspace-lock.test.ts
test/skill-install-ledger.test.ts

# The daemon's store suites again, with `LocalStore` opened over `PostgresSyncDatabase`
# instead of `node:sqlite`, against a Testcontainers `postgres:16-alpine`. The pool runs
# that SQL for real, so a SQLite-only construct fails here rather than on a cluster.
# Needs Docker, so it gets its own runner off the critical path.
daemon-store-postgres:
name: Daemon Store (PostgreSQL)
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v6
with:
fetch-depth: 0
persist-credentials: false
- uses: pnpm/action-setup@v6
with:
cache: true
- uses: actions/setup-node@v6
with:
node-version-file: .nvmrc
- name: Install
run: pnpm install --frozen-lockfile
- name: Daemon store suites on PostgreSQL
run: pnpm --filter @agentconnect.md/daemon test:store:postgres

# control-plane integration tests against a real Postgres booted by
# Testcontainers. Each Vitest worker gets an isolated database cloned from one
# migrated template, so files run concurrently without cross-test TRUNCATEs.
Expand Down
2 changes: 2 additions & 0 deletions packages/daemon/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"start": "node dist/index.js run",
"test": "vitest run",
"test:runtime-matrix": "vitest run test/acp-matrix/acp-matrix.test.ts",
"test:store:postgres": "vitest run --config vitest.postgres.config.ts",
"test:unit": "vitest run",
"test:watch": "vitest",
"typecheck": "tsc -p tsconfig.typecheck.json"
Expand Down Expand Up @@ -74,6 +75,7 @@
"zod": "^4.4.3"
},
"devDependencies": {
"@testcontainers/postgresql": "^12.0.4",
"@types/node": "^24.13.3",
"@types/pg": "^8.20.0",
"@types/ws": "^8.18.1",
Expand Down
10 changes: 7 additions & 3 deletions packages/daemon/src/daemon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,10 @@ const AGENT_CALL_HOP_LIMIT_NOTICE = `Agent conversation stopped after reaching t
*/
const ACTIVATION_PAIRING_TTL_MS = 10 * 60 * 1000

/** Composite-key separator for the activation rendezvous. NOT NUL: these keys and their
* transcript coordinates are stored, and the pool store is PostgreSQL, whose TEXT rejects 0x00. */
const ACTIVATION_KEY_SEPARATOR = '\u001f'

/**
* The key that makes one logical delivery admissible exactly once
* (send-message-routing-rework.md §3.2).
Expand All @@ -998,7 +1002,7 @@ function activationKey(
platformMessageId: string,
targetAgentId: string
): string {
return [platform, transportScope ?? '', platformMessageId, targetAgentId].join('\u0000')
return [platform, transportScope ?? '', platformMessageId, targetAgentId].join(ACTIVATION_KEY_SEPARATOR)
}

/** The platform `ts` inside a Slack `msgId` (`slack:<channel>:<ts>`). The ts — not the
Expand Down Expand Up @@ -6978,7 +6982,7 @@ export class Daemon {
{
agentCallDeliveryId: verified.agentCallDeliveryId,
platformMessageId,
transcriptCoordinates: `${transcriptChannelKey(msg.channel, msg.transportScope)}\u0000${msg.thread ?? ''}`
transcriptCoordinates: `${transcriptChannelKey(msg.channel, msg.transportScope)}${ACTIVATION_KEY_SEPARATOR}${msg.thread ?? ''}`
},
expiresAt
)
Expand Down Expand Up @@ -8355,7 +8359,7 @@ export class Daemon {
{
agentCallDeliveryId: msg.trustedAgentCallDeliveryId,
platformMessageId,
transcriptCoordinates: `${transcriptChannelKey(normalized.channel, normalized.transportScope)}\u0000${normalized.thread ?? ''}`
transcriptCoordinates: `${transcriptChannelKey(normalized.channel, normalized.transportScope)}${ACTIVATION_KEY_SEPARATOR}${normalized.thread ?? ''}`
},
this.clock.now() + ACTIVATION_PAIRING_TTL_MS
)
Expand Down
2 changes: 1 addition & 1 deletion packages/daemon/test/daemon-agent-mention-routing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ const route = (daemon: Daemon, msg: unknown, on: string[] = ['int-bot-b']) => (d
function pairingKey(daemon: Daemon, targetAgentId = 'bot-b'): string {
const integrationId = (daemon as any).resolveCpAgent(targetAgentId, 'slack')?.integrationId
const scope = integrationId ? (daemon as any).transportScopeForIntegrationIds([integrationId]) : undefined
return ['slack', scope ?? '', '1720000000.000200', targetAgentId].join('\u0000')
return ['slack', scope ?? '', '1720000000.000200', targetAgentId].join('\u001f')
}

describe('agent-authored platform mentions (send-message-routing-rework.md §6)', () => {
Expand Down
118 changes: 118 additions & 0 deletions packages/daemon/test/local-store-sql-portability.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/**
* The daemon pool runs every `local-store.ts` statement through `PostgresSyncDatabase`,
* so a SQLite-only construct there is a production fault on the pool, not a style issue.
* `store-postgres` catches one the moment a suite covers the statement; this check is the
* cheap half — it reads the SQL text itself, so an uncovered statement still fails fast.
*
* Only constructs the pool worker does NOT rewrite are listed. `INSERT OR IGNORE`,
* `BEGIN IMMEDIATE`, `INTEGER PRIMARY KEY AUTOINCREMENT`, `PRAGMA user_version`,
* `sqlite_master`, `LIMIT -1 OFFSET` and `length(CAST(x AS BLOB))` are translated in
* `postgres-store-worker.js#rewrite` and stay legal. Extend that list only by making a
* construct portable in the SQL — never by teaching the worker another function name.
*/
import { readFileSync } from 'node:fs'
import { fileURLToPath } from 'node:url'
import { describe, expect, it } from 'vitest'
import * as ts from 'typescript'

const storeSource = fileURLToPath(new URL('../src/store/local-store.ts', import.meta.url))

/** A statement may opt out with this marker plus the reason, inside the SQL itself. */
const ALLOW_MARKER = '-- pg-portable-exempt:'

const SQL_SHAPED =
/\b(SELECT|INSERT\s+INTO|INSERT\s+OR|UPDATE\s+|DELETE\s+FROM|CREATE\s+(TABLE|INDEX)|ALTER\s+TABLE)\b/i

interface Fragment {
line: number
text: string
}

/** Every string/template literal in the file, with `${…}` holes cooked to a bind placeholder. */
function sqlFragments(): Fragment[] {
const source = readFileSync(storeSource, 'utf8')
const file = ts.createSourceFile(storeSource, source, ts.ScriptTarget.ESNext, true)
const fragments: Fragment[] = []
const push = (node: ts.Node, text: string): void => {
if (!SQL_SHAPED.test(text)) return
fragments.push({ line: file.getLineAndCharacterOfPosition(node.getStart(file)).line + 1, text })
}
const visit = (node: ts.Node): void => {
if (ts.isStringLiteral(node) || ts.isNoSubstitutionTemplateLiteral(node)) push(node, node.text)
else if (ts.isTemplateExpression(node))
push(node, [node.head.text, ...node.templateSpans.map((span) => span.literal.text)].join(' ? '))
ts.forEachChild(node, visit)
}
visit(file)
return fragments
}

/** `MAX(a, b)` is a scalar in SQLite and an aggregate arity error in PostgreSQL — see #1068. */
function scalarMinMax(sql: string): boolean {
for (const match of sql.matchAll(/\b(MAX|MIN)\s*\(/gi)) {
let depth = 1
for (let i = match.index + match[0].length; i < sql.length && depth > 0; i += 1) {
const char = sql[i]
if (char === '(') depth += 1
else if (char === ')') depth -= 1
else if (char === ',' && depth === 1) return true
}
}
return false
}

const CHECKS: Array<{ name: string; portable: string; hit: (sql: string) => boolean }> = [
{ name: 'two-argument MAX/MIN', portable: 'CASE, or GREATEST/LEAST', hit: scalarMinMax },
{ name: 'IFNULL', portable: 'COALESCE', hit: (sql) => /\bIFNULL\s*\(/i.test(sql) },
{ name: 'IIF', portable: 'CASE', hit: (sql) => /\bIIF\s*\(/i.test(sql) },
{
name: 'datetime/strftime/julianday/unixepoch',
portable: 'store epoch numbers, as the schema already does',
hit: (sql) => /\b(datetime|strftime|julianday|unixepoch)\s*\(/i.test(sql)
},
{
name: 'INSERT OR REPLACE/ABORT/FAIL',
portable: 'ON CONFLICT … DO UPDATE',
hit: (sql) => /\bINSERT\s+OR\s+(REPLACE|ABORT|FAIL)\b/i.test(sql)
},
{ name: 'GROUP_CONCAT', portable: 'string_agg', hit: (sql) => /\bGROUP_CONCAT\s*\(/i.test(sql) },
{ name: 'printf', portable: 'format, or build the string in TypeScript', hit: (sql) => /\bprintf\s*\(/i.test(sql) },
{ name: 'TYPEOF', portable: 'a typed column', hit: (sql) => /\bTYPEOF\s*\(/i.test(sql) },
{ name: '|| concatenation', portable: 'CONCAT, or concatenate in TypeScript', hit: (sql) => sql.includes('||') },
{ name: 'comma LIMIT offset', portable: 'LIMIT … OFFSET …', hit: (sql) => /\bLIMIT\s+[@?$:]?\w+\s*,/i.test(sql) }
]

describe('local-store SQL portability', () => {
it('sees the SQL it is supposed to police', () => {
// A parser regression that stopped finding statements would make every check below vacuous.
expect(sqlFragments().length).toBeGreaterThan(100)
})

it('recognizes each construct it claims to police', () => {
// Without this the whole check could rot into a set of patterns that match nothing.
const samples: Record<string, string> = {
'two-argument MAX/MIN': 'UPDATE t SET a = MAX(a, @b) WHERE k = @k',
IFNULL: 'SELECT IFNULL(a, 0) AS a FROM t',
IIF: 'SELECT IIF(a > 0, 1, 0) AS a FROM t',
'datetime/strftime/julianday/unixepoch': "SELECT * FROM t WHERE at < datetime('now')",
'INSERT OR REPLACE/ABORT/FAIL': 'INSERT OR REPLACE INTO t (k) VALUES (@k)',
GROUP_CONCAT: 'SELECT GROUP_CONCAT(a) AS a FROM t',
printf: "SELECT printf('%s', a) AS a FROM t",
TYPEOF: 'SELECT TYPEOF(a) AS kind FROM t',
'|| concatenation': "SELECT a || '-' || b AS k FROM t",
'comma LIMIT offset': 'SELECT * FROM t LIMIT 10, 20'
}
for (const check of CHECKS) expect([check.name, check.hit(samples[check.name] ?? '')]).toEqual([check.name, true])
})

it('uses no SQLite-only construct the pool store cannot run', () => {
const offences = sqlFragments().flatMap(({ line, text }) =>
text.includes(ALLOW_MARKER)
? []
: CHECKS.filter((check) => check.hit(text)).map(
(check) => `local-store.ts:${line} uses ${check.name} — write ${check.portable} instead`
)
)
expect(offences).toEqual([])
})
})
Loading
Loading