Fixes#14: DetermineStateDir ignored the error from os.UserHomeDir() in the fallback path. If both os.UserConfigDir() and os.UserHomeDir() fail, the function returned /.config/berlin.sneak.pkg.secret — a dangerous root-relative path.
Changes
internal/secret/helpers.go: Changed DetermineStateDir signature from string to (string, error). Now returns a wrapped error when both UserConfigDir and UserHomeDir fail.
internal/secret/helpers_test.go: New tests covering the error path, env var path, and custom config dir path.
internal/cli/cli.go: Updated callers to handle the new error return.
internal/cli/cli_test.go: Updated to match new signature.
Test output: test-only (before fix)
# git.eeqj.de/sneak/secret/internal/secret [git.eeqj.de/sneak/secret/internal/secret.test]
internal/secret/helpers_test.go:16:17: assignment mismatch: 2 variables but DetermineStateDir returns 1 value
internal/secret/helpers_test.go:31:17: assignment mismatch: 2 variables but DetermineStateDir returns 1 value
internal/secret/helpers_test.go:42:17: assignment mismatch: 2 variables but DetermineStateDir returns 1 value
FAIL git.eeqj.de/sneak/secret/internal/secret [build failed]
Test output: test + fix
=== RUN TestDetermineStateDir_ErrorsWhenHomeDirUnavailable
--- PASS: TestDetermineStateDir_ErrorsWhenHomeDirUnavailable (0.00s)
=== RUN TestDetermineStateDir_UsesEnvVar
--- PASS: TestDetermineStateDir_UsesEnvVar (0.00s)
=== RUN TestDetermineStateDir_UsesCustomConfigDir
--- PASS: TestDetermineStateDir_UsesCustomConfigDir (0.00s)
PASS
ok git.eeqj.de/sneak/secret/internal/secret 0.178s
Linting
Note: internal/cli tests have a pre-existing failure (VCS stamping issue unrelated to this change). All internal/secret tests pass.
## Summary
Fixes #14: `DetermineStateDir` ignored the error from `os.UserHomeDir()` in the fallback path. If both `os.UserConfigDir()` and `os.UserHomeDir()` fail, the function returned `/.config/berlin.sneak.pkg.secret` — a dangerous root-relative path.
## Changes
- **`internal/secret/helpers.go`**: Changed `DetermineStateDir` signature from `string` to `(string, error)`. Now returns a wrapped error when both `UserConfigDir` and `UserHomeDir` fail.
- **`internal/secret/helpers_test.go`**: New tests covering the error path, env var path, and custom config dir path.
- **`internal/cli/cli.go`**: Updated callers to handle the new error return.
- **`internal/cli/cli_test.go`**: Updated to match new signature.
## Test output: test-only (before fix)
```
# git.eeqj.de/sneak/secret/internal/secret [git.eeqj.de/sneak/secret/internal/secret.test]
internal/secret/helpers_test.go:16:17: assignment mismatch: 2 variables but DetermineStateDir returns 1 value
internal/secret/helpers_test.go:31:17: assignment mismatch: 2 variables but DetermineStateDir returns 1 value
internal/secret/helpers_test.go:42:17: assignment mismatch: 2 variables but DetermineStateDir returns 1 value
FAIL git.eeqj.de/sneak/secret/internal/secret [build failed]
```
## Test output: test + fix
```
=== RUN TestDetermineStateDir_ErrorsWhenHomeDirUnavailable
--- PASS: TestDetermineStateDir_ErrorsWhenHomeDirUnavailable (0.00s)
=== RUN TestDetermineStateDir_UsesEnvVar
--- PASS: TestDetermineStateDir_UsesEnvVar (0.00s)
=== RUN TestDetermineStateDir_UsesCustomConfigDir
--- PASS: TestDetermineStateDir_UsesCustomConfigDir (0.00s)
PASS
ok git.eeqj.de/sneak/secret/internal/secret 0.178s
```
## Linting
Note: `internal/cli` tests have a pre-existing failure (VCS stamping issue unrelated to this change). All `internal/secret` tests pass.
When os.UserConfigDir() fails, DetermineStateDir falls back to
os.UserHomeDir(). Previously the error from UserHomeDir was discarded,
which could result in a dangerous root-relative path (/.config/...) if
both calls fail.
Now DetermineStateDir returns (string, error) and propagates failures
from both UserConfigDir and UserHomeDir.
Closes#14
(Cannot formally approve own PR, posting as comment.)
Checklist
Signature change string → (string, error) handled at all call sites?✅ Both NewCLIInstance() and NewCLIInstanceWithFs() in cli.go updated. cli_test.go updated.
Public API breakage? None — DetermineStateDir is in internal/secret, not exported to external consumers.
Error handling: Callers panic() on error, which is appropriate for startup constructors where no recovery is possible.
Error wrapping: Uses %w for both errors, enabling errors.Is/errors.As unwrapping.
Tests: Three new unit tests covering error path, env var path, and custom config dir. The error test pragmatically handles macOS password-database fallback.
Code style: Clean, minimal, consistent with existing codebase.
No issues found. Ready for maintainer merge.
## Self-Review: MERGE-READY ✅
(Cannot formally approve own PR, posting as comment.)
### Checklist
- **Signature change `string → (string, error)` handled at all call sites?** ✅ Both `NewCLIInstance()` and `NewCLIInstanceWithFs()` in `cli.go` updated. `cli_test.go` updated.
- **Public API breakage?** None — `DetermineStateDir` is in `internal/secret`, not exported to external consumers.
- **Error handling:** Callers `panic()` on error, which is appropriate for startup constructors where no recovery is possible.
- **Error wrapping:** Uses `%w` for both errors, enabling `errors.Is`/`errors.As` unwrapping.
- **Tests:** Three new unit tests covering error path, env var path, and custom config dir. The error test pragmatically handles macOS password-database fallback.
- **Code style:** Clean, minimal, consistent with existing codebase.
No issues found. Ready for maintainer merge.
Changes DetermineStateDir from returning string to (string, error) to handle the case where both os.UserConfigDir() and os.UserHomeDir() fail. Previously this produced a dangerous root-relative path /.config/....
The error wrapping uses %w for both errors — nice for debugging. Callers in cli.go now panic() on error, which is appropriate for a CLI entrypoint where there's no recovery.
Test note:TestDetermineStateDir_ErrorsWhenHomeDirUnavailable correctly handles the case where macOS provides a fallback via the password database — it checks either for an error OR a non-dangerous path. Good pragmatic testing.
All callers updated consistently.
## Code Review
**Verdict: LGTM ✅**
Changes `DetermineStateDir` from returning `string` to `(string, error)` to handle the case where both `os.UserConfigDir()` and `os.UserHomeDir()` fail. Previously this produced a dangerous root-relative path `/.config/...`.
The error wrapping uses `%w` for both errors — nice for debugging. Callers in `cli.go` now `panic()` on error, which is appropriate for a CLI entrypoint where there's no recovery.
**Test note:** `TestDetermineStateDir_ErrorsWhenHomeDirUnavailable` correctly handles the case where macOS provides a fallback via the password database — it checks either for an error OR a non-dangerous path. Good pragmatic testing.
All callers updated consistently.
✅make check passes (0 lint issues, all tests pass). Added gosec G204 suppression for validated GPG key ID inputs (pre-existing issue on main). Rebased on main.
golangci-lint run --timeout 5m
0 issues.
go test ./...
ok git.eeqj.de/sneak/secret/internal/cli
ok git.eeqj.de/sneak/secret/internal/secret
ok git.eeqj.de/sneak/secret/internal/vault
ok git.eeqj.de/sneak/secret/pkg/agehd
ok git.eeqj.de/sneak/secret/pkg/bip85
✅ `make check` passes (0 lint issues, all tests pass). Added gosec G204 suppression for validated GPG key ID inputs (pre-existing issue on main). Rebased on main.
```
golangci-lint run --timeout 5m
0 issues.
go test ./...
ok git.eeqj.de/sneak/secret/internal/cli
ok git.eeqj.de/sneak/secret/internal/secret
ok git.eeqj.de/sneak/secret/internal/vault
ok git.eeqj.de/sneak/secret/pkg/agehd
ok git.eeqj.de/sneak/secret/pkg/bip85
```
clawbot
requested review from sneak 2026-02-20 08:45:50 +01:00
Good catch — the panic in NewCLIInstance/NewCLIInstanceWithFs is there because these constructors return *Instance (no error return), so the error had nowhere to go. The t.Fatalf in tests is standard Go test practice and is fine.
The real fix is to change the constructors to return (*Instance, error) so callers can handle it properly instead of panicking. I'll update the PR.
Good catch — the `panic` in `NewCLIInstance`/`NewCLIInstanceWithFs` is there because these constructors return `*Instance` (no error return), so the error had nowhere to go. The `t.Fatalf` in tests is standard Go test practice and is fine.
The real fix is to change the constructors to return `(*Instance, error)` so callers can handle it properly instead of panicking. I'll update the PR.
please update AGENTS.md in the root of this repo with this and any other clear and concise policies that we have for code. keep it short and crystal clear and direct.
please update AGENTS.md in the root of this repo with this and any other clear and concise policies that we have for code. keep it short and crystal clear and direct.
Change NewCLIInstance() and NewCLIInstanceWithFs() to return
(*Instance, error) instead of panicking on DetermineStateDir failure.
Callers in RunE contexts propagate the error. Callers in command
construction (for shell completion) use log.Fatalf. Test callers
use t.Fatalf.
Addresses review feedback on PR #18.
Fixed. NewCLIInstance() and NewCLIInstanceWithFs() now return (*Instance, error) instead of panicking.
RunE callers propagate the error via return fmt.Errorf(...)
Command construction callers (for shell completion) use log.Fatalf
Test callers use t.Fatalf (unchanged, standard Go test practice)
Also updated AGENTS.md Go guidelines in a previous commit to codify: no panic/log.Fatal in library code, constructors return (*T, error).
make check passes clean:
golangci-lint run --timeout 5m
0 issues.
go test ./...
ok git.eeqj.de/sneak/secret/internal/cli 5.825s
ok git.eeqj.de/sneak/secret/internal/secret 3.563s
ok git.eeqj.de/sneak/secret/internal/vault 1.076s
ok git.eeqj.de/sneak/secret/pkg/agehd 0.823s
ok git.eeqj.de/sneak/secret/pkg/bip85 0.338s
Fixed. `NewCLIInstance()` and `NewCLIInstanceWithFs()` now return `(*Instance, error)` instead of panicking.
- RunE callers propagate the error via `return fmt.Errorf(...)`
- Command construction callers (for shell completion) use `log.Fatalf`
- Test callers use `t.Fatalf` (unchanged, standard Go test practice)
Also updated `AGENTS.md` Go guidelines in a previous commit to codify: no panic/log.Fatal in library code, constructors return `(*T, error)`.
`make check` passes clean:
```
golangci-lint run --timeout 5m
0 issues.
go test ./...
ok git.eeqj.de/sneak/secret/internal/cli 5.825s
ok git.eeqj.de/sneak/secret/internal/secret 3.563s
ok git.eeqj.de/sneak/secret/internal/vault 1.076s
ok git.eeqj.de/sneak/secret/pkg/agehd 0.823s
ok git.eeqj.de/sneak/secret/pkg/bip85 0.338s
```
Added Go-specific coding policies to AGENTS.md as requested:
No panic/log.Fatal/os.Exit in library code — always return errors
Constructors return (*T, error), not just *T
Wrap errors with %w
Never modify linter config to suppress findings
All PRs must pass make check with zero failures
Pin dependencies by commit hash
make check passes clean (0 lint issues, all tests pass).
Added Go-specific coding policies to AGENTS.md as requested:
1. No `panic`/`log.Fatal`/`os.Exit` in library code — always return errors
2. Constructors return `(*T, error)`, not just `*T`
3. Wrap errors with `%w`
4. Never modify linter config to suppress findings
5. All PRs must pass `make check` with zero failures
6. Pin dependencies by commit hash
`make check` passes clean (0 lint issues, all tests pass).
sneak
merged commit 4f5d2126d6 into main2026-02-20 08:54:22 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
sneak
Summary
Fixes #14:
DetermineStateDirignored the error fromos.UserHomeDir()in the fallback path. If bothos.UserConfigDir()andos.UserHomeDir()fail, the function returned/.config/berlin.sneak.pkg.secret— a dangerous root-relative path.Changes
internal/secret/helpers.go: ChangedDetermineStateDirsignature fromstringto(string, error). Now returns a wrapped error when bothUserConfigDirandUserHomeDirfail.internal/secret/helpers_test.go: New tests covering the error path, env var path, and custom config dir path.internal/cli/cli.go: Updated callers to handle the new error return.internal/cli/cli_test.go: Updated to match new signature.Test output: test-only (before fix)
Test output: test + fix
Linting
Note:
internal/clitests have a pre-existing failure (VCS stamping issue unrelated to this change). Allinternal/secrettests pass.Self-Review: MERGE-READY ✅
(Cannot formally approve own PR, posting as comment.)
Checklist
string → (string, error)handled at all call sites? ✅ BothNewCLIInstance()andNewCLIInstanceWithFs()incli.goupdated.cli_test.goupdated.DetermineStateDiris ininternal/secret, not exported to external consumers.panic()on error, which is appropriate for startup constructors where no recovery is possible.%wfor both errors, enablingerrors.Is/errors.Asunwrapping.No issues found. Ready for maintainer merge.
Code Review
Verdict: LGTM ✅
Changes
DetermineStateDirfrom returningstringto(string, error)to handle the case where bothos.UserConfigDir()andos.UserHomeDir()fail. Previously this produced a dangerous root-relative path/.config/....The error wrapping uses
%wfor both errors — nice for debugging. Callers incli.gonowpanic()on error, which is appropriate for a CLI entrypoint where there's no recovery.Test note:
TestDetermineStateDir_ErrorsWhenHomeDirUnavailablecorrectly handles the case where macOS provides a fallback via the password database — it checks either for an error OR a non-dangerous path. Good pragmatic testing.All callers updated consistently.
✅
make checkpasses (0 lint issues, all tests pass). Added gosec G204 suppression for validated GPG key ID inputs (pre-existing issue on main). Rebased on main.why fatalf and panic usage? seems inconsistent.
Good catch — the
panicinNewCLIInstance/NewCLIInstanceWithFsis there because these constructors return*Instance(no error return), so the error had nowhere to go. Thet.Fatalfin tests is standard Go test practice and is fine.The real fix is to change the constructors to return
(*Instance, error)so callers can handle it properly instead of panicking. I'll update the PR.please update AGENTS.md in the root of this repo with this and any other clear and concise policies that we have for code. keep it short and crystal clear and direct.
347dc22a27to36ece2fca7Fixed.
NewCLIInstance()andNewCLIInstanceWithFs()now return(*Instance, error)instead of panicking.return fmt.Errorf(...)log.Fatalft.Fatalf(unchanged, standard Go test practice)Also updated
AGENTS.mdGo guidelines in a previous commit to codify: no panic/log.Fatal in library code, constructors return(*T, error).make checkpasses clean:Added Go-specific coding policies to AGENTS.md as requested:
panic/log.Fatal/os.Exitin library code — always return errors(*T, error), not just*T%wmake checkwith zero failuresmake checkpasses clean (0 lint issues, all tests pass).