Skip to content

test(secrets): reduce passphrase fixture cost in the unit suite - #1427

Merged
skevetter merged 2 commits into
mainfrom
codex/fix-macos-secrets-test-timeout
Oct 13, 2026
Merged

skevetter merged 2 commits into
mainfrom
codex/fix-macos-secrets-test-timeout

Conversation

@skevetter

@skevetter skevetter commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Problem and change

The macOS race unit job exhausted Go's ten-minute pkg/secrets package timeout during production-cost scrypt fixture encryption (incident). The named test had only run for one second; earlier tests consumed the package budget.

Functional fixtures now use real age/scrypt at test-only work factor 10. A private per-manager rekey factory selects inexpensive keys in functional tests while its production default continues using the original resolver. The existing legacy test registry wrapper also lowers the cost of recipients reopened for fixture mutations. Existing recovery, wrong-credential, nonmutation, and transaction assertions remain intact.

One ordinary production compatibility test in rekey_test.go uses an unmodified manager and production key paths. It runs with the normal unit suite, including -race -short, and verifies work factor 18 through structured age headers, fresh-identity recovery, wrong-passphrase rejection, POSIX permissions, and nonmutating legacy reads.

The existing CI workflow, unit package selection, integration matrices, and timeouts are unchanged. This PR adds no jobs, special test tags, summary scripts, or reporting infrastructure.

Validation

Local macOS ARM64, Go 1.26.8:

  • Complete normal secrets suite with production compatibility, -race -short -count=1 -timeout=2m: passed in 45.585s.

  • Original complete package baseline before fixture changes: 473.839s on the same machine and dependency set.

  • Complete normal secrets suite, five race passes, -count=5 -timeout=5m: passed in 221.699s total, including production compatibility on every pass.

  • Former timeout test, twenty race passes: passed in 3.036s total.

  • CLI secrets tests: passed in 8.281s.

  • Fresh local CodeRabbit review of all eight final PR files: zero findings; independent complete source-diff review: no actionable findings.

  • CI-parity lint: zero issues. All applicable pre-commit hooks passed.

  • Existing hosted Ubuntu and macOS unit jobs passed on e06ff2288; the normal secrets race suite completed in 58.015s and 85.953s respectively, including production compatibility. Hosted lint and pre-commit also passed.

  • All 60 existing integration matrix jobs and the CI Success aggregator passed on the final head. One macOS build required a targeted infrastructure rerun after GitHub cancelled it before runner acquisition due to hosted ARM64 capacity; no test failure was retried.

  • Final-head Greptile review: 5/5 confidence, no actionable findings. Completed hosted CodeRabbit full review: no actionable comments, minimal merge/security risk.

CodeRabbit's docstring coverage warning is left as no-action: this diff introduces no public API, and comments solely restating private/test helper behavior would add noise. CI-parity lint passes.

Measurements are development-machine timings, not controlled benchmarks. Validation and reviewer gates pass on e06ff2288957065fbf264eee53dae6d292ce567c; this PR remains a draft for human review.

@netlify

netlify Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for devsydev canceled.

Name Link
🔨 Latest commit e06ff22
🔍 Latest deploy log https://app.netlify.com/projects/devsydev/deploys/6ac7c7d8634a800008d84ee8

@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8798b0af-c249-4acb-9b8e-5fcb1009b4f4
📥 Commits

Reviewing files that changed from the base of the PR and between 42ef02f and e06ff22.

📒 Files selected for processing (8)
  • pkg/secrets/backend_registry_review_test.go
  • pkg/secrets/legacy_file_recovery_test.go
  • pkg/secrets/passphrase_test_helpers_test.go
  • pkg/secrets/protection.go
  • pkg/secrets/protection_rekey.go
  • pkg/secrets/rekey_test.go
  • pkg/secrets/store_internal_test.go
  • pkg/secrets/unlock_test.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

📝 Walkthrough

The secrets tests now use low-cost scrypt keys for fixtures and check production passphrase encryption, including recovery through a legacy catalog layout. A test-only rekey target override supports fast passphrase targets.

Changes: Passphrase test coverage

Layer / File(s) Summary
Fast passphrase fixtures and test adoption
pkg/secrets/passphrase_test_helpers_test.go, pkg/secrets/protection.go, pkg/secrets/protection_rekey.go, pkg/secrets/legacy_file_recovery_test.go, pkg/secrets/backend_registry_review_test.go, pkg/secrets/rekey_test.go, pkg/secrets/store_internal_test.go, pkg/secrets/unlock_test.go
Test helpers create age/scrypt keys with work factor 10 and verify the stored header and value round-trip. A test-only override can select a rekey target. Existing passphrase tests use the shared key helper.
Production encryption compatibility coverage
pkg/secrets/rekey_test.go
The compatibility test checks production passphrase metadata, scrypt work factor 18, secret recovery, incorrect-passphrase behavior, file permissions, and reads through a legacy catalog layout.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to e06ff

The legacy compatibility test exercises the intended read path, and no actionable merge-blocking risk remains after normal checks.

Security architecture risk: ⚪ Minimal · up to e06ff

The cheaper encryption settings are confined to test fixtures. Production managers retain the original key resolver, and the change does not bypass credential checks or alter rekey recovery. No introduced or worsened security concern was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The reduced brute-force resistance applies to isolated test ciphertext, not production secrets. The low-cost factory is defined and assigned in test-only code, while production construction retains the original resolver.

Trust Boundaries and Controls

  • inferred — Caller-supplied passphrases do not select the test factory or gain a new route around current-credential validation. Factory authority remains private to the manager implementation, and production callers receive the unchanged default.
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 17 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: reducing the cost of passphrase fixtures in the secrets unit-test suite.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@netlify

netlify Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for images-devsy-sh canceled.

Name Link
🔨 Latest commit e06ff22
🔍 Latest deploy log https://app.netlify.com/projects/images-devsy-sh/deploys/6ac7c7d8634a800008d84eea

@github-actions github-actions Bot added the size/l label Oct 8, 2026
@skevetter skevetter changed the title test(secrets): fix macOS race test timeouts with cheaper fixtures test(secrets): reduce passphrase fixture cost in the unit suite Oct 8, 2026
@skevetter

Copy link
Copy Markdown
Contributor Author

@greptileai review

@greptile-apps

greptile-apps Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Test helpers refactored to speed up passphrase encryption tests.

The PR appears safe to merge, with production key selection unchanged.

What we checked:

  • Production encryption stays unchanged: Ordinary managers leave resolveNewFileKey unset. newTargetFileKey then calls the original resolveFileKey; the only hook assignment is in a test helper.

Summary

This PR lowers scrypt cost for functional test fixtures while keeping production key selection unchanged.

  • Adds a private per-manager hook for test rekey targets.
  • Keeps production credential checks when reopening legacy fixtures.
  • Adds a production compatibility test covering work factor 18, fresh-key reads, wrong-passphrase rejection, file permissions, and unchanged legacy files.

No actionable issues were found. Tests were not run during this review.

Reviews (1) · Last reviewed commit: "test(secrets): run production compatibil..." · Reviewed by Greptile

@skevetter

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@skevetter
skevetter marked this pull request as ready for review October 8, 2026 21:41
@mergify

mergify Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

@skevetter
skevetter marked this pull request as draft October 12, 2026 05:19
@skevetter
skevetter marked this pull request as ready for review October 13, 2026 08:53
@skevetter
skevetter merged commit e069799 into main Oct 13, 2026
106 of 108 checks passed
@skevetter
skevetter deleted the codex/fix-macos-secrets-test-timeout branch October 13, 2026 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant