Skip to content

fix(deps): repair dangling symlinks in sparse-cone subdir installs - #2710

Merged
Daniel Meppiel (danielmeppiel) merged 9 commits into
microsoft:mainfrom
MohammedAlkindi:fix/sparse-cone-dangling-symlinks
Aug 30, 2026
Merged

fix(deps): repair dangling symlinks in sparse-cone subdir installs#2710
Daniel Meppiel (danielmeppiel) merged 9 commits into
microsoft:mainfrom
MohammedAlkindi:fix/sparse-cone-dangling-symlinks

Conversation

@MohammedAlkindi

@MohammedAlkindi Mohammed Alkindi (MohammedAlkindi) commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Description

apm install of a git subdirectory dependency fails with FileNotFoundError when the subdir contains a symlink pointing outside it: the #1436 sparse-cone checkout excludes the target, leaving the link dangling, and the copy step dereferences it. Regression from #1433/#1436.

Fix, per the issue's suggested option 2: after each cone checkout or sparse cache hit, repair_dangling_cone_symlinks() queries the pinned HEAD tree with literal pathspecs for tracked symlink entries; if a tracked in-repository target is missing it runs git sparse-checkout disable, repopulating the full tree so the target resolves. Wired into all three materialization paths and persistent cache-hit reuse. Trade-off stated plainly: a dependency whose payload is mostly symlinks into the repo root loses the #1433 sparse win on every install; cone-widening (the issue's option 1) would preserve it and can supersede this if preferred. Clean installs pay one Git index query and filesystem checks only for tracked symlinks. In a partial clone the disable fetches missing blobs from the remote at repair time.

Scope: applies where git materializes real symlinks (core.symlinks=true). On Windows-default core.symlinks=false, git checks these entries out as plain files and no FileNotFoundError occurs; that separate behavior is untouched. Remote Git links are constrained to tracked files in the pinned checkout; generated Git metadata, external targets, and intrinsically broken links fail before copy.

Fixes #2707

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added tests for new functionality (if applicable)

New: tests/unit/utils/test_git_sparse.py plus dangling-symlink cases extending the existing #1436 suites (test_bare_cache_sparse.py, test_git_cache_sparse.py): 31/31 pass; the headline test fails on unmodified source (cone stays narrow). Real symlink creation needs privileges Windows dev boxes lack (WinError 1314), so tests simulate the dangling state via monkeypatched islink/exists, documented inline; the underlying git behavior was verified directly with a plumbing-inserted mode-120000 entry driven through apply_sparse_cone and a real git sparse-checkout disable. Full unit suite: no new failures vs upstream/main (pre-existing Windows symlink-privilege failures reproduce identically on a clean checkout). ruff check / ruff format --check clean on touched files. Not run: POSIX-native end-to-end (no POSIX box here); promisor-clone repair path has no dedicated test.

Spec conformance (OpenAPM v0.1)

  • N/A -- this PR does not change OpenAPM-observable behaviour.
    (Mode B detector run on the diff: only deps/bare_cache.py is on a
    critical path, 14 substantive lines, under the 20-line threshold.)

Scenario Evidence

# Scenario (user promise) Principle(s) Test(s) proving it Type
1 A Git subdirectory package with an in-repository symlink outside its sparse cone installs with a live target. Portability, DevX tests/integration/test_sparse_cone_symlink_repair.py::test_legacy_downloader_repairs_real_out_of_cone_symlink integration
2 A remote package cannot copy generated Git metadata or a target outside its pinned checkout. Secure by default tests/integration/test_sparse_cone_symlink_repair.py::test_downloader_rejects_real_symlink_into_git_metadata, tests/integration/test_sparse_cone_symlink_repair.py::test_materialization_rejects_symlink_outside_checkout integration
3 A package with an intrinsically broken link fails with an actionable error after widening. DevX tests/integration/test_sparse_cone_symlink_repair.py::test_repair_rejects_link_that_remains_broken integration
4 A colon-prefixed package path cannot invoke Git pathspec magic to hide an unsafe link. Secure by default tests/integration/test_sparse_cone_symlink_repair.py::test_downloader_treats_colon_prefixed_package_path_literally integration

