fix(runtime-node): report flush failures correctly - #18
Conversation
There was a problem hiding this comment.
🔴 Autter review in progress — running security, correctness & dependency checks on this PR. Follow live step-by-step progress on the autter/review-gate check in the merge box. Merge is blocked until the gate completes; Autter approves automatically when the review comes back clean, and releases this hold with a neutral review when it finds non-blocking issues.
🚦 Pre-merge checks ·
|
| Check | Status | Explanation |
|---|---|---|
| Mixed concerns (refactor + behavior change) | This PR mixes a behavior fix for auto-flush failure reporting with a separate behavior change in redaction logic. Split into one PR for the runtime-node auto-flush failure propagation in packages/runtime-node/src/server.ts and packages/runtime-node/src/lifecycle.ts plus `packages/runtime-node/test/lifecycle.test.mj ... [truncated 318 chars] |
|
| Removed observability | 1 potential issue(s) detected (max risk 55/100): packages/runtime-node/src/lifecycle.ts:205. | |
| Silent exception swallowing | 2 potential issue(s) detected (max risk 78/100): packages/runtime-node/src/server.ts:919, packages/runtime-node/src/lifecycle.ts:209. | |
| Multi-write without detected transaction | 1 potential issue(s) detected (max risk 58/100): packages/runtime-node/src/redact.ts:152. | |
| Possible non-atomic read-modify-write | 2 potential issue(s) detected (max risk 57/100): packages/runtime-node/src/server.ts:910, packages/runtime-node/src/lifecycle.ts:206. | |
| Batch size limit not detected | 2 potential issue(s) detected (max risk 67/100): packages/runtime-node/src/redact.ts:139, packages/runtime-node/src/server.ts:910. | |
| PII or internals leaked in error response | 1 potential issue(s) detected (max risk 74/100): packages/runtime-node/src/server.ts:924. | |
| Missing linked tracker issue | 1 potential issue(s) detected (max risk 58/100): packages/runtime-node/src/server.ts:910. | |
| Missing CODEOWNERS reviewer approval | 5 potential issue(s) detected (max risk 76/100): packages/runtime-node/src/server.ts:910, packages/runtime-node/src/lifecycle.ts:205, packages/runtime-node/src/redact.ts:134, packages/runtime-node/test/lifecycle.test.mjs:5, packages/runtime-node/test/redact.test.mjs:128. | |
| Multi-write without transaction wrapper | 1 potential issue(s) detected (max risk 90/100): packages/runtime-node/src/redact.ts:152. | |
| Hallucinated import (package not installed) | 1 potential issue(s) detected (max risk 80/100): packages/runtime-node/test/lifecycle.test.mjs:2. | |
| Generic placeholder identifier in production logic | 2 potential issue(s) detected (max risk 54/100): packages/runtime-node/src/redact.ts:148, packages/runtime-node/src/server.ts:910. | |
| Unhandled edge case (null / empty / zero / boundary) | 2 potential issue(s) detected (max risk 65/100): packages/runtime-node/src/lifecycle.ts:206, packages/runtime-node/src/redact.ts:204. | |
| Runtime error risk | 2 finding(s) on changed lines. | |
| Co-change coupling | 1 file(s) changed without a partner they usually change with. | |
| Excessive complexity | 1 finding(s) on changed lines. | |
| Complexity Guard | 1 finding(s) on changed lines. | |
| Bundle Size Monitor | 1 finding(s) on changed lines. |
✅ Passed checks (151)
| Check | Status | Explanation |
|---|---|---|
| Too many files changed | ✅ Passed | Changed 5 file(s), within the limit of 50. |
| Too many lines changed | ✅ Passed | Changed 237 line(s), within the limit of 1000. |
| Too many unrelated chapters | ✅ Passed | 2 chapter(s) detected, within the limit of 6. |
| Generated files hiding real changes | ✅ Passed | Generated-file volume (0 lines) does not obscure the 237 hand-written line(s). |
| Missing PR context | ✅ Passed | PR context looks sufficient. |
| Migration + app logic + UI combined in one PR | ✅ Passed | No database migration files or UI changes are present. |
| Sensitive data in logs | ✅ Passed | No sensitive data in logs issues detected. |
| Log injection | ✅ Passed | No log injection issues detected. |
| Missing audit logging | ✅ Passed | No missing audit logging issues detected. |
| Unhandled promise rejection | ✅ Passed | No unhandled promise rejection issues detected. |
| Circuit breaker not detected | ✅ Passed | No circuit breaker not detected issues detected. |
| Stack trace leakage | ✅ Passed | No stack trace leakage issues detected. |
| Possible TOCTOU in critical path | ✅ Passed | No possible toctou in critical path issues detected. |
| Idempotency key not detected | ✅ Passed | No idempotency key not detected issues detected. |
| Optimistic locking not detected | ✅ Passed | No optimistic locking not detected issues detected. |
| Rate limiting not detected | ✅ Passed | No rate limiting not detected issues detected. |
| Rate limiting removed | ✅ Passed | No rate limiting removed issues detected. |
| Pagination not detected | ✅ Passed | No pagination not detected issues detected. |
| Publicly exposed storage | ✅ Passed | No publicly exposed storage issues detected. |
| Over-permissive IAM policy | ✅ Passed | No over-permissive iam policy issues detected. |
| Security group open to the internet | ✅ Passed | No security group open to the internet issues detected. |
| Unencrypted storage at rest | ✅ Passed | No unencrypted storage at rest issues detected. |
| Infrastructure missing access logging | ✅ Passed | No infrastructure missing access logging issues detected. |
| Hardcoded secret in IaC | ✅ Passed | No hardcoded secret in iac issues detected. |
| Infrastructure misconfiguration | ✅ Passed | No infrastructure misconfiguration issues detected. |
| Deprecated Kubernetes API version | ✅ Passed | No deprecated kubernetes api version issues detected. |
| Compound IaC attack chain | ✅ Passed | No compound iac attack chain issues detected. |
| Prompt injection risk | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| LLM output used in a dangerous sink | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Sensitive data in prompt or system-prompt leakage | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Over-privileged LLM tool / excessive agency | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Missing validation on an LLM-driven decision | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Unbounded LLM usage (denial-of-wallet) | ✅ Passed | No LLM/AI-integration code touched by this diff. |
| Table exposed without row-level security | ✅ Passed | No row-level-security-related code touched by this diff. |
| Over-broad row-level security policy | ✅ Passed | No row-level-security-related code touched by this diff. |
| Code path that bypasses row-level security | ✅ Passed | No row-level-security-related code touched by this diff. |
| Privileged database credential reachable from the client | ✅ Passed | No row-level-security-related code touched by this diff. |
| Privileged query without row-level scoping | ✅ Passed | No row-level-security-related code touched by this diff. |
| Template-default gradient styling | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Interchangeable AI marketing copy | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Placeholder content shipped to users | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Emoji standing in for an icon system | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Call-to-action that goes nowhere | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Templated page composition | ✅ Passed | No added frontend pages or design-slop markers in this diff. |
| Merge-blocking marker left in the change | ✅ Passed | No pending-work markers added by this diff. |
| Known-defect marker shipped in code | ✅ Passed | No pending-work markers added by this diff. |
| Untracked TODO without an issue reference | ✅ Passed | No pending-work markers added by this diff. |
| Test disabled or left pending | ✅ Passed | No pending-work markers added by this diff. |
| PII in logs | ✅ Passed | No pii in logs issues detected. |
| PII stored without application-level encryption | ✅ Passed | No pii stored without application-level encryption issues detected. |
| User data stored without retention controls | ✅ Passed | No user data stored without retention controls issues detected. |
| PII sent to external / cross-border destination | ✅ Passed | No pii sent to external / cross-border destination issues detected. |
| Lockfile resolution / integrity tampered | ✅ Passed | No lockfile resolution / integrity tampered issues detected. |
| Dependency runs install-time lifecycle script | ✅ Passed | No dependency runs install-time lifecycle script issues detected. |
| Possible dependency-confusion attack | ✅ Passed | No possible dependency-confusion attack issues detected. |
| Lockfile resolves a dependency the manifest does not declare | ✅ Passed | No lockfile resolves a dependency the manifest does not declare issues detected. |
| Checked-in build artefact modified without source change | ✅ Passed | No checked-in build artefact modified without source change issues detected. |
| Dockerfile build-step is insecure | ✅ Passed | No dockerfile build-step is insecure issues detected. |
| External artefact pulled in without integrity pinning | ✅ Passed | No external artefact pulled in without integrity pinning issues detected. |
| Changed export, importer not updated | ✅ Passed | No changed export with an un-updated importer detected. |
| Missing security-team review on sensitive path | ✅ Passed | No missing security-team review on sensitive path issues detected. |
| Source changes without matching tests | ✅ Passed | No source changes without matching tests issues detected. |
| Migration missing rollback / down step | ✅ Passed | No migration missing rollback / down step issues detected. |
| Frontend importing database client directly | ✅ Passed | No frontend importing database client directly issues detected. |
| Route handler bypassing service layer | ✅ Passed | No route handler bypassing service layer issues detected. |
| Backend service importing UI module | ✅ Passed | No backend service importing ui module issues detected. |
| Cross-context internals import | ✅ Passed | No cross-context internals import issues detected. |
| Workspace package rule violation | ✅ Passed | No workspace package rule violation issues detected. |
| Inconsistent logging pattern | ✅ Passed | No inconsistent logging pattern issues detected. |
| Inconsistent error handling | ✅ Passed | No inconsistent error handling issues detected. |
| Endpoint missing input validation | ✅ Passed | No endpoint missing input validation issues detected. |
| New feature shipped without feature flag | ✅ Passed | No new feature shipped without feature flag issues detected. |
| Module placed in the wrong workspace package | ✅ Passed | No module placed in the wrong workspace package issues detected. |
| Direct env-var access bypasses config module | ✅ Passed | No direct env-var access bypasses config module issues detected. |
| Nonexistent package (not found in registry) | ✅ Passed | No nonexistent package (not found in registry) issues detected. |
| Call to function that does not exist | ✅ Passed | No call to function that does not exist issues detected. |
| Repetitive boilerplate (duplicated block) | ✅ Passed | No repetitive boilerplate (duplicated block) issues detected. |
| Overbroad try/catch swallowing all exceptions | ✅ Passed | No overbroad try/catch swallowing all exceptions issues detected. |
| TODO / FIXME on critical path | ✅ Passed | No todo / fixme on critical path issues detected. |
| Comment contradicts or fabricates code behaviour | ✅ Passed | No comment contradicts or fabricates code behaviour issues detected. |
| Abstraction defined but never used | ✅ Passed | No abstraction defined but never used issues detected. |
| Code style differs from rest of codebase | ✅ Passed | No code style differs from rest of codebase issues detected. |
| Established pattern ignored | ✅ Passed | No established pattern ignored issues detected. |
| Doc-copy code with insecure defaults | ✅ Passed | No doc-copy code with insecure defaults issues detected. |
| Dead code (defined but never referenced) | ✅ Passed | No dead code (defined but never referenced) issues detected. |
| Deprecated API call | ✅ Passed | No deprecated api call issues detected. |
| API pattern from wrong library version | ✅ Passed | No api pattern from wrong library version issues detected. |
| API endpoint removed | ✅ Passed | No api endpoint removed issues detected. |
| HTTP method changed (GET ↔ POST etc.) | ✅ Passed | No http method changed (get ↔ post etc.) issues detected. |
| New required field added to request | ✅ Passed | No new required field added to request issues detected. |
| Field removed from response schema | ✅ Passed | No field removed from response schema issues detected. |
| Response field type changed | ✅ Passed | No response field type changed issues detected. |
| HTTP status code changed | ✅ Passed | No http status code changed issues detected. |
| Auth requirement added / removed / changed | ✅ Passed | No auth requirement added / removed / changed issues detected. |
| Error response shape changed | ✅ Passed | No error response shape changed issues detected. |
| Pagination behaviour changed | ✅ Passed | No pagination behaviour changed issues detected. |
| Outbound webhook payload schema changed | ✅ Passed | No outbound webhook payload schema changed issues detected. |
| GraphQL field removed without deprecation | ✅ Passed | No graphql field removed without deprecation issues detected. |
| GraphQL enum value removed | ✅ Passed | No graphql enum value removed issues detected. |
| SQL injection | ✅ Passed | No sql injection issues detected. |
| Cross-site scripting (XSS) | ✅ Passed | No cross-site scripting (xss) issues detected. |
| Path traversal | ✅ Passed | No path traversal issues detected. |
| Command injection | ✅ Passed | No command injection issues detected. |
| Insecure deserialization | ✅ Passed | No insecure deserialization issues detected. |
| Weak cryptography | ✅ Passed | No weak cryptography issues detected. |
| Hardcoded secret | ✅ Passed | No hardcoded secret issues detected. |
| Insecure randomness for security material | ✅ Passed | No insecure randomness for security material issues detected. |
| Unsafe file upload | ✅ Passed | No unsafe file upload issues detected. |
| Missing input validation | ✅ Passed | No missing input validation issues detected. |
| Unsafe CORS configuration | ✅ Passed | No unsafe cors configuration issues detected. |
| Unsafe / open redirect | ✅ Passed | No unsafe / open redirect issues detected. |
| Missing CSRF protection | ✅ Passed | No missing csrf protection issues detected. |
| Unsafe cookie / session settings | ✅ Passed | No unsafe cookie / session settings issues detected. |
| Sensitive data exposure | ✅ Passed | No sensitive data exposure issues detected. |
| API key in source | ✅ Passed | No api key in source detected. |
| Access token in source | ✅ Passed | No access token in source detected. |
| Private key in source | ✅ Passed | No private key in source detected. |
| Database connection URL with embedded credentials | ✅ Passed | No database connection url with embedded credentials detected. |
| Cloud credential in source | ✅ Passed | No cloud credential in source detected. |
| Webhook signing secret in source | ✅ Passed | No webhook signing secret in source detected. |
| OAuth client secret in source | ✅ Passed | No oauth client secret in source detected. |
| JWT signing secret in source | ✅ Passed | No jwt signing secret in source detected. |
| Hardcoded password | ✅ Passed | No hardcoded password detected. |
| Auth middleware removed from route | ✅ Passed | No auth middleware removed from route issues detected. |
| Route protection changed (protected → public) | ✅ Passed | No route protection changed (protected → public) issues detected. |
| Permission / RBAC check removed | ✅ Passed | No permission / rbac check removed issues detected. |
| Required role weakened | ✅ Passed | No required role weakened issues detected. |
| Admin-only route exposed to lower privilege | ✅ Passed | No admin-only route exposed to lower privilege issues detected. |
| Token validation skipped in middleware chain | ✅ Passed | No token validation skipped in middleware chain issues detected. |
| JWT verification weakened or changed | ✅ Passed | No jwt verification weakened or changed issues detected. |
| Session expiration / TTL changed | ✅ Passed | No session expiration / ttl changed issues detected. |
| Password reset flow changed | ✅ Passed | No password reset flow changed issues detected. |
| OAuth callback / redirect handling changed | ✅ Passed | No oauth callback / redirect handling changed issues detected. |
| Webhook endpoint missing signature verification | ✅ Passed | No webhook endpoint missing signature verification issues detected. |
| Public route touches private/PII data | ✅ Passed | No public route touches private/pii data issues detected. |
| Frontend performance issue | ✅ Passed | No additional explanation was reported. |
| Frontend security issue | ✅ Passed | No additional explanation was reported. |
| Frontend correctness issue | ✅ Passed | No additional explanation was reported. |
| Accessibility issue | ✅ Passed | No additional explanation was reported. |
| Frontend maintainability issue | ✅ Passed | No additional explanation was reported. |
| Code correctness issue | ✅ Passed | No additional explanation was reported. |
| Resource leak risk | ✅ Passed | No additional explanation was reported. |
| Data integrity risk | ✅ Passed | No additional explanation was reported. |
| Maintainability issue | ✅ Passed | No additional explanation was reported. |
| Redundant alias / duplicate import | ✅ Passed | No additional explanation was reported. |
| Redundant type construct | ✅ Passed | No additional explanation was reported. |
| Simplifiable code | ✅ Passed | No additional explanation was reported. |
| Unnecessary type assertion | ✅ Passed | No additional explanation was reported. |
| Module smell | ✅ Passed | No additional explanation was reported. |
| Dead export (no callers) | ✅ Passed | No additional explanation was reported. |
| Code duplication / DRY violation | ✅ Passed | No additional explanation was reported. |
This comment is updated automatically whenever Autter reviews a new PR revision.
🧭 PR hygiene & process suggestionsAutter has 1 suggestion(s) about the shape of this PR (size, scope, reviewability). These are process guidance — not code defects — so they are consolidated here instead of posted as inline comments on individual files. 🟠 PR mixes refactor and behavior change — Risk: 74/100This PR mixes a behavior fix for auto-flush failure reporting with a separate behavior change in redaction logic. 🛠 AI fix prompt (copy & paste into your coding agent)Flagged by Autter PR-hygiene checks.
|
There was a problem hiding this comment.
Autter completed PR review for #18: 3 finding(s) remain below the merge-blocking bar, so this review stays neutral rather than approving. (Also detected: 2 finding(s) dismissed as likely false positives by verification.) See the findings below; the task checklist follows as the review's final comment.
| const drained = Promise.allSettled( | ||
| targets.map((target) => target.forceFlush()), | ||
| ).then(() => true); | ||
| ).then((results) => results.every((result) => result.status === "fulfilled")); |
There was a problem hiding this comment.
🔴 [ai] All built-in exporter failures are still reported as a successful flush — Risk: 82/100
The new aggregate check only observes whether each registered FlushTarget promise fulfills, but initAutterServer registers a single target whose forceFlush() itself calls Promise.allSettled on alwaysOnProvider, mainSpanProcessor, errorTraceBuffer, and metricReader without propagating rejected results (packages/runtime-node/src/server.ts:908-915). Therefore any of those underlying exporters can reject while the wrapper target fulfills, making results.every(...) true here; doFlush then marks all captures flushed and returns true even though no exporter confirmed the data was flushed. This reachable production path can both suppress the failure log and clear the unflushed-data warning during shutdown or signal handling, so the change is not safe unless the registered target propagates aggregate failure or the lifecycle layer receives per-provider results.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/lifecycle.ts,packages/runtime-node/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Make the server's registered FlushTarget reject or otherwise report when any underlying forceFlush operation rejects, and keep installAutterAutoFlush's boolean result tied to that aggregate status. Add coverage for a built-in-style target that internally uses Promise.allSettled with a rejected provider.
Flagged by Autter security & observability checks.
| const drained = Promise.allSettled( | ||
| targets.map((target) => target.forceFlush()), | ||
| ).then(() => true); | ||
| ).then((results) => results.every((result) => result.status === "fulfilled")); |
There was a problem hiding this comment.
🟠 [ai] Production flush target still hides exporter rejections from the new failure check — Risk: 78/100
The changed aggregate check correctly returns false only when a FlushTarget's forceFlush promise rejects, but the production caller installed by initAutterServer wraps every exporter and metric forceFlush in Promise.allSettled and then resolves normally. Consequently an exporter rejection is converted into a fulfilled active-server forceFlush promise, so installAutterAutoFlush receives a fulfilled result, marks telemetryStats as fully flushed, and reports success despite the failed export. The PR therefore does not provide the promised failure signal for the real cross-file call chain.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/lifecycle.ts,packages/runtime-node/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Propagate failure from the active-server FlushTarget instead of unconditionally awaiting Promise.allSettled, or inspect the allSettled results and reject/return a failure when any underlying exporter or metric forceFlush fails before installAutterAutoFlush consumes it.
Flagged by Autter security & observability checks.
| @@ -204,7 +204,7 @@ export function installAutterAutoFlush( | |||
| }); | |||
| const drained = Promise.allSettled( | |||
| targets.map((target) => target.forceFlush()), | |||
There was a problem hiding this comment.
🟠 [ai] Synchronous target throws escape the failed-flush result path — Risk: 72/100
A FlushTarget is explicitly allowed to implement forceFlush() as a synchronous operation, but invoking it directly inside targets.map means a synchronous throw occurs before Promise.allSettled receives the iterable. In that scenario doFlush rejects instead of returning false, the timeout created at line 203 is never cleared, and signal/beforeExit callers attach only a finally handler (or use void flush) so the failure is not reported as the controlled flush failure promised by this API and can surface as an unhandled rejection during shutdown. The changed test covers only Promise.reject and therefore does not prove the public FlushTarget failure contract is safe.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/lifecycle.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Wrap each target invocation in a promise boundary so synchronous throws and asynchronous rejections both become rejected promises consumed by Promise.allSettled, while ensuring the timeout is cleared in a finally block when doFlush exits unexpectedly.
Flagged by Autter security & observability checks.
🧪 Autter test runAutter checked Execution summary: 7 checks executed · 5 passed · 2 failed. Project test commands
Autter targeted verification7 tests executed · 5 passed · 2 failed. Declared tests: 9 test file(s) found — 0 ran, 0 not observed in suite output, 9 did not run.
|
| Changed file | Related test | Result |
|---|---|---|
packages/runtime-node/src/lifecycle.ts |
packages/runtime-node/test/lifecycle.test.mjs |
✅ existing test passes |
packages/runtime-node/src/redact.ts |
packages/runtime-node/test/redact.test.mjs |
✅ existing test passes |
packages/runtime-node/src/server.ts |
packages/runtime-node/test/autoflush.test.mjs |
✅ no declared test — temporary test passed |
🤖 Coverage-check evidence
packages/runtime-node/src/lifecycle.ts
Ran: npm run build -w @autter/runtime-node && node --test test/lifecycle.test.mjs
4/4 passed, including rejected target, synchronous throw, and built-in-style Promise.allSettled exporter rejection.
packages/runtime-node/src/redact.ts
Ran: npm run build -w @autter/runtime-node && node --test test/redact.test.mjs
14/14 passed, including redaction beyond the former traversal depth and circular-reference handling.
packages/runtime-node/src/server.ts
Ran: node --test ../../.autter/scratch/server-flush-failure.test.mjs (from packages/runtime-node)
1/1 passed: active-server flush reports false when a local OTLP collector responds HTTP 500; declared autoflush test covers initAutterServer e2e but not exporter failure propagation.
Temporary tests are written under .autter/scratch/ for verification only — they are never committed to the repository.
Test plan (from the PR description)
- ✅ Run
npm run build -w @autter/runtime-node. — verified by agent execution - ✅ Run
node --test test/lifecycle.test.mjs test/redact.test.mjsfrompackages/runtime-node. — verified by agent execution - ❌ Start a minimal Node application with
initAutterServer, configure an unreachable OTLP endpoint, enableAUTTER_DEBUG=1, and trigger shutdown; verify the flush is reported as failed rather than successful. — agent execution observed the wrong behavior - ❌ Capture an exception with deeply nested and circular custom attributes containing a
passwordor email value; verify the emitted attributes contain the configured redaction mask and the application does not throw. — agent execution observed the wrong behavior
🤖 Agent-executed checks
✅ Run npm run build -w @autter/runtime-node.
Ran: npm run build -w @autter/runtime-node
tsup built ESM, CJS, and DTS successfully.
✅ Run node --test test/lifecycle.test.mjs test/redact.test.mjs from packages/runtime-node.
Ran: node --test test/lifecycle.test.mjs test/redact.test.mjs (working directory: packages/runtime-node)
18 tests passed, 0 failed.
❌ Start a minimal Node application with initAutterServer, configure an unreachable OTLP endpoint, enable AUTTER_DEBUG=1, and trigger shutdown; verify the flush is reported as failed rather than successful.
Ran: AUTTER_DEBUG=1 node .autter/scratch/unreachable-otlp-sigterm.mjs
Unreachable http://127.0.0.1:1 run logged both '[autter] flush did not finish within 3000ms — some telemetry may be lost' and later '[autter] telemetry flushed'; shutdown was therefore also reported as successful.
❌ Capture an exception with deeply nested and circular custom attributes containing a password or email value; verify the emitted attributes contain the configured redaction mask and the application does not throw.
Ran: node .autter/scratch/deep-circular-redaction.mjs
{"threw":true,"name":"RangeError","message":"Maximum call stack size exceeded"}
⬜ items could not be verified automatically and still need a manual check.
There was a problem hiding this comment.
🔴 Autter review in progress — running security, correctness & dependency checks on this PR. Follow live step-by-step progress on the autter/review-gate check in the merge box. Merge is blocked until the gate completes; Autter approves automatically when the review comes back clean, and releases this hold with a neutral review when it finds non-blocking issues.
| mainSpanProcessor.forceFlush(), | ||
| ...(errorTraceBuffer ? [errorTraceBuffer.forceFlush()] : []), | ||
| metricReader.forceFlush(), | ||
| const results = await Promise.allSettled([ |
There was a problem hiding this comment.
🟠 [deterministic] packages/runtime-node/README.md usually changes with this file — Risk: 60/100
packages/runtime-node/src/server.ts and packages/runtime-node/README.md changed together in 6 of the last 6 commits that touched either (100%), but packages/runtime-node/README.md is not in this PR.
This is history, not a rule — if the coupling no longer applies, ignore it. It most often means a matching change was missed (a caller, a type, a fixture, a migration's rollback).
🛠 AI fix prompt (copy & paste into your coding agent)
In this repository, `packages/runtime-node/src/server.ts` and `packages/runtime-node/README.md` have historically changed together (6 shared commits, 100% co-change rate). The current change modifies `packages/runtime-node/src/server.ts` only. Open `packages/runtime-node/README.md` and determine whether it needs a corresponding change. If it does, make it. If it genuinely does not, explain why the coupling no longer holds.
Flagged by Autter security & observability checks.
| if (typeof value === "string") return redactString(value, r); | ||
|
|
||
| if (Array.isArray(value)) { | ||
| const existing = ancestors.get(value); |
There was a problem hiding this comment.
🟠 [ai] Unbounded recursive redaction can overflow the stack on deep acyclic values — Risk: 75/100
Cycle detection handles circular references, but removing maxDepth means redactValue now recursively walks every level of an acyclic object or array. A deeply nested runtime value passed to the exported redactor can exhaust the JavaScript call stack and throw into the host application. Retain a traversal-depth/node budget or use an iterative traversal while preserving cycle handling.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Functions/symbols:
redactAttributes,makeRedactor,redactString,redactValue,isSensitiveKey,redactWith,installAutterAutoFlush,initAutterServer - Dependent files:
@opentelemetry/api - Scopes:
@autter/runtime-node
🛠 AI fix prompt (copy & paste into your coding agent)
Preserve the previous depth limit while adding cycle detection: thread a `depth` parameter through `redactValue`, stop recursing when depth reaches 0, and only use the `WeakMap` to break cycles. Keep the existing whitelist/sensitive-key masking behavior unchanged. Blast radius — if this defect reaches production it can fail the downstream usage that depends on this file: functions `redactAttributes`, `makeRedactor`, `redactString`, `redactValue`, `isSensitiveKey`, `redactWith`, `installAutterAutoFlush`, `initAutterServer`; scopes `@autter/runtime-node`; dependent files `@opentelemetry/api`.
Flagged by Autter security & observability checks.
| ancestors.set(value, out); | ||
|
|
||
| for (const item of value) { | ||
| out.push(redactValue(item, r, ancestors)); |
There was a problem hiding this comment.
🟠 [ai] Unbounded redaction recursion can throw into capture callers — Risk: 79/100
The changed redactValue recursively descends every nested array/object with no depth or work bound (line 149). Consequently a sufficiently deeply nested but acyclic caller-supplied Attributes value throws RangeError before redaction returns. initAutterServer installs the compiled redactor (line 719), and both its captureException and captureMessage callers spread activeRedactor(attributes) without a guard (lines 824 and 868); the captured error/message is therefore not emitted and the synchronous capture API can unexpectedly throw into application or global-error call chains. The previous contract bounded traversal at four levels.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/server.ts,packages/runtime-node/src/redact.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Use an iterative traversal with an explicit depth/work limit, or restore a safe depth bound while preserving cycle handling. Ensure values beyond the limit are returned or replaced safely so activeRedactor never throws for arbitrary Attributes.
Flagged by Autter security & observability checks.
| ancestors.set(value, out); | ||
|
|
||
| for (const item of value) { | ||
| out.push(redactValue(item, r, ancestors)); |
There was a problem hiding this comment.
🟠 [ai] Unbounded recursive redaction can overflow on caller-supplied attributes — Risk: 78/100
The new deep traversal recurses once per array/object nesting level with no depth or work bound. redactAttributes and the compiled activeRedactor both invoke this path for caller-provided attributes; notably initAutterServer configures it at server startup and captureException passes its attributes into it before creating the span. An acyclic object (or array) nested beyond the JavaScript stack limit therefore throws RangeError at this recursive call instead of returning safe attributes, causing the telemetry capture path itself to fail. The WeakMap only terminates cycles and does not protect this reachable acyclic input.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/redact.ts,packages/runtime-node/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Bound redaction traversal with a maximum depth/work budget, or rewrite it iteratively. Preserve cycle handling and ensure values beyond the budget are returned or replaced safely without throwing.
Flagged by Autter security & observability checks.
| ancestors.set(value, out); | ||
|
|
||
| for (const item of value) { | ||
| out.push(redactValue(item, r, ancestors)); |
There was a problem hiding this comment.
🟠 [ai] Unbounded recursive redaction can throw during telemetry capture — Risk: 78/100
The new deep traversal calls redactValue recursively for every nested array element and object property with no depth or work limit. A caller can supply an acyclic attribute nested beyond the JavaScript stack limit; that produces RangeError before redactAttributes returns. The active capture path spreads activeRedactor(attributes) while creating error spans (server.ts:819-825), so this failure escapes captureException and prevents the exception from being recorded. WeakMap only breaks cycles and cannot protect an arbitrarily deep acyclic value.
⚠ Downstream affected — if this fails, it cascades to the usage that depends on this file:
- Dependent files:
packages/runtime-node/src/redact.ts,packages/runtime-node/src/server.ts
🛠 AI fix prompt (copy & paste into your coding agent)
Replace recursive traversal with an iterative walk bounded by an explicit depth/work budget, or restore a safe maximum depth. Preserve the copy/no-mutation and cycle behavior, and make inputs beyond the budget resolve to a safe non-throwing value that cannot leak nested secrets.
Flagged by Autter security & observability checks.
Autter task list
Generated from PR diff, blast radius, and context. Issues found
Also detected but not listed above: 21 finding(s) dismissed as likely false positives by verification — see the Autter review dashboard for their verdicts. 🛠 Fix optionsCheck one option and Autter will start a fix run for the unresolved issues above.
Checking a box triggers the fix run immediately — Autter comments back with the issues being fixed and the branch created for each. |
Summary
Fixes runtime-node auto-flush reporting when a flush target rejects.
Previously,
Promise.allSettled(...).then(() => true)causedflush()to report success even when a flush target rejected.This change checks the settled results and returns
trueonly when all flush targets fulfill.Tests
npm run build -w @autter/runtime-nodepasses.origin/mainand are pre-existing.Summary
Summary generated by Autter.
Corrects runtime-node auto-flush reporting so failed exporter/provider flushes are surfaced as failures rather than being reported as successfully flushed. It also strengthens server-side attribute redaction for deeply nested and circular objects, preventing sensitive values from escaping through custom telemetry attributes.
Changes
installAutterAutoFlushto treat rejected registered flush targets as a failed flush, preserving pending-capture state instead of marking telemetry as fully flushed.initAutterServerflush target to inspectPromise.allSettledresults from the always-on provider, span processor, error-trace buffer, and metric reader, and propagate an aggregate failure when any underlying flush rejects.redactAttributesrecursively traverse nested objects and arrays, retain circular-reference safety, and continue omittingundefinedvalues without mutating caller-owned attributes.Acceptance Criteria
forceFlush()causes the server’s registered flush target to fail.installAutterAutoFlushreturns a failed result and does not calltelemetryStats.markAllFlushed()when any registered target rejects.Test Plan
npm run build -w @autter/runtime-node.node --test test/lifecycle.test.mjs test/redact.test.mjsfrompackages/runtime-node.initAutterServer, configure an unreachable OTLP endpoint, enableAUTTER_DEBUG=1, and trigger shutdown; verify the flush is reported as failed rather than successful.passwordor email value; verify the emitted attributes contain the configured redaction mask and the application does not throw.Rollback Plan
20fce3949365f2e035a6a9cfa3b6a740b04e0fdf.@autter/runtime-nodepackage or consuming runtime image.Related Issues
No linked issue was identified.
Written for commit 20fce39. Summary will update on new commits.