From 83529d5cd10ae1b7d0389ac0b755b728c57d1cc3 Mon Sep 17 00:00:00 2001 From: bgagent Date: Tue, 16 Jun 2026 16:16:05 -0400 Subject: [PATCH] fix(cdk): normalize after cursor case and fix colon-truncation in approval-scope guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness bugs in the task API handlers: 1. get-task-events: a lower-case `after` ULID cursor passed validation (isValidUlid uppercases before matching) but was used verbatim in the DynamoDB key condition `event_id > :after`. Stored event_ids are upper-case Crockford Base32 and DDB compares raw bytes, so a lower-case cursor sorts after every stored id and the query returns zero events — a consumer using a contract-valid lower-case cursor silently skips the rest of the event stream. Fix: upper-case the cursor before the query. 2. create-task-core: the degenerate-pattern guard for bash_pattern:/ write_path: scopes used scope.split(':', 2)[1], which truncates the value at the next colon. A value whose leading colon-segment is short or wildcard-heavy (e.g. "ab:cdefgh") was truncated to a degenerate fragment and spuriously rejected with a 400. Fix: take everything after the first colon via slice(indexOf(':') + 1). Adds regression tests for both paths. --- cdk/src/handlers/get-task-events.ts | 11 +++++- cdk/src/handlers/shared/create-task-core.ts | 6 +++- cdk/test/handlers/get-task-events.test.ts | 16 +++++++++ .../handlers/shared/create-task-core.test.ts | 35 +++++++++++++++++-- 4 files changed, 64 insertions(+), 4 deletions(-) diff --git a/cdk/src/handlers/get-task-events.ts b/cdk/src/handlers/get-task-events.ts index f9b2a58a..9a4cf04a 100644 --- a/cdk/src/handlers/get-task-events.ts +++ b/cdk/src/handlers/get-task-events.ts @@ -118,7 +118,16 @@ export async function handler(event: APIGatewayProxyEvent): Promise :after`` would return zero rows — silently dropping + // the rest of the event stream for a contract-valid input. + const afterValid = typeof afterRaw === 'string' && afterRaw.length === ULID_LENGTH + ? afterRaw.toUpperCase() + : undefined; // 3b. ``desc`` combined with ``after`` makes no semantic sense: ``after`` // is a forward-walking ULID cursor. Reject the combination rather than diff --git a/cdk/src/handlers/shared/create-task-core.ts b/cdk/src/handlers/shared/create-task-core.ts index 3db2ebaa..8ec057a8 100644 --- a/cdk/src/handlers/shared/create-task-core.ts +++ b/cdk/src/handlers/shared/create-task-core.ts @@ -339,7 +339,11 @@ export async function createTaskCore( parseResult.scope.startsWith('bash_pattern:') || parseResult.scope.startsWith('write_path:') ) { - const value = parseResult.scope.split(':', 2)[1] ?? ''; + // Take everything after the first colon — the value itself may + // contain colons (e.g. ``bash_pattern:git log --format=%h:%s``), so a + // ``split(':', 2)`` would truncate it and could turn a legitimate + // pattern into a degenerate-looking fragment, producing a spurious 400. + const value = parseResult.scope.slice(parseResult.scope.indexOf(':') + 1); if (isDegeneratePattern(value)) { return errorResponse( 400, diff --git a/cdk/test/handlers/get-task-events.test.ts b/cdk/test/handlers/get-task-events.test.ts index ad8c70fa..fb8771e9 100644 --- a/cdk/test/handlers/get-task-events.test.ts +++ b/cdk/test/handlers/get-task-events.test.ts @@ -250,6 +250,22 @@ describe('get-task-events handler', () => { expect(queryInput.ExclusiveStartKey).toBeUndefined(); }); + test('lower-case ?after is normalized to upper-case before the DDB query', async () => { + // ``isValidUlid`` accepts lower-case callers, but stored event_ids are + // upper-case Crockford Base32 and DDB compares raw bytes (lower-case sorts + // after upper-case). Without normalization a lower-case cursor would be + // "greater than" every stored id and silently return zero events. + const event = makeEvent({ queryStringParameters: { after: VALID_AFTER.toLowerCase() } }); + await handler(event); + + const queryInput = MockQueryCommand.mock.calls[0][0]; + expect(queryInput.KeyConditionExpression).toBe('task_id = :tid AND event_id > :after'); + expect(queryInput.ExpressionAttributeValues).toEqual({ + ':tid': 'task-1', + ':after': VALID_AFTER, // upper-cased, not the lower-case input + }); + }); + test('both after and next_token → after wins + WARN logged', async () => { const stdoutWrite = jest.spyOn(process.stdout, 'write').mockImplementation(() => true); try { diff --git a/cdk/test/handlers/shared/create-task-core.test.ts b/cdk/test/handlers/shared/create-task-core.test.ts index 8edf6220..21d59e9b 100644 --- a/cdk/test/handlers/shared/create-task-core.test.ts +++ b/cdk/test/handlers/shared/create-task-core.test.ts @@ -114,8 +114,39 @@ describe('createTaskCore', () => { expect(mockLambdaSend).toHaveBeenCalledTimes(1); }); - test('returns 400 for invalid repo', async () => { - const result = await createTaskCore({ repo: 'invalid' } as any, makeContext(), 'req-1'); + test('accepts an initial_approvals pattern whose value contains a colon', async () => { + // Regression: the degenerate-pattern guard used split(':', 2)[1], which + // truncated the value at the next colon. For "ab:cdefgh" that yields the + // 2-char fragment "ab", which isDegeneratePattern flags as degenerate — + // a spurious 400. The full value "ab:cdefgh" is not degenerate, so the + // scope must be accepted. + const result = await createTaskCore( + { + repo: 'org/repo', + task_description: 'Fix the bug', + initial_approvals: ['bash_pattern:ab:cdefgh'], + } as any, + makeContext(), + 'req-1', + ); + expect(result.statusCode).toBe(201); + }); + + test('still rejects a genuinely degenerate initial_approvals pattern', async () => { + const result = await createTaskCore( + { + repo: 'org/repo', + task_description: 'Fix the bug', + initial_approvals: ['bash_pattern:*'], + } as any, + makeContext(), + 'req-1', + ); + expect(result.statusCode).toBe(400); + expect(JSON.parse(result.body).error.code).toBe('VALIDATION_ERROR'); + }); + + test('returns 400 for invalid repo', async () => { const result = await createTaskCore({ repo: 'invalid' } as any, makeContext(), 'req-1'); expect(result.statusCode).toBe(400); expect(JSON.parse(result.body).error.code).toBe('VALIDATION_ERROR'); });