The first row is the regression trap for #2707. Removing the no-cache repair call makes it fail with FileNotFoundError; the matching architecture-owner assertion also fails.

apm-spec-waiver: Internal sparse-checkout repair does not change the OpenAPM manifest contract.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes apm install failures for git subdirectory dependencies when a sparse-cone checkout leaves symlinks dangling (because their targets live outside the cone). It introduces a post-checkout repair step that detects missing symlink targets under the requested cone paths and falls back to a full working tree via git sparse-checkout disable.

Changes:

  • Add repair_dangling_cone_symlinks() (and _find_dangling_symlink()) to detect dangling symlinks under sparse-cone paths and widen the checkout to full-tree when needed.
  • Wire the repair step into both sparse-cone materialization call sites (bare_cache.materialize_from_bare and GitCache._create_checkout), with logging when repair triggers.
  • Add unit coverage for the detection helper and extend existing sparse-cone test suites with dangling-symlink scenarios; add a changelog entry.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/apm_cli/utils/git_sparse.py Adds dangling-symlink detection and the repair helper that disables sparse-checkout when needed.
src/apm_cli/deps/bare_cache.py Runs the repair step after sparse checkout materialization from the shared bare cache.
src/apm_cli/cache/git_cache.py Runs the repair step after sparse checkout materialization in the persistent GitCache path (with hardened git args).
tests/unit/utils/test_git_sparse.py New unit tests for the helper functions and the repair behavior.
tests/unit/deps/test_bare_cache_sparse.py Adds a regression test ensuring materialize_from_bare repairs a dangling symlink scenario.
tests/unit/cache/test_git_cache_sparse.py Adds regression tests ensuring GitCache.get_checkout repairs (or does not repair) as appropriate.
CHANGELOG.md Documents the fix under Unreleased.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread CHANGELOG.md Outdated
Comment on lines +12 to +17
- Sparse-cone subdirectory installs no longer leave dangling symlinks when a
dependency's payload contains a symlink pointing outside the requested
subdirectory; the checkout now widens to a full tree so the target
resolves. Applies where git materializes real symlinks (`core.symlinks=true`;
on Windows git defaults to `false` and checks these entries out as plain
files, a separate behavior #2707 does not cover). (#2707)
@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: needs_rework

Sparse-cone symlink repair addresses a real install failure, but a legacy downloader path still bypasses the fix and lacks end-to-end POSIX fixture evidence.

cc Mohammed Alkindi (@MohammedAlkindi) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The panel supports the repair strategy, and the 31 passing targeted tests provide useful confidence in the changed units. However, the Python Architect identified a correctness gap: GitHubPackageDownloader._try_sparse_checkout still bypasses the canonical repair policy, leaving ScriptRunner capable of reproducing the exact failure this PR claims to fix. The absence of a real POSIX symlink integration fixture means the complete user-facing install promise remains unproven.

The remaining signals reinforce safe completion rather than challenge the design: followed targets should stay inside repo_dir, repaired links should be revalidated after widening, and full-tree hydration should not depend on a fixed 30-second timeout. Logging is appropriately informative and verbose-only. Documentation and changelog refinements remain useful but rank below closing the correctness and integration-evidence gaps.

Aligned with: Portability by manifest: subdirectory dependencies should materialize reliably across sparse-cone symlink boundaries; Secure by default: repair must not follow targets outside the checkout; OSS community driven: contributor credit builds trust; Pragmatic as npm: installs should work or fail actionably without exposing Git internals.

Panel summary

Persona B R N Takeaway
Python Architect 1 1 0 Route the remaining legacy sparse-checkout path through the canonical repair policy.
CLI Logging Expert 0 0 0 Fallback logging is informative, verbose-only by default, and keeps normal output clean.
DevX UX Expert 0 1 0 Detect and explain links that remain broken after widening.
Supply Chain Security Expert 0 2 0 Add checkout-root containment and align the security contract and tests.
OSS Growth Hacker 0 1 0 Lead with the install outcome and credit the community contributor.
Doc Writer 0 1 1 The cache reference now overstates the sparse checkout invariant.
Test Coverage Expert 0 2 0 Real-symlink install coverage and Scenario Evidence mapping are missing.
Performance Expert 0 4 0 The linear repair needs a realistic hydration timeout and bounded fallback work.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 4 follow-ups

  1. [Python Architect] (blocking-severity) Route GitHubPackageDownloader._try_sparse_checkout through the canonical symlink-repair policy. -- The default ScriptRunner path remains exposed to the exact failure.
  2. [Test Coverage Expert] Add a real POSIX symlink fixture exercising the complete subdirectory install path. -- The current tests do not prove checkout, widening, copy, and final resolution together.
  3. [Supply Chain Security Expert] Enforce repository containment and revalidate links after widening. -- Repair must not expand trust beyond repo_dir.
  4. [Performance Expert] Replace or justify the 30-second hydration timeout and avoid repeated fallback work. -- Large repositories may fail legitimate hydration or duplicate transfer.

Architecture

classDiagram
    direction LR
    class GitHubDownloader {
      +download_subdirectory_package()
      +_try_sparse_checkout() bool
    }
    class GitCache {
      +get_checkout() Path
      -_create_checkout() Path
    }
    class BareCacheModule {
      +materialize_from_bare() str
    }
    class GitSparseService {
      +apply_sparse_cone()
      +repair_dangling_cone_symlinks() Path
    }
    GitHubDownloader o-- GitCache : persistent strategy
    GitHubDownloader ..> BareCacheModule : shared-bare strategy
    GitCache ..> GitSparseService : applies and repairs
    BareCacheModule ..> GitSparseService : applies and repairs
    GitHubDownloader ..> GitSparseService : legacy path must repair
Loading
flowchart TD
    A[apm install or run] --> D[GitHubPackageDownloader]
    D --> P{persistent cache?}
    P -- yes --> G[GitCache checkout]
    P -- no --> Q{shared bare cache?}
    Q -- yes --> B[Bare cache materialization]
    Q -- no --> L[Legacy sparse checkout]
    G --> R[Repair dangling cone symlinks]
    B --> R
    L -. missing repair .-> R
    R --> C[Copy package payload]
Loading

Recommendation

Complete the third call-site repair and add end-to-end POSIX fixture evidence before shipping; then harden repository containment and hydration behavior as the highest-priority follow-ups.


Full per-persona findings

Python Architect

  • [blocking] The legacy sparse-checkout path still bypasses dangling-symlink repair at src/apm_cli/deps/github_downloader.py:1262.
  • [recommended] Propagate final sparse materialization state to diagnostics at src/apm_cli/deps/github_downloader.py:1376.

CLI Logging Expert

No findings.

DevX UX Expert

  • [recommended] Verify the symlink resolves before reporting successful repair at src/apm_cli/utils/git_sparse.py:90.

Supply Chain Security Expert

  • [recommended] Contain every Git symlink target before copy materialization at src/apm_cli/utils/git_sparse.py:90.
  • [recommended] Synchronize the Security Model with remote Git symlink handling at docs/src/content/docs/enterprise/security.md:315.

OSS Growth Hacker

  • [recommended] Make the changelog entry outcome-first and credit the contributor at CHANGELOG.md:12.

Auth Expert -- inactive

The changed cache, sparse-checkout, changelog, and test files do not alter authentication behavior.

Doc Writer

  • [recommended] Update the cache layout reference because sparse-<hash>/ can widen to a full tree.
  • [nit] Simplify the Windows aside in the changelog.

Test Coverage Expert

  • [recommended] Add a real-symlink integration fixture for the POSIX install regression.
  • [recommended] Add the Scenario Evidence mapping to the PR body.

Performance Expert

  • [recommended] Give full-tree fallback the full fetch timeout budget.
  • [recommended] Deduplicate full hydration across sparse variants.
  • [recommended] Distinguish repairable outward links from intrinsically dangling links.
  • [recommended] Avoid one Python metadata syscall per materialized entry.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

Daniel Meppiel (danielmeppiel) added a commit to MohammedAlkindi/apm that referenced this pull request Aug 30, 2026
Route every cone checkout through the repair owner, validate symlink containment, add real Git regression evidence, and update affected docs. Addresses the in-scope apm-review-panel follow-ups for PR microsoft#2710.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: needs_rework

PR #2710 needs two targeted repairs before shipping: persistent sparse-cache hits can preserve stale shards, and package symlinks can reach generated Git metadata.

cc Mohammed Alkindi (@MohammedAlkindi) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

CI is green, but the reproduced dangling-shard path and the tracked-symlink access to generated .git/config are stronger evidence than the existing suite. Both findings affect core package-manager trust: cached installs must repair persistent state, and tracked package content must not gain access to generated Git metadata.

Fold both fixes into this PR with end-to-end regressions. The helper-level escape test is useful but insufficient without exercising download_subdirectory_package. The duplicate v0.29.0 heading should also be corrected. Cross-variant hydration dedup is a separate cache redesign and remains follow-up work.

Aligned with: Portability by manifest: cache hits must match fresh materialization; Secure by default: package links must not expose Git metadata; OSS community driven: reproduced defects are folded before release; Pragmatic as npm: one cache finalizer preserves simple behavior.

Panel summary

Persona B R N Takeaway
Python Architect 1 0 0 Existing persistent sparse-cache hits bypass repair.
CLI Logging Expert 0 0 0 Repair logging is accurate and quiet by default.
DevX UX Expert 0 0 0 Repair and actionable failures satisfy the user contract.
Supply Chain Security Expert 1 0 0 Restrict symlink targets to pinned-tree content, not .git metadata.
OSS Growth Hacker 0 1 0 Remove the duplicate release heading.
Doc Writer 0 1 0 Documentation aligns apart from the duplicate release heading.
Test Coverage Expert 0 1 0 Exercise escape rejection through the downloader.
Performance Expert 0 1 0 Defer cross-variant hydration dedup to a cache redesign.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Supply Chain Security Expert] (blocking-severity) Reject Git-directory targets or require pinned tracked content, with a real .git/config regression. -- Current containment permits generated repository metadata.
  2. [Python Architect] (blocking-severity) Route persistent sparse-cache hits through one repair finalizer. -- A stale dangling shard remains reproducible.
  3. [Test Coverage Expert] Exercise escape rejection through download_subdirectory_package. -- Helper-only coverage does not prove the user-facing boundary.
  4. [OSS Growth Hacker] Remove the duplicate v0.29.0 heading. -- One authoritative release record avoids ambiguity.
  5. [Performance Expert] Track cross-variant hydration dedup separately. -- It is broader than this correctness repair.

