feat(agent): telemetry + CI-gate quick wins from the "AI systems that ship" audit (#677)
* feat(agent): telemetry completeness + durability, CI gates, commit_method provenance Quick wins from the "Building AI systems that ship" audit: - mcp.tool_called gains errorMessage (message_sv, truncated 500 chars) on all failure exits; new mcp.skill_loaded event on every gnubok_load_skill (all tiers) so atom usage is finally measurable - event_log: (event_type, created_at) index; cleanup cron keeps mcp.*/agent.* telemetry 180 days (delivery events stay 30) - CI: lint ratchet (npm run check:lint — 60 legacy errors baselined, fails only on NEW errors) and a pg-real coverage gate (migrations touching trigger/RPC/RLS/DEFERRABLE require a *.pg.test.ts change; escape hatch: -- pg-test: covered-by/skip) - journal_entries.commit_method CHECK widened with 'api_key'/'agent'; the MCP approve path records 'api_key' truthfully instead of 'user_accept' (agent_first_vision §8 P0-1). 'agent' is reserved — ALL MCP traffic (incl. claude.ai OAuth, whose access_token is a minted API key) authenticates as api_key today Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(import): derive opening balances from prior-year #UB when SIE lacks #IB (#675) SIE files exported without #IB 0 rows (only #UB -1) previously imported with zero opening balances. getEffectiveOpeningBalances() now derives IB from prior-year UB for balance-sheet accounts when explicit #IB is absent, surfaces the derivation as an info issue in the import preview, and excludes share-capital vouchers from opening-balance detection. Detection regexes are shared between parser and importer so the two checks cannot drift. 507 lib/import tests pass. (Authored in a parallel session in this checkout; included per request.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(review): address PR #677 bot findings — RoPA entry, execFileSync, gate scope note Triage of the compliance-swarm + Greptile findings: Applied: - .compliance/ropa.yaml: new mcp.telemetry processing activity declaring the 180-day mcp.*/agent.* retention, lawful basis, data categories, and the no-args/no-results minimisation (ISO A.8.10, GDPR Art.5(1)(c) — the retention split is now formally documented, referenced from the cron) - check-pg-test-coverage.mjs: execFileSync with argv array — no shell, so a hostile base-ref can't inject (ASVS V13.2.1); verified an injection attempt exits 2 without executing - check-pg-test-coverage.mjs: documented the PR-level (not per-migration) scope of the gate so reviewers know to check coverage per migration when a PR carries several risky migrations (Greptile P2) Acknowledged, no change: - errorMessage PII risk: messages are domain-mapped strings; event_log already persists far richer delivery payloads under the same RLS; now declared in ropa.yaml - cron error envelope: errorResponse maps to the canonical safe envelope and the endpoint is CRON_SECRET-gated - two-pass delete "partial state": TTL deletes are idempotent — the next daily run sweeps whatever a failed pass left behind - skill_loaded actorLabel/sessionId: mirrors the pre-existing mcp.tool_called payload; sessionId is the join key the analytics exist for Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
076bb169f8
commit
bc61862e76
@@ -107,12 +107,40 @@ describe('gnubok_approve_pending_operation', () => {
|
||||
// commit options always include commitMethod; userEmail is added when
|
||||
// the supabase mock supports auth.admin.getUserById (it doesn't here, so
|
||||
// the resolution silently fails and we fall back to just commitMethod).
|
||||
expect(commitSpy.mock.calls[0][4]).toMatchObject({ commitMethod: 'user_accept' })
|
||||
// An api_key actor records 'api_key' in the immutable layer — MCP-relayed
|
||||
// acknowledgment, not a first-party human session (vision §8 P0-1).
|
||||
expect(commitSpy.mock.calls[0][4]).toMatchObject({ commitMethod: 'api_key' })
|
||||
expect(result.status).toBe('committed')
|
||||
expect(result.operation_id).toBe('op-1')
|
||||
expect(result.data?.invoice_id).toBe('inv-1')
|
||||
})
|
||||
|
||||
// No 'mcp_oauth' row: handleMcpRequest hardcodes actor.type='api_key' for
|
||||
// ALL MCP traffic (the OAuth connector's access_token is a minted API key),
|
||||
// so 'api_key' is the only agent-credential value a live request produces.
|
||||
it.each([
|
||||
{ actorType: 'api_key', expected: 'api_key' },
|
||||
{ actorType: 'user', expected: 'user_accept' },
|
||||
] as const)(
|
||||
'records commit_method=$expected when the approving actor is $actorType',
|
||||
async ({ actorType, expected }) => {
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
const op = { id: 'op-1', operation_type: 'create_invoice', company_id: 'company-1', status: 'pending', risk_level: 'medium', params: {} }
|
||||
enqueue({ data: op, error: null }) // fetch
|
||||
commitSpy.mockResolvedValue({ status: 'committed' })
|
||||
|
||||
await approveTool.execute(
|
||||
{ operation_id: 'op-1' },
|
||||
'company-1',
|
||||
'user-1',
|
||||
supabase as never,
|
||||
{ type: actorType }
|
||||
)
|
||||
|
||||
expect(commitSpy.mock.calls[0][4]).toMatchObject({ commitMethod: expected })
|
||||
}
|
||||
)
|
||||
|
||||
it('refuses to approve a risk_level=high op without confirmed=true', async () => {
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
const op = {
|
||||
|
||||
@@ -97,6 +97,7 @@ interface ToolCalledPayload {
|
||||
isError: boolean
|
||||
errorCode: string | null
|
||||
errorKind: 'execution' | 'scope_denied' | 'unknown_tool' | null
|
||||
errorMessage: string | null
|
||||
requestId: string | number | null
|
||||
userId: string
|
||||
companyId: string
|
||||
@@ -177,6 +178,7 @@ describe('mcp.tool_called telemetry', () => {
|
||||
expect(event.isError).toBe(false)
|
||||
expect(event.errorCode).toBeNull()
|
||||
expect(event.errorKind).toBeNull()
|
||||
expect(event.errorMessage).toBeNull()
|
||||
expect(event.actorType).toBe('api_key')
|
||||
expect(event.actorId).toBe('key-1')
|
||||
expect(event.actorLabel).toBe('Test Key')
|
||||
@@ -206,6 +208,9 @@ describe('mcp.tool_called telemetry', () => {
|
||||
expect(event.isError).toBe(true)
|
||||
expect(event.errorKind).toBe('scope_denied')
|
||||
expect(event.errorCode).toBe('INSUFFICIENT_SCOPE')
|
||||
// The human message rides along for failure clustering.
|
||||
expect(typeof event.errorMessage).toBe('string')
|
||||
expect((event.errorMessage as string).length).toBeGreaterThan(0)
|
||||
// Scope denial exits before tool.execute() runs.
|
||||
expect(event.latencyMs).toBe(0)
|
||||
})
|
||||
@@ -224,6 +229,9 @@ describe('mcp.tool_called telemetry', () => {
|
||||
expect(event.isError).toBe(true)
|
||||
expect(event.errorKind).toBe('unknown_tool')
|
||||
expect(event.errorCode).toBe('UNKNOWN_TOOL')
|
||||
// Short deterministic message — NOT the full available-tools list the
|
||||
// client response carries (that would blow the truncation budget).
|
||||
expect(event.errorMessage).toBe('Unknown tool: "gnubok_does_not_exist"')
|
||||
expect(event.latencyMs).toBe(0)
|
||||
})
|
||||
|
||||
@@ -245,6 +253,11 @@ describe('mcp.tool_called telemetry', () => {
|
||||
expect(event.isError).toBe(true)
|
||||
expect(event.errorKind).toBe('execution')
|
||||
expect(event.errorCode).toBeTruthy()
|
||||
// The structured error's human message is captured and bounded at 500
|
||||
// chars — the raw material for clustering execution failures into gotchas.
|
||||
expect(typeof event.errorMessage).toBe('string')
|
||||
expect((event.errorMessage as string).length).toBeGreaterThan(0)
|
||||
expect((event.errorMessage as string).length).toBeLessThanOrEqual(500)
|
||||
// Execution path measures real latency, even if the tool exits quickly.
|
||||
expect(event.latencyMs).toBeGreaterThanOrEqual(0)
|
||||
})
|
||||
@@ -356,8 +369,67 @@ describe('mcp.resource_read telemetry', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('mcp.skill_loaded telemetry', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
eventBus.clear()
|
||||
})
|
||||
|
||||
it('emits on every successful load — alongside mcp.workflow_started for workflow tier', async () => {
|
||||
const skillLoadedPromise = new Promise<Record<string, unknown>>((resolve) => {
|
||||
const off = eventBus.on('mcp.skill_loaded', (payload) => {
|
||||
off()
|
||||
resolve(payload as Record<string, unknown>)
|
||||
})
|
||||
})
|
||||
const workflowStartedPromise = new Promise<Record<string, unknown>>((resolve) => {
|
||||
const off = eventBus.on('mcp.workflow_started', (payload) => {
|
||||
off()
|
||||
resolve(payload as Record<string, unknown>)
|
||||
})
|
||||
})
|
||||
|
||||
await handleMcpRequest(
|
||||
mcpRequest('tools/call', {
|
||||
name: 'gnubok_load_skill',
|
||||
arguments: { slug: 'month-end-close' },
|
||||
})
|
||||
)
|
||||
|
||||
const event = await skillLoadedPromise
|
||||
expect(event.slug).toBe('month-end-close')
|
||||
expect(event.tier).toBe('workflow')
|
||||
expect(event.actorType).toBe('api_key')
|
||||
expect(event.actorId).toBe('key-1')
|
||||
expect(event.userId).toBe('user-1')
|
||||
expect(event.companyId).toBe('company-1')
|
||||
|
||||
// The pre-existing workflow-funnel event still fires for workflow tier.
|
||||
const wf = await workflowStartedPromise
|
||||
expect(wf.slug).toBe('month-end-close')
|
||||
})
|
||||
|
||||
it('does not emit when the slug is unknown (load throws before emission)', async () => {
|
||||
const seen: unknown[] = []
|
||||
eventBus.on('mcp.skill_loaded', (payload) => {
|
||||
seen.push(payload)
|
||||
})
|
||||
|
||||
await handleMcpRequest(
|
||||
mcpRequest('tools/call', {
|
||||
name: 'gnubok_load_skill',
|
||||
arguments: { slug: 'nope-not-real' },
|
||||
})
|
||||
)
|
||||
// Flush microtasks — emission is fire-and-forget.
|
||||
await new Promise((resolve) => setTimeout(resolve, 0))
|
||||
|
||||
expect(seen).toHaveLength(0)
|
||||
})
|
||||
})
|
||||
|
||||
describe('event_log persistence registration', () => {
|
||||
it('includes all three MCP telemetry events in the persisted event types', async () => {
|
||||
it('includes all MCP telemetry events in the persisted event types', async () => {
|
||||
// Read the file as text — the constant is module-private. This is a
|
||||
// deliberate string-level guard so a future refactor that drops one
|
||||
// of the events from the list trips the test.
|
||||
@@ -368,5 +440,6 @@ describe('event_log persistence registration', () => {
|
||||
expect(text).toMatch(/'mcp\.tool_called'/)
|
||||
expect(text).toMatch(/'mcp\.tools_list_called'/)
|
||||
expect(text).toMatch(/'mcp\.resource_read'/)
|
||||
expect(text).toMatch(/'mcp\.skill_loaded'/)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user