Skip to content

fix(archera): keep sensitive diagnostics out of provider logs - #1078

Merged
cristim merged 1 commit into
mainfrom
fix/1077-provider-safe-logs
Oct 19, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/1077-provider-safe-logs

Conversation

@cristim

@cristim cristim commented Oct 18, 2026 •

Copy link
Copy Markdown
Member

Secret resolution failures could write arbitrary resolver diagnostic text to logs, including sensitive values supplied in an error. Replace the two raw-error warning calls with fixed stage messages. Caller errors, retries, cache identity and locking remain unchanged. The constructor’s current diagnostics already redact the key; this also removes that raw-diagnostic logging boundary without claiming a constructor key leak.

Regression tests capture actual provider warnings, require a warning at each failure stage, reject diagnostic/sentinel text, restore logger state, and check zero HTTP requests plus successful retry and cached-client identity. The known-issue record documents the verified synthetic scope.

Verification at 0985bea53a6945a6ced1ddaaac3eaf0e01c9ae73:

  • Parent: nine runtime assertion failures, including actual synthetic key/backend/reference text and constructor diagnostics. No compilation or constructor-precondition failure.
  • Fixed targeted and full package race tests pass; an independent reviewer reproduced parent RED and fixed GREEN using an exact-parent source overlay.
  • Build, full unit suite across 35 packages, pinned golangci-lint v2.10.1 and normal commit hooks pass.
  • Two independent Sol High reviewers each completed two adversarial passes and approved the exact head; evidence follows below.

No live secret backend, vendor request, credential incident, installation or deployment is claimed. Platform merge remains held pending clarification of existing automatic dev deployments.

Closes #1077
Refs #1076
Refs #799

Summary by CodeRabbit

  • Bug Fixes
    • Initialization failures no longer expose credential, configuration, or backend error details in warning logs or returned errors. Failures are reported with a consistent, safe message and the key-unavailable error.
    • Added coverage to verify sensitive diagnostic details are not revealed and that retry behavior remains intact.

Use fixed stage warnings at resolver and client initialization boundaries.
Add captured-log regressions for real provider failures and retry caching.

Fixes #1077
@cristim cristim added urgency/this-sprint Within the current sprint triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm impact/few Limited audience effort/s Hours type/security Security finding labels Oct 18, 2026
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: f88b73af-1fe3-4dc6-9489-9b4674c1dde0
📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and 0985bea.

📒 Files selected for processing: 3
  • internal/archera/provider.go
  • internal/archera/provider_log_test.go
  • known_issues/resolved/37_archera_provider_error_logs.md

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

Archera provider initialization now logs fixed warnings when secret resolution or client construction fails. It continues to return ErrKeyUnavailable. New tests check log and error contents, transport calls, and retry behavior.

Changes: Archera provider error log redaction

Layer / File(s) Summary
Safe initialization warnings and validation
internal/archera/provider.go, internal/archera/provider_log_test.go, known_issues/resolved/37_archera_provider_error_logs.md
Resolver and client-construction failures now log fixed warnings instead of error details. Tests check that diagnostic and sensitive values are absent from logs and returned errors, and cover transport calls and retry behavior. The resolved-issue record documents the change and test coverage.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 0985b

No merge-blocking issue was identified in the provider warning change. The separate platform deployment hold remains in effect.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preventing sensitive diagnostic details from appearing in Archera provider logs.
Linked Issues check Passed Issue #1077 is open and directly linked. The provider diff replaces both raw-error warnings with fixed stage messages and leaves ErrKeyUnavailable, retry behavior, cache identity, and locking unchange…
Out of Scope Changes check Passed The changes stay within #1077. The provider change is limited to the two warning calls. The added tests support the issue acceptance criteria. The resolved known-issue record documents this same provi…
Full details: Linked Issues check

Explanation

Issue #1077 is open and directly linked. The provider diff replaces both raw-error warnings with fixed stage messages and leaves ErrKeyUnavailable, retry behavior, cache identity, and locking unchanged. The new tests capture pkg/logging output with SetOutput, exercise resolver and real insurance.NewClient failures, reject synthetic key/reference/backend/diagnostic text from logs and returned errors, require the fixed warnings, verify zero HTTP calls, restore logger state, and cover retry and cached-client identity. The change does not add the #1076 cancellation mechanism.

Full details: Out of Scope Changes check

Explanation