Architecture

flowchart TD
    A[GitCache get_checkout] --> B{Verified sparse cache hit?}
    B -- yes --> C[Repair and validate finalizer]
    B -- no --> D[Create sparse checkout]
    D --> C
    C --> E[Return valid checkout]
    F[Package symlink] --> G{Target is pinned tracked content?}
    G -- yes --> E
    G -- no --> H[Fail before copy]
Loading

Recommendation

Fold the cache finalizer and Git-metadata symlink defense with end-to-end regressions, then re-run CI. Keep cross-variant hydration dedup separate.


Full per-persona findings

Python Architect

  • [blocking] Repair pre-existing sparse cache entries before returning cache hits at src/apm_cli/cache/git_cache.py:167.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

  • [blocking] Reject symlinks into generated Git metadata at src/apm_cli/utils/git_sparse.py:82.

OSS Growth Hacker

  • [recommended] Remove the duplicate v0.29.0 release heading at CHANGELOG.md:19.

Auth Expert -- inactive

The sparse-checkout, test, documentation, and lock-hash changes do not alter authentication behavior.

Doc Writer

  • [recommended] Retain only the canonical v0.29.0 release heading.

Test Coverage Expert

  • [recommended] Drive checkout-escape rejection through download_subdirectory_package.

Performance Expert

  • [recommended] Deduplicate full hydration across repaired sparse variants in a separate cache redesign.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: needs_rework

