Skip to content

docs: clarify apm_modules is derived - #2601

Open
Aryan Singh K. (aryansk) wants to merge 1 commit into
microsoft:mainfrom
aryansk:docs/2064-apm-modules-derived
Open

docs: clarify apm_modules is derived#2601
Aryan Singh K. (aryansk) wants to merge 1 commit into
microsoft:mainfrom
aryansk:docs/2064-apm-modules-derived

Conversation

@aryansk

Copy link
Copy Markdown

Closes #2064

Summary

  • document that apm_modules/ is derived and can be safely gitignored
  • distinguish the committed lockfile/source manifest from regenerable installed content
  • document the reinstall pattern for CI without inventing a currently unsupported persistence flag

Validation

  • documentation-only change; patch syntax was checked against current main

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

Note

Copilot was unable to run its full agentic suite in this review.

Adds documentation clarifying that apm_modules/ is a derived, recreatable install directory and should typically be ignored in version control, positioning the lockfile as the durable source of truth.

Changes:

  • Documented that apm_modules/ is derived from apm.yml + apm.lock.yaml and can be regenerated via apm install
  • Added a recommended .gitignore entry for apm_modules/
  • Noted CI/ephemeral workflow guidance and current lack of an automatic post-install cleanup flag

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

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@sergio-sisternes-epam Sergio Sisternes (sergio-sisternes-epam) added the panel-review Trigger the apm-review-panel gh-aw workflow label Aug 18, 2026
@sergio-sisternes-epam

Copy link
Copy Markdown
Collaborator

APM Review Panel: ship_with_followups

Docs-only PR correctly positions apm_modules/ as derived; doc-writer surfaced a factual contradiction with auto-add behavior that should be folded before merge.

cc Aryan Singh K. (@aryansk) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

All nine panelists agree this PR is directionally correct and introduces no regressions. The only substantive tension is between the PR's current prose (which implies users must manually gitignore apm_modules/) and the existing corpus (consumer/install-packages.md line 134) which states APM auto-adds the entry on first install. This is not a style disagreement -- it is a factual contradiction between two documentation pages that will confuse readers. The doc-writer's remaining recommended findings (use 'package cache' for corpus consistency, reference 'apm deps clean' instead of raw rm -rf, and wrap the limitation disclosure in a callout) are low-cost edits that improve maintainability and accuracy without changing the section's structure. The supply-chain-security-expert's recommendation to add a single sentence about lockfile SHA verification is well-placed and closes a trust-model gap at near-zero editorial cost. None of these require design discussion or code changes; all can be addressed in the same PR cycle.

Aligned with: Portable by manifest -- PR reinforces that apm.yml + apm.lock.yaml are the portable, committable artifacts and apm_modules/ is ephemeral. Directly supports the manifest-as-source-of-truth principle. Pragmatic as npm -- answers the 'do I commit the vendor folder?' question every npm/pip user asks on day one.

Growth signal. Contributors are independently asking the node_modules question -- strong first-funnel signal. Track whether this section gets cross-linked from quickstart and the .gitignore template; if not, add a link in the first-package guide to reduce 'should I commit this?' drop-off.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 0 Documentation-only PR accurately describes the apm_modules/ materialization model. Matches architectural reality. No concerns.
CLI Logging Expert 0 0 0 CLI commands referenced (apm install) are accurate and CI pattern is consistent with actual tool behavior.
DevX UX Expert 0 0 2 Mental model correct, well-aligned with npm/cargo conventions. Prose is clear and actionable. No blocking issues.
Supply Chain Security Expert 0 1 1 Correctly positions apm_modules/ as derived output and the lockfile as the integrity anchor. No security regressions.
OSS Growth Hacker 0 0 1 Solid friction-reducer answering the #1 question every Node/Python dev asks on first encounter with a new package manager.
Doc Writer 0 4 2 Section is accurate and well-placed; three issues to fold: auto-add contradiction, missing apm-deps-clean reference, and inline limitation prose.

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

Top 5 follow-ups

  1. [Doc Writer] Fix contradiction: note that APM auto-adds apm_modules/ to .gitignore on first install -- Factual accuracy between pages is non-negotiable for trust. (install-packages.md:134 already says this; the new section should be consistent or cross-reference it.)
  2. [Doc Writer] Replace manual rm -rf guidance with apm deps clean reference -- The command exists and is documented; omitting it teaches users a worse workflow and leaves the CLI surface undiscoverable.
  3. [Supply Chain Security Expert] Add one sentence stating apm install verifies content against lockfile commit-SHA -- Closes the trust-model explanation for CI readers who need to justify the reinstall pattern to security reviewers.
  4. [Doc Writer] Use 'package cache' instead of 'install cache/materialization directory' for corpus consistency -- Three other pages use 'package cache'; inconsistent terminology erodes the glossary contract.
  5. [Doc Writer] Wrap the no --clean flag limitation in a :::note[Planned] callout -- Inline limitation prose is hard to find-and-remove when the flag ships; a callout is a one-block delete.

Architecture

