fix: batch bug fixes -- installer fallback, target registry, lockfile idempotency#456
Merged
danielmeppiel merged 5 commits intomainfrom Mar 25, 2026
Merged
fix: batch bug fixes -- installer fallback, target registry, lockfile idempotency#456danielmeppiel merged 5 commits intomainfrom
danielmeppiel merged 5 commits intomainfrom
Conversation
The `set -e` in install.sh caused the script to exit immediately when the binary test failed (e.g. glibc 2.36 in Debian Bookworm-based devcontainers vs glibc 2.39 required by the binary). This made the pip-fallback path (lines 398-445) dead code. Wrap the binary test in if/else so the exit code is captured without triggering set -e, allowing the existing fallback logic to execute. Verified with Docker (mcr.microsoft.com/devcontainers/typescript-node:24-bookworm): - Before: script dies silently at exit 255 - After: fallback detects glibc mismatch and offers pip install Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
) Replace hardcoded .github/.claude/.cursor copy blocks in skill_integrator with a single loop over active_targets() from targets.py. This: 1. Fixes #453: .github/ is no longer force-created when only .opencode/ or .cursor/ exists. The install guard now uses auto_create from the target profile (only copilot has auto_create=True). 2. Fixes #447: Skills and sub-skills now deploy to ALL active targets that support skills, including .opencode/ (previously missing). 3. Log messages now show actual deployment targets instead of hardcoded '.github/skills/'. The active_targets() function returns target profiles where detect_by_dir=False (always deploy) OR root_dir exists. Copilot (.github) is always active; others are presence-driven. Phase 1 of target registry consolidation -- only skills are migrated. Other integrators (agents, hooks, instructions) remain on the existing integrate_* flag system for now. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add LockFile.is_semantically_equivalent() that compares deps, mcp_servers, mcp_configs, and lockfile_version while ignoring generated_at and apm_version. Guard lockfile.save() in install.py with an equivalence check against the existing lockfile on disk. When content is unchanged, the write is skipped -- eliminating generated_at timestamp churn in version control. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Encoding rule: all source must stay within printable ASCII (U+0020-U+007E) to prevent charmap codec errors on Windows cp1252 terminals. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Batch of fixes across installer, skill target integration, and lockfile writing to address several reported bugs and reduce churn during repeated installs.
Changes:
- Prevent
set -efrom aborting the Unix installer before reaching the pip fallback when the downloaded binary cannot run (e.g., glibc mismatch). - Deploy skills to every active target (via
active_targets()), including.opencode/skills/, and improve install-time logging to reflect actual targets. - Add lockfile semantic equivalence comparison and skip rewriting
apm.lock.yamlwhen only timestamps/version metadata changed; update changelog and tests; fix one ASCII-only docstring character.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
install.sh |
Wraps binary --version test in if/else so the fallback path remains reachable under set -e. |
src/apm_cli/integration/skill_integrator.py |
Replaces hardcoded target copies with a loop over active_targets() for skill and sub-skill promotion. |
src/apm_cli/commands/install.py |
Logs skill deployment targets from actual deployed paths; adds lockfile skip-write guard; changes target auto-create logic. |
src/apm_cli/deps/lockfile.py |
Adds LockFile.is_semantically_equivalent() for idempotent lockfile writes. |
tests/test_lockfile.py |
Adds unit tests for semantic equivalence behavior. |
src/apm_cli/utils/path_security.py |
Replaces an em dash in a docstring with ASCII --. |
CHANGELOG.md |
Adds Unreleased entries for the fixes. |
Comments suppressed due to low confidence (1)
src/apm_cli/commands/install.py:1315
- Non-ASCII em dash in this comment violates the repository ASCII-only source rule. Replace it with ASCII "--".
# Ensure auto_create targets exist.
# Copilot (.github) has auto_create=True — it is always created so
# there is a guaranteed skills root even for greenfield projects.
from apm_cli.integration.targets import active_targets as _active_targets
src/apm_cli/commands/install.py
Outdated
Comment on lines
+878
to
+882
| _skill_targets = sorted({ | ||
| tp.relative_to(project_root).parts[0] | ||
| for tp in skill_result.target_paths | ||
| if tp.relative_to(project_root).parts | ||
| }) |
There was a problem hiding this comment.
This comprehension calls tp.relative_to(project_root) multiple times per path; storing the relative path once would avoid redundant work and avoid double-evaluating an expression that can raise ValueError if a path is unexpectedly outside project_root.
Suggested change
| _skill_targets = sorted({ | |
| tp.relative_to(project_root).parts[0] | |
| for tp in skill_result.target_paths | |
| if tp.relative_to(project_root).parts | |
| }) | |
| _skill_target_dirs = set() | |
| for tp in skill_result.target_paths: | |
| rel = tp.relative_to(project_root) | |
| if rel.parts: | |
| _skill_target_dirs.add(rel.parts[0]) | |
| _skill_targets = sorted(_skill_target_dirs) |
Comment on lines
+886
to
+889
| _log_integration(f" |-- Skill integrated -> {_skill_target_str}") | ||
| if skill_result.sub_skills_promoted > 0: | ||
| result["sub_skills"] += skill_result.sub_skills_promoted | ||
| _log_integration(f" └─ {skill_result.sub_skills_promoted} skill(s) integrated -> .github/skills/") | ||
| _log_integration(f" |-- {skill_result.sub_skills_promoted} skill(s) integrated -> {_skill_target_str}") |
There was a problem hiding this comment.
These skill integration tree lines use a different ASCII prefix ("|--") than the rest of the install integration output in this function (which uses "└─"). This makes the output formatting inconsistent; consider standardizing on one prefix (preferably ASCII everywhere if following the encoding rule) and updating all related tree_item call sites accordingly.
Comment on lines
+750
to
+759
| sub_skills_count = sum( | ||
| 1 for p in all_target_paths | ||
| if p.parent.parent.name == "skills" and p.name != skill_name | ||
| ) // max(len(targets), 1) | ||
| # Approximate: count unique sub-skills from primary target only | ||
| primary_root = project_root / ".github" / "skills" | ||
| sub_skills_count = sum( | ||
| 1 for p in all_target_paths | ||
| if p.parent == primary_root and p.name != skill_name | ||
| ) |
There was a problem hiding this comment.
sub_skills_count is computed twice in a row, and the first computation is immediately overwritten. Remove the dead computation (or restructure to compute the intended value once) to avoid confusion about which behavior is correct.
CHANGELOG.md
Outdated
Comment on lines
+13
to
+16
| - `install.sh` now falls back to pip when binary fails in devcontainers with older glibc (#451) | ||
| - Skills now deploy to all active targets (`.opencode/`, `.cursor/`) instead of only `.github/` (#453, #447) | ||
| - `.github/` no longer force-created when only `.opencode/` or `.cursor/` exists (#453) | ||
| - `apm install` no longer rewrites `apm.lock.yaml` when dependencies are unchanged, eliminating `generated_at` churn in version control (#450) |
There was a problem hiding this comment.
Comment on lines
+1312
to
+1326
| # Ensure auto_create targets exist. | ||
| # Copilot (.github) has auto_create=True — it is always created so | ||
| # there is a guaranteed skills root even for greenfield projects. | ||
| from apm_cli.integration.targets import active_targets as _active_targets | ||
|
|
||
| _targets = _active_targets(project_root) | ||
| for _t in _targets: | ||
| if _t.auto_create: | ||
| _target_dir = project_root / _t.root_dir | ||
| if not _target_dir.exists(): | ||
| _target_dir.mkdir(parents=True, exist_ok=True) | ||
| if logger: | ||
| logger.verbose_detail( | ||
| f"Created {_t.root_dir}/ ({_t.name} target)" | ||
| ) |
There was a problem hiding this comment.
The new auto-create loop runs before target detection and uses active_targets(), which always includes the Copilot profile (detect_by_dir=False, auto_create=True). This means .github/ will still be created even when the repo only has .opencode/ or .cursor/, so it does not actually address the reported behavior in #453. Consider moving auto-create to after detect_target()/should_integrate_* and only creating root dirs for targets that will be integrated (e.g., only create .github/ when integrate_vscode is true).
This issue also appears on line 1312 of the same file.
- Remove dead sub_skills_count computation (overwritten immediately) - Replace double relative_to() call with single-pass loop - Fix em dash in install.py comment (ASCII encoding rule) - CHANGELOG: use PR number (#456) per convention, remove misleading '.github/ no longer force-created' entry (auto_create is by design) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Mar 26, 2026
Merged
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.
Summary
Consolidated batch of bug fixes from triage of open issues.
Fixes
Installer fallback in devcontainers (Closes [BUG] installer fails inside a devcontainer #451)
install.shline 377 binary test now wrapped inif/elsesoset -edoes not kill the pip-fallback path when glibc is too old (e.g. Debian Bookworm devcontainers with glibc 2.36 vs binary built on Ubuntu 24.04 with glibc 2.39).Skills deploy to all active targets (Closes [BUG] always installing to .github #453, Closes [BUG] Neither
apm installnorapm compileadd skills from third-party transitive dependencies #447).github/.claude/.cursorcopy blocks inskill_integrator.pywith a single loop overactive_targets()fromtargets.py..opencode/skills/now works.install.pynow usesauto_createfrom the target profile instead of an ad-hoc.github/.claudeexistence check..github/is no longer force-created when only.opencode/exists..github/skills/.Lockfile idempotent write (Closes [BUG] apm.lock.yaml changes on every apm install due to generated_at timestamp #450)
LockFile.is_semantically_equivalent()that compares deps, mcp_servers, mcp_configs, and lockfile_version while ignoringgenerated_atandapm_version.lockfile.save()ininstall.pyto skip write when content is unchanged -- eliminatesgenerated_attimestamp churn in version control.ASCII encoding fix
path_security.pydocstring with ASCII--per encoding rules.Testing
mcr.microsoft.com/devcontainers/typescript-node:24-bookworm)LockFile.is_semantically_equivalent()Not included (separate PRs)
copilot/fix-path-unpacking-issue