Repository navigation
fix(collect,install): collect only real skills; let --track take a local git path - #508
Conversation
…cal git path collect treated every directory in a target as a local skill, so a scratch folder was listed and `collect --force` copied it into the source. A folder now counts only when it holds SKILL.md; the dashboard scan shares FindLocalSkills and the dashboard collect request rejects such a folder. `install /path/to/repo --track` failed with "requires a git repository source" while file:///path worked. A local path that is a git repository root is now normalized to the same file:// clone URL for --track (also past the --branch guard); other local folders get an error that names file://. Help text, docs (English and i18n) and the built-in skill reference follow.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e31309fe7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // fileCloneURL builds the file:// URL git clones a local repository from. | ||
| func fileCloneURL(path string) string { | ||
| return "file://" + filepath.ToSlash(path) |
There was a problem hiding this comment.
On Windows, source.Path is shaped like C:\repo, so this produces file://C:/repo; the C: is then the URI authority rather than part of the local path. RFC 8089’s Windows examples use an empty authority such as file:///c:/path, so Git can resolve the drive correctly. Consequently, the newly supported install C:\repo --track can clone the wrong path or fail, particularly when the repository is on a different drive; construct a proper file URI with an empty authority instead of concatenating the prefix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aa9670f. fileCloneURL now adds the leading slash when the slash-converted path has none, so C:/repo becomes file:///C:/repo (POSIX paths are unchanged); this also covers parseFileURL, which shares the helper. Covered by TestFileCloneURL_KeepsWindowsDriveInPath, which failed before the change.
…-track paths A Windows path such as C:/repo produced file://C:/repo, which makes C: the URI authority instead of part of the path, so a tracked install from a local Windows repository could clone the wrong location. Prefix the missing slash so the URL is file:///C:/repo; POSIX paths are unchanged. Also cover the dashboard install endpoint with a local-path --track test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa9670fb07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !strings.HasPrefix(p, "/") { | ||
| p = "/" + p |
There was a problem hiding this comment.
When an existing explicit UNC URL such as file://server/share/repo is parsed, matches[1] is server/share/repo, so this unconditional slash insertion rewrites it to file:///server/share/repo. That changes server from the URI authority into a local path component, causing Windows installs from network shares to clone the wrong location or fail. Preserve the authority for explicit file URLs while still adding the empty authority for drive-letter paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb72272. The leading-slash logic moved out of the helper that parseFileURL shares into localFileURL, used only for local-path --track; explicit file://server/share/repo URLs are parsed as before. Covered by TestParseSource_ExplicitUNCFileURLKeepsAuthority and TestLocalFileURL_KeepsWindowsDriveInPath.
| } | ||
|
|
||
| // A folder without SKILL.md (a scratch dir) is not a skill. | ||
| if _, err := os.Stat(filepath.Join(skillPath, "SKILL.md")); err != nil { |
There was a problem hiding this comment.
If a scratch folder contains a subdirectory named SKILL.md, os.Stat succeeds and this code still reports and collects the folder as a skill, even though the normal discovery path only recognizes a non-directory SKILL.md. The copied result is therefore not a usable skill; inspect the returned file info and skip directories here, and apply the same validation in the dashboard POST handler.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb72272. New sync.HasSkillFile requires SKILL.md to be a file; FindLocalSkills (CLI and dashboard scan) and the POST /api/collect handler both use it. Covered by TestFindLocalSkills_SkipsDirWithoutSkillMd and TestHandleCollect_RejectsFolderWithoutSkillMd, which failed before the change with a directory named SKILL.md.
…be a file The drive-letter fix lived in the helper shared with explicit file:// URLs, which rewrote file://server/share/repo into a local path. Keep the slash logic in the local-path --track case only and leave explicit URLs as parsed. A subdirectory named SKILL.md passed the existence check, so a scratch folder holding one was still collected. Both collect entry points (scan and the dashboard request) now share sync.HasSkillFile, which rejects directories.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb72272b37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Git decodes a file:// URL, so a repository path containing %20, %2F, a space or # was cloned from a different path than the one IsGitRepo accepted. Build the URL with net/url so those characters are escaped; drive-letter and POSIX paths keep their form. The integration test now installs from a directory whose name contains a space and a literal %20.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26e4c5279b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // normalizeTrackSource turns a local path that is a git repository into the | ||
| // file:// form that parseFileURL builds, and rejects other non-git sources. | ||
| func normalizeTrackSource(source *Source) error { | ||
| if source.Type == SourceTypeLocalPath && IsGitRepo(source.Path) { |
There was a problem hiding this comment.
--track
When the supplied path is a bare Git repository, it has HEAD, objects, and refs at its root but no <path>/.git, so IsGitRepo returns false and this new local-path flow rejects it. The equivalent file:///same/path source proceeds to git clone, while the new documentation promises that any local path at a Git repository root is accepted; recognize bare repositories here as well or narrow the documented contract.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b021544. New install.IsLocalGitRepo accepts a working tree (.git) or a bare repository (HEAD file plus objects directory at the root); --track normalization and the --branch guard both use it. Covered by TestInferTrackedKind_LocalBareRepoPath and integration TestInstall_Track_LocalBareRepoPath, both failing before the change.
A bare repository has HEAD and objects at its root but no .git entry, so the local-path check rejected it while file:///same/path cloned fine. Add IsLocalGitRepo (working tree or bare) and use it for both the --track normalization and the --branch guard.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b021544065
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // normalizeTrackSource turns a local path that is a git repository into the | ||
| // file:// form that parseFileURL builds, and rejects other non-git sources. | ||
| func normalizeTrackSource(source *Source) error { | ||
| if source.Type == SourceTypeLocalPath && IsLocalGitRepo(source.Path) { |
There was a problem hiding this comment.
On Windows, install \\server\share\repo --track never reaches this new local-repository branch: ParseSource's isLocalPath recognizes drive-letter and .\/..\ paths, but not the canonical UNC form beginning with \\, so parsing fails as an unrecognized source. This leaves the documented local-git-path support unusable for repositories on network shares even though explicit file://server/share/repo URLs are supported; extend local-path parsing and conversion to handle native UNC paths.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. A native UNC source (\\server\share\repo) is not recognized by ParseSource for any install form today (isLocalPath only knows /, ~, ./, ../ and drive letters), so it fails the same way without --track; adding UNC parsing is a separate change to local-path handling. Network-share repositories work through the explicit file://server/share/repo form, which this PR leaves as parsed (TestParseSource_ExplicitUNCFileURLKeepsAuthority). Noted in the PR body.
…ctly makeRemote returns file:///C:/dir on Windows; trimming only file:// left /C:/dir, which filepath.Abs turned into D:\C:\dir, so the bare-repo test failed on the Windows runner. Drop the slash before the drive letter.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c613d7342
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func normalizeTrackSource(source *Source) error { | ||
| if source.Type == SourceTypeLocalPath && IsLocalGitRepo(source.Path) { | ||
| source.Type = SourceTypeGitHTTPS | ||
| source.CloneURL = localFileURL(source.Path) |
There was a problem hiding this comment.
When the local repository already occupies the computed tracked destination—for example, skills/_foo installed from its own path with --track --force—this URL points back to that same directory. The existing force path removes the destination before cloning, so it deletes the only clone source and the subsequent clone fails, potentially losing dirty or unpushed work. Compare the resolved source and destination before removal, or clone to staging before replacing it.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9585616. installTrackedRepoImpl now compares a local source with the destination using os.SameFile (symlinked paths included) and returns an error before anything is removed. Covered by TestInstallTrackedRepo_RefusesLocalSourceThatIsTheDestination, which failed before the change; I also ran it through the CLI with and without --force, via a symlink, and with --dry-run, and the destination stayed intact. The same hazard exists for an explicit file:///dest URL, which this PR does not touch.
| // normalizeTrackSource turns a local path that is a git repository into the | ||
| // file:// form that parseFileURL builds, and rejects other non-git sources. | ||
| func normalizeTrackSource(source *Source) error { | ||
| if source.Type == SourceTypeLocalPath && IsLocalGitRepo(source.Path) { |
There was a problem hiding this comment.
When project mode receives its own Git root as the local source (for example, skillshare install ./ --track -p), both cmdInstallProjectParsed and the dashboard handler call RejectProjectRootLocalInstall before InferTrackedKind, so this normalization is never reached and the request is rejected. The equivalent file:///project/root source works, and cloning rather than recursively copying avoids the hazard that guard addresses; make that guard track-aware or normalize the source before applying it.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. RejectProjectRootLocalInstall protects the copy-based local install, and installing a project's own repository as a tracked repo inside that project's .skillshare/skills is not a workflow this PR sets out to support; the explicit file:///project/root form still works. Relaxing a safety guard shared by the CLI and the dashboard for that case is a separate decision. Noted in the PR body.
…nation --force removes the tracked destination before cloning. When the local path given to --track is that same directory, the removal deleted the only clone source (and any unpushed work) before the clone failed. Compare the two with os.SameFile, which also covers symlinked paths, and stop before touching the destination.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 958561690d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _, err := os.Stat(destPath); err == nil { | ||
| if destInfo, err := os.Stat(destPath); err == nil { | ||
| // --force removes the destination before cloning; never do that to the clone source. | ||
| if srcInfo, srcErr := os.Stat(source.Path); source.Path != "" && srcErr == nil && os.SameFile(srcInfo, destInfo) { |
There was a problem hiding this comment.
When a local repository is anywhere below the computed destination—for example, source /skills/_foo/nested with --name foo --track --force—os.SameFile is false because the paths are not identical. The subsequent src.RemoveAll(destRel) recursively deletes the repository that source.CloneURL points to, so cloning fails after potentially destroying unpushed work. Check whether the source is contained within the destination, not only whether both paths identify the same directory. This is fresh evidence beyond the previously fixed equal-path case because a descendant survives the current guard.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6adeea6. The check in installTrackedRepoImpl now resolves the local clone source (a plain path or an explicit file:// URL) and the destination with EvalSymlinks, and refuses when the source is the destination or lies under it, before anything is removed and in dry runs too. Covered by TestInstallTrackedRepo_RefusesLocalSourceInsideTheDestination (same path, nested path, explicit file URL, dry run) and integration TestInstall_Track_Force_KeepsLocalSourceInsideDestination; the nested, file URL and dry-run cases failed before the change.
| // (a scratch dir) is not a skill. | ||
| func HasSkillFile(dir string) bool { | ||
| info, err := os.Stat(filepath.Join(dir, "SKILL.md")) | ||
| return err == nil && !info.IsDir() |
There was a problem hiding this comment.
When a target skill contains a FIFO named SKILL.md on Unix, os.Stat succeeds and !info.IsDir() treats it as a valid skill. Collection then reaches copyFile, whose open/read blocks waiting for a FIFO writer, hanging the CLI and the dashboard handler while it holds the server mutex. Require info.Mode().IsRegular() instead. This is distinct from the fixed directory case because the current predicate still admits non-regular special files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6adeea6. sync.HasSkillFile now requires info.Mode().IsRegular() (a symlinked SKILL.md still resolves through os.Stat); it is the only SKILL.md check this PR adds, and both FindLocalSkills and POST /api/collect use it. Covered by TestHasSkillFile_RejectsFifoSkillMd (Unix) and TestHasSkillFile_RejectsDirectoryNamedSkillMd; the FIFO case failed before the change.
…eject special SKILL.md files --track --force removed the destination before cloning, so a local source nested below it (or given as an explicit file:// URL) was deleted along with any unpushed work, and the clone then failed. Resolve the source and refuse when it is the destination or lies under it, for dry runs too. HasSkillFile used a not-a-directory test, so a FIFO named SKILL.md counted as a skill and the collect copy blocked on it. Require a regular file; a symlinked SKILL.md still resolves through os.Stat.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6adeea6409
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err != nil || u.Scheme != "file" || u.Host != "" { | ||
| return "" |
There was a problem hiding this comment.
When an explicit source such as file://localhost/<sourceDir>/_foo is installed with --name foo --track --force, Git treats the URL as local—I verified that git ls-remote file://localhost/workspace/skillshare HEAD succeeds—but this host check returns "", bypassing the containment guard. The force path then deletes _foo before trying to clone from that same directory, losing the source repository. This is fresh evidence beyond the covered empty-authority file:///... case: recognize the localhost authority (and supported UNC authorities where applicable) as a local filesystem source before removal.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f222847a. localCloneSource now treats a localhost authority (any case) like an empty one, so the containment guard also covers file://localhost/<dest>/.... Covered by the localhost file URL case of TestInstallTrackedRepo_RefusesLocalSourceInsideTheDestination, which failed before the change. A UNC authority (file://server/share) names a remote share and is left alone: it cannot be the local destination directory.
There was a problem hiding this comment.
Correction: the fix commit is e9ea9a8 (the sha in my previous reply was a local one that was rebased onto the main merge before pushing). Content is as described: localhost authority is treated as local; covered by the localhost file URL case, which failed before the change; make check passes on the merged tree.
There was a problem hiding this comment.
Fixed at the root in 7375400 instead of adding another URL form. --force now clones into a hidden sibling staging directory, runs the checks and the audit there, and only then moves the old repo aside, renames the new one in and removes the backup; a failure removes the staging copy and leaves the old repo. No alias of the source (localhost, UNC, symlink, short path, case) can be deleted before git reads it, and the ad-hoc url.Parse branch is gone. Covered by TestInstallTrackedRepo_ForceNeverDestroysItsLocalSource (file URL and localhost cases failed before), TestInstallTrackedRepo_ForceKeepsExistingRepoWhenCloneFails and TestInstallTrackedRepo_ForceReplacesExistingRepoInto.
|
@codex review |
Git clones file://localhost/path from the local filesystem, but the containment guard only recognised an empty authority, so --track --force could still delete a source given that way. Accept localhost (any case) as local.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9ea9a8e07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| u, err := url.Parse(source.CloneURL) | ||
| // file://localhost/path is the same local path as file:///path. | ||
| if err != nil || u.Scheme != "file" || (u.Host != "" && !strings.EqualFold(u.Host, "localhost")) { |
There was a problem hiding this comment.
On Windows, an explicit supported UNC source such as file://server/share/skills/_foo/nested is local filesystem data, but this condition classifies every non-localhost authority as remote. If the destination is \\server\share\skills\_foo and --force is used, the containment check is skipped and RemoveAll deletes the source repository before Git clones it. At head e9ea9a8, this remains fresh evidence beyond the fixed localhost case: convert UNC file authorities to their native path for the containment check before permitting removal.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Covered by the same change in 7375400: a UNC file://server/share/... source is no longer removed before the clone, because nothing is removed until the clone from it has finished. The plain-path refusal stays for a local path inside the destination.
On Windows the temp path starts with a drive letter, so prefixing it onto file://localhost produced file://localhostC:\... instead of a valid URL and the case failed on the Windows runner. Convert to slashes and add the separator explicitly.
… last --force removed the existing tracked repo before cloning, so any source that lived under it (plain path, file:///, file://localhost, UNC, symlink or 8.3 alias) was destroyed before git could read it, and a failed clone also lost the old repo. Clone beside the destination, run the checks and the audit on the staging copy, then move the old repo aside, rename the new one in and drop the backup; on any failure the staging copy is removed and the old repo stays. The pre-removal containment refusal now covers plain local paths only (the url.Parse special-casing is gone).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7375400e0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return nil, fmt.Errorf("failed to move the new clone into place: %w", err) | ||
| } | ||
| if err := src.RemoveAll(backup); err != nil { |
There was a problem hiding this comment.
When --force receives a file:///.../_foo/nested or UNC source beneath the destination, the staged clone succeeds, but renaming _foo to backup moves the original source into that backup and this recursive removal then deletes it. Because cloning does not preserve uncommitted or untracked work, this still causes the data loss discussed in the previous threads; the fresh evidence is that deletion now occurs after the clone rather than before it. The new test commits its only marker and checks only the replacement checkout, so it misses the loss of the original nested worktree. Refuse this containment case or preserve the nested source before removing the backup.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. --force replaces the whole destination tree on purpose, so anything inside it, including a repository nested under it, is overwritten just like the destination's own uncommitted files. What the staging change guarantees is that nothing is deleted before the clone has finished, so committed content of the source always survives in the new checkout and a failed clone loses nothing. A plain local path inside the destination is still refused up front. Making an explicit file:// URL under the destination a hard error would be one more spelling-by-spelling guard, which the previous threads showed does not converge.
There was a problem hiding this comment.
Fixed in c1e6c21. The containment refusal now maps file:///, file://localhost and UNC file:// sources to their local path (localCloneSource) and refuses a source that is, or lies inside, the destination before anything is cloned or moved, so the backup never holds the source. TestInstallTrackedRepo_ForceNeverDestroysItsLocalSource now leaves an uncommitted wip.txt in the source and requires every case (plain path, dry run, file URL, localhost URL) to be refused with it intact; TestLocalCloneSource_UNCFileURL covers the UNC mapping.
…nation The staged swap in 7375400 still lost data for a file:///, file://localhost or UNC source nested under the destination: the clone succeeded, the old repo (with the source inside it) was renamed to the backup, and removing the backup deleted the source's uncommitted work, which a clone cannot carry. Restore the file URL to local path mapping (now including UNC hosts) so the containment refusal covers every local spelling, not only plain paths. The test now keeps an uncommitted file in the source and requires it to survive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1e6c21f24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if p := localCloneSource(source); p != "" && pathWithin(destPath, p) { | ||
| return nil, fmt.Errorf("source %s is inside the install destination %s; nothing to install", p, destPath) | ||
| } |
There was a problem hiding this comment.
When a user runs skillshare install /path/to/_foo --track --update (or uses an equivalent file:// URL), the existing destination contains the source, so this check returns before the immediately following opts.Update branch. updateTrackedRepo pulls the existing checkout rather than replacing or removing it, so the data-loss rationale does not apply and a valid tracked update is rejected; apply the containment refusal only to the forced-replacement path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 98e833a. The containment refusal moved into the --force branch, after the --update and non-force returns, so an update pulls in place as before; a dry run with --force is still refused. Covered by TestInstallTrackedRepo_UpdateAcceptsSourceThatIsTheDestination, while TestInstallTrackedRepo_ForceNeverDestroysItsLocalSource keeps the forced cases.
…orce The refusal ran before the --update branch, so `install <tracked repo path> --track --update` was rejected even though an update pulls the existing checkout in place and removes nothing. Only the forced replacement can destroy a source that lives under the destination, so check there.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98e833ae83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
With --force the repo is discovered in the .skillshare-clone-<stamp> staging directory, and discoverSkills names a root SKILL.md after the directory, so the CLI summary, dashboard response and oplog listed a random staging name instead of _foo. Use the tracked name for the root entry.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c090983683
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defer func() { | ||
| if !installed { | ||
| _ = src.RemoveAll(cloneRel) | ||
| } |
There was a problem hiding this comment.
When two CLI processes concurrently install the same previously absent tracked name, both can pass the os.Stat(destPath) check, after which one clone claims destRel and the other clone fails because that directory now exists. The failing process then executes this unconditional deferred RemoveAll(cloneRel), deleting the checkout created by the successful process. Fresh installs should clone to a per-invocation staging path or otherwise clean up only after confirming that this invocation created the destination.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 890463a. Fresh installs now clone into the same per-invocation .skillshare-clone-<stamp> staging directory as --force and are renamed into place last, so the deferred cleanup only ever removes this invocation's staging copy; a destination another installer created meanwhile makes the rename fail and is left alone. Covered by TestInstallTrackedRepo_KeepsDestinationClaimedByAnotherInstaller, which creates the destination from the clone's progress callback and requires the install to fail with that checkout intact.
…r installer A fresh tracked install cloned straight into the destination, and the deferred cleanup removed it on any failure. If another process created the same tracked repo between the existence check and the clone, the failing install deleted that process's checkout. Clone every install into its own staging directory and rename it in last; a claimed destination makes the rename fail and only the staging copy is removed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 890463abaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if err := src.Rename(cloneRel, destRel); err != nil { | ||
| return nil, fmt.Errorf("failed to move the new clone into place: %w", err) |
There was a problem hiding this comment.
On Unix, if another process creates destRel as an empty directory after the initial os.Stat check but before this move, rename may replace that directory and this install reports success rather than respecting the concurrent claim; a process that is about to populate the directory can then fail or write into the unexpected checkout. Fresh evidence beyond the prior claimed-destination test is that the test makes the destination non-empty, while the empty-directory interval remains unprotected. Use a no-replace operation or an equivalent atomic destination reservation for this final move.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this one. An empty directory carries no data, so replacing it loses nothing. After 890463a every tracked install, fresh or --force, clones into its own staging directory and only renames at the end, so a concurrent skillshare install never creates the destination empty and then fills it; the slower process just fails its own rename and removes its own staging copy. The non-tracked path does the same (swapStagedIntoSource in internal/install/source_write.go uses a plain MoveIn/rename), and the repository has no no-replace rename (renameat2/renamex_np) to build on; adding per-OS syscalls for an external process that creates an empty _<name> in this window is out of scope for this PR.
#508 made --track --force clone first and replace the existing tracked repo only after the clone succeeds, and refuse a local source (path or file:// URL) that is, or lies inside, that tracked repo. Users relying on --force to recover a broken repo need to know the old one is kept on failure, and why a source under the destination is rejected.
The generated entry listed every commit, including about thirty fixes to target_naming: prefixed made before it was ever released, a test commit and an internal helper. Group what users will notice: the prefixed naming feature, the shared-folder conflict notices, and the fixes to released behavior (#498, #505, #506, #508, combined --mode/--target-naming).
1 participant
Type
proposals/only — see CONTRIBUTING.md)Linked Issue
No issue. Found by real-user simulations (S1 and S5 in the lifecycle run) while testing #497 prefixed naming; both are pre-existing.
What changed
collectonly collects skills.sync.HasSkillFilerequires a regularSKILL.md;FindLocalSkills(CLI global/project and the dashboard scan) andPOST /api/collectboth use it, so scratch folders (and a directory or FIFO namedSKILL.md) are skipped or rejected (400). Tests:TestFindLocalSkills_SkipsDirWithoutSkillMd,TestHasSkillFile_*,TestHandleCollect_RejectsFolderWithoutSkillMd.install <local git repo> --trackworks. A local path that is a working-tree or bare git repository root is normalized to afile://clone URL (localFileURL, escaped withnet/url, drive letters kept). The--branchguard accepts it. Any other local folder gets an error that namesfile://. Tests:TestInferTrackedKind_*,TestLocalFileURL_*,TestParseSource_ExplicitUNCFileURLKeepsAuthority,TestHandleInstall_TrackAcceptsLocalGitPath, integrationTestInstall_Track_LocalGitPath/_LocalBareRepoPath.--track --forcenever deletes before it has cloned. The clone goes to a hidden sibling staging directory, the checks and audit run there, then the old repo is moved aside, the new one renamed in and the backup removed. A failure keeps the old repo. A plain local source that is, or lies inside, the destination is refused up front. Tests:TestInstallTrackedRepo_ForceNeverDestroysItsLocalSource,_ForceKeepsExistingRepoWhenCloneFails,_ForceReplacesExistingRepoInto, integrationTestInstall_Track_Force_KeepsLocalSourceInsideDestination.install.md/collect.md(English, ja, ko, zh-Hans, zh-Hant) andskills/skillshare/references/install.mddescribe the behavior.Existing
FindLocalSkillstests that used empty directories now write aSKILL.md.Not done / notes
install . --track -pfrom the project's own repository root is still rejected byRejectProjectRootLocalInstall(a guard for copy installs);file:///project/rootworks.\\server\share\repo) are not recognized byParseSourcefor any install; usefile://server/share/repo.--force, a repository nested under the destination via an explicitfile://URL is replaced together with the destination (committed content survives in the new clone); only plain local paths are refused up front..skillshare-clone-*/.skillshare-*.old.*directory in the source, like the existingsourcefstemp directories.net/url(spaces,#,%) and bare repositories are accepted, after Codex rounds on this PR.mainthat was pushed into it during review.Checklist
make check) — for code changesChecks run
make checkin the devcontainer: passed on every pushed commit; CI (unit, integration on Ubuntu and macOS, Windows, docker sandbox, redteam) green on the head.python3 scripts/ai-context.py check: ok.--dry-run,--force,--json, spaces and%in paths, bare repos, symlinked paths and a failing--branch./simplify: first pass renamedrequireTrackableSourcetonormalizeTrackSourceand extracted a shared helper (later replaced); the staging pass moved cleanup into onedefer, updated audit-accept options in place and kept the old repo as a backup until the swap succeeds./code-review(standards + spec): first pass added the built-in skill reference, thePOST /api/collectcheck and the--branchguard fix; the staging pass found no standards violation and led to the backup/restore swap and its error text.