Repository navigation
Keychain fixes, docs, smoke tests, and keychain delete - #1231
theoephraim wants to merge 7 commits into
Conversation
Ports the reusable parts of #847: gated macOS Keychain smoke tests for `set` and `import`, a daemon reset helper, a signing preflight, and a `keychain delete` command. Tests run with `pnpm run test:keychain` in smoke-tests and require a helper signed with a stable Team ID. Co-authored-by: Benjamin Jesuiter <bjesuiter@gmail.com>
Closes #842 Co-authored-by: Benjamin Jesuiter <bjesuiter@gmail.com>
|
The changes in this PR will be included in the next version bump.
|
📦 Bundle size
dist/ only; native binaries are versioned separately and not counted here. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
varlock-website | 1ac0019 | Commit Preview URL Branch Preview URL |
Oct 15 2026, 06:19 AM |
varlock
@varlock/native-helper-darwin
@varlock/native-helper-linux-arm64
@varlock/native-helper-linux-x64
@varlock/native-helper-win32-x64
commit: |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The opt-in smoke suites share a daemon but run in parallel, so their reset hooks can interrupt each other's requests.
Reviewed changes: Reviewed the keychain deletion flow, smoke-test setup and cleanup, documentation, and Deno peer allowlists.
- Keychain deletion: Adds the CLI command, typed IPC client method, Swift handler, and missing-item error reporting.
- Keychain smoke coverage: Adds opt-in set/delete and import/readback suites with signature preflight and fixture cleanup.
- Deno daemon access: Allows Deno executable names alongside Node and Bun on macOS, Linux, and Windows.
- User guidance: Documents deletion and persistent access permission for externally created items.
Build and targeted unit tests passed (17 passed, 6 skipped). Real macOS Keychain behavior could not be exercised on this Linux runner.
gpt-6.1-sol ｜ 𝕏
| "test": "vitest run", | ||
| "test:watch": "vitest" | ||
| "test:watch": "vitest", | ||
| "test:keychain": "VARLOCK_RUN_KEYCHAIN_SMOKE=1 vitest run tests/keychain-set.test.ts tests/keychain-import.test.ts" |
There was a problem hiding this comment.
[P2] Both files run in parallel by default, but their beforeEach/afterEach hooks call resetVarlockDaemon() against the same user daemon. One worker can therefore kill the helper and unlink its socket while the other is importing, reading back a secret, or running signature preflight, making this smoke command fail intermittently even when the implementation works.
Technical details
# Shared daemon reset races between smoke suites
## Affected sites
- `smoke-tests/package.json:10`: launches both keychain test files without disabling file parallelism.
- `smoke-tests/tests/keychain-set.test.ts:32-40`: resets the shared daemon before and after every test.
- `smoke-tests/tests/keychain-import.test.ts:47-57`: resets the same daemon before and after its test.
- `smoke-tests/helpers/keychain-daemon.ts:18-49`: sends SIGTERM and unconditionally removes the shared PID, info, socket and lock paths.
## Required outcome
- Keychain suites must not reset a daemon that another suite is using, both for `test:keychain` and when opted in via `VARLOCK_RUN_KEYCHAIN_SMOKE=1` with the ordinary test command.
## Suggested approach
- Serialize the opted-in keychain run in the Vitest configuration, or isolate each suite's daemon state and propagate that configuration to every child command and reset hook. Merely using separate keychain service names does not isolate the daemon.
- Vitest documents `fileParallelism` as true by default: https://vitest.dev/config/fileparallelism
- Confirm both suites complete together on macOS without connection interruptions.Report missing, access-denied and ambiguous items separately, each with the exact command that fixes it. Also stop the helper from reporting an access-denied generic item as "not found", and restart the daemon once per smoke-test file instead of per test to cut Touch ID prompts.
There was a problem hiding this comment.
Important
The new access-denied guidance can suggest a command that fails validation or targets a different Keychain service.
Reviewed changes: Reviewed the updates since the prior review, with emphasis on error propagation and recovery-command correctness.
- Improved read errors: Distinguished missing, inaccessible, and ambiguous items and added ref-specific recovery guidance.
- Preserved Swift failures: Limited internet-password fallback to missing generic items and classified canceled access prompts as access denied.
- Reduced daemon resets: Moved smoke-suite resets to file-level hooks and asserted missing-item guidance through
load. - Simplified deletion wording: Removed generic-password terminology from the command description and docs.
The varlock build and targeted unit tests passed (17 passed, 6 skipped). Real macOS Keychain behavior could not be exercised on this Linux runner.
gpt-6.1-sol ｜ 𝕏
| throw new ResolutionError(`Varlock is not allowed to read this Keychain item: ${(err as Error).message}`, { | ||
| tip: [ | ||
| 'Grant access with:', | ||
| ` varlock keychain fix-access ${refFlags}`, |
There was a problem hiding this comment.
[P2] Only suggest fix-access when the command preserves the lookup target. For a valid service-only ref such as keychain("com.company.db"), this prints varlock keychain fix-access --service com.company.db, which immediately fails with Missing --account for keychain fix-access; for an account-only ref, the printed command silently defaults the service to varlock rather than fixing the item found by the original cross-service lookup.
Technical details
# Recovery commands for incomplete refs
## Affected sites
- `packages/varlock/src/lib/local-encrypt/keychain-resolver.ts:178-184`: emits a fix-access command for every password access denial, including service-only and account-only refs.
- `packages/varlock/src/cli/commands/keychain.command.ts:460-471`: requires an account and forwards the CLI service.
- `packages/varlock/src/cli/commands/keychain.command-spec.ts:27-34`: defaults the service to varlock.
## Required outcome
- Suggested commands must be accepted by the CLI and identify the same item as the failing ref.
- When the ref lacks identifiers needed by fix-access, direct the user to discover and supply those identifiers instead of presenting an incorrect executable command.
## Suggested approach
- Gate the exact fix-access command on both service and account, as the missing-item set suggestion already does; provide discovery or disambiguation guidance otherwise.
- Cover accessDenied for service-only, account-only, and fully specified refs in resolver tests.keychain("svc", account="x") ignored the positional service and could
return another service's item; keychain("svc", keychain="y") ignored
the keychain arg. Positional service now combines with named args, and
non-static or duplicate args are schema errors.
Docs: fix the invalid field="password" example (field only returns
metadata), correct the list filter description, replace the fallback()
advice for non-Mac teammates, update troubleshooting for the new error
messages, and add a short section on sharing committed refs.
theoephraim
changed the title
keychain delete, and Deno daemon accesskeychain delete
There was a problem hiding this comment.
ℹ️ No new issues found in this delta. Previously reported concerns remain open, so this is not an approval.
Reviewed changes: Reviewed the resolver argument fix and documentation updates since 98252e3f.
- Fixed mixed arguments: Preserved the positional service when combined with named account, keychain, or metadata field arguments.
- Validated lookup arguments: Rejected duplicate service specifications, extra positional arguments, and non-static or non-string named lookup arguments.
- Corrected user guidance: Clarified metadata fields, non-macOS limitations, list filtering, profile defaults, access management, and sharing committed refs across developers.
The varlock build and targeted unit tests passed (17 passed, 6 skipped). Direct resolver argument checks passed with a simulated macOS platform; real macOS Keychain behavior could not be exercised on this Linux runner.
gpt-6.1-sol ｜ 𝕏
allowMissing=true resolves to empty when the item is missing or the machine is not a Mac, so fallback() can move to the next source. Other failures (access denied, cancelled prompt, daemon errors) still error. Checking the platform at resolve time means a keychain() in an if() branch that does not run no longer breaks the schema on Linux/Windows. Docs cover the three patterns for non-Mac teammates.
There was a problem hiding this comment.
ℹ️ No new issues found in this delta. Previously reported concerns remain open, so this is not an approval.
Reviewed changes: Reviewed the optional Keychain lookup behavior and platform-check changes since 3d766a16.
- Added optional lookups: Introduced
allowMissing=truefor missing items and non-macOS hosts while preserving other failures. - Deferred platform validation: Moved the macOS check to resolution so untaken
if()branches can contain Keychain refs. - Added regression coverage: Tested non-macOS fallback, conditional branches, resolution errors, and duplicate service arguments.
- Documented alternative sources: Explained optional fallback, OS branching, and developer-local overrides.
The varlock build and targeted tests passed (21 passed, 6 skipped). Simulated macOS checks verified missing-item handling and preservation of other errors; real macOS Keychain behavior could not be exercised on this Linux runner.
gpt-6.1-sol ｜ 𝕏
Running on another OS is no longer treated as missing; use an if() on $VARLOCK_OS or a per-developer .env.local for that. Keeps the meaning in line with the plugins and @import, and keeps the actionable "run keychain set" error for Mac users who have not stored the item.
There was a problem hiding this comment.
ℹ️ No new issues found in this delta. Previously reported concerns remain open, so this is not an approval.
Reviewed changes: Reviewed the allowMissing contract correction since 6014727a.
- Narrowed optional lookups: Kept
allowMissing=truelimited to missing items; unsupported platforms now fail before evaluating that argument. - Updated regression coverage: Asserted that non-macOS fallback still errors with
allowMissing=trueand that platform guidance recommends OS branching. - Aligned documentation: Separated cross-platform alternatives from optional-item fallback and corrected the reference and release description.
The varlock build and targeted tests passed (21 passed, 6 skipped). Real macOS Keychain behavior could not be exercised on this Linux runner.
gpt-6.1-sol ｜ 𝕏
Lands the reusable parts of #847 by @bjesuiter (credited as co-author on both commits).
keychain set,keychain import, and the newkeychain delete. They're opt-in (pnpm run test:keychaininsmoke-tests, macOS only) because they touch the real login keychain and need a helper signed with a stable Team ID. A preflight asks varlock which helper it resolves and fails early if that binary is ad-hoc signed.varlock keychain delete: removes exactly one Keychain item (service + account required), with a clear error when nothing matches. Docs updated, plus guidance on choosing "Always Allow" for items Varlock didn't create.keychain()errors: a failed read now says whether the item is missing, not accessible, or ambiguous, and prints the exactvarlock keychain set ...orvarlock keychain fix-access ...command for that ref. The helper also no longer reports an access-denied generic item as "not found" (it used to swallow the error and retry as an internet password).keychain()positional service with named args:keychain("svc", account="x")ignored"svc"and could return a different service's item with the same account;keychain("svc", keychain="y")ignored the keychain. They now combine, and duplicate or non-static args are schema errors.keychain()on other platforms and optional items: the macOS check moved from schema processing to resolve time, soif(eq($VARLOCK_OS, "darwin"), keychain(...), ...)no longer breaks the schema on Linux/Windows. NewallowMissing=trueresolves to empty when the item does not exist, sofallback(keychain(..., allowMissing=true), ...)works (same meaning as the plugins'allowMissing; other OSes and real failures still error).field="password"example (fieldonly returns metadata), thelistfilter description, the non-Macfallback()advice, and the troubleshooting section; added a short section on sharing committed refs with a team.denoas a daemon client in the Swift and Rust helpers, matchingnode/bun. Closes Allow Deno as a VarlockEnclave IPC client #842.Left out from #847: the
fix-accessrewrite (the existing ACL-based flow works when tested against an item that trusts no apps),cloneToOwned, and the Deno completion and smoke-runner changes.