Skip to content

fix(audit): make per-deployed-file content hash line-ending invariant (#1952)#1959

Merged
danielmeppiel merged 4 commits into
mainfrom
danielmeppiel-fix-1952-crlf-content-integrity
Jun 29, 2026
Merged

fix(audit): make per-deployed-file content hash line-ending invariant (#1952)#1959
danielmeppiel merged 4 commits into
mainfrom
danielmeppiel-fix-1952-crlf-content-integrity

Conversation

@danielmeppiel

Copy link
Copy Markdown
Collaborator

TL;DR

apm install records deployed_file_hashes over raw on-disk bytes, so a text file that git checks out as CRLF on a Windows core.autocrlf=true machine and as LF on Linux CI hashes differently on each platform — and apm audit --ci reports a false content-integrity drift that fails the gate. This PR moves the per-deployed-file hash to a canonical content domain (UTF-8 text normalized CRLF → LF, bare CR preserved, binary hashed raw) at the single compute_file_hash chokepoint both record and verify share, so the hash is identical on every OS. It is a behavior-changing normative amendment to spec req-lk-012 (revision 0.1.8).

Note

Closes #1952. Migration is one apm install: lockfiles whose hashes were recorded on Windows before this fix re-record once into the canonical domain; LF-recorded lockfiles are byte-identical under the new domain and never drift.

Problem (WHY)

  • compute_file_hash hashes the raw on-disk bytes, so the recorded hash carries the checkout's line endings. A Windows core.autocrlf=true checkout materializes text as \r\n; the lockfile then stores CRLF-domain hashes.
  • On a Linux CI checkout the same files are LF, so apm audit --ci re-hashes \n content and the envelope mismatches — a content-integrity failure with exit 1 even though no human touched the file.
  • [!] A false positive on an integrity gate is worse than a false negative: it trains developers to ignore or disable the gate.

Why these matter: the per-deployed-file hash is meant to witness content, but line endings are not content — they are a platform/git-config artifact. No peer package manager (npm, pip, cargo) false-positives on autocrlf, because they hash the canonical published artifact, never the platform-local working tree. The drift-replay audit in this same codebase already tolerates line endings (_normalize_line_endings in src/apm_cli/utils/normalization.py, "Convert CRLF to LF; leaves bare CR alone"); content-integrity was the lone surface that did not, so the two audits disagreed about what counts as a change.

Approach (WHAT)

# Fix (and why, if non-obvious)
1 Normalize at the single chokepoint compute_file_hash, so record (apm install) and verify (apm audit) become symmetric by construction — every call site inherits the fix.
2 Detect text by content (UTF-8-decodable + no NUL byte), not by suffix — integrators rename deployed files (e.g. .md.mdc for Cursor), so a suffix allowlist would miss them and reintroduce the bug.
3 Collapse only \r\n\n; preserve a lone \r — the carriage-return smuggling vector stays hash-visible; only the benign platform difference is made invisible.
4 Amend normative req-lk-012 (domain redefinition) + req-lk-017 wording + a 0.1.8 revision row; harmonize content-integrity with the drift-replay normalizer.

Implementation (HOW)

  • src/apm_cli/utils/content_hash.py — new module helper _canonical_hash_bytes(raw) (binary → raw; non-UTF-8 → raw; else CRLF → LF via the existing normalize_crlf_to_lf); compute_file_hash now hashes _canonical_hash_bytes(...). compute_package_hash (the registry archive / package-tree hash, a separate contract) is deliberately untouched.
  • tests/unit/test_content_hash.py — flipped the old test_hashes_raw_bytes_as_written (which asserted CRLF ≠ LF) into three tests: CRLF == LF for text, bare-CR ≠ LF, binary hashed raw.
  • docs/src/content/docs/specs/openapm-v0.1.md — rewrote req-lk-012 to define the canonical content domain; reworded req-lk-017 to re-verify against that domain; added the 0.1.8 revision-history row (normative amendment, semver-zero 0.x minor; no statement-count change).
  • scripts/crlf_invariance_probe.py + .github/workflows/crlf-invariance.yml — empirical proof harness (see Diagrams + Validation).
  • docs/src/content/docs/reference/lockfile-spec.md, packages/apm-guide/.apm/skills/apm-usage/governance.md, CHANGELOG.md — doc-sync.

Diagrams

Legend: the record and verify sides both route through one function; normalizing there makes a Windows-recorded hash equal a Linux-verified hash. Look at the merge point — that symmetry is the whole fix.

flowchart LR
  subgraph Record["apm install (any OS)"]
    R1["read_bytes"] --> R2["compute_file_hash"]
  end
  subgraph Verify["apm audit (any OS)"]
    V1["read_bytes"] --> V2["compute_file_hash"]
  end
  R2 --> C["_canonical_hash_bytes"]
  V2 --> C
  C --> D{"NUL byte or non-UTF-8?"}
  D -->|"yes (binary)"| RAW["hash raw bytes"]
  D -->|"no (text)"| NORM["CRLF to LF, keep bare CR"]
  RAW --> H["sha256 envelope"]
  NORM --> H
  H --> EQ["platform-invariant: Windows record == Linux verify"]
Loading

Legend: the CI workflow proves invariance empirically by driving real git autocrlf translation through the product function on three OSes and asserting the hashes are byte-identical.

flowchart TD
  P["crlf_invariance_probe.py: git init, commit LF, autocrlf=true, checkout, compute_file_hash"]
  P --> U["ubuntu-latest -> hash.txt"]
  P --> W["windows-latest -> hash.txt"]
  P --> M["macos-latest -> hash.txt"]
  U --> G{"all 3 hashes identical?"}
  W --> G
  M --> G
  G -->|"yes"| OK["job passes"]
  G -->|"no"| FAIL["exit 1: apm#1952 regression"]
Loading

Trade-offs

  • Hash domain vs. deploy-time normalization. Chose hash-domain normalization (fix what the hash means) over normalizing files at deploy time. Deploy-time only fixes the record side; a Windows apm audit on a fresh checkout (consumer autocrlf re-CRLFs committed files) would still drift and recur on every checkout — it moves the false positive rather than closing it.
  • Content-based vs. suffix-based text detection. Chose UTF-8-decodable + no-NUL. A suffix allowlist is simpler but misses integrator-renamed files and is asymmetric across platforms.
  • Security surface bounded. CRLF↔LF is made hash-invisible; a bare \r (the real overwrite-smuggling vector) still changes the hash, and binary is untouched.
  • Spec cost is real. This amends a published normative MUST and is subject to the §9.3 amendment panel + comment window — handled at review time, not bypassed.

Benefits

  1. apm audit --ci no longer false-positives on CRLF/LF across platforms — the same lockfile verifies green on Windows, Linux, and macOS.
  2. Record and verify are symmetric by construction: one chokepoint, every caller fixed (install record, audit verify, cleanup provenance).
  3. Content-integrity and drift-replay now agree on line endings, removing a latent contradiction between the two audit surfaces.
  4. Cross-platform invariance is continuously proven by a 3-OS CI job, not asserted — a regression fails the build.
  5. Migration cost is one apm install, identical to any lockfile-format bump.

Validation

Full local lint chain (the CI Lint job mirror) is green: ruff check, ruff format --check, pylint R0801 (10.00/10), lint-auth-signals.sh.

Cross-platform probe, run locally on macOS — note the file is CRLF on disk yet the hash equals the pure-LF hash:

os=Darwin on_disk_eol=CRLF hash=sha256:253aebd9aeb6473ecc91d2bfaa196fc90e1eef2827c3f0cbf5e30ef9023448b8
Targeted suites: content_hash + policy + conformance + audit/drift/install (all green)
tests/unit/test_content_hash.py ............................. 29 passed
tests/unit/policy + tests/spec_conformance ... 1111 passed, 2 skipped, 1 xfailed
audit + drift + install suites ............... 1851 passed

Important

The decisive proof is the CRLF Invariance (apm#1952) workflow on this PR: the windows-latest runner (real core.autocrlf=true, CRLF on disk) must emit the same hash as the ubuntu-latest runner (LF on disk). The gather job fails if they differ.

Scenario Evidence

# Scenario (user promise) Principle(s) Test(s) proving it Type
1 A file recorded on Windows (CRLF) verifies clean on Linux (LF) — no false drift Portability tests/unit/test_content_hash.py::TestComputeFileHash::test_text_crlf_and_lf_hash_equal unit
2 A tampering \r overwrite still trips the integrity hash Secure by default tests/unit/test_content_hash.py::TestComputeFileHash::test_bare_cr_still_distinct_from_lf unit
3 Binary (images, UTF-16) is hashed raw, never line-normalized Governed by policy tests/unit/test_content_hash.py::TestComputeFileHash::test_binary_hashed_raw unit
4 The recorded hash is identical on Windows, Linux, and macOS Portability / Multi-harness scripts/crlf_invariance_probe.py via .github/workflows/crlf-invariance.yml e2e

How to test

  • uv run --extra dev pytest tests/unit/test_content_hash.py → 29 passed (CRLF==LF, bare-CR!=LF, binary-raw).
  • uv run python scripts/crlf_invariance_probe.py → prints a hash and exits 0 (asserts in-process invariants).
  • Open this PR and watch the CRLF Invariance (apm#1952) workflow → the Assert identical hashes job passes (ubuntu/windows/macos hashes match).
  • On a Windows checkout with git config core.autocrlf true: apm install then apm audit --ci on Linux → no content-integrity finding.

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

…#1952)

`apm install` records `deployed_file_hashes` over raw on-disk bytes. On a
Windows checkout with git `core.autocrlf=true`, text files materialize as
CRLF, so the lockfile records CRLF-domain hashes; on a Linux CI checkout the
same files are LF, so `apm audit --ci` re-hashes LF and reports a false
`content-integrity` drift. A false positive on an integrity gate trains
developers to ignore it.

Fix at the single chokepoint both record and verify share
(`compute_file_hash`): hash UTF-8 text over its canonical CRLF -> LF form
(a lone CR is preserved, so the carriage-return smuggling vector stays
hash-visible) and hash binary raw. Detection is content-based (UTF-8
decodable + no NUL byte), not suffix-based, so integrator renames such as
`.md` -> `.mdc` are still normalized. The record side (install) and verify
side (audit) become symmetric across operating systems by construction.

This is a behavior-changing normative amendment to spec req-lk-012
(revision 0.1.8): the hash domain moves from "bytes as written to disk" to
canonical content. req-lk-017 reworded for consistency. Migration: lockfiles
recorded on Windows before this fix carry CRLF-domain hashes and re-record
once on the next `apm install`; LF-domain lockfiles are unaffected.

Empirical proof: `scripts/crlf_invariance_probe.py` drives real git
`core.autocrlf=true` translation through `compute_file_hash`, and
`.github/workflows/crlf-invariance.yml` runs it on ubuntu/windows/macos and
asserts all three recorded hashes are byte-identical.

- src/apm_cli/utils/content_hash.py: add `_canonical_hash_bytes`; hash it
- tests/unit/test_content_hash.py: CRLF==LF, bare-CR!=LF, binary-raw
- docs/specs/openapm-v0.1.md: amend req-lk-012/req-lk-017 + 0.1.8 row
- docs/reference/lockfile-spec.md, apm-guide governance.md, CHANGELOG.md
- scripts/crlf_invariance_probe.py + .github/workflows/crlf-invariance.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 29, 2026 20:19
…eps against 3.14

The probe job let setup-uv default to the runner's latest Python (3.14 on
macos-latest), where pydantic-core's pyo3-ffi native wheel fails to build.
Pin to PYTHON_VERSION 3.12 via setup-python before setup-uv, matching the
ci.yml convention, so all three OS probes resolve the same interpreter.

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

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 makes the per-deployed-file content hash used by apm install / apm audit --ci line-ending invariant, preventing false content-integrity drift when the same text file is materialized as CRLF on Windows and LF on POSIX. It does this by canonicalizing UTF-8 text content (\r\n -> \n, preserving bare \r) at the shared compute_file_hash chokepoint, and updates unit tests, spec/docs, and adds a cross-OS probe workflow.

Changes:

  • Canonicalize per-file hashing for UTF-8 text to be CRLF/LF invariant (binary hashed raw).
  • Update/expand unit tests to cover CRLF==LF for text, bare-CR != LF, and binary raw hashing.
  • Amend OpenAPM spec + lockfile docs + governance docs + changelog, and add a CI probe workflow to validate invariance.
Show a summary per file
File Description
src/apm_cli/utils/content_hash.py Adds canonicalization logic for per-file hashes to avoid CRLF/LF drift across platforms.
tests/unit/test_content_hash.py Updates tests to validate the new canonical hashing behavior for text vs binary inputs.
scripts/crlf_invariance_probe.py Adds an empirical probe script intended to exercise core.autocrlf behavior and compare hashes.
.github/workflows/crlf-invariance.yml Adds a 3-OS workflow to run the probe and assert identical hashes across platforms.
docs/src/content/docs/specs/openapm-v0.1.md Updates normative requirements (req-lk-012, req-lk-017) and revision history for the new hash domain.
docs/src/content/docs/reference/lockfile-spec.md Documents canonical hashing semantics for deployed_file_hashes.
packages/apm-guide/.apm/skills/apm-usage/governance.md Updates baseline-check documentation for content-integrity to include hash drift + canonicalization note.
CHANGELOG.md Adds a Fixed entry describing the CRLF/LF drift false-positive fix.

Review details

  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread scripts/crlf_invariance_probe.py Outdated
Comment on lines +66 to +69
# Turn on the exact setting that triggers apm#1952, then re-materialize.
_git(["config", "core.autocrlf", "true"], work)
sample.unlink()
_git(["checkout", "--", "sample.md"], work)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch — fixed in the latest commit. The probe now picks the setting each platform actually ships: core.autocrlf=true on Windows (CRLF working tree) and core.autocrlf=input on POSIX (LF working tree). The git roundtrip now reproduces the genuine Windows-CRLF vs POSIX-LF on-disk split instead of forcing CRLF everywhere, so the cross-OS gather is a direct proof of line-ending invariance. Verified locally on macOS: on_disk_eol=LF, canonical hash unchanged (sha256:253aebd9...). The CI matrix will confirm Windows reports CRLF and both collapse to the same hash.

…s-POSIX split

The probe hardcoded core.autocrlf=true on every platform, so the POSIX
roundtrip also materialized CRLF on checkout -- meaning the git roundtrip
did not actually exercise the Windows-CRLF vs POSIX-LF divergence that
produced the apm#1952 false drift (caught by copilot review on PR #1959).

Pick the setting each platform really ships: true on Windows (CRLF working
tree), input on POSIX (LF working tree). The cross-OS gather now proves the
genuine on-disk split (CRLF on Windows, LF on POSIX) collapses to one
canonical hash. Verified locally on macOS: on_disk_eol=LF, hash unchanged
(sha256:253aebd9...). Docstring corrected to match.

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

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_with_followups

Eliminates CRLF false-positive audit failures on Windows, making content-integrity a trustworthy gate across all three OS targets.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All eight active panelists converge on ship. Supply-chain-security -- the highest-weight voice for a default-behavior integrity gate -- explicitly signed off: bare-CR smuggling stays caught, binary is fail-closed, and the normalization is a narrowing (less triggers, not less protection). Performance found no regression. The only material gap is at the integration-test tier: test-coverage identified a missing audit-VERIFY-side test that exercises the exact user scenario (#1952). This is an outcome: missing finding on a governed-by-policy + secure-by-default surface, which per weighting rules ranks above all opinion-tier recommended findings. It is small (~15 lines) and belongs in THIS PR before merge to ensure the regression-trap actually guards the promise. The doc-writer caught two enterprise pages (drift-detection.md, enforce-in-ci.md) that still overclaim 'any byte-level change'. These are factually false post-merge and should be fixed in this PR -- a one-clause edit each.

Convergent lower-signal items (python-architect's layering relocation to utils/normalization.py, devx's --rehash escape hatch, security + python-architect's docstring on compute_package_hash asymmetry) are all real but scoped for follow-ups -- none block the safety promise delivered here. The governance.md bullet density note from three panelists is a nit; I defer to the author's judgement on that one.

Aligned with: Portable by manifest (hash now produces identical envelopes on Windows (CRLF), macOS, and Linux (LF) for the same logical text), Secure by default (normalization is narrowly scoped: bare CR stays hash-visible, binary is hash-raw, fail-closed preserved), and Governed by policy (normative spec req-lk-012 amended alongside lockfile-spec.md, governance.md, and CHANGELOG atomically).

Growth signal. This fix removes a day-one false alarm for the exact cross-platform team APM targets: Windows devs running audit in Linux CI. The 3-OS crlf-invariance.yml workflow is a reusable proof-of-correctness pattern that signals maturity to enterprise evaluators. Worth a release-note one-liner: "apm audit now passes identically on Windows, macOS, and Linux -- no more CRLF false alarms."

Panel summary

Persona B R N Takeaway
Python Architect 0 1 1 Chokepoint design is architecturally sound; content-based text detection correct; minor layering smell importing normalize_crlf_to_lf from atomic_io.
CLI Logging Expert 0 0 1 New user-facing strings follow ASCII/STATUS_SYMBOLS conventions; governance wording accurate and consistent with ci_checks.py messaging.
DevX UX Expert 0 1 1 Migration (re-record on next install) is acceptable for most teams but leaves CI-only flows without an explicit escape hatch; CHANGELOG wording is clear.
Supply Chain Security Expert 0 1 1 CRLF normalization carve-out is sound; bare-CR smuggling stays hash-visible, binary bypass guarded, fail-closed semantics preserved.
OSS Growth Hacker 0 0 1 CHANGELOG entry speaks directly to the Windows-dev papercut in user terms; no conversion-surface issues.
Doc Writer 0 1 1 Spec/lockfile/changelog/apm-guide edits are accurate and well-voiced; no blocking normative error. Two enterprise pages now overclaim content-integrity.
Test Coverage Expert 0 1 2 Unit + install-side integration tests cover the hash invariant; missing: audit-side integration test proving apm audit does not false-flag a CRLF-deployed file.
Performance Expert 0 0 3 Per-file canonicalization adds ~4-5us/file (total <1ms for 200 files); negligible on the install/audit hot path. Ship as-is.

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

Top 5 follow-ups

  1. [Test Coverage Expert] Add audit-VERIFY-side integration test: install (LF hash recorded) -> mutate deployed file to CRLF -> assert apm audit --ci passes content-integrity. -- outcome: missing on governed-by-policy + secure-by-default surface. This is the exact [BUG] apm install deploys skill-bundle files verbatim with CRLF, causing content-integrity hash drift in CI #1952 user scenario; without it the regression-trap only covers the RECORD side. ~15 lines in TestLocalContentAudit. Fold into THIS PR.
  2. [Doc Writer] Fix drift in drift-detection.md:128 and enforce-in-ci.md:115/126 -- reword 'any byte-level change' to 'any content change' with line-ending caveat. -- Factually false post-merge; two one-clause edits. Fold into THIS PR to avoid shipping contradictory docs.
  3. [Supply Chain Security Expert] Add docstring to compute_package_hash explaining why it intentionally hashes raw bytes (no normalization). -- Prevents a future maintainer from 'unifying' the two hash surfaces and accidentally weakening tree integrity. One-liner, low-cost; fold or immediate follow-up.
  4. [Python Architect] Relocate normalize_crlf_to_lf from atomic_io to utils/normalization.py to fix the read-path-imports-write-path layering smell. -- Compounds if more read-path callers appear; correct layering, but a larger-scope refactor. Defer to a follow-up PR.
  5. [DevX UX Expert] Emit a hint in apm audit failure output when a mismatch would pass under canonical normalization; or add apm audit --rehash for CI-only flows. -- Discoverability gap for frozen-lockfile CI teams; not urgent (workaround is documented) but good devx. Defer to a future PR.

Architecture

classDiagram
    direction LR
    class content_hash {
        <<Module>>
        +compute_package_hash(Path) str
        +compute_file_hash(Path) str
        -_canonical_hash_bytes(bytes) bytes
    }
    class atomic_io {
        <<Module - Write Path>>
        +normalize_crlf_to_lf(str) str
    }
    class normalization {
        <<Module - Read Path>>
        -_normalize_line_endings(bytes) bytes
    }
    class lockfile_phase {
        <<Module>>
        +compute_deployed_hashes(list, Path) dict
    }
    class ci_checks {
        <<Module>>
        -_check_content_integrity(Path) list
    }
    content_hash ..> atomic_io : imports normalize_crlf_to_lf
    lockfile_phase ..> content_hash : compute_file_hash
    ci_checks ..> content_hash : compute_file_hash
    atomic_io ..> normalization : mirrors logic
    note for content_hash "Chokepoint: single seam for\nper-file line-ending invariance"
    class content_hash:::touched
    class atomic_io:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A["apm install -- compute_deployed_hashes"] --> C["compute_file_hash(file_path)"]
    AA["apm audit --ci -- _check_content_integrity"] --> C
    C --> F["read_bytes()"]
    F --> G["_canonical_hash_bytes(raw)"]
    G --> H{"NUL byte in raw?"}
    H -- Yes --> I["return raw (binary)"]
    H -- No --> J{"UTF-8 decodable?"}
    J -- No --> I
    J -- Yes --> K["normalize CRLF to LF (bare CR preserved)"]
    K --> M["sha256 over canonical bytes"]
    I --> M
    M --> N["sha256:hex envelope"]
    N --> O["record: lockfile deployed_file_hashes"]
    N --> AD["verify: compare expected vs actual"]
    style G fill:#fff3b0,stroke:#d47600
    style K fill:#fff3b0,stroke:#d47600
Loading

Recommendation

Merge after folding two small in-PR items: (1) the audit-side CRLF integration test (~15 lines, closes the regression-trap gap on the exact #1952 scenario), and (2) the two-clause doc fix in drift-detection.md + enforce-in-ci.md. Both are bounded, same-scope, and prevent shipping a promise without a guardrail or docs that contradict it. Everything else defers cleanly.


Full per-persona findings

Python Architect

  • [recommended] normalize_crlf_to_lf lives in atomic_io (write-path) but is now consumed by content_hash (read-path) -- layering smell at src/apm_cli/utils/content_hash.py:7
    atomic_io.py is the atomic file-write primitive module; normalize_crlf_to_lf is a pure transform. content_hash now imports it for read-path normalization, coupling write-path and integrity-verify read path. utils/normalization.py already houses _normalize_line_endings (same logic). Promoting the transform there keeps the dependency arrow coherent. Not blocking (both in utils/, no circular import), but compounds if more read-path callers appear.
    Suggested: Move normalize_crlf_to_lf to utils/normalization.py as public API; have atomic_io and content_hash both import from normalization.
  • [nit] compute_package_hash still hashes raw bytes -- add a comment explaining the asymmetry at src/apm_cli/utils/content_hash.py:65
    compute_package_hash calls read_bytes() directly without _canonical_hash_bytes. A one-line comment prevents a future contributor from 'fixing' the apparent inconsistency.
    Suggested: Add comment: package hash mixes path + content for tree integrity; per-file normalization lives in compute_file_hash (apm#1952).

CLI Logging Expert

  • [nit] Probe stdout line lacks a STATUS_SYMBOL prefix at scripts/crlf_invariance_probe.py
    encoding.instructions.md uses [i]/[+]/[*] prefixes for CLI-adjacent output. The probe print line appears in CI logs; adding [i] keeps it consistent with the workflow's [+] success line.
    Suggested: print(f"[i] os={platform.system()} on_disk_eol={eol} hash={envelope}")

DevX UX Expert

  • [recommended] CI-only teams with frozen lockfiles have no explicit one-command escape from stale Windows-domain hashes at CHANGELOG.md
    Migration is 'hashes re-record on next apm install', but a team whose lockfile is committed and whose CI only runs apm audit --ci must discover on their own they need to run install. npm/pip/cargo offer rehash/fix subcommands or auto-upgrade on audit. A frozen-CI user hitting a new audit failure has no copy-paste command in the error output. Not blocking (workaround documented), but the discoverability gap matters for the exact cross-platform team this PR targets.
    Suggested: Follow-up (not this PR): when apm audit detects a mismatch that WOULD pass under canonical normalization, emit a hint 'this may be a stale CRLF-domain hash; run apm install to re-record', or add apm audit --rehash / apm install --lockfile-only for CI flows.
  • [nit] Governance content-integrity bullet is now long enough to wrap awkwardly at packages/apm-guide/.apm/skills/apm-usage/governance.md
    The bullet grew to a full sentence with parenthetical; a sub-bullet would scan better.

Supply Chain Security Expert

  • [recommended] compute_package_hash still hashes raw bytes while compute_file_hash normalizes -- document the intentional divergence at src/apm_cli/utils/content_hash.py:25
    compute_package_hash (whole-tree integrity) feeds raw bytes; compute_file_hash (per-file provenance) now normalizes. Not exploitable in practice (different gates, mismatch either way is still caught), but a future maintainer might unify them incorrectly. A docstring note explaining WHY package hash stays raw prevents accidental weakening.
    Suggested: Add to compute_package_hash docstring: line-ending normalization intentionally omitted -- the package tree is hashed at the git-checkout boundary where content is already platform-canonical, and path-binding makes cross-platform identity unnecessary for this surface.
  • [nit] UTF-8 BOM is hash-visible (correct) but untested at tests/unit/test_content_hash.py
    Docstring states BOM stays hash-visible (unlike drift-replay). Correct security choice, but no test proves BOM distinctness; a test would regression-trap against future over-normalization.
    Suggested: Add test_bom_stays_hash_visible asserting compute_file_hash(with_bom) != compute_file_hash(without_bom).

OSS Growth Hacker

  • [nit] Governance bullet is dense; consider a linebreak or shorter inline phrasing at packages/apm-guide/.apm/skills/apm-usage/governance.md
    The updated content-integrity bullet went from terse to a 2-clause sentence; shorter phrasing keeps the scan-friendly checklist format.
    Suggested: content-integrity -- deployed-file SHA-256 drift detection (line-ending invariant) and critical Unicode check

Auth Expert -- inactive

The diff touches content_hash.py, a probe script, a CI workflow, spec/docs, and tests -- no token management, credential resolution, host classification, AuthResolver, or authorization-header surface; content hashing is auth-neutral.

Doc Writer

  • [recommended] Drift: 'any byte-level change is caught' claims in enforce-in-ci.md and drift-detection.md are now contradicted and were not updated at docs/src/content/docs/enterprise/drift-detection.md:128
    This PR makes a CRLF<->LF-only change deliberately hash-invisible, so two enterprise pages asserting content-integrity catches any byte change are now strictly false for line-ending-only diffs. Benign direction (catches less, only for benign line endings), so not blocking -- but it is an orphaned claim the doc-sync rule says to reconcile.
    Suggested: drift-detection.md:128 and enforce-in-ci.md:115/126 -> reword 'any byte-level change' to 'any content change' and add the line-ending normalization caveat, cross-referencing req-lk-012. One clause each (non-bloat).
  • [nit] apm-guide governance.md 'line endings are normalized' is slightly loose (a bare CR is preserved) at packages/apm-guide/.apm/skills/apm-usage/governance.md:351
    Accurate in intent (parenthetical scopes to CRLF/LF), but 'line endings are normalized' could read as all CR/LF variants. Optional tightening; current text is defensible.
    Suggested: Optionally: 'CRLF is normalized to LF (a bare CR stays caught)'. Skip if it pushes the bullet over one line.

Test Coverage Expert

  • [recommended] No integration test exercises the audit-side CRLF invariant (deployed file CRLF vs recorded LF hash passes apm audit) at tests/integration/test_local_content_audit.py
    The PR fixes apm#1952 where apm audit false-flags CRLF/LF. RECORD side is integration-tested (test_crlf_source_records_same_hash_as_lf_source). But no test drives the VERIFY side end-to-end: install (LF hash recorded), mutate deployed file to CRLF on disk, assert apm audit --ci still passes content-integrity -- the exact user scenario. Probed tests/integration/: grep for 'crlf'/'CRLF' in test_local_content_audit.py -> zero hits; test_audit_detects_drift only tampers content, never line-endings.
    Suggested: Add test_audit_passes_when_deployed_file_has_crlf: after _install, rewrite deployed file with CRLF (same text), run _audit_json, assert content-integrity passed. ~15 lines in existing TestLocalContentAudit.
    Proof (missing at): tests/integration/test_local_content_audit.py::TestLocalContentAudit::test_audit_passes_when_deployed_file_has_crlf -- proves: apm audit does not false-flag a deployed file whose only difference from the recorded hash is CRLF vs LF. [governed-by-policy,secure-by-default,portability-by-manifest]
    assert ci['passed'] is True
  • [nit] Cross-OS CI workflow exercises compute_file_hash directly, not the full apm audit command path at scripts/crlf_invariance_probe.py
    scripts/crlf_invariance_probe.py + workflow give strong cross-platform evidence for the hash FUNCTION layer but are not an apm audit command-level test. Context for the maintainer; the missing integration test above closes the command-level gap.
    Proof (passed): scripts/crlf_invariance_probe.py::crlf_invariance_probe (CI workflow) -- proves: compute_file_hash returns identical hash for CRLF and LF text across Windows/macOS/Linux. [portability-by-manifest,secure-by-default]
    UNIQUE=$(sort -u hashes) ; assert == 1
  • [nit] Unit tests for the new behavior all pass; install-side integration test confirms record symmetry at tests/unit/test_content_hash.py
    TestComputeFileHash (6 tests, 1.27s) covers CRLF==LF, bare-CR distinct, binary raw. Integration test test_crlf_source_records_same_hash_as_lf_source proves the install pipeline records matching hashes. Together they certify the record side at unit and integration tiers.
    Proof (passed): tests/unit/test_content_hash.py::TestComputeFileHash::test_text_crlf_and_lf_hash_equal -- proves: CRLF and LF variants of identical text produce the same per-file hash. [portability-by-manifest,secure-by-default]
    assert compute_file_hash(lf) == compute_file_hash(crlf)

Performance Expert

  • [nit] Byte-level replace alternative is NOT faster -- current decode+str.replace+encode is optimal for the common case at src/apm_cli/utils/content_hash.py:82
    Benchmarked the byte-level alternative vs current. For typical 3-5KB no-CRLF POSIX files, byte-level is ~0.5us SLOWER (bytes.replace scans for an absent 2-byte pattern; str.replace fast-paths). For 1MB files both converge (~1ms, decode-dominated). Current implementation is correct. No change.
  • [nit] 3x transient memory per file is acceptable given sequential processing and typical sizes at src/apm_cli/install/phases/lockfile.py:45
    compute_deployed_hashes iterates sequentially; peak transient is 3x the largest single file (~60KB for typical 20KB files; ~30MB even for a hypothetical 10MB artifact). GC reclaims between iterations. No pathology.
  • [nit] The NUL scan is O(n) but <1us for typical files and gives correct binary detection at src/apm_cli/utils/content_hash.py:83
    b'\x00' in raw is a single SIMD sweep (~0.1us for 5KB), correctly short-circuiting binary before the costlier UTF-8 decode. Ordering (NUL -> decode -> normalize) is cheapest-rejection-first. Optimal.

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

Folds the in-scope advisory-panel recommendations on PR #1959:

- Add audit-side integration regression trap
  test_audit_passes_when_deployed_file_has_crlf: install records the
  canonical LF hash, the deployed file is rewritten to CRLF, and
  `apm audit --ci` must still pass content-integrity. This is the exact
  #1952 user scenario at the command layer (the cross-OS workflow only
  exercised the hash function; the prior integration coverage only
  exercised the record side).
- Reconcile two enterprise docs that overclaimed content-integrity
  ("any byte-level change is caught"): line-ending-only differences are
  now normalized away per req-lk-012, so a CRLF/LF flip alone is not
  flagged.
- Document why compute_package_hash intentionally hashes raw bytes while
  compute_file_hash normalizes, so the two are not accidentally unified.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel danielmeppiel merged commit 2d5ce15 into main Jun 29, 2026
19 checks passed
@danielmeppiel danielmeppiel deleted the danielmeppiel-fix-1952-crlf-content-integrity branch June 29, 2026 20:53
danielmeppiel added a commit that referenced this pull request Jun 30, 2026
Bump pyproject.toml + uv.lock to 0.23.1 and move the [Unreleased] CHANGELOG block into [0.23.1] - 2026-06-29 (two Fixed entries: audit content-hash CRLF invariance #1952/#1959, --skill additive deploy #1955). Lint mirror green locally (ruff check + format, pylint R0801, auth-signals).

Post-merge: tag v0.23.1 to trigger the release workflow.

Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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] apm install deploys skill-bundle files verbatim with CRLF, causing content-integrity hash drift in CI

2 participants