PR #2710 improves sparse-cache repair, but real-Git evidence shows colon-prefixed paths can evade external-symlink detection.

cc Mohammed Alkindi (@MohammedAlkindi) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The supply-chain finding is load-bearing: a dynamic colon-prefixed package path is interpreted as Git pathspec magic despite --, causing tracked-symlink discovery to miss an external link. Green CI does not rebut this demonstrated gap because the suite lacks this real-Git scenario. Literal path handling, pinned-HEAD tree evidence, and a regression test should be folded into this PR.

The remaining feedback is compatible and lower risk: make legacy no-cache widening visible under documented --verbose, and clarify that widening applies to a tracked file excluded from the cone. Cross-variant dedup is a separate cache redesign and remains out of scope.

Dissent. There is no substantive specialist disagreement. Clean reviews from other lenses do not outweigh the concrete real-Git security reproduction.

Aligned with: Secure by default: pathspec syntax cannot create a detection gap; Portability by manifest: pinned-HEAD tree inspection is deterministic; Pragmatic as npm: verbose mode should expose scope widening.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 1 The single-owner facade and guarded delegation are sound.
CLI Logging Expert 0 1 0 Route legacy repair through the verbose logging channel.
DevX UX Expert 0 0 0 Install recovery, errors, cache reuse, tests, and docs are complete.
Supply Chain Security Expert 1 0 0 Git pathspec magic can bypass external-link detection.
OSS Growth Hacker 0 0 0 Outcome-first contributor credit and release narrative are strong.
Doc Writer 0 1 0 Clarify the exact tracked-file widening trigger.
Test Coverage Expert 0 0 0 Current real-Git scenarios pass; add the reproduced pathspec case.
Performance Expert 0 1 0 Keep cross-variant hydration dedup as a separate redesign.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 3 follow-ups

  1. [Supply Chain Security Expert] (blocking-severity) Treat dynamic paths literally, inspect the pinned HEAD tree, and add a real-Git colon-path regression. -- Current pathspec interpretation can hide an external symlink.
  2. [CLI Logging Expert] Route legacy no-cache widening through standard verbose logging. -- --verbose users should see repair broadening.
  3. [Doc Writer] Clarify the tracked-file-outside-cone trigger in cache.md. -- This avoids overstating cache behavior.

