Conversation
JWThewes
left a comment
There was a problem hiding this comment.
The structured logging migration is a reasonable direction, but I found two material regressions: MCP diagnostics are written into the protocol stream, and enabling event logging exposes credentials. Please address the two inline comments before merging.
Validation: all 2,837 backend tests passed locally, and all Lambda bundles built successfully. Separate before/after probes reproduced both issues on this commit and confirmed they are absent from the base commit.
| console.error( | ||
| `[mcp-trace] ${name} ok=${!env?.isError} bytes=${bytes} ms=${Date.now() - startedAt} args=${Object.keys(args ?? {}).join(',')}`, | ||
| ); | ||
| logger.info('mcp-trace', { |
There was a problem hiding this comment.
[P1] Keep MCP diagnostics on stderr
At the default INFO level, this writes a Powertools JSON log to stdout. This module runs behind StdioServerTransport, so stdout is the MCP JSON-RPC stream consumed by the agent CLI. Each traced tool call now emits a non-protocol message, and the new startup log in mcp/index.js:77 has the same problem. I reproduced a ZodError using the actual MCP SDK stdio client against the entrypoint; the base commit produces no protocol errors. This can break initialization or tool calls in clients that reject malformed protocol output. Route all diagnostics emitted by the MCP process to stderr and add a regression test through the real stdio transport.
| export const handler = async (event, context) => { | ||
| if (context) logger.addContext(context); | ||
| logger.resetKeys(); | ||
| logger.logEventIfEnabled(event); |
There was a problem hiding this comment.
[P1] Redact credentials before logging API events
logEventIfEnabled(event) logs the entire API Gateway event without redaction. With POWERTOOLS_LOGGER_LOG_EVENT=true, I reproduced a successful PUT /agents/settings that writes the raw Bedrock bearer token, Kiro API key, and Authorization header to the log. The base commit logs none of these values. The same full-event logging approach also covers project MCP-secret updates and tracker OAuth callbacks, so excluding git-handler.js does not preserve the credential-redaction boundary. Although the flag defaults to false, enabling diagnostics should not disclose credentials. Log allowlisted request metadata or redact sensitive headers, bodies, and query parameters before logging, and cover the credential-bearing routes with regression tests.
|
Addressed both review findings:
Validation: 2,856 backend tests pass; lint, formatting, and Terraform validation pass. |
f7b45ed to
abdfc5c
Compare
JWThewes
left a comment
There was a problem hiding this comment.
Re-reviewed the latest changes. The MCP stdout issue is addressed: diagnostics use stderr again, and the real stdio transport regression test passes. The original Bedrock, Kiro, and Authorization values are also now redacted.
One blocker remains: credential redaction is incomplete, as detailed in the inline comment. I reproduced the remaining leaks using synthetic credentials in a successful settings update.
Validation: all 2,945 backend tests passed locally, all Lambda bundles built successfully, and CI is green.
| if (normalized === 'ticket' && route.includes('/trackers/connections/')) { | ||
| return [key, REDACTED]; | ||
| } | ||
| if (inMcpConfig && (normalized === 'headers' || normalized === 'env')) { |
There was a problem hiding this comment.
[P1] Complete credential redaction before logging API events
The MCP branch masks env and headers, but leaves url and args unchanged. With POWERTOOLS_LOGGER_LOG_EVENT=true, I reproduced a successful PUT /agents/settings using configurations accepted by validateMcpServersJson where API keys in a remote URL (?api_key=...) and command arguments (--api-key ...) both reach the log.
The same request also logs X-Origin-Verify unchanged through redactNamedValues. CloudFront injects this deployment secret into API requests, and the optional API origin policy uses it to verify that traffic came through CloudFront. It needs protection in both headers and multiValueHeaders.
The original Bedrock, Kiro, and Authorization fields are correctly redacted now, but these remaining cases keep the credential-logging finding open. For simplicity and maintainability, prefer allowlisted request metadata rather than an expanding credential blacklist. Alternatively, redact entire MCP configurations and add the origin-verification header to the sensitive headers. Please cover these cases with regression tests.
Summary
Migrates the entire Lambda codebase (plus shared modules and the agentcore/yjs container runtimes) from raw
console.*to AWS Lambda PowertoolsLogger, producing structured JSON logs with consistent correlation context.Logging convention (three layers)
collaborative-aidlc, set once viaPOWERTOOLS_SERVICE_NAME(a Terraformlocalreferenced by all 28 Lambda blocks + the AgentCore and yjs container runtimes), never hardcoded in constructors.persistentKeysentry.[prefix]tag (e.g. both MCP files →module: 'mcp').Correlation context
appendKeys+resetKeysadd request-scoped ids (intentId/projectId/taskId/userId) on the orchestration-heavy handlers (intents, projects, agents) and v2-orchestrator (migrated off the durable-SDKctx.logger).Event logging (opt-in)
POWERTOOLS_LOGGER_LOG_EVENTbool (default false) wired to all Lambda blocks;logEventIfEnabled(event)on the API-GW handlers. The git OAuth-callback paths are excluded —git-handler.jskeeps its bespoke redacted Request log so enabling event logging can't leak secrets.Failure visibility
http-server.js: unknown/missing command, any{ ok: false }result, any throw) and the orchestrator runtime-invoke path (defaultInvokeRuntime), so silent failures (e.g. a runtime error returned as a non-throwing body) surface in CloudWatch withcommand+ reason/detail/error.Correctness fixes surfaced during review
logsinks (info vs error), normalized error logging to the positionallogger.error(msg, err, {ctx})shape, stripped redundant[component]message prefixes.Verification
terraform fmtclean.Follow-ups (not in this PR)
logparam seam in the agentcore commands (TODOs in place).Covers #281 partially