fix(cdk): normalize after cursor case + colon-truncation in approval-scope guard (#360)

* fix(cdk): normalize after cursor case and fix colon-truncation in approval-scope guard

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.

* chore(cdk): apply eslint --fix formatting to create-task-core test Resolves the CI 'Fail build on mutation' step: the build runs eslint --fix, which reformats the 'returns 400 for invalid repo' test (splitting the arrow body onto its own line). The autofix was never committed, so a clean build tree diverged from the checkout. Commit the formatted file so the build is a no-op mutation-wise.

---------

Co-authored-by: bgagent <bgagent@noreply.github.com>
Co-authored-by: Alain Krok <alkrok@amazon.com>

Author: 3 people

cdk/src/handlers/get-task-events.ts

@@ -118,7 +118,16 @@ export async function handler(event: APIGatewayProxyEvent): Promise<APIGatewayPr
       );
     }
 
-    const afterValid = typeof afterRaw === 'string' && afterRaw.length === ULID_LENGTH ? afterRaw : undefined;
+    // Normalize the cursor to upper-case before it reaches the DynamoDB key
+    // condition. ``isValidUlid`` accepts lower-case callers (it uppercases
+    // before matching), but stored ``event_id``s are upper-case Crockford
+    // Base32. DynamoDB compares raw bytes, and lower-case ASCII sorts *after*
+    // upper-case, so a lower-case cursor would be "greater than" every stored
+    // id and ``event_id > :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

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,

cdk/test/handlers/get-task-events.test.ts

@@ -269,6 +269,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 {

cdk/test/handlers/shared/create-task-core.test.ts

@@ -114,6 +114,38 @@ describe('createTaskCore', () => {
     expect(mockLambdaSend).toHaveBeenCalledTimes(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);