The changes stay within #1077. The provider change is limited to the two warning calls. The added tests support the issue acceptance criteria. The resolved known-issue record documents this same provider logging fix and its regression coverage. No unrelated runtime behavior, deployment work, live backend access, or vendor request is included.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Oct 18, 2026

Copy link
Copy Markdown
Member Author

Session: archera_reviewer_a
APPROVED 0985bea by archera_reviewer_a

Two adversarial passes found no actionable findings. Verified head, tree 339d92a5253af97b35f29ba08e331d0b1b36e2d2 and parent 8caed39cab57ef883da861185c9c38a5ecfe03e6. The three-file committed diff exactly matches reviewed patch SHA-256 b2036df800a544e88a40ce9882d839edfde64b607161d561a4c821a934081d84; worktree was clean.

Independent Go 1.26.9 macOS race replay retained the new tests and overlaid only exact-parent provider source. Nine runtime assertion failures included actual synthetic key/backend/reference leakage and both real constructor diagnostics. Fixed source passed both top-level tests and constructor subcases without a race report.

Checked actual logger capture and cleanup, warning presence, fixed caller errors, zero HTTP, failure retry and cached-client identity. Only two constant warning calls change production behavior. The constructor already redacted the key; no real credential incident is claimed. Evidence is synthetic provider/constructor/logging coverage, without a live secret backend, vendor request or deployment.

CI remains a separate gate. This approval does not lift the platform merge/deployment hold.

@cristim

cristim commented Oct 18, 2026

Copy link
Copy Markdown
Member Author

Session: archera_reviewer_b
APPROVED 0985bea by archera_reviewer_b

Two adversarial passes found no actionable issues. Committed tree 339d92a5253af97b35f29ba08e331d0b1b36e2d2 and all three file hashes match the reviewed patch exactly. Worktree was clean.

Inspected RED evidence shows actual resolver-sentinel and constructor-diagnostic failures; fixed targeted/package race, unit and pinned-lint evidence passes. Tests restore global logger output and level, remain nonparallel, preserve warning presence and cover zero HTTP, retry and cached identity. The issue record accurately distinguishes the resolver leak from constructor diagnostics that already redact the key.

Reviewer B inspected evidence rather than independently executing tests; Reviewer A separately reproduced RED/GREEN. CI and merge authority remain separate gates. This code-review approval does not lift the platform merge/deployment hold.

@cristim

cristim commented Oct 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@cristim

cristim commented Oct 18, 2026

Copy link
Copy Markdown
Member Author

resume-cc-audit

CodeRabbit's full review now covers exact head 0985bea53a6945a6ced1ddaaac3eaf0e01c9ae73 (summary comment 6248861392, reviewed coverage metadata matches the same SHA). It reports no actionable comments; the inline-comment list is empty. The two Actions runs passed, including integration tests. Both independent exact-head approvals and local verification remain recorded above.

The optional docstring-coverage warning is dismissed: this change replaces two warning messages in the existing provider method and adds focused regression tests, without introducing a public API. Explanatory comments on those straightforward tests would duplicate their names and assertions, contrary to the repository's comment guidance. Required CI checks pass; no check or protection is bypassed.

Technical verification is complete for this head. Merge remains held because a platform merge can trigger existing automatic dev deployments, and direct user authorization for that side effect is still pending. No merge or deployment occurred.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Session: claude-cc-platform

Taking over this PR from resume-cc-audit / resume-cc-platform (Codex, out of quota); the old CI/CR watchers are no longer live. The user has since authorized merging cloud-commitments-platform once all gates pass, including the auto-deploy on push to main, which clears the deployment hold above. Before merging I will run a local build and tests at 0985bea53a6945a6ced1ddaaac3eaf0e01c9ae73, post the results here, and recheck CI, CodeRabbit and threads.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Session: claude-cc-platform

Local verification at 0985bea53a6945a6ced1ddaaac3eaf0e01c9ae73 (clean worktree, macOS arm64, go1.26.9):

  • go build ./...: OK
  • go vet -tags=integration ./internal/archera: OK
  • go test -race -count=1 ./internal/archera: ok (2.2s), includes the new provider log tests

@cristim
cristim merged commit 519d8ee into main Oct 19, 2026
25 checks passed
@cristim
cristim deleted the fix/1077-provider-safe-logs branch October 19, 2026 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/security Security finding urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(insurance): keep credential initialization errors out of provider logs

1 participant