docs: clarify apm_modules is derived - #2601
Conversation
There was a problem hiding this comment.
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 fromapm.yml+apm.lock.yamland can be regenerated viaapm install - Added a recommended
.gitignoreentry forapm_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.
APM Review Panel:
|
| 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
- [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.)
- [Doc Writer] Replace manual rm -rf guidance with
apm deps cleanreference -- The command exists and is documented; omitting it teaches users a worse workflow and leaves the CLI surface undiscoverable. - [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.
- [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.
- [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
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]
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, runapm deps cleanas 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 cleaninstead of implying manual directory removal atdocs/src/content/docs/reference/lockfile-spec.md
reference/cli/deps.md:134 documentsapm deps cleanwhich removes apm_modules/ without touching apm.yml or apm.lock.yaml.
Suggested: Replace 'remove the directory as a separate cleanup step' with 'runapm deps cleanas 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.
Closes #2064
Summary
apm_modules/is derived and can be safely gitignoredValidation
main