Recommendation

Address the demonstrated literal-path security gap and lock it down with pinned-HEAD real-Git regression coverage before shipping. Keep cross-variant dedup separate.


Full per-persona findings

Python Architect

  • [nit] The facade and single-owner pattern are appropriate; no further abstraction is warranted.

CLI Logging Expert

  • [recommended] The legacy repair diagnostic is invisible to --verbose at src/apm_cli/deps/github_downloader.py:1126.

DevX UX Expert

No findings.

Supply Chain Security Expert

  • [blocking] A colon-prefixed package path can bypass external-symlink validation at src/apm_cli/utils/git_sparse.py:40.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

The sparse checkout, cache, test, documentation, and hash changes do not alter authentication behavior.

Doc Writer

  • [recommended] cache.md overstates when a sparse variant widens.

Test Coverage Expert

No findings on current scenarios.

Performance Expert

  • [recommended] Deduplicate full hydration across sparse variants in a separate cache redesign.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: ship_with_followups

PR #2710 is ready to ship after folding a bounded GitCache cleanup fix that prevents staged sparse-checkout debris on validation failures.

cc Mohammed Alkindi (@MohammedAlkindi) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The panel converges positively: CI is green, all active personas except Python Architect report zero findings, and cross-variant hydration dedup remains appropriately deferred as a broader cache redesign.

