Cache successful validate results in hook mode - #457
Conversation
a09c0bc to
c36a76d
Compare
|
needs rebase |
…othing has changed Adds a generic FileCache[T] (internal/filecache) and wires it into chunk validate's Stop hook path. On a successful run the outcome is stored keyed by a SHA-256 of the command config, HEAD commit, and git status. Subsequent hook invocations with the same inputs skip execution entirely and print a one-line "skipped" message instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The working-tree component of the cache key hashed `git status --porcelain` output, which encodes paths and status codes but not content. That output is byte-identical before and after a further edit to an already-dirty file (both report " M path"), so the agent edit loop the Stop hook runs in could hit a false cache hit: edit, validate, edit the same file again, and validate reports "skipped (no changes since last successful run)" without checking the new code. worktreeDigest now hashes the porcelain entries plus the current contents of every changed path. Untracked files are listed individually with -uall so their contents are hashed too, rather than collapsing an untracked directory into a single "dir/" entry that hides edits inside it. Paths come from -z output and are resolved against `rev-parse --show-toplevel`, since porcelain paths are repo-root-relative while workDir may be a subdirectory. BuildCacheKey now returns ok=false when git state cannot be established: no repo, no commits yet, or a changed path that cannot be read. Previously gitOut swallowed those errors and returned "", collapsing the key to config-only — stable across code changes, and so another route to false hits. hookResultCache disables caching entirely in that case. Also narrows the commands parameter from `any` to []config.Command, and fixes a pre-existing ARCHITECTURE.md claim that manual runs cache per-file; they don't cache at all, and the unit is the whole run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Reset the hook failure counter on a cache hit, so a cached success and a real success behave identically. Without it a stale count survives into the next re-signal and brings the give-up message forward a turn. - Fold the execution target (configured snapshot image, active sidecar ID) into the cache key. Sidecar routing depends on state outside the repo, so switching sidecars previously left the key unchanged and the second sidecar was never validated against. BuildCacheKey now takes a CacheKeyInputs struct. - Make filecache.Put atomic via temp file + rename, and correct the type comment: entry filenames hash the key, not the value, so concurrent Puts wrote different bytes to one path and could tear. - Cap the working-tree digest at 64 MiB and fail closed past it, so a large un-gitignored untracked tree can't be re-hashed on every hook invocation. - Document that gitignored files and env vars do not invalidate the cache. Also use gitutil.HeadRef instead of a third git rev-parse shell-out, hoist the repo-root trim out of the digest loop, and note that CachedAt is informational. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove ResultCache single-impl interface, use *filecache.FileCache[CachedResult] directly at call sites. Log Put errors to streams at dim level instead of silently discarding them. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
danmux
left a comment
danmux
left a comment
There was a problem hiding this comment.
Description mentions a ResultCache interface that isn't in the diff. Worth dropping from the body since there is only one implementation anyway.
|
|
||
| execErr := runValidate(ctx, circleCIClient, rc, workDir, name, opts.inlineCmd, opts.save, opts.sidecarID, freshlyCreated, opts.workdir, allRemote, envVars, cfg, statusFn, streams) | ||
|
|
||
| if execErr == nil && resultCache != nil { |
There was a problem hiding this comment.
This condition is the whole "failures are never cached" guarantee, and nothing tests it. TestValidateHookCacheHitResetsAttempts only runs exit 0.
If this ever gets refactored so a failure is stored, every later hook invocation on the same tree prints skipped and returns nil, so the agent stops with the build broken and no test goes red. Can we get a sibling test with Run: "exit 1" asserting the second run does not skip? A miss-path case (edit a file between runs, assert re-execution) would cover the other unchecked box in the test plan.
| // as needed. The entry is staged in a temporary file alongside its final path | ||
| // and renamed into place, which is atomic within a single directory. Entries | ||
| // have mode 0600, the mode os.CreateTemp applies and the rename preserves. | ||
| func (c FileCache[T]) Put(key string, v T) error { |
There was a problem hiding this comment.
Superseded entries are never removed, so this is roughly one file per Stop turn, per project, indefinitely. CachedResult.CachedAt is written and, per its own comment, read by nothing.
Also, os.CreateTemp orphans a .tmp-* file if the process dies between staging and rename, which is what happens when a Stop hook is interrupted. Those aren't collected either.
Since CachedAt is already there, would a sweep on Put (drop entries older than N days, or keep the newest few) be cheap enough to avoid the manual rm -rf that HOOKS.md currently prescribes?
| // WrapHookResult does for a real one. Leaving a stale count behind | ||
| // would bring the "ask the user for guidance" bail-out forward by a | ||
| // turn. resultCache is only non-nil in hook mode, so hook is set. | ||
| validate.ResetAttempts(hook.sessionID) |
There was a problem hiding this comment.
This is a second hook success path that has to stay in sync with finishValidate by hand. The comment is honest about it, but the next change to the success branch there (another counter, an event log write, a telemetry attribute) silently won't happen on a cache hit, and the symptom is drifting hook state rather than a failing test.
Would return finishValidate(cmd, hook, nil, start, opts.sidecarID, cfg, statusFn, streams) work here instead? Collapses the two paths and the duplicated reasoning goes with it.
| if in.Worktree.Head == "" || in.Worktree.Digest == "" { | ||
| return "", false | ||
| } | ||
| cfgBytes, err := json.Marshal(in.Commands) |
There was a problem hiding this comment.
Only Commands is hashed. execTarget rescues Validation.SidecarImage, but cfg.Environment is in the key nowhere, and survives today only because .chunk/config.json shows up in git status and gets content-hashed (or moves HEAD when committed).
In a project that gitignores .chunk/, editing the environment block to add an env var or package the commands need changes neither the fingerprint nor this hash, so the next invocation reports a hit and never re-runs against the new environment. The gitignore caveat in HOOKS.md covers files a command reads, not chunk's own config driving the run.
Any reason not to marshal the fields that determine the run, or cfg wholesale, given it is already loaded?
| defer func() { _ = f.Close() }() | ||
|
|
||
| info, err := f.Stat() | ||
| if err != nil || !info.Mode().IsRegular() { |
There was a problem hiding this comment.
One unreadable file, one non-regular changed path (a dirty submodule), or one changed file over the budget fails the whole fingerprint, which makes hookResultCache return nil for that repo on every invocation.
So a repo with a submodule gets no benefit from this PR and no way to find out why: no skipped line ever, no diagnostic, and the HOOKS.md remedy doesn't apply because the cache was never consulted. A dim stderr note when Fingerprint fails in hook mode would make it diagnosable. That wants an error return rather than a bool to say which condition hit, which reads better at the call site too.
|
|
||
| // writePart mixes s into h length-prefixed, so that concatenations of different | ||
| // parts cannot collide (["ab","c"] and ["a","bc"] hash differently). | ||
| func writePart(h io.Writer, s string) { |
There was a problem hiding this comment.
Byte-identical to writePart in internal/validate/cache.go, doc comment included. It is the length-prefixing primitive both collision-avoidance arguments rest on, so if one copy is ever strengthened the other stays as-is, both remain internally consistent, and all tests keep passing while the two digests quietly diverge.
Separately: validate.CacheKey is exported but its only caller is BuildCacheKey in the same package.
c36a76d to
d033e3d
Compare
Tests for the two guarantees that had none. A failing run must never be cached, and an edit between runs must miss — both verified by mutation (caching failures reddens the first, dropping the working-tree digest from the key reddens the second). Hash the whole project config into the cache key, not just the commands. The environment block decides what the commands run against, and a project that gitignores .chunk/ got no invalidation from the working-tree digest when it changed. Fingerprint returns an error rather than a bool, and hook mode prints it. A repo with a dirty submodule got no benefit from the cache and no way to find out why: no "skipped" line ever, and no diagnostic. FileCache.Put sweeps entries past MaxAge (7 days by default) and staged files orphaned by an interrupted write, so the directory stays bounded without the manual rm -rf HOOKS.md prescribed. A cache hit now returns through finishValidate instead of repeating its hook bookkeeping, and the length-prefixing digest primitive both collision-avoidance arguments rest on lives in one place, internal/hashutil, rather than in two byte-identical copies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The key covers all of .chunk/config.json and the execution target, not just the commands, and entries now expire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Summary
FileCache[T](internal/filecache) — one JSON file per entry, filename issha256(key), silent miss on corrupt entries. Each write sweeps entries older than 7 days plus any staged file an interrupted run left behind, so the directory stays boundedCachedResultandBuildCacheKeytointernal/validate,gitutil.Fingerprintfor the working-tree half of the key, andinternal/hashutilfor the length-prefixed digest primitive both of those rest onvalidate.HasGitChangeswith that same fingerprint, so the Stop hook's existing clean-tree skip and the new cache read one pass over the tree instead of disagreeing about what changedchunk validate's Stop hook path: on a successful run the outcome is stored, and subsequent hook invocations with identical inputs skip execution and print a one-line "skipped" messageMotivation
In hook mode
chunk validatefires on every Claude Code Stop turn. Re-runninggo test ./...(or an SSH exec to a sidecar) when nothing has changed since the last successful run is pure waste.Invalidation is by content hash. The key is the command name plus a SHA-256 over:
.chunk/config.jsonwhole — commands and the environment block that decides what they run against, so a project that gitignores.chunk/still re-runs when either changesgit statusoutput alone reports identicallyFailures are never cached, so the agent always retries after applying a fix.
Entries also expire 7 days after they were written, swept on the next write. That is not the invalidation mechanism — a stale key simply never matches again — it just stops a content-addressed directory from growing one file per Stop turn forever.
When the tree cannot be fingerprinted at all (a dirty submodule, an unreadable changed file, more than 64 MiB of changed files) no cache is consulted, and hook mode prints the reason — otherwise a repo that gets no benefit from this has no way to find out why.
Test plan
go test -race ./...andtask acceptance-testpassTestValidateHookCacheMissAfterEdit— second run on an untouched tree skips; editing a file puts the commands back onTestValidateHookFailureIsNotCached— a failing command is never stored, so the next invocation re-runs rather than reporting a hitTestFileCache_Put_Sweeps*— stale entries and orphaned staged files are collected; a staged file still being written, and any file the cache did not write, are left alone🤖 Generated with Claude Code