classDiagram
    direction LR
    class ApmManifest {
        <<SourceOfTruth>>
        +dependencies list
        +file apm.yml
    }
    class ApmLockfile {
        <<SourceOfTruth>>
        +resolved_graph dict
        +integrity_hashes dict
        +file apm.lock.yaml
    }
    class ApmModules {
        <<Derived>>
        +materialized_packages list
        +dir apm_modules/
    }
    ApmManifest --> ApmLockfile : resolves to
    ApmLockfile --> ApmModules : materializes via install
Loading
flowchart TD
    A[apm.yml\ncommitted manifest] --> B[apm install\nresolution]
    B --> C[apm.lock.yaml\ncommitted lockfile]
    C --> D[apm install\nmaterialization]
    D --> E[apm_modules/\nderived - gitignored]
    E -->|discardable| F[recreate via apm install]
Loading

Recommendation

Merge after folding the auto-add contradiction fix and the apm-deps-clean reference (first two followups). The remaining three are valuable but can land as a fast follow without blocking this contribution. The PR is a net positive for onboarding and answers a real first-funnel question.


Full per-persona findings

Python Architect

No findings.

CLI Logging Expert

No findings.

DevX UX Expert

  • [nit] Consider linking to a quickstart/init docs page that auto-generates the gitignore entry at docs/src/content/docs/reference/lockfile-spec.md
    Users should not have to remember to manually add the gitignore entry; linking to an init command or quickstart would complete the flow.

  • [nit] Reframe the 'no --clean flag' paragraph as an action rather than an absence at docs/src/content/docs/reference/lockfile-spec.md
    A more cargo-like framing: 'To remove materialized sources after use, run apm deps clean as a post-step.' States the action, not the absence of a flag.

Supply Chain Security Expert

  • [recommended] The CI reinstall paragraph should state that apm install verifies content against the lockfile commit-SHA at docs/src/content/docs/reference/lockfile-spec.md
    Without this, a reader could wonder whether re-fetching from a remote guarantees integrity. The lockfile is not merely a convenience cache key -- it is an integrity control.
    Suggested: Add: 'Because the lockfile pins each dependency to an exact commit SHA, apm install verifies that materialized content matches the recorded identity before deployment.'

  • [nit] Cleanup paragraph could note that cleanup should occur after all consuming tools have finished at docs/src/content/docs/reference/lockfile-spec.md
    Avoids a potential TOCTOU-style gap where a later pipeline step references a now-deleted dependency.

OSS Growth Hacker

  • [nit] Mention actions/cache keyed on lockfile hash for faster CI at docs/src/content/docs/reference/lockfile-spec.md
    CI users coming from npm/pnp expect a caching pattern. A one-liner referencing actions/cache with hashFiles('apm.lock.yaml') would complete the mental model and give them a copy-pasteable win.

Auth Expert -- inactive

PR #2601 touches only docs/src/content/docs/reference/lockfile-spec.md -- a documentation clarification about apm_modules being derived. No auth-related files or flows affected.

Doc Writer

  • [recommended] The new section omits that APM auto-adds apm_modules/ to .gitignore on first install at docs/src/content/docs/reference/lockfile-spec.md
    install-packages.md line 134 states 'APM adds the entry automatically on first install.' The new section implies users must add it manually, creating a contradiction. Either add 'APM adds this entry automatically on first install' or replace the snippet with a cross-reference to install-packages.md.

  • [recommended] Use 'package cache' instead of 'install cache/materialization directory' for corpus consistency at docs/src/content/docs/reference/lockfile-spec.md
    The rest of the docs corpus uses 'package cache' consistently (install-packages.md:133, troubleshooting/install-failures.md, package-anatomy.md).
    Suggested: Replace opening sentence with: 'apm_modules/ is the package cache -- installed dependency source, not a source of truth.'

  • [recommended] Link to apm deps clean instead of implying manual directory removal at docs/src/content/docs/reference/lockfile-spec.md
    reference/cli/deps.md:134 documents apm deps clean which removes apm_modules/ without touching apm.yml or apm.lock.yaml.
    Suggested: Replace 'remove the directory as a separate cleanup step' with 'run apm deps clean as a separate cleanup step'.

  • [recommended] Wrap the 'no --clean flag' limitation in a callout block for easier future removal at docs/src/content/docs/reference/lockfile-spec.md
    Inline prose about a temporary limitation is harder to remove when the flag ships. A :::note[Planned] callout makes future removal a one-block delete.

  • [nit] Add cross-link to integrations/ci-cd.md from the CI paragraph at docs/src/content/docs/reference/lockfile-spec.md
    The CI paragraph covers the same territory as integrations/ci-cd.md but does not link to it.

  • [nit] Add cross-link to consumer/install-packages.md at the gitignore recommendation at docs/src/content/docs/reference/lockfile-spec.md
    install-packages.md has the full 'what to commit' table; a cross-reference keeps the lockfile-spec page lean.

Test Coverage Expert -- inactive

documentation-only PR -- no runtime code paths to defend

Performance Expert -- inactive

documentation-only PR; no hot-path code changed and no performance claims in the diff

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

@sergio-sisternes-epam Sergio Sisternes (sergio-sisternes-epam) removed the panel-review Trigger the apm-review-panel gh-aw workflow label Aug 18, 2026
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.

apm.yml + apm.lock.yaml + apm_modules left after installation

3 participants