feat(project): Precommit hooks#12
Merged
Merged
Conversation
scoropeza
pushed a commit
to scoropeza/sample-autonomous-cloud-coding-agents
that referenced
this pull request
May 5, 2026
…rokoko review aws-samples#1, aws-samples#5, aws-samples#9, aws-samples#12) Four related fixes on the fanout + github-comment surface, from the code review on PR aws-samples#52. Grouped because they share the narrative "defense-in-depth on the fanout dispatcher" — any one landing without the others leaves a hole. ## Findings addressed **aws-samples#1 — Fanout handler returns void despite reportBatchItemFailures: true** The ``FanOutConsumer`` construct (``cdk/src/constructs/fanout-consumer.ts:146``) has ``reportBatchItemFailures: true`` on its DDB Stream event-source mapping. The handler returned ``void``, so Lambda retried the entire batch on any unhandled throw instead of isolating the poisonous record. Combined with aws-samples#5 this could cascade into retry storms and violated the per-task ordering guarantee we rely on (§6.4, AD-9). Fix: handler return type becomes ``Promise<DynamoDBBatchResponse>``. Per-record processing is wrapped in try/catch; caught throws push ``{ itemIdentifier: record.eventID }`` to ``batchItemFailures`` and emit ``fanout.record.failed`` warn. Final ``fanout.batch.complete`` log grows a ``failed`` count. Note: ``DynamoDBStreamHandler`` constrains return to ``void | Promise<void>``, so the handler is typed as a plain 3-arg async function. Lambda's runtime accepts either shape; existing tests (passing ``event, context, cb``) work unchanged. **aws-samples#5 — Unhandled exception in routeEvent crashes batch** ``routeEvent`` uses ``Promise.allSettled`` internally, but ``resolveTokenSecretArn`` can throw ``AccessDeniedException`` SYNCHRONOUSLY before the ``allSettled`` guard is reached. The new per-record try/catch from aws-samples#1 catches these too. **aws-samples#9 — renderCommentBody not self-defending against uncoerced DDB strings** The ``.toFixed(4)`` call on ``costUsd`` is the same bug class as the ``toFixed is not a function`` crash we fixed at the fanout boundary in commit 9fe704e. Today the sole call site coerces via the shared helper; a future caller that forgets to would crash. Fix: ``renderCommentBody`` coerces ``durationS`` and ``costUsd`` internally via the shared ``coerceNumericOrNull`` helper (second line of defense; caller's coercion remains the first). Widened ``CommentBodyInput`` fields to ``number | string | null`` to honestly model the DDB Document-client boundary. **aws-samples#12 — Markdown injection possible via prUrl in GitHub comment body** ``prUrl`` was interpolated directly into a Markdown link target (``[link](${input.prUrl})``). A crafted URL containing ``)`` / ``|`` / ``\n`` could break the table layout or inject content, and a ``javascript:`` scheme could produce a click-to-execute link on some Markdown renderers. Fix: new exported ``sanitizeMarkdownLinkTarget`` helper in ``shared/github-comment.ts`` rejects URLs containing ``\r\n\t\s)|]"<>`` characters, validates via ``new URL()``, and rejects non-http(s) schemes. Returns ``null`` on rejection so ``renderCommentBody`` omits the Pull-request row entirely rather than emitting a broken or unsafe link. ## Tests +22 regression tests net (fanout 7 for aws-samples#1+aws-samples#5 + 3 for aws-samples#9; github-comment 12 for aws-samples#12): - Fanout partial-batch: poison-pill isolation, mixed-batch (good record NOT in failures), observability warn, empty-failures regression guard, baseline pin that today's ``Promise.allSettled`` containment still works. - renderCommentBody numeric self-defense: string-typed values render correctly; non-finite strings collapse to null with warn; null does NOT warn. - sanitizeMarkdownLinkTarget unit tests: accept clean http/https, reject 9 injection patterns, reject 4 non-http schemes (``javascript:``, ``data:``, ``file:``, ``ftp:``), reject malformed, handle null/undefined. Plus end-to-end assertions on ``renderCommentBody`` proving the PR row is omitted on rejection. CDK suite: 1029 passing (was 1001). Refs: krokoko code review on PR aws-samples#52 (findings 1, 5, 9, 12)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Area
cdk— infrastructure, handlers, constructsagent— Python runtime / Docker imagecli—bgagentclientdocs— guides or design sources (docs/guides/,docs/design/)tooling— rootmise.toml, scripts, CI workflowsTip: AGENTS.md lists where to edit and which tests to extend.
Related
Changes
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.