Replaced external.dir with declared packs referenced by name (0.10.0) - #14
Merged
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 declared packs (packs:) as the canonical way to define remote sources once (url/ref/mirror) and reference them by name (@<pack>[/<subpath>]), replacing the prior external.dir mechanism and removing per-section duplication of git+<url>@<ref>#<subpath>.
Changes:
- Added
packs:+@packreferences (and upfront validation) to make remote pins single-sourced and fail closed on undeclared pack names. - Implemented
migrate_to_0_10_0to rewrite 0.9.0-style inlinegit+sources /external.dirinto declared packs + mirrors, and bumped engine version to 0.10.0. - Updated documentation, examples, and CI workflows to reflect packs + mirroring behavior and new migration post-conditions.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates wording to describe declared packs: instead of inline git+/external:. |
| intelligence/sync/skills/intelligence-review-skills/SKILL.md | Updates review guidance to recognize @<pack> / packs: as 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 up-front, and lists declared mirrors. |
| intelligence/sync/scripts/lib/migrations.sh | Adds migrate_to_0_10_0 to convert external: + inline git+ sources to declared packs. |
| intelligence/sync/scripts/lib/common.sh | Adds pack-aware source resolution (@pack), mirror materialization, pack field readers, and upfront validation helpers. |
| intelligence/sync/INIT.md | Updates bootstrap template sync_version to 0.10.0. |
| intelligence/sync/docs/CONVENTIONS.md | Documents packs, references, mirroring semantics, and inline git+ as anonymous packs. |
| intelligence/sync/docs/ADAPTERS.md | Updates adapter contract docs for resolve_source_dir to include packs and new helper predicates. |
| docs/CONVENTIONS.md | Mirrors pack documentation updates in the repo-root docs. |
| docs/ADAPTERS.md | Mirrors adapter contract documentation updates in the repo-root docs. |
| examples/with-remote-skills/config.yaml | Updates example to declare a pack + reference it via @shared-intel/... and mirror it. |
| examples/platform-with-submodules/config.yaml | Bumps example sync_version to 0.10.0. |
| examples/go-api/config.yaml | Bumps example sync_version to 0.10.0. |
| examples/go-api-with-pi-and-codex/config.yaml | Bumps example sync_version to 0.10.0. |
| examples/go-api-with-opencode/config.yaml | Bumps example sync_version to 0.10.0. |
| examples/dotnet-api-with-react-frontend/config.yaml | Bumps example sync_version to 0.10.0. |
| CHANGELOG.md | Adds 0.10.0 release notes (breaking change + fixes) documenting packs and migration behavior. |
| .github/workflows/ci.yml | Renames/rewrites CI job to exercise packs, moved-upstream refresh, undeclared pack failure, and migration coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
783
to
787
| @@ -716,8 +786,16 @@ | |||
| in_section && in_sub && /^ [a-zA-Z]/ { in_sub=0 } | |||
| in_section && in_sub && $0 ~ "^ " key ":" { | |||
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
+1155
to
+1163
| # Validate every declared mirror once, before a single clone runs, so an | ||
| # unsafe path fails the run rather than being discovered mid-materialization. | ||
| local rel | ||
| while IFS= read -r name; do | ||
| [ -z "$name" ] && continue | ||
| rel="$(get_pack_field "$config_file" "$name" "mirror")" | ||
| [ -n "$rel" ] || continue | ||
| resolve_mirror_dir "$repo_root" "$config_file" "$name" "$rel" >/dev/null | ||
| done < <(read_yaml_keys "$config_file" "packs") |
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.