Replaced external.dir with declared packs referenced by name (0.10.0) - #13
Closed
dzykovic wants to merge 2 commits into
Closed
Replaced external.dir with declared packs referenced by name (0.10.0)#13dzykovic wants to merge 2 commits into
dzykovic wants to merge 2 commits into
Conversation
…ased it as 0.10.0
…s and a moved upstream
There was a problem hiding this comment.
Pull request overview
This PR introduces a new packs: configuration model for remote sources: remote repos are declared once (url/ref/mirror) and referenced from sources.* by @<pack>[/<subpath>], replacing the prior external.dir + duplicated inline git+… entries. It also adds a breaking-change migration (migrate_to_0_10_0), updates docs/examples/CI, and bumps the engine version to 0.10.0.
Changes:
- Add first-class pack resolution (
@pack) with up-front validation and optional per-pack mirroring viapacks.<name>.mirror. - Add
migrate_to_0_10_0to convert 0.9.0-style inlinegit+…sources andexternal.dirinto declaredpacks:. - Update documentation, examples, CI coverage, and versioning to reflect the new packs model.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates README wording/examples from inline git+…/external: to declared packs:. |
| intelligence/sync/skills/intelligence-review-skills/SKILL.md | Updates review skill guidance to treat @<pack> / packs: as the remote-source mechanism. |
| intelligence/sync/scripts/VERSION | Bumps engine version from 0.9.0 to 0.10.0. |
| intelligence/sync/scripts/sync.sh | Exports config path for pack resolution, validates pack refs upfront, and logs pack mirrors. |
| intelligence/sync/scripts/lib/migrations.sh | Adds breaking migration migrate_to_0_10_0 to rewrite configs to packs: + @pack. |
| intelligence/sync/scripts/lib/common.sh | Implements pack token detection/resolution, mirroring mechanics, YAML nested value fix, and new pack validation helpers. |
| intelligence/sync/INIT.md | Updates bootstrap template sync_version to 0.10.0. |
| intelligence/sync/docs/CONVENTIONS.md | Documents packs declaration/reference format and mirroring semantics. |
| intelligence/sync/docs/ADAPTERS.md | Updates adapter API docs for @pack and source_is_local_path usage. |
| examples/with-remote-skills/config.yaml | Converts example to use packs: and @shared-intel/... references. |
| examples/platform-with-submodules/config.yaml | Updates example sync_version to 0.10.0. |
| examples/go-api/config.yaml | Updates example sync_version to 0.10.0. |
| examples/go-api-with-pi-and-codex/config.yaml | Updates example sync_version to 0.10.0. |
| examples/go-api-with-opencode/config.yaml | Updates example sync_version to 0.10.0. |
| examples/dotnet-api-with-react-frontend/config.yaml | Updates example sync_version to 0.10.0. |
| docs/CONVENTIONS.md | Mirrors the engine conventions doc updates for packs. |
| docs/ADAPTERS.md | Mirrors the engine adapters doc updates for packs. |
| CHANGELOG.md | Adds 0.10.0 release notes including breaking change + fixes. |
| .github/workflows/ci.yml | Renames/expands CI coverage to validate packs declaration/mirroring/migration behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1145
to
+1151
| # `targets:` accepts the flow form, so a user reasonably writes | ||
| # `packs:\n shared: { url: … }` — which reads as zero declared packs and | ||
| # makes the message above point at a typo that is not there. | ||
| if grep -qE '^[[:space:]]+[A-Za-z0-9._-]+:[[:space:]]*\{' "$config_file"; then | ||
| echo " Note: a pack must be declared in block form — 'name:' on its own line," >&2 | ||
| echo " then indented 'url:' / 'ref:' / 'mirror:'. The '{ … }' form is not read here." >&2 | ||
| fi |
Comment on lines
+236
to
+246
| resolve_pack_source() { | ||
| local repo_root="$1" config_file="$2" token="$3" | ||
|
|
||
| local rest="${token#@}" name subpath="" | ||
| case "$rest" in | ||
| */*) name="${rest%%/*}"; subpath="${rest#*/}" ;; | ||
| *) name="$rest" ;; | ||
| esac | ||
|
|
||
| local url ref mirror_rel mirror_abs="" | ||
| url="$(get_pack_field "$config_file" "$name" "url")" |
Contributor
Author
|
Superseded by #14 — same commits, branch renamed to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A remote source is now declared once as a pack and referenced by name, so its url and its pin live in exactly one place.
Why
0.9.0 put the mirror location in one global
external:block while the identity of each pack — url and ref — stayed duplicated inside everysources.*entry that used it. A pack spanning rules, agents and skills carried itsurl@refthree times, and nothing detected the drift when only two of them were bumped: rules pinned at one commit and skills at another is a config that looks fine and reads wrong.packs:inverts it. Each pack is declared once (url, optionalref, optionalmirror) and referenced as@<pack>[/<subpath>].Breaking
external: { dir: … }is replaced bypacks:;migrate_to_0_10_0rewrites the config automatically, preserving comments. Post-condition: noexternal:key inconfig.yaml, and apacks:block declaring every remote previously reached inline. Inlinegit+specs remain legal as anonymous packs — no name, no mirror, always transient.Notable
pack-<key>fallback, no collision suffix.@packreference fails the run (exit 1, naming the pack), deliberately unlike a missing local path, which only warns. The check runs up front invalidate_pack_refs, becauseresolve_source_diris always called inside$( ).get_nested_yaml_valuecut values at the last colon, not the first —url: https://host/repo.gityielded//host/repo.git. Latent in 0.9.0 (nothing read a URL through it), but it also truncated anymodels.<ide>.<tier>value containing a colon.Review fixes in the second commit
Self-review and
/code-reviewfound seven defects in the first commit; all are fixed and covered:packs:key on re-run. Inline specs stay legal, so the precondition fired again on any config that had one —run_migrationsruns the whole chain on everyupdate.sh, so adding one inline spec to a migrated config injected duplicate-key YAML.external:andpacks:are now treated as one-way markers, and the staged-file verify asserts the exactpacks:key count.git+<url>#subpathwith no@reflost its subpath. The three fields were passed through a tab-delimited string, andreadcollapses a run of IFS-whitespace — an unpinned spec came back asref=<subpath>, rewriting a whole source into a branch that does not exist. Now passed as globals..packurl, so editingpacks.<n>.urlleft the old content committed forever while the generated output silently followed the new repo — defeating the reviewability the mirror exists for. Ownership is now the presence of the stamp; a stamped directory refreshes, an unstamped one is still never touched.sync.shfallback cache root, rebuilding a mirror with no.packand freezing it against the guard above. Claims now carry$$.packs:was parsed in block form only, with a misdirecting error —targets:accepts the flow form, sopacks:\n shared: { url: … }reported a typo that was not there. The error now says block form is required.\|(a GNU BRE extension BSD/macOS grep reads as a literal) and missed single-quoted specs.docs/ADAPTERS.mdstill documentedsource_is_remoteas the way a custom adapter tells remote from local; a@packtoken is neither, so an existing adapter would have built$repo_root/@shared-intel/rules.Verification
Both CI jobs were run locally end-to-end (Git Bash) before pushing.
packs(rewritten): declaration, mirroring, idempotency, the refresh diff, the moved-upstream refresh, the never-clear guard, the unsafe-mirror refusal, the undeclared-pack failure and the transient path, against afile://pack repo.migration: a 0.9.0-shaped config goes throughupdate.shand every breaking post-condition is asserted, including the unpinned-spec case and re-run idempotency.migrate_to_0_10_0had no CI coverage in the first commit.