fix(lockfile): omit generated_at from new lockfiles - #2616
fix(lockfile): omit generated_at from new lockfiles#2616Lachlan Heywood (lachieh) wants to merge 11 commits into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR removes the volatile generated_at timestamp from newly created apm.lock.yaml files to reduce merge conflicts, while preserving legacy behavior for repositories that already have the field. It also ensures apm lock export remains deterministic by falling back to the Unix epoch when no timestamp source is available.
Changes:
- Make
LockFile.generated_atoptional and omit it from YAML when absent, with legacy-preserve/refresh behavior during writes. - Remove MCP’s unconditional timestamp assignment so MCP-created lockfiles remain timestamp-free by default.
- Update CLI/help text, docs, changelog, and tests to reflect the new/legacy behaviors and the SBOM timestamp fallback order.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/apm_cli/deps/lockfile.py | Makes generated_at optional, omits it in YAML when absent, and refreshes/preserves it only for legacy lockfiles. |
| src/apm_cli/integration/mcp_integrator.py | Stops assigning generated_at during MCP lockfile persistence to keep new lockfiles timestamp-free. |
| src/apm_cli/commands/lock.py | Updates lock export help text to include the Unix epoch as the final deterministic fallback. |
| tests/test_lockfile.py | Adds coverage to ensure generated_at is omitted from new YAML and refreshed for legacy lockfiles on substantive writes. |
| tests/unit/commands/test_lock_export_command.py | Adds a test asserting SBOM export uses the Unix epoch when lockfile has no generated_at. |
| tests/unit/install/test_mcp_lockfile_determinism.py | Updates determinism expectations for timestamp-free lockfiles and adds a legacy refresh test. |
| tests/unit/integration/test_mcp_integrator.py | Verifies a missing lockfile created via MCP does not include generated_at. |
| tests/integration/test_install_mcp_lockfile_determinism.py | Aligns MCP lifecycle assertions with timestamp-free lockfiles. |
| tests/integration/test_install_lsp_lockfile_determinism.py | Aligns LSP lifecycle assertions with timestamp-free lockfiles. |
| tests/integration/test_cache_lockfile_parity.py | Updates parity docs/assertion intent to treat generated_at as optional legacy metadata. |
| tests/integration/test_config_surface_lifecycle_contract.py | Updates lifecycle contract wording to refer to optional legacy generated_at metadata. |
| tests/integration/test_oci_mcp_lifecycle_contract.py | Updates idempotency wording to refer to optional legacy timestamp metadata. |
| docs/src/content/docs/reference/lockfile-spec.md | Updates spec examples and marks generated_at as optional legacy metadata with opt-out semantics. |
| docs/src/content/docs/reference/cli/lock.md | Documents SBOM timestamp fallback order including Unix epoch. |
| docs/src/content/docs/reference/cli/pack.md | Removes timestamp from generated lockfile examples. |
| docs/src/content/docs/concepts/package-anatomy.md | Updates the canonical example and marks generated_at as deprecated/optional legacy metadata. |
| packages/apm-guide/.apm/skills/apm-usage/commands.md | Updates bundled command reference to match the new SBOM timestamp fallback order. |
| CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the timestamp omission and legacy behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/unit/commands/test_lock_export_command.py:143
- This test is asserting the final fallback behavior when
generated_atis missing, but it currently relies onSOURCE_DATE_EPOCHbeing unset in the process environment. IfSOURCE_DATE_EPOCHis set (common in reproducible-build contexts), the command will prefer it and this assertion will fail.
def test_export_without_generated_at_uses_fixed_epoch(runner, tmp_path):
with runner.isolated_filesystem(temp_dir=tmp_path):
_seed(Path.cwd())
lock_path = Path("apm.lock.yaml")
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Clean backward-compatible deprecation of a volatile field. The write()-mutates-self pattern is the only architectural concern worth flagging. |
| CLI Logging Expert | 0 | 0 | 1 | No CLI output regressions. One observability gap: legacy generated_at preservation/refresh in write() has no verbose-mode breadcrumb. |
| DevX UX Expert | 0 | 1 | 1 | Clean UX win for new users (no surprise timestamp diff noise). Migration path for existing users documented but relies on reading the lockfile-spec page -- no CLI hint surfaces the action. |
| Supply Chain Security Expert | 0 | 0 | 1 | No supply-chain security regressions. Removing volatile generated_at from new lockfiles improves reproducibility. TOCTOU window in write() is pre-existing and bounded. SBOM epoch fallback does not weaken integrity or provenance. |
| OSS Growth Hacker | 0 | 0 | 0 | This PR removes a top contributor-friction source (timestamp merge conflicts) -- a story-shaped change worth a release beat. No conversion-surface regressions detected. |
| Doc Writer | 0 | 2 | 2 | Documentation changes are accurate and well-scoped. Three gaps: deprecation label formatting inconsistency between pages, migration mechanism not specified (manual edit), missing one-way guarantee on anatomy page. |
| Test Coverage Expert | 0 | 1 | 0 | Critical lockfile surfaces are well-covered. One recommended gap: legacy no-op preservation path at unit tier lacks a dedicated test. |
| Performance Expert | 0 | 0 | 1 | The new type(self).read(path) inside write() adds an unconditional disk read+YAML parse (~2-5 ms) on every lockfile write. Double-read pattern vs callers, but write() is called at most once per CLI invocation -- not measurable against install wall-time. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add test_write_preserves_existing_generated_at_on_noop unit test -- The no-op preservation contract (legacy lockfile timestamp survives unchanged when deps are identical) has no automated guardrail. Missing test on a governed-by-policy surface; drift here silently reintroduces the churn this PR eliminates.
- [Doc Writer] Align deprecation label in lockfile-spec.md to match package-anatomy.md format -- The normative spec page should carry the same (Deprecated) signal as the anatomy page. One-line edit, prevents reader confusion.
- [Doc Writer] Make migration instruction explicit: 'Delete the generated_at line; APM will not add it back' -- Current wording says 'remove the field' without specifying the mechanism is a manual edit. Users will search for a CLI command that does not exist.
- [DevX UX Expert] Consider a one-time CLI hint when legacy generated_at is refreshed -- Surfaces the opt-out to users who still experience timestamp diffs without requiring them to find the lockfile-spec doc. Risk: log noise for a naturally-disappearing field -- defer to issue.
- [Python Architect] Document write() mutation contract in a docstring -- write() mutates self.generated_at as a side effect. A one-line docstring makes the command-query separation violation intentional and discoverable.
Architecture
classDiagram
direction LR
class Lockfile {
<<Dataclass>>
+lockfile_version: str
+generated_at: str | None
+packages: list
+read(path) Lockfile$
+write(path) None
+to_yaml() str
+is_semantically_equivalent(other) bool
}
class McpIntegrator {
<<BaseIntegrator>>
+integrate(targets) None
}
class LockCommand {
+export_lock() None
}
class atomic_write_text {
<<Utility>>
}
McpIntegrator ..> Lockfile : builds and writes
LockCommand ..> Lockfile : reads for export
Lockfile ..> atomic_write_text : delegates I/O
note for Lockfile "write() owns legacy-preservation decision:\nread existing -> compare -> mutate -> serialize"
class Lockfile:::touched
class McpIntegrator:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm install / apm lock"] --> B["McpIntegrator.integrate()"]
B --> C["Lockfile() constructor\ngenerated_at=None by default"]
C --> D["Lockfile.write(path)"]
D --> E{"path.exists()?"}
E -->|No| F["atomic_write_text\nno generated_at field"]
E -->|Yes| G["Lockfile.read(path)\nexisting lockfile"]
G --> H{"existing.generated_at?"}
H -->|None| F
H -->|present| I{"semantically equivalent?"}
I -->|Yes, no-op| J["preserve existing timestamp"]
I -->|No, changed| K["refresh to now()"]
J --> F
K --> F
F --> L["Done"]
sequenceDiagram
participant User
participant CLI as apm install
participant MI as McpIntegrator
participant LF as Lockfile
participant FS as Filesystem
User->>CLI: apm install
CLI->>MI: integrate(targets)
MI->>LF: Lockfile(...) [generated_at=None]
MI->>LF: write(path)
LF->>FS: read existing lockfile
alt existing has generated_at
alt semantically equivalent
LF->>LF: self.generated_at = existing.generated_at
else changed
LF->>LF: self.generated_at = now()
end
end
LF->>FS: atomic_write_text(yaml)
FS-->>User: apm.lock.yaml updated
Recommendation
Ship now. The two doc-writer one-line fixes (deprecation label + explicit migration wording) are worth folding before merge if convenient but are not blocking. The missing no-op preservation test is the highest-signal follow-up -- file as a fast-follow issue. No panelist raised a blocking concern; the change is backward-compatible, well-scoped, and directly addresses a real contributor-friction report.
Full per-persona findings
Python Architect
-
[recommended] write() mutates self.generated_at as a side effect, breaking command-query separation at
src/apm_cli/deps/lockfile.py
The write() method both persists state AND mutates self.generated_at in place. This means calling write() changes the object identity silently. A purer design would compute the final generated_at in a private method. However, since LockFile is a mutable dataclass used as a short-lived builder, this is tolerable. Docstring documenting the mutation contract is the right fix.
Suggested: Add a docstring to write() explicitly stating it may mutate self.generated_at for legacy preservation.
Proof (passed):tests/test_lockfile.py::test_write_refreshes_existing_generated_at-- proves: The mutation path is exercised and asserted -
[nit] generated_at type annotation uses str | None (3.10+ union syntax) -- fine for the supported Python range
The codebase already uses this syntax elsewhere and pyproject.toml targets 3.10+.
CLI Logging Expert
- [nit] No debug log breadcrumb in write() for legacy timestamp decisions at
src/apm_cli/deps/lockfile.py
The write() method silently preserves or refreshes generated_at for legacy lockfiles but emits no logger.debug() breadcrumb. A logger.debug('Preserving legacy generated_at') / logger.debug('Refreshing legacy generated_at') would help --verbose users and CI debug lockfile churn.
DevX UX Expert
-
[recommended] No CLI guidance surfaces when legacy generated_at is preserved during a substantive write at
src/apm_cli/deps/lockfile.py:910
When write() detects an existing generated_at and refreshes it, the user never learns they can remove the field to stop timestamp churn. A one-time _log.info hint like 'hint: remove generated_at from apm.lock.yaml to eliminate timestamp diffs' would surface the opt-out without requiring users to find the lockfile-spec doc.
Suggested: Add a single hint log on first legacy refresh: _log.info('[i] Tip: remove the generated_at line from apm.lock.yaml to eliminate timestamp-only diffs') -
[nit] Help text 'Unix epoch' may be unclear without the literal value at
src/apm_cli/commands/lock.py:280
'then the Unix epoch' may confuse users unfamiliar with the term. Consider 'then 1970-01-01T00:00:00+00:00 (deterministic zero)' to make the fallback value immediately obvious.
Supply Chain Security Expert
- [nit] TOCTOU: write() reads existing lockfile then writes without holding a lock at
src/apm_cli/deps/lockfile.py
The write() method reads the on-disk lockfile then calls atomic_write_text. If another process writes between those two operations, the preserved timestamp may be stale. Pre-existing behavior, low-risk for a single-user CLI, but a comment documenting the assumption would help.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
No auth-surface files touched; PR only alters lockfile timestamp serialization with no credential, token, or host-classification impact.
Doc Writer
-
[recommended] Deprecation label formatted differently in lockfile-spec.md vs package-anatomy.md at
docs/src/content/docs/reference/lockfile-spec.md
package-anatomy.md uses '(Deprecated) generated_at'. lockfile-spec.md keeps plain generated_at and buries 'Legacy' in the Notes column. The normative spec page should carry the same deprecation signal.
Suggested: Change the field cell in lockfile-spec.md to '(Deprecated)generated_at' matching package-anatomy.md. -
[recommended] Migration instruction does not specify mechanism (manual file edit vs command) at
docs/src/content/docs/reference/lockfile-spec.md
'Remove the field once to opt an existing project into timestamp-free output' does not tell the reader how. There is no apm command for this -- it must be a manual edit.
Suggested: Rewrite to: 'Delete the generated_at: ... line from apm.lock.yaml once; APM will not add it back.' -
[nit] package-anatomy.md brief note omits one-way guarantee that APM will not re-add generated_at at
docs/src/content/docs/concepts/package-anatomy.md
'Remove from lockfile to avoid conflicts.' omits 'APM will not add it back.' That guarantee is the reason a user would bother removing the field.
Suggested: Append ' APM will not add it back.' to the anatomy table note. -
[nit] CHANGELOG entry uses PR number (fix(lockfile): omit generated_at from new lockfiles #2616) instead of closing issue ([BUG]
generated_atkey inapm-lock.ymlis constant source of merge conflicts. #2572) atCHANGELOG.md
Adjacent CHANGELOG entries use closes #NNNN pointing to issues. Issue [BUG]generated_atkey inapm-lock.ymlis constant source of merge conflicts. #2572 is the canonical tracker; prefer 'closes [BUG]generated_atkey inapm-lock.ymlis constant source of merge conflicts. #2572' for consistency.
Suggested: Change '(fix(lockfile): omit generated_at from new lockfiles #2616)' to 'closes [BUG]generated_atkey inapm-lock.ymlis constant source of merge conflicts. #2572' in the CHANGELOG entry.
Test Coverage Expert
- [recommended] Legacy lockfile no-op write (semantically equivalent content) lacks a dedicated test proving generated_at is preserved unchanged at
tests/test_lockfile.py
The PR changes write() to refresh generated_at only on substantive changes. The preservation contract -- no-op write preserves the original timestamp -- has no dedicated test. test_write_refreshes_existing_generated_at proves the refresh path but not the preservation path. A silent drift here would cause unnecessary lockfile churn on legacy files.
Suggested: Add test_write_preserves_existing_generated_at_on_noop: seed legacy lockfile with generated_at, call write() without changing deps, assert generated_at in output equals original value.
Proof (missing at unit):tests/test_lockfile.py::test_write_preserves_existing_generated_at_on_noop-- proves: No-op write on legacy lockfile does not mutate generated_at, preventing unnecessary lockfile churn [devx,governed-by-policy]
Performance Expert
- [nit] Double-read: write() re-reads the lockfile that callers already parsed at
src/apm_cli/deps/lockfile.py:910
Every call site reaching write() has already called LockFile.read(path) upstream (MCP integrator, install pipeline). The existing = type(self).read(path) here re-reads and re-parses the file. Cost is ~2-5 ms, write() is called at most once per CLI run -- not measurable. But accepting an optional existing: LockFile | None = None parameter would eliminate the redundancy cleanly.
Suggested: Add existing: LockFile | None = None parameter to write(), read from disk only when existing is None and path.exists().
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Sergio Sisternes (sergio-sisternes-epam)
removed
the
panel-review
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/apm_cli/deps/lockfile.py:916
- The on-disk opt-in is not authoritative in two cases: a new or timestamp-free destination still emits any non-null
self.generated_at, and a semantic no-op against a legacy file keeps a differing in-memory timestamp instead of the persisted one. This contradicts the documented remove-once opt-out and stable no-op behavior. Normalize the field entirely from the current on-disk state before serialization.
existing = type(self).read(path) if path.exists() else None
if existing is not None and existing.generated_at is not None:
if self.is_semantically_equivalent(existing):
if self.generated_at is None:
self.generated_at = existing.generated_at
else:
self.generated_at = datetime.now(timezone.utc).isoformat()
src/apm_cli/deps/lockfile.py:916
- This block becomes the canonical owner of lockfile timestamp policy, and the PR removes the parallel MCP assignment, but no static architecture guard prevents a caller from reintroducing direct
generated_atwrites. Add a boundary check inscripts/lint-architecture-boundaries.shplus its matching architecture test so every persistence path must continue routing throughLockFile.write().
existing = type(self).read(path) if path.exists() else None
if existing is not None and existing.generated_at is not None:
if self.is_semantically_equivalent(existing):
if self.generated_at is None:
self.generated_at = existing.generated_at
else:
self.generated_at = datetime.now(timezone.utc).isoformat()
|
Thanks for triggering the review, Sergio Sisternes (@sergio-sisternes-epam). I've implemented the follow-ups now instead of waiting. Let me know if you need anything else. |
c177d44 to
1249581
Compare
|
Daniel Meppiel (@danielmeppiel), could you try merging this again? I’ve pushed the CI fix, and both coverage test shards now pass locally. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 1 | 1 | 0 | Restore Agent Plugin archive reproducibility and guard canonical timestamp ownership. |
| CLI Logging Expert | 0 | 0 | 1 | CLI output is accurate; only the help text lacks the legacy qualifier. |
| DevX UX Expert | 1 | 0 | 0 | Preserve a stable pack timestamp for new lockfiles. |
| Supply Chain Security Expert | 1 | 0 | 0 | Identical Agent Plugin exports must retain identical archive digests. |
| OSS Growth Hacker | 0 | 1 | 1 | Clarify when existing repositories receive the conflict-reduction benefit. |
| Doc Writer | 0 | 1 | 0 | Separate the new-lockfile benefit from legacy preservation. |
| Test Coverage Expert | 0 | 0 | 0 | Existing lockfile and SBOM promises have regression coverage. |
| Performance Expert | 0 | 1 | 0 | The write path reparses existing YAML unnecessarily. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Use
SOURCE_DATE_EPOCH, then the Unix epoch, when packing from timestamp-free lockfiles and add the archive reproducibility test -- wall-clock metadata currently makes identical exports produce different bytes and digests. - [Python Architect] Add a static architecture check preventing
generated_atassignments outside the canonical lockfile timestamp owner -- automated enforcement prevents future call sites from bypassing the centralized policy. - [Doc Writer] Clarify the changelog and
--timestamphelp wording for new versus legacy lockfiles -- conflict reduction comes from omission, while existing repositories retain the legacy field until removal.
Architecture
classDiagram
class LockFile {
+generated_at str?
+to_yaml() str
+write(path) None
}
class MCPIntegrator
class AgentPluginExporter
class LockfileEnrichment
class LockExportModule
MCPIntegrator ..> LockFile : delegates persistence
AgentPluginExporter ..> LockFile : reads generated_at
AgentPluginExporter ..> LockfileEnrichment : passes packed_at
LockExportModule ..> LockFile : reads
flowchart TD
A["Install or lock"] --> W["LockFile.write"]
W --> G{"Existing generated_at?"}
G -- no --> O["Omit generated_at"]
G -- yes --> S{"Semantic content changed?"}
S -- no --> P["Preserve legacy timestamp"]
S -- yes --> R["Refresh legacy timestamp"]
O --> D["Deterministic lockfile"]
P --> D
R --> D
AP["Agent Plugin pack"] --> E["generated_at passed as packed_at"]
E --> N{"packed_at is None?"}
N -- yes --> C["Current wall clock"]
C --> X["Archive bytes differ"]
Recommendation
Revise the PR to restore deterministic Agent Plugin archive timestamps and add the timestamp-free reproducibility regression test. Include the timestamp-ownership guard and in-scope migration wording before asking maintainers to ship.
Full per-persona findings
Python Architect
- [blocking] Timestamp-free lockfiles make Agent Plugin archives non-reproducible at
src/apm_cli/deps/lockfile.py:709.
agent_plugin_exporterforwardsNoneaspacked_at, and lockfile enrichment substitutes the wall clock.
Suggested: UseSOURCE_DATE_EPOCH, then the Unix epoch, and extend archive reproducibility coverage. - [recommended] Guard centralized lockfile timestamp ownership statically at
src/apm_cli/deps/lockfile.py:914.
The change centralizes timestamp policy and removes the MCP assignment, so canonical authority discipline requires a static boundary check and matching architecture test.
CLI Logging Expert
- [nit] Label
generated_atas legacy in--timestamphelp atsrc/apm_cli/commands/lock.py:280.
Documentation uses the qualifier, and CLI help should match.
DevX UX Expert
- [blocking] Timestamp-free lockfiles break reproducible Agent Plugin archives at
src/apm_cli/deps/lockfile.py:709.
Repeated packs from identical inputs receive different wall-clock metadata.
Proof (missing at):tests/unit/test_agent_plugin_exporter.py::test_agent_bundle_archives_are_reproducible.
Supply Chain Security Expert
- [blocking] Timestamp-free lockfiles make identical Agent Plugin exports produce different archive bytes at
src/apm_cli/bundle/agent_plugin_exporter.py:499.
Changing archive digests weakens independent artifact verification.
Proof (missing at):tests/unit/test_agent_plugin_exporter.py::test_agent_bundle_archives_are_reproducible timestamp-free variant.
OSS Growth Hacker
- [recommended] Separate the new-lockfile benefit from legacy compatibility in release messaging at
CHANGELOG.md:26.
Existing users need a clear one-time removal migration to receive the benefit. - [nit] Call
generated_atlegacy in CLI help too atsrc/apm_cli/commands/lock.py:280.
Auth Expert -- inactive
The touched lockfile, MCP integration, and lock command files do not affect authentication.
Doc Writer
- [recommended] Separate the new-lockfile benefit from legacy timestamp preservation at
CHANGELOG.md:27.
The current grammar can imply that refreshing legacy timestamps prevents conflicts.
Test Coverage Expert
No findings.
Performance Expert
- [recommended] Avoid reparsing the existing lockfile on every write at
src/apm_cli/deps/lockfile.py:928.
Timestamp-free files add one full YAML parse and legacy files can parse twice.
Suggested: Parse existing text once and consider accepting an already-loaded lockfile.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
APM Spec Guardian:
|
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 0 | Runtime ownership is coherent; the recommended fallback guard is folded. |
| CLI Logging Expert | 0 | 0 | 0 | CLI help clearly states the deterministic timestamp fallback. |
| DevX UX Expert | 0 | 1 | 0 | The implementation and examples now converge on the timestamp-free default. |
| Supply Chain Security Expert | 0 | 0 | 0 | Lockfile integrity and deterministic output remain intact. |
| OSS Growth Hacker | 0 | 1 | 0 | The deterministic-lockfile story and release wording are aligned. |
| Doc Writer | 0 | 2 | 0 | Epoch wording and OpenAPM examples now match the implementation. |
| Test Coverage Expert | 0 | 1 | 0 | Archive reproducibility is tested and mapped in Scenario Evidence. |
| Performance Expert | 0 | 1 | 0 | Changed installs reuse the fresh destination parse. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
direction LR
class LockfileModule {
<<CanonicalAuthority>>
+resolve_reproducible_timestamp(explicit, generated_at) str
}
class LockFile {
<<DataclassAggregate>>
+generated_at str?
+write(path, existing_lockfile) None
+to_yaml() str
+is_semantically_equivalent(other) bool
}
class MCPIntegrator
class LockCommandModule {
<<CLIAdapter>>
}
class AgentPluginExporterModule {
<<BundleAdapter>>
}
LockfileModule *-- LockFile : defines
MCPIntegrator ..> LockFile : delegates persistence
LockCommandModule ..> LockfileModule : resolves timestamp
AgentPluginExporterModule ..> LockfileModule : resolves packed_at
class LockfileModule:::touched
class LockFile:::touched
class MCPIntegrator:::touched
class LockCommandModule:::touched
class AgentPluginExporterModule:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["Install builds lockfile"] --> B["Read destination once"]
B --> C["LockFile.write with existing state"]
C --> D{"Destination has generated_at?"}
D -- No --> E["Omit timestamp"]
D -- Yes --> F{"Semantic content changed?"}
F -- No --> G["Preserve legacy timestamp"]
F -- Yes --> H["Refresh legacy timestamp"]
I["SBOM or Agent Plugin export"] --> J["resolve_reproducible_timestamp"]
J --> K["Explicit timestamp"]
J --> L["SOURCE_DATE_EPOCH"]
J --> M["Legacy generated_at"]
J --> N["Fixed epoch"]
Recommendation
Ship this change. All panel recommendations are incorporated, exact-head validation passes, all GitHub workflows are green, and no additional follow-up remains.
Full per-persona findings
Python Architect
- [recommended] Extend the static boundary to defend reproducible fallback ownership at
scripts/lint-architecture-boundaries.sh:247.
The owner table covers both timestamp writes and fallback policy. Folded: the AST guard and mutation test now reject directSOURCE_DATE_EPOCHor fixed-epoch policy outsidedeps/lockfile.py.
CLI Logging Expert
No findings.
DevX UX Expert
- [recommended] Align remaining spec examples with the timestamp-free default at
docs/src/content/docs/specs/openapm-v0.1.md:822.
Folded: all three unqualified producer examples now omitgenerated_at.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
- [recommended] Align the release-note timestamp claim with shipped behavior at
CHANGELOG.md:31.
Folded: the entry now describes byte reproducibility throughSOURCE_DATE_EPOCHor a fixed epoch without conflating metadata fields.
Auth Expert -- inactive
Touched files cover lockfile timestamp handling, bundle export, and related docs, specs, and tests; no authentication behavior changed.
Doc Writer
- [recommended] Correct the Agent Plugin fallback epoch description at
CHANGELOG.md:31.
Folded with precise byte-reproducibility wording. - [recommended] Align the OpenAPM lockfile examples with the new default at
docs/src/content/docs/specs/openapm-v0.1.md:1147.
Folded by removinggenerated_atfrom the three default examples.
Test Coverage Expert
- [recommended] Map Agent Plugin archive reproducibility in Scenario Evidence at
src/apm_cli/bundle/agent_plugin_exporter.py:504.
Folded into the PR body withtests/unit/test_agent_plugin_exporter.py::test_agent_bundle_archives_are_reproducible; its four archive variants pass.
Performance Expert
- [recommended] Avoid reparsing the existing lockfile during changed writes at
src/apm_cli/deps/lockfile.py:957.
Folded: the install builder passes its fresh destination read intoLockFile.save, and a regression test proves no second YAML load occurs.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
e3813d9 to
668a03f
Compare
e3813d9 to
4e22dbc
Compare
|
Daniel Meppiel (@danielmeppiel) originally clobbered some of your commits, so restored and rebased on main to correct merge conflicts. Should be good now! |
d4bd46f to
991efc4
Compare
991efc4 to
e067b3e
Compare
apm-spec-waiver: behavior is governed by existing req-lk-005 amendment
e067b3e to
5d31068
Compare
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 0 | Timestamp policy is correctly centralized, with two recommended guardrail and repairability fixes. |
| CLI Logging Expert | 0 | 0 | 0 | CLI help and verbose diagnostics remain accurate, concise, and appropriately scoped. |
| DevX UX Expert | 0 | 0 | 0 | The change improves lockfile determinism and merge ergonomics while documenting legacy behavior clearly. |
| Supply Chain Security Expert | 0 | 0 | 0 | No supply-chain security concerns; integrity, provenance, and fail-closed behavior remain intact. |
| OSS Growth Hacker | 0 | 0 | 0 | Clear user benefit: new lockfiles avoid timestamp-only conflicts while preserving deterministic exports. |
| Doc Writer | 0 | 0 | 1 | Documentation is accurate and discoverable; only the CHANGELOG wording needs minor clarification. |
| Test Coverage Expert | 0 | 0 | 0 | Timestamp-free creation, legacy handling, SBOM fallback, and MCP/LSP determinism have regression traps. |
| Performance Expert | 0 | 1 | 0 | An avoidable O(B) lockfile reparse adds measurable cost per existing-file write. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Python Architect] Make malformed legacy timestamp-bearing lockfiles repairable during substantive replacement. -- Optional
generated_atmetadata should not determine whether APM can atomically repair an otherwise replaceable destination. - [Performance Expert] Pass preloaded lockfile snapshots through hot writers, beginning with MCPIntegrator, and add parse-count or scaling guards. -- The measured redundant parse is avoidable where callers already hold the parsed value.
- [Python Architect] Extend the architecture boundary guard to detect
LockFile(generated_at=...)constructor bypasses. -- Protecting the canonical timestamp-policy owner prevents future call sites from silently fragmenting behavior.
Architecture
classDiagram
direction LR
class LockfileModule {
<<PolicyModule>>
+resolve_reproducible_timestamp(explicit, legacy) str
}
class LockFile {
<<Facade>>
+generated_at optional str
+write(path, existing_lockfile) None
+save(path, existing_lockfile) None
+is_semantically_equivalent(other) bool
}
class LockfileBuilder {
<<Orchestrator>>
+_write_if_changed(lockfile, path, type) None
}
class MCPIntegrator {
+update_lockfile(names, path) None
}
class LockCommandModule {
<<Module>>
+lock_export(fmt, output, global, timestamp) None
}
LockfileModule o-- LockFile : defines
LockfileBuilder ..> LockFile : saves preloaded destination
MCPIntegrator ..> LockFile : persists MCP state
LockCommandModule ..> LockfileModule : resolves SBOM timestamp
flowchart TD
I["apm install"] --> B["LockfileBuilder._write_if_changed"]
I --> M["MCPIntegrator.update_lockfile"]
B --> S["LockFile.save(existing_lockfile=existing)"]
M --> S2["LockFile.save(path)"]
S --> W["LockFile.write"]
S2 --> W
W --> G{"Existing destination carries generated_at?"}
G -->|no| N["Omit generated_at"]
G -->|yes, no-op| P["Preserve generated_at"]
G -->|yes, changed| R["Refresh generated_at"]
N --> A["Atomic write"]
P --> A
R --> A
E["apm lock export"] --> H["resolve_reproducible_timestamp"]
H --> D{"Explicit or SOURCE_DATE_EPOCH?"}
D -->|yes| X["Use supplied timestamp"]
D -->|no| L{"Legacy generated_at?"}
L -->|yes| X
L -->|no| U["Use Unix epoch"]
Recommendation
Ship the reproducibility fix on its current scope, then prioritize the malformed legacy-file repair path; track the constructor-boundary guard and redundant parsing optimization as focused hardening work.
Full per-persona findings
Python Architect
- [recommended] Cover
LockFileconstructor timestamps in the ownership guard atscripts/lint-architecture-boundaries.sh:293
The AST guard detects attribute assignments but notLockFile(generated_at=...), leaving a direct path for split authority to return.
Suggested: DetectLockFilecalls with agenerated_atkeyword outsidedeps/lockfile.pyand add a matching negative fixture. - [recommended] Keep malformed legacy destinations repairable at
src/apm_cli/deps/lockfile.py:975
A malformed timestamp-free destination is repairable, while the same destination with deprecatedgenerated_atraises before atomic replacement.
Suggested: Treat an unconstructable legacy destination as a substantive replacement while retaining its timestamp policy, and add the timestamp-bearing counterpart test.
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 diff does not touch authentication, token resolution, host classification, authorization headers, or AuthResolver call sites.
Doc Writer
- [nit] Clarify when legacy
generated_atis preserved versus refreshed atCHANGELOG.md:106
Existing lockfiles preserve it on semantic no-ops and refresh it only on substantive writes.
Test Coverage Expert
No findings.
Performance Expert
- [recommended] Thread preloaded lockfiles through every hot-path write at
src/apm_cli/deps/lockfile.py:968
LockFile.write()reparses an existing destination whenexisting_lockfileis omitted. MCPIntegrator already holds the parsed value but omits it when saving.
Suggested: Pass pre-mutation snapshots through hot writers and add a parse-count regression guard.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Keep legacy timestamp metadata from making otherwise replaceable malformed lockfiles irreparable. Addresses panel lockfile repair follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid reparsing lockfile YAML when MCP persistence already loaded the exact destination. Addresses panel performance follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extend the canonical-owner guard to catch LockFile generated_at constructor writes outside the owner. Addresses panel architecture follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e9d6f82 to
5d31068
Compare
Shepherd terminal advisoryAll in-scope panel follow-ups were folded into this branch:
Regression-trap mutation checks failed as expected with each guard removed, then passed after restoration. The affected unit set passed (161 tests), the architecture boundary lint passed, and the branch CI lint mirror completed successfully. GitHub Actions did not create a |
|
Lachlan Heywood (@lachieh) thanks a lot for this work, took me a bit of time but here we are, merged! |
fix(lockfile): omit generated_at from new lockfiles
TL;DR
New
apm.lock.yamlfiles no longer include the volatilegenerated_atfield, preventing unrelated dependency changes from manufacturing a merge conflict. Existing lockfiles that already contain the field keep their legacy behavior: no-op writes preserve it and substantive writes refresh it.Note
Removing
generated_atonce opts an existing lockfile into timestamp-free output; later writes will not add it back.Problem (WHY)
generated_atkey is consistently causing merge conflicts."apm lock exportused the legacy field as one timestamp fallback, so timestamp-free lockfiles need a deterministic final fallback.Why these matter: the expected behavior is explicit--"Merging unrelated changes should not cause merge conflicts". The fix must also preserve compatibility for repositories that already carry the field and deterministic SBOM output for repositories that do not.
Approach (WHAT)
LockFile.generated_atoptional and omit it during serialization when absent.LockFile.write()policy.SOURCE_DATE_EPOCHand a legacy lockfile timestamp.Implementation (HOW)
src/apm_cli/deps/lockfile.pygenerated_atoptional, conditionally serializes it, and centralizes legacy preservation/refresh behavior inLockFile.write().src/apm_cli/integration/mcp_integrator.pysrc/apm_cli/commands/lock.pyCHANGELOG.mddocs/src/content/docs/concepts/package-anatomy.mddocs/src/content/docs/reference/cli/lock.mddocs/src/content/docs/reference/cli/pack.mddocs/src/content/docs/reference/lockfile-spec.mdpackages/apm-guide/.apm/skills/apm-usage/commands.mdtests/test_lockfile.pytests/unit/commands/test_lock_export_command.pytests/unit/install/test_mcp_lockfile_determinism.pytests/unit/integration/test_mcp_integrator.pygenerated_at.tests/integration/test_install_lsp_lockfile_determinism.pytests/integration/test_install_mcp_lockfile_determinism.pytests/integration/test_cache_lockfile_parity.pytests/integration/test_config_surface_lifecycle_contract.pytests/integration/test_oci_mcp_lifecycle_contract.pyDiagrams
Legend: dashed nodes are the changed timestamp decisions; first check whether the on-disk lockfile opted into legacy metadata, then preserve or refresh it.
flowchart LR N["New LockFile"] --> W["LockFile.write"] E["Existing apm.lock.yaml"] --> R["LockFile.read"] R --> W W --> H{"Existing generated_at?"} H -->|No| O["to_yaml omits generated_at"] H -->|Yes| S{"Semantic content changed?"} S -->|No| P["Preserve existing timestamp"] S -->|Yes| U["Refresh UTC timestamp"] P --> I["to_yaml includes generated_at"] U --> I classDef new stroke-dasharray: 5 5; class H,O,P,U new;Trade-offs
generated_atexcluded from equivalence checks; the field remains metadata rather than dependency state.Benefits
Validation
uv run --frozen --extra dev ruff check src/ tests/:uv run --frozen --extra dev ruff format --check src/ tests/:Focused unit results (124 tests)
uv run --frozen --extra dev pytest -p no:cacheprovider -q tests/test_lockfile.py tests/unit/commands/test_lock_export_command.py tests/unit/install/test_mcp_lockfile_determinism.py tests/unit/integration/test_mcp_integrator.py::TestUpdateLockfile:Affected integration results (26 passed, 8 skipped)
uv run --frozen --extra dev pytest -p no:cacheprovider -q --tb=short tests/integration/test_cache_lockfile_parity.py tests/integration/test_config_surface_lifecycle_contract.py tests/integration/test_install_lsp_lockfile_determinism.py tests/integration/test_install_mcp_lockfile_determinism.py tests/integration/test_oci_mcp_lifecycle_contract.py:The remaining lint guards, duplication check, and architecture/auth boundary
scripts completed with exit code 0. Earlier broad validation also completed
with 19,940 unit tests passing and 159 specification tests passing.
Scenario Evidence
tests/test_lockfile.py::TestLockFile::test_to_yamltests/integration/test_install_mcp_lockfile_determinism.py::test_installed_mcp_lifecycle_is_no_write_until_real_target_change(regression-trap for #2572)tests/test_lockfile.py::TestLockFile::test_write_refreshes_existing_generated_attests/unit/install/test_mcp_lockfile_determinism.py::test_changed_mcp_dependencies_refresh_legacy_generated_attests/unit/commands/test_lock_export_command.py::test_export_without_generated_at_uses_fixed_epochtests/integration/test_install_lsp_lockfile_determinism.py::test_repeated_install_with_unchanged_lsp_keeps_lockfile_bytestests/unit/test_agent_plugin_exporter.py::test_agent_bundle_archives_are_reproducibleHow to test
generated_atis absent.generated_atto an existing lockfile, make a substantive dependency change, and confirm the field remains present with a refreshed value.generated_atfrom that lockfile, write it again, and confirm the field is not reintroduced.apm lock exportwithoutSOURCE_DATE_EPOCHand confirm the SBOM timestamp is1970-01-01T00:00:00+00:00.Closes #2572