feat(credentials): pure-Node CredentialStore for headless (#32, PR A) - #33
Merged
Conversation
This was referenced Jun 17, 2026
Astro-Han
added a commit
that referenced
this pull request
Jun 17, 2026
…parse, behavioral tests (#33) - write path (store + importer): unguessable temp name (randomUUID, not pid.timestamp) + exclusive create ('wx'/O_EXCL) so the plaintext write can't follow a pre-planted symlink at a predictable path before the mode is locked; remove the temp on any failure before rename. Store enforces 0600 hard on POSIX (no longer swallows the chmod error on its own temp). - fail-closed parse: a v1 file with a missing/non-object `values`, or any non-string value, is now rejected instead of silently read as an empty store. The importer likewise refuses a malformed legacy file rather than tombstoning it empty (no data loss). - testability: migrateLegacyCredentials takes safeStorage as an injected dependency, so the module no longer imports `electron` at load time; main.ts passes the real safeStorage. Adds credential-store-migration.test.ts — behavioral tests over real temp files (decrypt-all, unavailable leaves legacy intact, idempotent v1, malformed refused, missing-file no-op). storage 93 tests + desktop 1468 tests green. The cross-process refresh lock stays deliberately deferred to PR B.
Astro-Han
added a commit
that referenced
this pull request
Jun 17, 2026
… write, drop sealer (review #33) Addresses the four review findings: - P2 (concurrent writes): the in-instance Promise queue couldn't stop two createFileCredentialStore(dir) instances/processes from a read-modify-write lost update (no single-instance lock exists, so two desktop processes are reachable). Adds an O_EXCL lockfile (stale-steal + timeout, pure Node, no new dep) around the whole read-modify-write. Regression test: two independent instances writing different slugs concurrently both survive. - P1 (scope honesty): the migration decrypts EVERY secret — bot tokens, proxy/gateway tokens, Tavily key, not just API keys — to plaintext-0600, because the desktop abandons safeStorage entirely (anything left encrypted becomes unreadable). Made that explicit in the doc comment; behavioral test now covers apiKey + botToken + proxyPassword and asserts 0600 at rest. - P3 (dedup): extracted the owner-only atomic write (mkdir 0700, temp 'wx', 0600, secureChmod, rename, cleanup) into one exported @maka/storage helper writeSecretFileAtomic, shared by the live store and the migration — the migration no longer best-effort-chmods; both paths are identical now. - P3 (YAGNI): removed the SecretSealer seam (its only impl was identity, and a sync seal(string):string is likely the wrong shape for a future async keychain). Store/migration write plaintext directly; re-design the seam against a real backend when one lands. storage 93 tests + desktop 1468 tests green. (Per review: both suites re-run.)
Astro-Han
added a commit
that referenced
this pull request
Jun 17, 2026
…view #33 round 2) Two concurrency holes in the lock added last round: - The migration wrote credentials.json OUTSIDE the live store's cross-process lock, so a slow/suspended migration could overwrite a newer v1 write with its stale snapshot. migrateLegacyCredentials now runs under the SAME withCredentialFileLock (now exported from @maka/storage) and re-reads the file inside the lock — if a racing process already migrated, it no-ops instead of clobbering. - The lockfile held only a pid and was deleted unconditionally on release, so a holder wrongly judged stale (and stolen from) would delete the new owner's lock on its way out, reopening the lost-update window. The lockfile now carries a random per-holder token; release deletes it only when the token still matches. Tests: storage 94 (+ release must not delete a thief's stolen lock), desktop 1469 (+ migration blocks on the shared lock instead of racing a writer).
Astro-Han
added a commit
that referenced
this pull request
Jun 17, 2026
…ld (review #33) LOCK_TIMEOUT_MS (5s) was below LOCK_STALE_MS (10s), so after a crash left a lockfile behind, the first contending write timed out at 5s — before the leftover lock was old enough (10s) to steal — instead of recovering. The "crashed holder is recovered by stealing a stale lock" comment was therefore only true for some later write, not the next one. Raise LOCK_TIMEOUT_MS to 15s so a single write outlasts the stale threshold and steals the dead lock within its own wait. The two timing constants are exported to pin the timeout > stale invariant in a test. Tests: storage 96 (+ invariant: timeout > stale; + a write steals a crashed holder's back-dated stale lock and succeeds).
Astro-Han
added a commit
that referenced
this pull request
Jun 17, 2026
Stale-steal recovered crashed holders by mtime alone, but the lockfile mtime is never renewed during a critical section — so a live holder that ran past LOCK_STALE_MS (a slow/large write, or a paused process) was judged stale and stolen, letting a second writer into the read-modify-write and losing an update. (Codex review reproduced overlapped=true.) Gate the steal on owner liveness: a lock is reclaimed at the stale threshold only when its owner pid is dead (process.kill(pid, 0)); a live owner is left alone until a 60s hard fallback — which breaks a deadlock from pid reuse or a cross-machine holder, and sits above the acquire timeout so a single contender never expires a live holder. credentials.json is local (kept out of sync paths per SECURITY), so the holder pid is meaningful. Tests: storage 97 (+ a stale lock is stealable only when its owner is dead; the crash-recovery test now uses a genuinely dead pid).
Astro-Han
added a commit
that referenced
this pull request
Jun 17, 2026
…e fallback (review #33) Per review: a live owner must never be expired. The previous 60s LOCK_HARD_STALE_MS could still steal a live holder that ran past it (e.g. a suspended process), reopening a silent lost-update window. Remove it — a lock is now stealable only when its owner pid is dead AND the lock is stale; a live holder is never stolen, and a contender that can't get it fails loudly ("locked by another live process; please retry") rather than overwriting. The rare pid-reuse case (a crashed holder's pid reused by an unrelated live process) now fails loud and temporary — self-healing once that process exits — which beats a silent overwrite for credential data. Tests: storage 97 (the predicate test now also pins that even a 1h-old live lock is not stealable — no hard fallback).
…+ legacy migration (#32) A pure-Node credential store the headless runtime can use without Electron: versioned credentials.json, owner-only 0600 atomic writes inside a 0700 dir, and a cross-process write lock — an atomic mkdir of credentials.json.lock that is never stolen (a held or leftover lock is waited on, then fails loud naming the path), so two processes sharing one file can't lose a read-modify-write update. The API is the generic getSecret/setSecret/deleteSecret over a CredentialKind; the lock and the atomic writer stay module-internal and the package exports only the typed store + the migration. migrateLegacyCredentialFile imports a legacy (safeStorage-encrypted, unversioned) credentials.json to v1 plaintext under the same lock, with the crypto injected so a headless caller never runs it. It fails closed on an unknown version, a malformed values map, a non-string value, or an unavailable decryptor (an empty file still stamps to v1). chmod failures fail closed on POSIX. At-rest posture is plaintext behind 0600 (file-first, #32 / SECURITY.md). Part of #32 (PR A of two). Unblocks the headless lab #31.
…e shared store (#32) The desktop abandons safeStorage for credentials.json entirely — the live store is now the pure-Node FileCredentialStore. credential-store.ts shrinks to a one-time importer that injects a safeStorage-backed decryptor (base64-decode + decryptString) into migrateLegacyCredentialFile, tombstoning the legacy file in place and aborting intact if safeStorage is unavailable. safeStorage is injected so the module no longer imports electron and is unit-testable. The importer decrypts EVERY secret in the file (API keys, bot tokens, app secrets, proxy password, gateway token, Tavily key) — any value left encrypted would become permanently unreadable once the live store is pure-Node. main.ts runs the migration in whenReady before any credential use, non-fatally; onboarding reads through getSecret; dead setOAuthToken/getOAuthToken removed. Part of #32 (PR A of two).
Astro-Han
force-pushed
the
claude/credential-store-pure-node
branch
from
June 17, 2026 12:03
95e71f7 to
2d1eba7
Compare
likun666661
pushed a commit
to likun666661/maka-agent
that referenced
this pull request
Jun 17, 2026
apache#31) First end-to-end slice of the agent experiment lab: run one Config against one Task in an isolated sandbox, capture the trajectory, score it, and return a ResultRecord. De-risks the integration (headless SessionManager run + sandbox + evaluator composing into one real run); matrix/compare/CLI and real-model backends are later additions. - contracts.ts: Task (instruction + fixture workspace + verification command), Config (backend/connection/model), ResultRecord. Minimal; systemPrompt/Execution/SWE-bench-pack ingestion deferred. - sandbox.ts: prepareWorkspace copies the fixture to a throwaway dir so the agent never mutates the source (isolation, not asking). - evaluator.ts: runVerification runs the Task's test command in the throwaway workspace after the agent finishes (config can't grade itself), with output cap + timeout kill. - runner.ts: runExperiment wires a pure-Node SessionManager (injected registerBackends keeps model/credential wiring out of the lab core), drives one turn, captures the InvocationResult trajectory via runtimeInvocationObserver, scores it, returns a ResultRecord. - tests: FakeBackend e2e (pass + fail fixtures, trajectory persisted as runtime-events.jsonl), sandbox isolation, evaluator pass/fail/timeout. Independent of the credential migration (apache#32/apache#33): the skeleton runs on FakeBackend and needs no credentials.
jackwener
pushed a commit
that referenced
this pull request
Jun 21, 2026
feat(credentials): pure-Node CredentialStore for headless (#32, PR A)
jackwener
pushed a commit
that referenced
this pull request
Jun 21, 2026
#31) First end-to-end slice of the agent experiment lab: run one Config against one Task in an isolated sandbox, capture the trajectory, score it, and return a ResultRecord. De-risks the integration (headless SessionManager run + sandbox + evaluator composing into one real run); matrix/compare/CLI and real-model backends are later additions. - contracts.ts: Task (instruction + fixture workspace + verification command), Config (backend/connection/model), ResultRecord. Minimal; systemPrompt/Execution/SWE-bench-pack ingestion deferred. - sandbox.ts: prepareWorkspace copies the fixture to a throwaway dir so the agent never mutates the source (isolation, not asking). - evaluator.ts: runVerification runs the Task's test command in the throwaway workspace after the agent finishes (config can't grade itself), with output cap + timeout kill. - runner.ts: runExperiment wires a pure-Node SessionManager (injected registerBackends keeps model/credential wiring out of the lab core), drives one turn, captures the InvocationResult trajectory via runtimeInvocationObserver, scores it, returns a ResultRecord. - tests: FakeBackend e2e (pass + fail fixtures, trajectory persisted as runtime-events.jsonl), sandbox isolation, evaluator pass/fail/timeout. Independent of the credential migration (#32/#33): the skeleton runs on FakeBackend and needs no credentials.
jackwener
pushed a commit
that referenced
this pull request
Jun 21, 2026
feat(credentials): pure-Node CredentialStore for headless (#32, PR A)
jackwener
pushed a commit
that referenced
this pull request
Jun 21, 2026
feat(credentials): pure-Node CredentialStore for headless (#32, PR A)
jackwener
pushed a commit
that referenced
this pull request
Jun 21, 2026
#31) First end-to-end slice of the agent experiment lab: run one Config against one Task in an isolated sandbox, capture the trajectory, score it, and return a ResultRecord. De-risks the integration (headless SessionManager run + sandbox + evaluator composing into one real run); matrix/compare/CLI and real-model backends are later additions. - contracts.ts: Task (instruction + fixture workspace + verification command), Config (backend/connection/model), ResultRecord. Minimal; systemPrompt/Execution/SWE-bench-pack ingestion deferred. - sandbox.ts: prepareWorkspace copies the fixture to a throwaway dir so the agent never mutates the source (isolation, not asking). - evaluator.ts: runVerification runs the Task's test command in the throwaway workspace after the agent finishes (config can't grade itself), with output cap + timeout kill. - runner.ts: runExperiment wires a pure-Node SessionManager (injected registerBackends keeps model/credential wiring out of the lab core), drives one turn, captures the InvocationResult trajectory via runtimeInvocationObserver, scores it, returns a ResultRecord. - tests: FakeBackend e2e (pass + fail fixtures, trajectory persisted as runtime-events.jsonl), sandbox isolation, evaluator pass/fail/timeout. Independent of the credential migration (#32/#33): the skeleton runs on FakeBackend and needs no credentials.
jackwener
pushed a commit
that referenced
this pull request
Jun 21, 2026
feat(credentials): pure-Node CredentialStore for headless (#32, PR A)
jackwener
pushed a commit
that referenced
this pull request
Jun 21, 2026
#31) First end-to-end slice of the agent experiment lab: run one Config against one Task in an isolated sandbox, capture the trajectory, score it, and return a ResultRecord. De-risks the integration (headless SessionManager run + sandbox + evaluator composing into one real run); matrix/compare/CLI and real-model backends are later additions. - contracts.ts: Task (instruction + fixture workspace + verification command), Config (backend/connection/model), ResultRecord. Minimal; systemPrompt/Execution/SWE-bench-pack ingestion deferred. - sandbox.ts: prepareWorkspace copies the fixture to a throwaway dir so the agent never mutates the source (isolation, not asking). - evaluator.ts: runVerification runs the Task's test command in the throwaway workspace after the agent finishes (config can't grade itself), with output cap + timeout kill. - runner.ts: runExperiment wires a pure-Node SessionManager (injected registerBackends keeps model/credential wiring out of the lab core), drives one turn, captures the InvocationResult trajectory via runtimeInvocationObserver, scores it, returns a ResultRecord. - tests: FakeBackend e2e (pass + fail fixtures, trajectory persisted as runtime-events.jsonl), sandbox isolation, evaluator pass/fail/timeout. Independent of the credential migration (#32/#33): the skeleton runs on FakeBackend and needs no credentials.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes part of #32 (PR A of two). Unblocks the headless lab #31.
What & why
Credentials in
credentials.jsonwere stored via ElectronsafeStorage— an Electron-only API, so a pure-Node headless runtime (CLI / eval harness / third party) cannot decrypt them. This moves that store offsafeStorageto a shared pure-NodeFileCredentialStore, so desktop and headless read the samecredentials.json.Scope — the whole credentials.json, not just API keys
The desktop abandons
safeStorageforcredentials.jsonentirely (the live store is now the pure-NodeFileCredentialStore). So the one-time importer decrypts every secret in that file — API keys and bot tokens, bot app secrets, the proxy password, the gateway token, and the Tavily key — to plaintext-0600. This is required, not incidental: any value leftsafeStorage-encrypted would become permanently unreadable. The accepted at-rest posture for all of them is plaintext behind0600(file-first, #32 /SECURITY.md), the same as Codex CLI / Claude Code / aider.The 4 separate per-provider OAuth subscription token files (claude / codex / antigravity / cursor) are NOT in
credentials.jsonand remain a higher-risk follow-up (PR B).Changes
@maka/storage— newFileCredentialStore(mirrorsFileConnectionStore):getSecret(slug, kind)/setSecret(slug, kind, value)/deleteSecret(slug, kind?)(no kind ⇒ clear every kind for the slug).CredentialKindcovers all stored kinds (api_key, oauth_token, bot_token, app_secret, proxy_password, gateway_token, tavily_api_key) so migrated data round-trips; no per-kind alias methods.valuesalso fails closed. The fail-closed error namescredentials.json.lockso a migration stuck on a stale lock has an obvious recovery path.mkdirofcredentials.json.lock(acquire) +rm(release), never stolen — a held or leftover lock is waited on, then fails loud naming the path. Pure-Node stdlib, no dependency. The trade: a hard crash (SIGKILL/ power loss) mid-write leaves the lock dir for one-command manual removal — explicit, never a silent lost update.ensureSecretDir(mkdir + re-chmod0700) and atomic writerwriteSecretFileAtomic(tempwx/O_EXCL,0600, rename, cleanup on failure), used by the live store, the lock, and the migration so the hardening can't drift between paths.migrateLegacyCredentialFile(path, decryptor)— the one-time migration lives here, runs under the same lock, re-reads inside it, decrypts every value, and tombstones the file in place. The crypto is injected: desktop supplies asafeStorage-backed decryptor; a headless caller has none and never runs it. The package exports only the typed store + the migration; the lock and writer stay module-internal.credential-store.tsshrinks to glue that injectssafeStorage(base64-decode +decryptString) intomigrateLegacyCredentialFile, aborting and leaving the legacy file intact ifsafeStorageis unavailable. The module no longer importselectron(safeStorage is injected), so it's unit-testable.main.tsruns the migration inwhenReadybefore any credential use, non-fatally.setOAuthToken/getOAuthToken.Security posture (deliberate change)
At-rest for all
credentials.jsonsecrets becomes plaintext behind0600(file-first, matching Codex CLI / OpenCode / npm), agreed in #32. The OS user account is the boundary (SECURITY.md);safeStorageonly protected secondary threats and is Electron-bound. Windows has no real0600(chmodis a no-op there) — ACL/DPAPI hardening is tracked in #32, deliberately not faked here. Keychain-based at-rest encryption is deferred until there's a real backend so its (likely async) shape is designed against that backend rather than guessed — the earlier identity-onlySecretSealerseam was removed as YAGNI.Out of scope (→ PR B)
safeStorage, and their cross-process refresh race (the Claude Code #48786 logout class).credentials.jsonwrites are cross-process locked here; those token files are separate and still need their own handling.Testing
@maka/storage: builds clean; 102 tests pass incl. fail-closed on unknown version + malformedvalues,0600file + re-chmod of a pre-existing loose-mode file and workspace dir, no temp left behind, the legacy stored-key suffix contract for every kind, generic round-trip +deleteSecret(slug)clearing all kinds, a two-instance concurrent-write regression (both survive via the lock), a held lock is waited on, never stolen, a never-released lock fails loud with the.lockpath + recovery hint, and the migration suite (decrypts all kinds, aborts when unavailable, an empty file migrates without the decryptor, idempotent v1, refuses malformed, no-op missing, two racing migrations serialize).tsctypecheck clean; 1464 tests pass, 0 fail incl. the migration glue (base64-decode +decryptString, availability propagated) and the contract that the live store is the pure-Node backend (not asafeStoragestore).Review history
The cross-process lock went through several review rounds before settling on the design above. Earlier iterations used a token-owned lockfile with
process.kill(pid, 0)liveness-gated stale-steal, thenproper-lockfile— both turned out to share the same non-atomic check-then-steal TOCTOU: two processes recovering one crash-left lock could both judge it stale, both remove it, and both re-acquire, entering the read-modify-write together and losing an update (proper-lockfile@4.1.2only detects this asynchronously, ~seconds after a ~ms write). The conclusion — there is no safe userspace compare-and-steal — is why the final lock never steals and the dependency was dropped, returning@maka/storageto pure-Node stdlib. Later rounds removed the speculative per-kind helper methods (zero callers) for the uniform generic API, hardened the lock dir like the writer, added the fail-loud-timeout test, and pointed the user-visible schema error at a stale lock.