cli-common state components: statedir resolver + hermetic test helper + tier-1 cache core#18
Conversation
Adds the cli-common state path components (working-with-state.md §5a): - statedir: Scope (credential-scope config dir, §3) and Cache (per-binary cache dir, §4.1) over os.UserConfigDir/os.UserCacheDir — no hand-rolled ~/.config, no %APPDATA% branch. ConfigDir/CacheDir are side-effect free; *Ensured is the only mkdir path (0700) and never re-chmods a pre-existing dir (per-port concern). LegacySource is the migration-source seam — a documented type with no behavior; the resolver never enumerates or reads legacy paths (per-port §3.2 policy stays per-CLI). - statedirtest: the single Hermetic helper isolating the full 7-var env set (HOME, USERPROFILE, AppData, LocalAppData, XDG_CONFIG_HOME, XDG_CACHE_HOME, XDG_DATA_HOME) so os.User*Dir never resolves to a real dir on any OS. HOME-only is a Windows real-dir leak. Pure stdlib; no go.mod/go.sum change. [MON-5364]
Tier-1 cache core (working-with-state.md §5b), lifted from jtk's envelope/freshness shapes and made directory-agnostic via an injected Locator (receives Root, never derives it): - Envelope[T] + ReadResource[T]/WriteResource[T]; atomic temp-file-in-same-dir -> chmod 0600 -> rename (dir 0700), temp removed on every failure branch; version-mismatch-as-miss (self-healing schema bump); reads never check freshness. - Locator validates all three path inputs before composing: Root must be non-empty and absolute (a zero-value Locator must not write relative to cwd -> ErrInvalidRoot); InstanceKey and resource name pass the ^[A-Za-z0-9][A-Za-z0-9.-]*$ guard plus ".." rejection (ErrInvalidName). - Classify returns only Fresh|Stale|Manual; Uninitialized is caller-derived (ErrCacheMiss) and Unavailable is registry-derived (tier-2) — both kept for parity, exercised only via String(). Age: -/Ns/Nm/Nh/Nd. Tier 2 (registry/DAG/fetchers/refresh) deferred (rule-of-three). Pure stdlib; no go.mod/go.sum change. [MON-5364]
One status line on §6 step 1 recording that the cli-common state components (statedir, statedirtest, cache) now exist. No semantic change to the standard; no CLI ported; no INT-310 tag cut (the §5 release-train guardrail is unaffected). Closes #17 [MON-5364]
|
Major
Minor
Nit
Everything else lines up: no Migrate engine, |
Address Codex PR review: - validateComponent now rejects both / and \ on every OS (was OS-dependent: \ slipped through on Unix). Add a\b to the invalid-name tests (statedir + cache). - Narrow the atomicWriteEnvelope doc: rename is atomic, error branches remove temps, but a hard crash can orphan a *.tmp (superseded by the next write, never read as an envelope). - Repeat the no-t.Parallel constraint on Hermetic's own doc comment. - docs §6: ReadResource[T]/WriteResource[T] (was Read/WriteResource[T]). [MON-5364]
TDD coverage assessment (independent Sonnet pass)All 48 tests pass ( Gap
Weak
The happy path and pre-validation (invalid root/name) are covered. The three error branches within the atomic write itself are not exercised:
The
The branch at line 54–55 handles
Both functions have an error branch for when the stdlib helper itself fails (e.g. Nit
The
Summary table
Verdict: Coverage is solid for happy paths and the security-sensitive validation logic; the one concrete gap is the fully-untested |
Address independent TDD assessment: - TestCacheDirEnsured_Creates0700 — the previously fully-untested CacheDirEnsured export. - TestReadResource_IOErrorIsNotMiss — resource path is a directory, so os.ReadFile returns a non-NotExist error: verifies the "I/O error, not a miss" contract. - Classify exact-boundary row (now-ttl == d -> stale, the >= edge). - Add "." to the cache unsafe-component list. [MON-5364]
|
Re: TDD coverage assessment (#18 (comment)) — addressed in the latest push: Fixed
Declined, with rationale
|
Codex re-review — post Major-fix + TDD additionsFindings
Round-1 fixes landed cleanly in the current PR diff. The separator validation is now OS-independent and tested, atomic-write docs no longer overclaim crash cleanup, Architectural checks also hold: STATUS: blockers=0 majors=0 minors=0 nits=0 — converged (session 019e4019). |
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: a517574
Approved with 6 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 1 |
| harness-engineering:harness-enforcement-reviewer | 2 |
| harness-engineering:harness-knowledge-reviewer | 2 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
harness-engineering:harness-architecture-reviewer (1 findings)
💡 Suggestion - cache/locator.go:26
safeComponent regex rejects underscores (
[A-Za-z0-9.\-]has no_). Non-hostname identifiers likemy_toolor AWS account-scoped IDs would fail with ErrInvalidName — a silent footgun for future adopters. Consider adding_to the character class.
harness-engineering:harness-enforcement-reviewer (2 findings)
💡 Suggestion - cache/envelope.go:56
Uses
os.IsNotExist(err)instead oferrors.Is(err, os.ErrNotExist). The legacy helper does not unwrap error chains, so a wrappedos.ErrNotExistwould fall through to the generic I/O error branch instead of being treated as a cache miss.
💡 Suggestion - cache/envelope.go:68
ReadResourcedoes not verify thatenv.Resourcematches the requested name or thatenv.Instancematchesloc.InstanceKey. A manually-edited or misplaced file will silently return an envelope whose metadata disagrees with the requested key. Adding a consistency guard (return ErrCacheMiss on mismatch) would prevent consumer confusion.
harness-engineering:harness-knowledge-reviewer (2 findings)
💡 Suggestion - cache/locator.go:26
safeComponent regex permits names ending with a dot (e.g.
"foo."). On Windows, NTFS/FAT silently strips trailing dots, sofilepath.Join(root, "foo.", "r.json")resolves identically tofilepath.Join(root, "foo", "r.json"). Anchor the last character to[A-Za-z0-9]or add a!strings.HasSuffix(s, ".")guard.
💡 Suggestion - cache/cache_test.go:147
TestUnsafeComponentsdoes not include a name with an embedded..(e.g."a..b"). The implementation correctly rejects it via thestrings.Contains(s, "..")guard, but the test never exercises that branch. A future refactor consolidating the guard into the regex alone would go undetected. Adding"a..b"to the bad slice would pin this invariant.
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - cache/envelope.go:48
The
nolintcomment says "not user input", but InstanceKey and resource names can originate from user-supplied config (e.g. a hostname). The accurate rationale is that the path has already been validated byLocator.resourceFile. The current comment may lead a future reader to skip re-evaluation if new unvalidated call sites are added.
4 info-level observations excluded. Run with --verbose to include.
4 PR discussion threads considered.
Completed in 2m 32s | $0.90 | sonnet | daemon 0.2.120 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 2m 32s wall · 8m 01s compute (Reviewers: 1m 58s · Synthesis: 31s) |
| Cost | $0.90 |
| Tokens | 139.6k in / 33.1k out |
| Turns | 7 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 27.9k | 2.0k | 0 | 27.9k (1h) | $0.14 |
| documentation:docs-reviewer | haiku | 3.8k | 6.1k | 0 | 0 | $0.04 |
| harness-engineering:harness-architecture-reviewer | sonnet | 21.7k | 3.8k | 2.1k | 19.6k (1h) | $0.15 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 21.7k | 5.1k | 2.1k | 19.6k (1h) | $0.16 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 21.7k | 7.3k | 2.1k | 19.6k (1h) | $0.20 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 21.3k | 4.2k | 2.1k | 19.1k (1h) | $0.15 |
| security:security-code-auditor | haiku | 21.5k | 4.6k | 0 | 21.5k (1h) | $0.06 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
| // starting alphanumeric. Path separators, whitespace, and control characters | ||
| // are rejected rather than trusted (the values are caller-supplied — e.g. a | ||
| // hostname derived from config). | ||
| var safeComponent = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9.\-]*$`) |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): safeComponent regex rejects underscores ([A-Za-z0-9.\-] has no _). Non-hostname identifiers like my_tool or AWS account-scoped IDs would fail with ErrInvalidName — a silent footgun for future adopters. Consider adding _ to the character class.
Reply to this thread when addressed.
There was a problem hiding this comment.
Declined: the [A-Za-z0-9.\-] set is a deliberate verbatim lift of jtk's isSafeInstanceKey (instance keys are hostnames / Atlassian cloud-ids — no underscores). Broadening it for hypothetical my_tool/AWS-account adopters is speculative and out of this ticket's scope (commons buildout, no consumer). If a real adopter needs _, that's a tracked widening with its own test, not a silent footgun introduced now.
| // starting alphanumeric. Path separators, whitespace, and control characters | ||
| // are rejected rather than trusted (the values are caller-supplied — e.g. a | ||
| // hostname derived from config). | ||
| var safeComponent = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9.\-]*$`) |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): safeComponent regex permits names ending with a dot (e.g. "foo."). On Windows, NTFS/FAT silently strips trailing dots, so filepath.Join(root, "foo.", "r.json") resolves identically to filepath.Join(root, "foo", "r.json"). Anchor the last character to [A-Za-z0-9] or add a !strings.HasSuffix(s, ".") guard.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed (b9 commit): validComponent now also !strings.HasSuffix(s, "."), and statedir.validateComponent rejects a trailing dot too. Pinned by trailingdot. in both unsafe lists. Good cross-OS catch.
| return Envelope[T]{}, ErrCacheMiss | ||
| } | ||
| return Envelope[T]{}, fmt.Errorf("cache: reading resource file: %w", err) | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): Uses os.IsNotExist(err) instead of errors.Is(err, os.ErrNotExist). The legacy helper does not unwrap error chains, so a wrapped os.ErrNotExist would fall through to the generic I/O error branch instead of being treated as a cache miss.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed: os.IsNotExist(err) → errors.Is(err, os.ErrNotExist) (chain-safe; future-proof if a wrapped read helper is introduced).
| } | ||
| return env, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): ReadResource does not verify that env.Resource matches the requested name or that env.Instance matches loc.InstanceKey. A manually-edited or misplaced file will silently return an envelope whose metadata disagrees with the requested key. Adding a consistency guard (return ErrCacheMiss on mismatch) would prevent consumer confusion.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed: ReadResource now returns ErrCacheMiss when env.Resource != name or env.Instance != loc.InstanceKey, folded into the existing version-mismatch self-healing guard. New TestIdentityMismatchIsMiss covers it.
| } | ||
| if _, err := cache.ReadResource[payload](loc, "r"); !errors.Is(err, cache.ErrInvalidRoot) { | ||
| t.Fatalf("root=%q ReadResource err = %v, want ErrInvalidRoot", root, err) | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): TestUnsafeComponents does not include a name with an embedded .. (e.g. "a..b"). The implementation correctly rejects it via the strings.Contains(s, "..") guard, but the test never exercises that branch. A future refactor consolidating the guard into the regex alone would go undetected. Adding "a..b" to the bad slice would pin this invariant.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed: a..b added to TestUnsafeComponents so the strings.Contains(s,"..") invariant is pinned independently of the regex.
| path, err := loc.resourceFile(name) | ||
| if err != nil { | ||
| return Envelope[T]{}, err | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): The nolint comment says "not user input", but InstanceKey and resource names can originate from user-supplied config (e.g. a hostname). The accurate rationale is that the path has already been validated by Locator.resourceFile. The current comment may lead a future reader to skip re-evaluation if new unvalidated call sites are added.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed: nolint rationale corrected to "path already validated + composed by Locator.resourceFile" (was misleadingly "not user input" — instance keys can be config-derived).
…miss Address pr-review-daemon (APPROVE) low findings worth taking: - validComponent / statedir.validateComponent now reject a trailing dot: Windows strips it, so "foo." and "foo" would collide cross-OS. - ReadResource treats env.Resource != name or env.Instance != loc.InstanceKey as a miss (same self-healing rule as version mismatch): a misplaced/hand-edited file is never returned under the wrong key. - os.IsNotExist -> errors.Is(err, os.ErrNotExist) (chain-safe idiom). - nolint rationale corrected: "validated by Locator.resourceFile", not "not user input". - Tests: identity-mismatch-as-miss; "a..b" and trailing-dot pinned in the unsafe-component lists (statedir + cache). Declined: adding "_" to the safe set — deliberate jtk isSafeInstanceKey parity (hostnames/cloud-ids); broadening it is out of ticket scope. [MON-5364]
…match Final Codex alignment nit: the public doc comments still described miss behavior as absent-or-version-mismatch only; the contract now also covers a stored resource/instance that disagrees with the requested key. Comment only; behavior already shipped in the prior commit. [MON-5364]
Final Codex alignment pass (post TDD + daemon changes) — session 019e4019No architectural drift. Codex confirmed the late changes hold the line: still tier-1 only (no Migrate engine, no registry/DAG), One nit (fixed): the STATUS: blockers=0 majors=0 minors=0 nits=0 — converged. pr-review-daemon: APPROVE (all 6 low findings addressed in-thread; 5 taken, 1 declined with rationale). |
2 participants
Implements §6 step 1 ("commons-first") of
docs/working-with-state.md: the sharedcli-commonstate components so the per-unit CLI ports (later, separate tickets) have something to adopt. No CLI is ported here; no §3.2 migration matrix (per-port, no consumer yet); no INT-310 tag cut (the §5 release-train guardrail is unaffected).What
Three decomposed commits, each green on its own:
statedir—Scope(credential-scope config dir, §3) +Cache(per-binary cache dir, §4.1) overos.UserConfigDir/os.UserCacheDir.ConfigDir/CacheDirare side-effect free;*Ensuredis the onlymkdir(0700) and never re-chmods a pre-existing dir (per-port concern).LegacySourceis the §5a migration-source seam — a documented behavior-free type; the resolver never enumerates/reads legacy paths.statedirtest— the singleHermetichelper isolating the full §3.1 7-var env set (HOME,USERPROFILE,AppData,LocalAppData,XDG_CONFIG_HOME,XDG_CACHE_HOME,XDG_DATA_HOME). HOME-only is a Windows real-dir leak.cache— directory-agnostic tier-1:Envelope[T]+Read/WriteResource[T]; atomic temp-file→chmod 0600→rename (dir 0700), temp removed on every failure branch; version-mismatch-as-miss; injectedLocatorthat validatesRoot(non-empty + absolute →ErrInvalidRoot, closes the zero-value cwd-write hole),InstanceKey, and resourcename(regex +..reject →ErrInvalidName).Classifyreturns only Fresh|Stale|Manual;Status.String()parity for all five. Tier 2 deferred (rule-of-three).Constraints honored
go.mod/go.sumbyte-identical (themake checktidygate enforces this).make checkgreen: tidy clean,golangci-lint0 issues,go test -race ./...passes including the existingcredstoresuite (no regression). 45 new tests.Review provenance
Codex architect plan review (session
019e4019, gpt-5.5 xhigh) converged round 2 (blockers=0 majors=0 minors=0 nits=1). Round 1 caught 2 majors (zero-valueLocator.Rootcwd-write hole;Classifystate-set overclaim) + 2 minors + 1 nit — all fixed before implementation. Full disposition on #17.Closes #17
[MON-5364]