Repository navigation
fix(cli): templates pull reuses only a template it pulled, not any path in the way - #7116
Conversation
There was a problem hiding this comment.
Request changes at f4d74619. The fix itself is right, but CI is red.
Blocker: the Comments check fails on this head.
RAISED comment share: packages/cli/src/commands/templates.ts is 4.3% (8/187), at the base 3.8% (6/160)
2 comment line(s) added. Cut them, or move the explanation into a name or a type.
That contradicts the PR body's "4.03% to 4.02%". The two lines are the new JSDoc on pulledHere and the trailing // only ever the empty folder pulledHere let through on the rmdirSync. Cut both, or move what they say into names (for example removeEmptyPlaceholder(dest)). Updating the existing pullTemplate JSDoc is fine as it is.
What I verified, all good:
- On this head,
templates.test.tspasses 11/11. With the parent'stemplates.tsrestored, the three new behaviour tests fail: "fills an empty folder", "stops at a file or someone else's folder", and "keeps an empty folder when the pull is refused". So the "Fails without the fix" lines hold. pulledHerechecks before any request. It refuses with aPullRefusedand deletes nothing. The empty placeholder is removed only after the tar has unpacked, right before the rename.rmdirSyncfails safely (ENOTEMPTY) if someone fills the folder in between.
Non-blocking: when pack.sha256 is bumped, a template this CLI pulled at the old pin has a record that no longer matches. It gets the "is there but is not this template" message, which misreads as someone else's folder. A separate message for "record present, different hash", such as "an older pull of this template; move it aside to update", would read better.
There was a problem hiding this comment.
Approve at 74010b8b. My blocker from f4d74619 is fixed.
- Comments: the
Commentsworkflow passes on this head. The two comment lines are gone, and their meaning is now carried by the namesalreadyPulled,removeEmptyPlaceholderandPULLED_HASH. - Non-blocking note: also addressed. A record with a different hash now gets "holds an older pull of this template". The new test checks that message, checks the folder is left untouched and that no fetch happens.
- Behavior: unchanged otherwise.
templates.test.tspasses 12/12 locally at this head.
What
hyperframes templates pull <style>no longer reports a template as installed whentemplates/<style>is only an empty folder or a plain file. An empty folder gets the template; a file, or a folder of someone else's files, stops the pull with one line saying so, and stays as it was.Why
The pull treated any path at
templates/<style>as an installed template: it exited 0 with a checkmark and downloaded nothing, so the agent went on as if the template were there.Related work
None.
How
.template-sha256(the pinned hash it unpacked) into the template just before the template moves in whole, so the record exists only in a complete template.alreadyPulledis now the one place that decides "already installed": a folder whose record matches the pinned hash. An empty folder is let through and filled (removed right before the move-in, so a refused pull leaves it alone). Anything else stops the pull before any request, and nothing of the person's is deleted.No visible change
CLI output only.
Test plan
templates.test.ts› "stops at a file or someone else's folder where the template goes, and leaves it as it was". Fails without the fix: on main the pull returns with a checkmark.templates.test.ts› "fills an empty folder where the template goes". Fails without the fix: on main the folder stays empty.templates.test.ts› "names a pull of an older pin as such and leaves it". Fails without the fix.templates.test.ts12 of 12, three runs in a row; with main'stemplates.tsthe four new behavior tests fail. CLI typecheck clean.comment-ratchetjudge: 6 of 193 lines, base 6 of 160).Size
Two files, about 80 lines with the tests.