Skip to content

fix(logging): enforce values-free operational logs - #20

Merged
rschlaefli merged 9 commits into
mainfrom
rs/codeapi-values-free-logs
Sep 1, 2026
Merged

fix(logging): enforce values-free operational logs#20
rschlaefli merged 9 commits into
mainfrom
rs/codeapi-values-free-logs

Conversation

@rschlaefli

@rschlaefli rschlaefli commented Aug 31, 2026

Copy link
Copy Markdown
Member

What This Fixes

This PR creates one values-free operational logging boundary for CodeAPI and
also keeps download failures generic at the public HTTP boundary.

  1. Sanitizes every enabled CodeAPI Winston and Pino sink before serialization.
  2. Replaces runtime message strings with fixed event text.
  3. Retains only closed, code-declared operational metadata.
  4. Returns { "error": "Error downloading file" } for download failures
    without exposing the caught error in the response.

This is an independent replacement for the logging/privacy portion of draft
PR #18. Issuer trust remains in PR #19. Optional OpenAPI/type maintenance
remains in PR #21 and is not required by this package or the deployment gate.

How It Works

  • shared/operational-log.ts owns the pure allowlist and error classifier.
  • The service's three Winston constructors apply it before serialization.
  • The sandbox API's Pino hook sanitizes each call and removes host/process
    bindings.
  • The sole production Pino child binding is the fixed value
    component: job.
  • The standalone tool-call proxy emits fixed failure text.
  • The download route retains detailed diagnostics only in the sanitized
    server-side sink and returns one fixed public 500 body.

Important Details

  • Arbitrary identifiers, payloads, filenames, paths, errors, stacks, and
    child-process output are removed. Unknown errors become internal.
  • Circular, repeated, buffered, array, error, and throwing-getter inputs are
    handled without mutating caller-owned values or throwing.
  • The download correction is deliberately one response-line change. The
    repository has no router/Supertest harness; adding one solely for a static
    JSON literal would expand this upstream patch without meaningful additional
    coverage.
  • No dependency, lockfile, authentication, schema, database, deployment,
    image, or telemetry change is included.
  • Rollback is an ordinary source revert.

Branch Coverage

  • Base: main@83c4f7b105b6b3e69eda12701ad4ec437acba08f
  • Head: d71f81a9c84634b27d30e85b097d88f6a3a495ba
  • Reviewed range: 83c4f7b..d71f81a
  • Changed: 14 files, +613/-20 lines; no dependency or lockfile delta.
  • Covered: shared policy, one Pino factory, three Winston constructors, one
    child binding, one fixed console bypass, generic download failure, capture
    tests, build inclusion, and fork ledger.

Review Focus

  • Confirm every enabled app sink reaches the shared policy or documented fixed
    console exception.
  • Confirm no retained key or value class can contain identifiers, payloads,
    arbitrary errors, stacks, filenames, paths, or child-process output.
  • Confirm Pino child bindings and Winston metadata cannot bypass the policy.
  • Confirm the public download response is fixed while diagnostics remain
    server-side and sanitized.

Verification

At the exact head:

  • bun run test from service/ -> 556 pass, 0 fail.
  • bun run test from api/ -> 354 pass, 16 skip, 0 fail.
  • Focused service and API logger tests -> 3 pass, 0 fail.
  • bun run build from service/ -> exit 0.
  • Isolated API entrypoint and proxy builds -> exit 0.
  • git diff --check origin/main...HEAD -> pass.
  • GLM simplifier and privacy/security slice review -> passed after scoped
    correction rechecks.
  • Integrated GLM final review -> passed with no unresolved findings.
  • CI run 33442771940
    -> all five required jobs passed at d71f81a.

The service build retains existing Rollup/TypeScript warnings outside this
package. No deployment, cluster, production-log, or live-execution proof was
performed.

Security / Privacy

  • Tests use synthetic sentinels only.
  • No credentials, personal data, production payloads, tokens, execution
    content, or raw production logs are included.
  • The review found no change-introduced security or privacy finding at the
    committed head.

Blocking Before Merge

  • All five required CI jobs pass at the exact updated head.
  • Human review and explicit merge approval remain required.

Follow-Up After Merge

  • Qualify one exact UZH main SHA containing PR fix(auth): bind JWT trust by issuer #19 issuer trust and this
    PR before building an image or changing deployment desired state.
  • Treat PR fix(api): align maintained CodeAPI contracts #21 as independent optional contract maintenance, not a source,
    image, or deployment prerequisite.
  • Perform image publication, deployment, and live log-sink proof only
    through their separately approved rollout steps.

@rschlaefli
rschlaefli marked this pull request as ready for review September 1, 2026 07:10
Copilot AI lite review requested due to automatic review settings September 1, 2026 07:10
@rschlaefli
rschlaefli merged commit d2382d6 into main Sep 1, 2026
5 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants