Repository navigation
feat(cli): check replays an unchanged project's last pass and takes --no-craft - #7398
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…sts run before the signal test
jerrai-bot-heygen
left a comment
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
At 4f3f314, stale-PASS cache paths and a failing required CLI test remain; details and concrete fixes inline. A pass must not be replayed for different executable inputs. No merge or enqueue by reviewer. — Jerrai
| project: ProjectDir, | ||
| options: CheckOptions, | ||
| ): Promise<SavedCheckReport> { | ||
| const key = checkCacheable(options) ? checkCacheKey(project.dir, options, VERSION) : undefined; |
There was a problem hiding this comment.
Cache key can be associated with a different revision than the one checked. The fingerprint and replay happen before withProjectLock, and storePass uses that original key after the locked run without re-fingerprinting. If a writer holds the lock while revision A would fail, check hashes A and waits; the writer finishes passing revision B, check checks B and stores PASS under A's key. Restoring A now returns exit 0 without checking it. A cache hit also bypasses a held project lock. Move fingerprint/lookup inside the lock and verify the fingerprint again after the run before storing a reusable PASS; add a controlled writer/check interleaving test. This is distinct from merely making cache writes atomic.
| const path = join(dir, entry.name); | ||
| const name = `${prefix}${entry.name}`; | ||
| if (entry.isDirectory()) hashTree(path, `${name}/`, hash); | ||
| else hash.update(`${name}\0${fileFingerprint(path)}\0`); |
There was a problem hiding this comment.
Local executable inputs are omitted. This walk skips every node_modules directory, and a directory symlink is treated as a file whose fileFingerprint returns the constant "dir"; it never traverses its target. A composition that imports a local JS module through node_modules or a symlinked asset directory can pass, then have that imported module changed to throw while the tree key remains identical. Fresh browser check would report a page error, but cached check returns PASS. Hash the actual resolved dependency graph/content (or disable replay when a skipped/symlinked executable dependency is reachable); add an external-assets edit regression, not just an index.html edit.
| const stat = fstatSync(fd); | ||
| if (stat.isDirectory()) return "dir"; | ||
| return stat.size <= HASH_CONTENT_UP_TO | ||
| ? createHash("sha256").update(readFileSync(fd)).digest("hex") |
There was a problem hiding this comment.
Size+mtime is not a content-equivalence check above 2 MiB. A passing HTML/composition or media file over this threshold can be modified to the same byte length while its mtime is preserved/restored (some editors or sync tools do this). The key remains unchanged even if a required composition ID is removed or media corrupted; a fresh check would fail but the cache returns PASS. For a success verdict, hash content regardless of size or make a non-hashed large input uncacheable; test a same-size/same-mtime changed asset. This is especially important when an agent treats check as the release preflight.
| ): string | undefined { | ||
| const hash = createHash("sha256").update(`${version}\0`); | ||
| const { timeout: _timeout, budget: _budget, cache: _cache, ...verdictOptions } = options; | ||
| hash.update(`${JSON.stringify(verdictOptions)}\0`); |
There was a problem hiding this comment.
Verdict-changing parameters are stripped or absent. timeout is excluded here even though it controls browser navigation/readiness and the pipeline deadline: pass with 45 s, rerun identical files with --timeout 500, and the old PASS is replayed instead of exercising the shorter timeout. The key also omits HYPERFRAME_RUNTIME_URL, which changes the injected runtime URL; an unchanged project can replay PASS after that runtime starts returning an error. Mutable remote scripts/assets have the same invalidation gap because no response identity or freshness is captured. Include the timeout and behavior-changing env in the key, and either revalidate remote dependencies or explicitly make projects with them uncacheable; add tests for a changed runtime/timeout after a cached pass.
miguel-heygen
changed the title
|
Fixed at a34814c: lookup, run and store now sit under the project lock and a pass is stored only if the tree still hashes to its key afterwards (test: a writer changes the tree during the run, then the old content returns); node_modules and linked directories make a project uncacheable; files over 2 MiB are keyed by size, mtime, ctime and inode (test: same size, mtime put back); timeout and the HYPERFRAME*/PRODUCER_/HF_ env are in the key (test: timeout and runtime URL change). Each new test fails when its guard is removed. Remote scripts and assets are still not re-fetched on a hit; the cached notice and docs say so, and --no-cache re-checks them. The failing CLI test was the warm check test comparing a run with its own cached pass; it now passes --no-cache. |
|
Re-review requested at ef994ce. Findings from 4f3f314 are answered in a34814c and 6aad021 (lock + recheck, node_modules/links uncacheable, ctime for large files, timeout and env in the key, each with a test shown to fail without its guard). Main is merged in, so CI now runs on the ffmpeg install fix. |
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: APPROVE · 0 blocker · 0 important · 2 nits · head ef994ced
I checked all five points from @jerrai-bot-heygen's 4f3f314 review against the code, and each one holds. Lookup, run and store sit inside withProjectLock, and the tree is fingerprinted again before a store. node_modules and linked directories return Uncacheable. Files over 2 MiB are keyed by size, mtime, ctime and inode. The timeout and the HYPERFRAMES?_/PRODUCER_/HF_ env are in the key, and the warm daemon declines when the env digest differs. Remote assets are disclosed and left to --no-cache.
- nit: the browser is not in the key. When the CLI falls back to system Chrome (
browser/manager.ts:445), a Chrome auto-update replays a pass made in the old browser. Folding the resolved executable path and its version into the key would close this. - nit: running from source,
VERSIONis0.0.0-dev(version.ts:3). Someone editing the checker and re-runningcheckon the same example gets the old pass. The notice line points to--no-cache, so this only affects development.
Scope
Read in full: checkCache.ts, the check.ts changes (checkOrReplay, flags, notice), craftEnabled, the new tests and the warm-check env proof. The existing tests use /project, which does not exist, so they never write a cache. Every CI workflow on this head passed, including Test (cli 1/2, 2/2), CLI smoke and Typecheck. I did not run the tests locally because this sandbox has no bun.
jerrai-bot-heygen
left a comment
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
At ef994ced, the earlier five fixes and current-head CI are verified, but the two inline stale-PASS cases remain. Please close those cache-key omissions before a stamp. No merge/enqueue by reviewer.
— Jerrai
| a.name < b.name ? -1 : 1, | ||
| ); | ||
| for (const entry of entries) { | ||
| if (SKIPPED_DIRS.has(entry.name)) continue; |
There was a problem hiding this comment.
The excluded .hyperframes tree can contain executable composition input. hashTree skips any .hyperframes directory before checking its files or symlinks, but the HTML bundler reads relative local scripts and the project server serves any in-root file, including .hyperframes/probe.js. A project whose HTML already references that script can pass, then the script alone changes to console.error(...); an identical cached check returns PASS while --no-cache reports console_error. A directory symlink inside this skipped tree likewise bypasses the new linked-directory rejection. Please fingerprint referenced authored files in the namespace or make such projects uncacheable; test a change solely under .hyperframes.
| const env = Object.entries(process.env) | ||
| .filter(([name]) => VERDICT_ENV.test(name)) | ||
| .sort(); | ||
| hash.update(`${JSON.stringify([verdictOptions, env])}\0`); |
There was a problem hiding this comment.
The key does not identify the browser that produced the PASS. It captures the HYPERFRAMES_BROWSER_PATH string but not the executable at that path or its version; browser/manager.ts may also select a managed/system Chrome that updates independently of the project. For a concrete failure, pass with a browser wrapper at a stable override path, replace the wrapper with one that exits unsuccessfully, and rerun the same project/options: the cache returns the old PASS while --no-cache cannot run browser validation. Include the resolved browser executable identity/version in the key (or disable replay when it can change), and add a changed-browser regression.
| a.name < b.name ? -1 : 1, | ||
| ); | ||
| for (const entry of entries) { | ||
| if (entry.name === ".git") continue; |
There was a problem hiding this comment.
important (non-blocking): this skips the wrong outcome rarely but misses the speed-up often. Projects whose video Chrome cannot decode never store a pass, so the cache never hits for them. check runs preResolveHostileMediaProxies → resolveProxies → resolveProxy. For a copy that already exists, resolveProxy calls markCacheEntryUsed, which runs utimesSync (studio-server/src/helpers/proxyTranscoder.ts:362). The copies live in <project>/.transcode-cache and are usually over 2 MiB, so their mtime and ctime are in the key. The re-key after the run therefore never matches. The copies are derived from sources that are already in the key, so skipping that directory like .git keeps the key sound:
| if (entry.name === ".git") continue; | |
| if (entry.name === ".git" || entry.name === ".transcode-cache") continue; |
A test with a >2 MiB file under .transcode-cache that is touched between two runs would pin it (PROXY_CACHE_DIR_NAME is the shared name).
jerrai-bot-heygen
left a comment
jerrai-bot-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed 4fc5f875 against main and my two ef994ced findings. The project-tree key now descends into .hyperframes and excludes only named tool-owned state at its root; a script there and a linked directory under it are visible to the fingerprint/uncacheable rule. The resolved browser binary is identified before lookup and again before store using path, size, mtime, ctime and inode; if no browser is installed there is no cache hit or store. The tests cover a changed .hyperframes/probe.js, a linked directory there, a changed browser identity and a missing browser. The cached human notice and CLI reference now explicitly say remote scripts/assets are not re-fetched; --no-cache is the fresh-read option. The intermediate .transcode-cache fix uses the shared proxy-transcoder constant and avoids invalidation from derived video-copy touches. Earlier lock/post-run hash, visible node_modules/symlink, large-file ctime/inode, timeout/env and warm-check fixes remain. Exact-head hosted CLI shards, typecheck and other checks executed green. No confirmed remaining blocker in the scoped cache contract. Runtime replay and author's speed numbers were not independently reproduced. This approval supersedes my prior change requests; no reviewer merge/enqueue.
— Jerrai
…red-row line counts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
Two switches that make a repeated
hyperframes checkcheap.hyperframes checkon a project that has not changed since its last passing check returns that pass in under a second instead of re-running the browser pass. The output says it is the stored result, and--no-cacheruns the check again.--no-craftskips the craft pass (dead stretches, unread lines, silent films); the report shows Craft as skipped and the pass stays on by default.Why
A repeated check on an untouched tree costs the full run (about 12 s on a small film, over a minute on a long one), which is longer than the render it guards when an agent re-checks after every step. On a 78 s film the craft pass is about half of a full check (599 of its 600 samples), and a render preflight had no way to leave it out.
Related work
Refs the request to skip a tree unchanged since the last pass and to make the craft pass switchable. Start-up costs follow separately.
How
utils/checkCache.tskeys a result on the CLI version, every flag (timeout included), theHYPERFRAME*,PRODUCER_*andHF_*environment, and the project files: content hash up to 2 MB per file, above that size plus mtime, ctime and inode (a tool cannot put ctime back). The browser binary (path, size, mtime, ctime, inode) is in the key, and no installed browser means no cache. Under.hyperframesonly what the CLI writes (artifacts,cache,history-id,video-state*,project.lock) and.gitare left out; a project withnode_modulesor a linked directory is never cached..hyperframes/cache/check.json. A failure, a browser-skipped run, a--budgetrun and the artifact-producing flags (--snapshots, finding frames, inspect) are never stored or replayed.cached: { at }in--jsonand a one-line notice in the human report.--no-cachere-checks them.craftis read bycraftEnabled, the one place that decides whether craft frames are sampled;--rangealready turned it off through the same line.git grep -n -i "contentHash\|treeHash\|hashTree"overpackages/cli/src): only lint telemetry hashes single files, so there was nothing to reuse.Test plan
--no-cacheand--budgetre-run. Each fails when its guard is removed (stored-failure guard, content hashing).registry/examples/kinetic-type):--no-cache, same work as before this change: 12235 ms, 12164 ms, 11840 ms--no-crafttimings, same machine, a 13-scene 78 s synthetic film, alternating runs:--no-craft: 4310, 4117, 4315, 4280, 4498 ms--no-cachein the CLI reference)Size
About 300 changed lines across two small switches that share the check command's flag and option plumbing, kept in one PR so one screen has one PR.