The remaining finding is concrete and lifecycle-local. GitCache cleanup omits validation exceptions, while incomplete cleanup scans only the immediate cache root. Fold real-Git coverage proving nested .inc debris is removed.

Aligned with: Secure by default: unsafe paths fail without staged state; Pragmatic as npm: cleanup remains invisible and predictable.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 0 Clean staged cache shards on validation failures.
CLI Logging Expert 0 0 0 Verbose repair diagnostics use standard logging.
DevX UX Expert 0 0 0 Recovery, errors, cache behavior, tests, and docs align.
Supply Chain Security Expert 0 0 0 Literal pinned-tree queries and containment fail closed.
OSS Growth Hacker 0 0 0 Changelog outcome and contributor credit are clear.
Doc Writer 0 0 0 Documentation accurately describes repair boundaries.
Test Coverage Expert 0 0 0 Mapped repair and security scenarios have regression traps.
Performance Expert 0 0 0 Bounded tree checks avoid common-path network or tree scans.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 1 follow-up

  1. [Python Architect] Broaden GitCache failure cleanup and add a real-Git regression covering nested .inc debris. -- Validation errors currently bypass staged cleanup.

Recommendation

Fold the bounded GitCache cleanup and real-Git regression into this PR, then ship. Keep cross-variant hydration dedup deferred to a separately designed cache change.


Full per-persona findings

Python Architect

  • [recommended] Clean staged cache shards on symlink-validation failures at src/apm_cli/cache/git_cache.py:670.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

The sparse checkout, cache, test, and documentation changes do not alter authentication behavior.

Doc Writer

No findings.

Test Coverage Expert

No findings.

Performance Expert

No findings.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: ship_with_followups

PR #2710 is ready to ship: all in-scope findings are folded, CI is green, and validation-failure cleanup has real-Git regression coverage.

cc Mohammed Alkindi (@MohammedAlkindi) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The active panel converges on clean shipping at 2a1cb6975f99259aea52b9ba0f3acb61ea674ef2. The Python Architect's sole nit correctly concludes that further abstraction is unwarranted, while every other active panelist reports zero findings; auth is validly inactive.

Prior in-scope feedback is fully resolved, including validation-failure staging cleanup defended by a mutation-tested real-Git regression. Cross-variant full-hydration dedup remains intentionally separate because it requires a broader cache storage and locking redesign.

Aligned with: Secure by default: validation failures clean staged state; Pragmatic as npm: the implementation remains focused and avoids abstraction without demonstrated need.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 1 Central policy, lock-aware mutation, cleanup, and dual guardrails are coherent.
CLI Logging Expert 0 0 0 Verbose repair diagnostics use standard logging and stay hidden by default.
DevX UX Expert 0 0 0 Recovery, diagnostics, cache behavior, tests, and docs align.
Supply Chain Security Expert 0 0 0 Literal pinned-tree validation and cleanup fail closed.
OSS Growth Hacker 0 0 0 The outcome-first changelog preserves contributor credit.
Doc Writer 0 0 0 Documentation matches repair and security boundaries.
Test Coverage Expert 0 0 0 Critical repair, containment, cache, and cleanup paths have regression traps.
Performance Expert 0 0 0 Bounded tree queries preserve the sparse common path.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 1 follow-up

  1. [Performance Expert] Design cross-variant full-hydration dedup separately. -- The optimization crosses cache storage and locking boundaries and deserves independent scope.

Architecture

