Skip to content

fix: retain safe background error codes - #33

Merged
jerelvelarde merged 2 commits into
CopilotKit:mainfrom
kvnloo:fix/background-error-code
Sep 24, 2026
Merged

jerelvelarde merged 2 commits into
CopilotKit:mainfrom
kvnloo:fix/background-error-code

Conversation

@kvnloo

@kvnloo kvnloo commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

What changed

Follow-up to the maintainer note on #19: backgroundFailure() currently keeps only Error.name, so PostgreSQL failover errors can collapse to an unhelpful error: "error".

Retain a sanitized string code when it contains only a short identifier-safe value. This preserves SQLSTATE values such as 57P01 and common runtime codes such as ECONNRESET without logging error messages, provider payloads, URLs, or arbitrary code strings.

The regression verifies that:

  • a SQLSTATE-like code is retained;
  • a credential-bearing error message is not logged;
  • an unsafe code string is omitted.

This is independent of #19 and simply makes the existing centralized background logger more diagnostic.

Verification

  • Reviewed the branch against current main at 82ff35b.
  • Added focused unit coverage for safe-code retention and unsafe-string omission.
  • Repository tests were not run locally because this execution environment cannot clone/install the repository; CI on this PR is the executable verification.

Integration limits

No provider/database integration run; this only changes structured background error logging.

AI-use note: I used an AI assistant to inspect the logging path, draft the sanitizer and regression, and review the final diff. I verified that no error message or payload is added to the log output before opening this PR.

@jerelvelarde

Copy link
Copy Markdown
Collaborator

Security review — no issues found (reviewed head 6bfe7e8)

  • The new field is tightly filtered. code is logged only when it's a string matching ^[A-Za-z0-9_.-]{1,64}$. That's enough for SQLSTATE and errno values like 57P01 and ECONNRESET, but it can't carry a URL, a credential-bearing message or a provider payload: :, /, @ and whitespace are all rejected.
  • Messages stay out of the logs. The error message is still never logged, and the test confirms a postgres://user:secret@… message doesn't show up in the output.
  • Non-string codes are ignored. A numeric or object code is dropped, so this can't inject structured data into the log.
  • Scope is limited. It touches only apps/server/src/log.ts and tests/log.test.ts: no dependencies, lockfile, CI or config changes.

CI hasn't run on this PR yet; this should merge after #19 once it's green.

@jerelvelarde
jerelvelarde merged commit 2c83474 into CopilotKit:main Sep 24, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants