feat(project): support a visible skillshare/ project directory (#256) - #258
feat(project): support a visible skillshare/ project directory (#256)#258salmonumbrella wants to merge 2 commits into
skillshare/ project directory (#256)#258Conversation
![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: 9d352283f4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if err != nil || !info.IsDir() { | ||
| continue | ||
| } | ||
| if name == Default || hasResourceDir(dir) { |
There was a problem hiding this comment.
When a repo was initialized with skillshare init -p --visible --config local before any skills or agents were committed, a fresh clone only has the tracked skillshare/.gitignore while config.yaml is intentionally ignored and Git drops the empty skills/ and agents/ directories. This guard then rejects the visible directory as a partial project, so ensureProjectConfig falls through to a default performProjectInit and creates a new hidden .skillshare/config.yaml instead of repairing skillshare/config.yaml. Treat the visible directory's gitignored config marker as a partial project as well as resource dirs.
Useful? React with 👍 / 👎.
|
Thanks for taking the time to investigate this and for putting together such a detailed implementation and test coverage. I think the missing-config protection is valuable on its own. Preventing an existing project with skills or agents from silently creating a new empty config—and losing its configured targets—is an important fix. The intentional auto-init behavior for new projects and Could you split just this fix into a separate, smaller PR without the visible |
Every project-mode command auto-initializes when .skillshare/config.yaml is missing. That is right for a repository with no project directory at all, and for the shared skills repo produced by 'init -p --config local', which gitignores config.yaml so each developer regenerates their own. It was wrong for the case in between. A .skillshare/ that already held skills or agents, whose config.yaml was simply gone, was re-initialized into an empty config. Every configured target was dropped, the target symlinks were left behind pointing at a project the config no longer described, and the command exited 0. Nothing said anything had happened. Add ensureProjectConfig, which replaces the open-coded auto-init at every project command entry point. It initializes as before when there is no project directory, and when config.yaml is gitignored, so both intentional cases keep working untouched. When the directory holds skills or agents and its config is neither present nor gitignored, it now returns an error pointing at version control or an explicit 'skillshare init -p' instead of writing over the project. The gitignore check is treated as inconclusive on error, so a repository that cannot be inspected is guarded rather than overwritten.
Project mode is identified only by .skillshare/config.yaml, and trash, backups, logs and audit-rules.yaml anchor to that same hidden directory. The sources option (runkids#153) made project content relocatable, but the marker and operational state stay hidden, so repositories that treat skills as reviewable content still carry a hidden directory whose only job is to hold config.yaml. Resolve the project directory through a new leaf package that checks .skillshare/config.yaml first and skillshare/config.yaml second, and derive every config, state, source-default and gitignore path from the result. The hidden name wins whenever both exist, so an existing project never moves and no migration is needed. The package imports nothing else internal because internal/audit cannot import internal/config without a cycle. Add init -p --visible to create the visible layout. Default init -p behaviour and output are unchanged. Two ambiguities the visible name introduces: - The global config directory is also called skillshare (<config-home>/skillshare), so the two places that infer scope from a config path now exclude the global location by full path rather than by directory name. TestLogDir_Global asserted a fixture path without declaring a config home, which hid this; it now sets XDG_CONFIG_HOME. - An unrelated directory named skillshare/ must not be adopted as a partially initialized project, so the visible name is only claimed when it carries a marker of its own: a skills/ or agents/ directory, or a .gitignore that ignores config.yaml. The missing-config guard added in the previous commit is generalized to resolve through the new package, so it covers both layouts. The gitignored-config marker matters there too. A repository shared by 'init -p --visible --config local' before any skill was committed clones with nothing but that .gitignore, because git drops the empty skills/ and agents/ directories; recognizing it keeps the repair pointed at the visible directory instead of writing a hidden one beside it. Refs runkids#256
|
Done — the guard is now its own PR: #260, with none of the visible-directory changes. It is the literal- I have force-pushed this branch to match. It is now two commits:
One functional addition since the last push, addressing the Codex P2 finding above: a repository shared with |
9d35228 to
fd068e5
Compare
2 participants
Type
proposals/only — see CONTRIBUTING.md)Ticking the closest box, but being straight about it: this is the implementation of that proposal, not a
proposals/-only PR. I opened #257 as the proposal doc first, then converted to this after re-reading CONTRIBUTING's note that an implementation PR still "serves as a concrete reference that shapes the final implementation". #257 is closed in favour of this one, and its content is folded into the sections below. If you would rather have the proposal document back and write the implementation yourself, say so and I will reopen it — no hard feelings either way.Linked Issue
Closes #256
Problem
Project mode is identified solely by
.skillshare/config.yaml, and the project's operational state —trash/,backups/,logs/,audit-rules.yaml— anchors to that same hidden directory.sources(#153 / #162) solved this for content, on the accepted premise that some repositories want skills "reviewed, discovered, and maintained alongside normal contributor documentation instead of being tied to a tool-specific hidden directory". The marker andconfig.yamlwere left behind, so those repositories end up with a visible skills directory plus a hidden directory whose only remaining job is to hold the config — the file that lists sync targets and the audit threshold, and arguably the one reviewers most need to see.There is no flag, environment variable or config key to relocate it today, and a config key could not work: the marker has to be found before any config is parsed.
Approach
A new leaf package
internal/projectdirresolves the project directory by checking.skillshare/config.yamlfirst andskillshare/config.yamlsecond. Config, source defaults, gitignore management and all operational state derive from that result instead of joining a literal.The package has no internal dependencies on purpose:
internal/auditneeds it, andauditcannot importinternal/configbecauseconfig→install→auditwould cycle.skillshare init -p --visiblecreates the visible layout. Defaultinit -pis unchanged.Two collisions the visible name introduces
The global config directory is also called
skillshare.BaseDir()is<config-home>/skillshare, so the two places that inferred project-vs-global scope from a directory name (oplog.LogDir/ensureProjectLogGitignore, andisProjectLogConfig) would have classified the global config as a project and written global operation logs into the config directory instead of the state directory. Both now match on the name and then exclude the global location by full path.TestLogDir_Globalpassed a/home/user/.config/skillshare/...fixture without declaring a config home, which hid this; it now setsXDG_CONFIG_HOMEso the assertion means what it claims.An unrelated
skillshare/directory must not be adopted. The partial-init repair path treats a project directory without aconfig.yamlas one to repair. The hidden name is unambiguous; the visible name is only claimed when it carries a marker of its own: askills/oragents/directory, or — after the Codex finding below — a.gitignorethat ignoresconfig.yaml, since a shared repo created withinit -p --visible --config localbefore any skill was committed clones with nothing but that.gitignore(git drops the empty directories). The gitignore check lives inprojectdiritself so the package keeps its zero-internal-imports invariant.ProjectTargetDotDirsis deliberately untouched — it documents itself as a set of hidden directory names to skip during discovery, and adding a non-hiddenskillsharewould skip legitimate content in any scanned repository.Back-compat
.skillshare/behaves identically; the second candidate is never reached..skillshare/, so adding a visible directory can never move an existing project.sourcessemantics are unchanged: it still wins for content and still resolves from the project root, not from the marker.mv .skillshare skillshare.The missing-config guard
Split out into #260 at the maintainer's request. This branch is now stacked on that commit: the first commit here is #260 unchanged, so this PR collapses to a one-commit diff once the fix merges — until then, the feature is the second commit (
git diff d353554d..HEAD). The feature commit only generalizes the guard, routing its resolution throughinternal/projectdirso it covers both layouts.Test coverage
Written test-first for the guard (red on all five references to the missing helper, then green).
Covered:
internal/projectdir— resolution order for hidden-only, visible-only, both-present and neither;config.yamlpresent as a directory; the marker requirement before a visible directory is claimed (skills//agents/, the gitignored-config.gitignore, and rejection of an unrelated.gitignore);Names()not aliasing package state.internal/config— config path resolution and.skillshareprecedence,LoadProject, all three source defaults,sourcesstill winning,SaveIn, and the gitignore target.internal/trash,internal/backup— state directories follow the active marker.internal/oplog— global, hidden-project and visible-project log directories, including the name collision.cmd/skillshare—--visibleparsing;init -p --visiblecreating only the visible layout; defaultinit -pcreating only the hidden one; re-init of a visible project reporting already-initialized; the guard in all four states (initialized, no project, gitignored-config shared repo, content-bearing project without config); a fresh clone of a visible shared repo repairing intoskillshare/rather than creating.skillshare/; and a command-level regression test thatstatus -prefuses rather than overwriting.Not covered, and why: the web UI has no test for the one handler path that changed (
handleOverview's extras source), matching the existing test layout for that handler; and I did not add integration tests undertests/integration, since the existing ones drive the global config and I did not want to reshape that fixture setup in this PR.gofmt,go vet ./...andgo build ./...are clean.go test ./...shows no failures beyond two that already fail on unmodifiedmainin my environment (TestCommitSourceFiles_CommitFailureIsReturnedandTestGitRootMismatch, both dependent on host git config and the real~/.config/skillshare); I verified this by diffing failures against a cleanupstream/mainworktree rather than assuming. I could not runmake checkin the devcontainer because Docker was unavailable to me, so that step is unverified on my side.CHANGELOG.mdis untouched — those entries look maintainer-authored per release with version headers.Checklist