classDiagram
    direction LR
    class GitHubPackageDownloader {
      +download_subdirectory_package()
      -_try_sparse_checkout() bool
    }
    class GitCache {
      +get_checkout() Path
      -_create_checkout() Path
      -_finalize_sparse_checkout() Path
    }
    class BareCacheModule {
      +materialize_from_bare() str
    }
    class GitSparseModule {
      +apply_sparse_cone()
      +repair_dangling_cone_symlinks() Path
      +validate_materialized_symlinks()
      +sparse_checkout_active() bool
    }
    class PathSecurity {
      +ensure_path_within() Path
    }
    GitHubPackageDownloader ..> GitSparseModule : setup repair validate
    GitCache ..> GitSparseModule : finalize every return
    BareCacheModule ..> GitSparseModule : setup and repair
    GitSparseModule ..> PathSecurity : validate targets
Loading

Recommendation

Ship this commit with confidence. Track cross-variant full-hydration dedup as a separate cache storage and locking redesign; it is outside this PR's scope.

Folded in this run

  • (panel) Complete legacy no-cache sparse repair and canonical setup routing -- resolved in bb3c14f23e.
  • (panel) Add real Git repair, broken-link, and containment regression evidence -- resolved in bb3c14f23e.
  • (panel) Use pinned tracked-file validation, a 300-second hydration budget, and bounded tree queries -- resolved in bb3c14f23e.
  • (panel) Correct sparse-state diagnostics, cache/security docs, changelog, and Scenario Evidence -- resolved in bb3c14f23e.
  • (panel) Align synthetic fixtures and generated instruction integrity after CI feedback -- resolved in e6126341ce.
  • (panel) Repair pre-existing persistent sparse-cache hits -- resolved in 973704b745.
  • (panel) Reject generated Git metadata targets at the downloader boundary -- resolved in 973704b745.
  • (panel) Treat colon-prefixed package paths as literal pinned-HEAD pathspecs -- resolved in 0140353efe.
  • (panel) Route legacy widening through standard verbose logging -- resolved in 0140353efe.
  • (panel) Clean staged persistent-cache shards after validation failures -- resolved in 2a1cb6975f.

Deferred (out-of-scope follow-ups)

  • (panel) Deduplicate full hydration across sparse variants -- scope boundary: PR scope is dangling-link correctness; this requires a cross-cache storage and locking redesign; suggested follow-up: open a performance design issue for shared full-tree hydration.

Regression-trap evidence (mutation-break gate)

  • test_legacy_downloader_repairs_real_out_of_cone_symlink plus the architecture owner test -- removed legacy repair routing; tests failed as expected; guard restored.
  • test_persistent_cache_hit_repairs_preexisting_dangling_shard plus the architecture owner test -- removed the cache-hit finalizer; tests failed as expected; guard restored.
  • test_downloader_rejects_real_symlink_into_git_metadata -- removed tracked-target membership rejection; test failed as expected; guard restored.
  • test_downloader_treats_colon_prefixed_package_path_literally plus the architecture owner test -- removed literal pathspec wrapping; tests failed as expected; guard restored.
  • test_repair_logs_via_verbose_channel -- replaced standard logging; test failed as expected; guard restored.
  • test_invalid_symlink_cleans_persistent_cache_staging -- removed validation-error cleanup; test failed as expected; guard restored.

Lint contract

uv run --frozen --extra dev ruff check src/ tests/ and uv run --frozen --extra dev ruff format --check src/ tests/ were silent. Pylint R0801, auth, architecture, YAML I/O, file-length, and relative_to guards also passed.

CI

All checks passed on CI run 33303188799, including Linux shards, Windows compatibility, architecture ratchets, APM self-check, CodeQL, spec conformance, and the merge gate, after 1 CI fix iteration.

Mergeability status

PR head SHA CEO stance iters folds defers Copilot rounds CI mergeable mergeStateStatus notes
#2710 2a1cb69 ship_with_followups 4 10 1 2 green MERGEABLE BLOCKED pending required review

Convergence

4 outer iterations; 2 Copilot rounds. Final panel stance: ship_with_followups.

Ready for maintainer review.


Full per-persona findings

Python Architect

  • [nit] No additional architectural pattern is warranted.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

Only sparse-cache and symlink-related files were touched; no authentication surface changed.

