fix(config): retry transient .env read errors and surface a persistent failure loudly - #82
Open
sosidudku1 wants to merge 2 commits into
Open
fix(config): retry transient .env read errors and surface a persistent failure loudly#82sosidudku1 wants to merge 2 commits into
sosidudku1 wants to merge 2 commits into
Conversation
On Windows the startup read of <stateDir>/.env can fail with EPERM while the file exists (issue #59). The loader used to print one stderr line and continue, which the alt-screen TUI hides entirely, so stored API keys silently vanished and setup asked for them again. Changes: - load-dotenv: retry the read up to 3 times with a short backoff (50/150 ms) on EPERM/EACCES/EBUSY/EAGAIN, the codes transient antivirus/sync locks surface as. ENOENT stays a silent no-op and other codes fail fast. - load-dotenv: report a surviving failure in the new DotenvLoadResult.error field (errno code, Node message, attempt count) and print an actionable warning naming the path and platform-specific fixes (read-only attribute, antivirus exclusion, icacls /reset on Windows; owner/chmod on POSIX). Variable names only, never values. - load-config/config-schema: carry the load outcome as config.dotenv so frontends can surface it. - tui-command: when config.dotenv.error is set, emit a warn-variant system chat message with the same guidance, because stderr is invisible under the alt screen. - windows-acl (extracted from dotenv-writer): after tightening the .env ACL with icacls, verify the file is still readable by this process and roll back with icacls /reset when it is not. An account-name grant that resolves to the wrong principal after /inheritance:r would otherwise leave a file the agent itself can no longer read. Tests: retry-then-success loads keys, persistent EPERM warns with path and guidance without throwing, non-retryable codes fail fast, missing file stays silent, warning text carries no values, ACL tighten/verify/rollback paths covered with injected deps. Closes #59 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-ups on the unreadable .env change: - Drop DotenvReadFailure.message. For errno failures Node's message is a fixed function of code, syscall, and path, so it duplicated what the warning already prints from the code and DotenvLoadResult.path; the field was carried through config but never rendered anywhere. The failure now carries the errno code and the attempt count only. - Stop echoing the raw .env line in the missing '=' diagnostic and report the 1-based line number and length instead. A line without '=' can be a fragment of a secret value left behind by an external non-atomic writer, so the diagnostic must locate it, not repeat it. - Reword the retry-code comment honestly: EACCES on POSIX is almost always a permanent denial; it is retried for the sake of the Windows antivirus and sync-lock family, and on POSIX that costs one ~200ms backoff loop before the loud warning. - Soften the windows-acl comment: a wrong-principal grant is one plausible cause of the symptom reported in #59, not a confirmed root cause; the probe read closes it off either way. - Correct the tui-command comment: the stderr line scrolls away before the alt screen takes over and is easy to miss; it is not hidden entirely. - Cover config.dotenv propagation in load-config.test.ts: loaded names on the happy path and the error case, using a directory named .env as the portable stand-in for an existing-but-unreadable file. - Pin the loader contract in AGENTS.md: retried codes, attempts and backoff, silent ENOENT, the config.dotenv outcome, the TUI warning, and the icacls probe-read rollback, with the pinning tests named. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Problem
On Windows the startup read of
<stateDir>/.envcan fail with EPERM while the file exists (#59). The loader printed a single stderr line and continued; that line scrolls away before the TUI takes over the alt screen and is easy to miss, so stored API keys silently vanished and setup asked for them again.What changed
load-dotenv: the read of an existing.envis retried up to 3 attempts with 50/150 ms backoff onEPERM/EACCES/EBUSY/EAGAIN, the codes transient antivirus and sync locks surface as on Windows (on POSIX anEACCESis usually a permanent denial and costs one ~200 ms loop before the warning).ENOENTstays a silent no-op, all other codes fail fast.load-dotenv: a failure that survives the retries lands in the newDotenvLoadResult.error(errno code plus attempt count) and prints an actionable warning naming the path and platform-specific fixes: read-only attribute, antivirus exclusion,icacls /reseton Windows; owner andchmodon POSIX. Variable names only, never values. Malformed lines are now reported by line number and length without echoing content, since a torn line from an external non-atomic writer can be a fragment of a secret value.load-config/config-schema: the load outcome travels asconfig.dotenvso frontends can surface it.tui-command: whenconfig.dotenv.erroris set, the TUI repeats the warning as a warn-variant system chat message, because the stderr line is easy to miss under the alt screen.windows-acl(extracted fromdotenv-writer): after tightening the.envACL withicacls /inheritance:r /grant:r, verify the file is still readable by this very process and roll back withicacls /resetwhen it is not. A grant that resolves to the wrong principal would leave a file nobody can read; that is one plausible cause of the symptom in Windows: EPERM when reading <stateDir>/.env, secrets silently not loaded #59, and the probe closes it off either way.Deliberate boundaries
setDotenvKey) has no retry loop; this PR covers the startup read only..envexists and stays unreadable; the common paths (no file, readable file) are unchanged.Tests
Retry-then-success loads keys; persistent EPERM warns with path and guidance without throwing; non-retryable codes fail fast; a missing file stays silent; the warning and parse diagnostics carry no values or file content;
loadConfigpropagates the outcome intoconfig.dotenv(happy path and the unreadable-file case); ACL tighten/probe/rollback paths are covered with injected deps. The loader contract is pinned in AGENTS.md.Closes #59