Doc Writer

No findings.

Test Coverage Expert

No findings.

Performance Expert

No findings.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

Daniel Meppiel (danielmeppiel) added a commit to MohammedAlkindi/apm that referenced this pull request Aug 30, 2026
Route every cone checkout through the repair owner, validate symlink containment, add real Git regression evidence, and update affected docs. Addresses the in-scope apm-review-panel follow-ups for PR microsoft#2710.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Daniel Meppiel (danielmeppiel) force-pushed the fix/sparse-cone-dangling-symlinks branch from 2a1cb69 to 5b072c1 Compare August 30, 2026 14:34
@danielmeppiel

Copy link
Copy Markdown
Collaborator

Rebased onto current main at b68fcad -> 5b072c1.

Conflicting paths resolved (faithful merge of both intents):

  • apm.lock.yaml

Lint contract: uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both silent
post-rebase.

Post-push mergeability: gh pr view --json mergeStateStatus,mergeable
reports BLOCKED / MERGEABLE. BLOCKED is the required-review gate,
not a conflict. Push used
git push --force-with-lease=fix/sparse-cone-dangling-symlinks:2a1cb6975f99259aea52b9ba0f3acb61ea674ef2 pr-MohammedAlkindi-apm HEAD:fix/sparse-cone-dangling-symlinks
(--force-with-lease, never bare --force).

Ready for maintainer review.

Daniel Meppiel (danielmeppiel) added a commit to MohammedAlkindi/apm that referenced this pull request Aug 30, 2026
Route every cone checkout through the repair owner, validate symlink containment, add real Git regression evidence, and update affected docs. Addresses the in-scope apm-review-panel follow-ups for PR microsoft#2710.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Daniel Meppiel (danielmeppiel) force-pushed the fix/sparse-cone-dangling-symlinks branch from 5b072c1 to 5e69f91 Compare August 30, 2026 14:49
Sparse-cone subdir materialization (perf microsoft#1433) only checks out the
requested top-level path. A dependency whose payload contains a
symlink pointing outside that path (e.g. skills/x/ref.md ->
../../../shared/ref.md) ends up with the symlink entry present but
its target excluded, so the copy phase's dereference fails with
FileNotFoundError.

After the cone checkout, walk the requested paths for a dangling
symlink and fall back to `git sparse-checkout disable` so the target
resolves. Applied to both cone call sites: bare_cache.materialize_from_bare
and GitCache._create_checkout.

Closes microsoft#2707.
Route every cone checkout through the repair owner, validate symlink containment, add real Git regression evidence, and update affected docs. Addresses the in-scope apm-review-panel follow-ups for PR microsoft#2710.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mock the new materialized-symlink boundary in synthetic checkout tests and refresh the generated instruction integrity hashes. This recovers the exact-head CI test and APM self-check failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Repair verified sparse cache hits and restrict package symlinks to tracked files in the pinned commit. Real Git regressions cover stale shards and Git metadata targets, addressing the second panel pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Read pinned HEAD trees with literal Git pathspecs so colon-prefixed package paths cannot hide unsafe links. Also route legacy widening through verbose logging and tighten cache documentation, addressing the terminal panel pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove incomplete sparse cache shards when materialized-symlink validation rejects a checkout, while preserving the original validation error. Addresses the final Python Architect follow-up.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the sparse symlink fix under Unreleased while preserving the current v0.29.0 release notes from main.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Daniel Meppiel (danielmeppiel) force-pushed the fix/sparse-cone-dangling-symlinks branch from 5e69f91 to 248d5da Compare August 30, 2026 16:06
Model GitCache's valid-worktree return contract so sparse symlink validation can inspect the pinned tree while the auth retry regression remains intact.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Daniel Meppiel (danielmeppiel) merged commit a7cc945 into microsoft:main Aug 30, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Sparse-cone subdir materialization leaves dangling symlinks — install fails with ENOENT on the symlink targets

3 participants