feat(audit): surface unmanaged artifacts with reason, type, and deny-conflict#1793
Conversation
…conflict
Extend the existing _check_unmanaged_files policy check (one unified
report, not a parallel scan engine) so apm audit answers "what is here
that APM did not put here?":
- Enrich each finding with a factual reason ("not tracked in
apm.lock.yaml") and a deny-conflict note ("matches deny rule
(<pattern>)") when the path matches the policy's own
dependencies.deny / mcp.deny.
- Lazily classify the primitive type (skill/agent/instruction/mcp) of
the already-flagged files only -- never the whole tree.
- Surface deny-conflicts through a shared first_matching_pattern matcher
reused by the dependency/MCP deny-list checks (no second matcher).
- Add unmanaged_files.exclude (glob allow-list) to suppress known
harness-managed paths; carried through inheritance merge as a union.
- Guard traversal against symlinks escaping the workspace.
This is drift / divergence visibility, not supply-chain-attack
prevention -- the lockfile is hand-editable YAML. Report only; APM
never removes or blocks a flagged file.
Part of #1774
Closes #1775
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends the existing unmanaged-files audit check to produce a single enriched report for unmanaged governance artifacts (reason + primitive type + deny-conflict context), adds unmanaged_files.exclude to suppress known harness-managed paths, and refactors deny matching through a shared first_matching_pattern helper.
Changes:
- Enriched
_check_unmanaged_filesoutput with reason/type/deny-conflict annotations and added exclude suppression + symlink containment logic. - Refactored allow/deny glob membership to a shared
first_matching_pattern()matcher and added targeted unit tests. - Updated policy schema, inheritance merge, discovery serialization, docs, and CHANGELOG for
unmanaged_files.excludeand enriched findings.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/policy/test_policy_checks.py | Updates existing unmanaged-files assertions and adds new enriched-output, exclude, symlink, and non-duplication tests. |
| tests/unit/policy/test_matcher.py | Adds coverage for the shared first_matching_pattern matcher and verifies deny checks route through it. |
| tests/unit/policy/test_inheritance.py | Adds tests ensuring unmanaged_files.exclude is union-merged and inherited correctly. |
| src/apm_cli/policy/schema.py | Extends UnmanagedFilesPolicy with exclude and documents merge semantics. |
| src/apm_cli/policy/policy_checks.py | Implements enriched unmanaged-files reporting, deny-conflict surfacing, exclude suppression, and symlink guard behavior. |
| src/apm_cli/policy/parser.py | Parses/validates unmanaged_files.exclude from YAML into the policy model. |
| src/apm_cli/policy/matcher.py | Introduces first_matching_pattern and refactors allow/deny checks to reuse it. |
| src/apm_cli/policy/inheritance.py | Merges unmanaged_files.exclude across extends: chains (union + transparency). |
| src/apm_cli/policy/discovery.py | Serializes unmanaged_files.exclude in discovered policy output. |
| packages/apm-guide/.apm/skills/apm-usage/governance.md | Updates the policy template to include unmanaged_files.exclude. |
| docs/src/content/docs/reference/policy-schema.md | Documents exclude, merge rules, and enriched unmanaged finding format. |
| docs/src/content/docs/enterprise/policy-reference.md | Adds enterprise guidance for exclude and describes enriched findings. |
| CHANGELOG.md | Adds an Unreleased entry describing the new enriched unmanaged-files report and exclude. |
Copilot's findings
- Files reviewed: 13/13 changed files
- Comments generated: 3
…l-transparency, classifier precedence Fold three Copilot review findings on the unmanaged-artifacts check: - Symlink-dir traversal guard (security): switch the scan from Path.rglob (which can recurse into directory symlinks) to os.walk(followlinks=False) -- the house pattern from security/gate.py -- so a symlinked directory resolving outside the workspace is never traversed. The file-symlink escape guard now applies only to file symlinks. Adds a regression test asserting a symlinked dir's contents never surface. - exclude null-transparency: parser now mirrors the deny/require pattern so `exclude: null` (or an absent key) -> None (transparent in merge), `exclude: []` -> () (explicit override), `exclude: [..]` -> tuple. Previously `null` collapsed to (), breaking the documented inheritance semantics. Still passes the #1791 coverage guard. - Classifier precedence: explicit filename conventions (.agent.md / .instructions.md / mcp.json / SKILL.md) now win before directory-segment hints, and MCP detection is narrowed to known config filenames / a `.mcp/` root -- so a path under a dir merely named `mcp` (e.g. .github/agents/mcp/rogue.agent.md) classifies as agent, not mcp. Strict TDD (red tests first), mutation-break verified on each new assert. ASCII-only, report-only. Part of #1774 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Doc Writer | 1 | 1 | 2 | One shipped deny-conflict example is unreproducible against the anchored matcher; defaults list omits .kiro. |
| Dev Experience | 0 | 1 | 2 | Output consistent with existing policy surface; add a concrete next-action hint to findings. |
| Python Architect | 0 | 0 | 1 | Sound procedural extension; shared matcher, frozen dataclass, single-report invariant held. |
| Supply Chain Security | 0 | 0 | 1 | Traversal sound; matcher refactor preserves deny semantics exactly; report-only framing honest. |
| Test Coverage | 0 | 0 | 1 | All six new behaviors have green regression-trap tests. Ship. |
| CLI Logging | 0 | 0 | 2 | Output ASCII-clean and consistent; warn-mode detail visibility is a known renderer limitation. |
| OSS Growth | 0 | 0 | 2 | Governance story strengthened; CHANGELOG honest but could be punchier. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc Writer] (blocking-severity) Fix the unreproducible deny-rule example --
rogue*cannot match.claude/skills/rogue/SKILL.mdunder the anchored matcher, so use a pattern that matches via the/rogue/segment -- so the documented output can actually be emitted. - [Dev Experience] Add a next-action hint to the findings output (how to track vs how to suppress) so a flagged file is self-resolving.
- [Doc Writer] Add
.kiroto the documented default governance directories to match_DEFAULT_GOVERNANCE_DIRS. - [Test Coverage] Tighten the type-tag assertion to match
[type: agent]exactly, so the trap fails if the tag is dropped. - [Dev Experience] Correct the
excludedefault on the schema page ([]->null) to match the null-transparent model default.
Recommendation
Fold the one-line documentation fix so the deny-conflict example is reproducible, pick up the next-action hint and the .kiro defaults correction while in here, and tighten the one broad test assertion. None of this touches the traversal or matcher logic, which the panel reads as correct and well-covered. Re-run the panel after folding.
Full per-persona findings
Doc Writer
- [blocking] Deny-conflict example is unreproducible at
docs/src/content/docs/enterprise/policy-reference.md:364
The example claims.claude/skills/rogue/SKILL.md ... matches deny rule (rogue*), but_compile_patternfully anchors patterns (rogue*->^rogue[^/]*$) and_unmanaged_deny_conflicttests only the rel path and the basename (SKILL.md). Neither starts withrogue, so the rule matches nothing and the documented line can never be produced.
Suggested: Use**/rogue/**, which matches the rel path via the/rogue/directory segment. - [recommended] Documented defaults omit
.kiroatdocs/src/content/docs/enterprise/policy-reference.md:326
_DEFAULT_GOVERNANCE_DIRSincludes.kiro, but the rendered defaults list stops at.opencode.
Suggested: Add- .kiroto the defaults block. - [nit] Deny-source phrasing at the deny-conflict note
Docs say the conflict is checked againstdependencies.deny, but the call site passesdependencies.effective_deny(defaults-inclusive) while the MCP side passes rawmcp.deny.
Suggested: Note the dependency side is defaults-inclusive. - [nit]
excludedefault phrasing on the schema page mirrors the Dev Experience nit ([]vsnull).
Dev Experience
- [recommended] Findings give no next-action cue at
src/apm_cli/policy/policy_checks.py:879
A user seeing a flagged file has no inline hint whether to track it or suppress it.
Suggested: Append one guidance line: track withapm install <ref>, or suppress viaunmanaged_files.exclude. - [nit]
excludedefault documented as[]atdocs/src/content/docs/reference/policy-schema.md:155, but the model default isNone(null-transparent).
Suggested: Change tonull. - [nit]
[type: mcp]tagging for a directory merely namedmcp-- already narrowed to a.mcp/root in code; a one-line doc note would pre-empt the question.
Python Architect
- [nit] Deny-conflict basename fallback could surprise a reader at
src/apm_cli/policy/policy_checks.py:740
_unmanaged_deny_conflictmatches the full rel path, then falls back to the basename; a one-line comment documents the two-shot intent.
Suggested: Add an inline comment at the fallback.
Supply Chain Security
- [nit]
project_rootis resolved on every symlink check in_symlink_escapes_workspace; resolving once up front is marginally clearer.
Suggested: Hoist the root resolution. (Traversal soundness and deny-semantics preservation confirmed; report-only framing is honest.)
Test Coverage
- [nit]
test_type_present_in_detailasserts"agent" in detail, which still passes if the tag is removed because the path itself containsagent.
Suggested: Assert"[type: agent]" in detail. (All six new behaviors are covered by green regression-trap tests.)
CLI Logging
- [nit] Warn-mode enriched details are not shown in the default human table at
src/apm_cli/commands/audit.py:496
Warn downgrades the check topassed=True, and the plain renderer prints details only whennot check.passed, so reason/type/deny annotations surface in--ci/JSON but not the default table. This is pre-existing renderer behavior shared by all checks, not introduced here.
Suggested: Track as a known limitation; a renderer change is global and outside this check's scope. - [nit] Minor wording divergence between message phrasings is intentional given the advisory-vs-enforcement context.
OSS Growth
- [nit] CHANGELOG entry is honest but could be punchier about the drift-visibility win.
- [nit] The
rogueexample word is abstract; a concrete artifact name would land better.
Performance -- inactive
No install/update/run hot path is touched; the audit-time walk is bounded by the 10k-file cap.
Auth -- inactive
No auth, token, credential, or host-classification surface is affected; all touched files are policy-engine modules, their tests, and docs.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
…en type trap Fold the apm-review-panel advisory on the unmanaged-files audit: - docs(policy-reference): the deny-conflict example claimed `matches deny rule (rogue*)` for `.claude/skills/rogue/SKILL.md`, but the matcher fully anchors patterns (`rogue*` -> `^rogue[^/]*$`) and the conflict check tests only the rel path and basename -- neither starts with `rogue`, so that line could never be emitted. Use `**/rogue/**`, which matches via the `/rogue/` segment so the documented output is reproducible. - docs(policy-reference): add `.kiro` to the documented default governance directories so the list matches `_DEFAULT_GOVERNANCE_DIRS`; note the `mcp` directory-name narrowing on the type-tag bullet. - feat(policy): append one next-action hint to a non-empty unmanaged-files report (track with `apm install <ref>` / suppress via `unmanaged_files.exclude`) so a flagged file is self-resolving. Covered by a new regression-trap test (mutation-break verified). - docs(policy-schema): correct the `exclude` default to `null` (null-transparent in the extends merge) to match the model. - test(policy): tighten the type-tag assertion to `[type: agent]` so the trap fails if the tag is dropped (the path itself contains `agent`). - docs(code): clarify in the docstring that the dependency deny side is defaults-inclusive (`effective_deny`) while MCP uses raw `mcp.deny`, and document the basename fallback intent. Report-only drift-visibility behavior is unchanged; the traversal and matcher logic is untouched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…/surface-unmanaged-artifacts
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Doc writer | 0 | 0 | 1 | All four prior findings resolved; both regex examples reproduce against matcher.py; no new in-scope correctness blocker. |
| Python architect | 0 | 0 | 1 | Clean helper decomposition, single matcher source of truth confirmed, no layering violations. |
| Test coverage expert | 0 | 0 | 0 | Tightened [type: agent] assertion plus a mutation-tested next-action regression trap; no silent-drift gap. |
| CLI logging expert | 0 | 0 | 0 | Next-action footer is ASCII-only, single-line, actionable; sole deferral is an out-of-scope global renderer gate. |
| DevX UX expert | 0 | 0 | 1 | Next-action hint concise with two correct verbs; null-transparency documented consistently across schema and reference. |
| Supply chain security expert | 0 | 0 | 1 | Symlink guard correct for report-only drift (followlinks=False, fail-closed); single anchored matcher, no silent suppression. |
| OSS growth hacker | 0 | 0 | 1 | User-facing copy frames report-only drift visibility without over-claiming security enforcement. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Python architect + Supply chain security] Hoist
project_root.resolve()out of the symlink-check loop in_check_unmanaged_files-- marginal perf micro-opt relevant only when governance directories contain many symlinks (rare). Raised independently by two personas. - [CLI logging expert] DEFERRED (out of scope): the global renderer gate at audit.py:496 (
if not check.passed and check.details) hides warn-mode details for ALL audit checks, not just unmanaged-files. Track as a separate audit-rendering issue; data still surfaces in --ci/JSON. - [DevX UX expert] Add an inline comment '(null = inherit parent)' next to the
unmanaged_files.excludeexample in governance.md -- a convenience for skimmers; the reference merge table already documents null-transparency. - [OSS growth hacker] Consider a punchier CHANGELOG lead sentence that foregrounds user value before the mechanism description -- current framing is accurate but slightly implementation-forward.
- [Doc writer] Cosmetic: doc examples use
[!]prefix markers while the renderer emits indented dash bullets -- both readable; aligning them is purely aesthetic.
Recommendation
All seven active personas voted ship_now with zero blocking or recommended findings. The previously-blocking doc bug is verified fixed. CI is green. The remaining nits are cosmetic polish opportunities that do not affect correctness, user-facing behavior, or governance-promise integrity. Ship without delay; address nits opportunistically in subsequent PRs if desired.
Full per-persona findings
Doc writer
- [nit] Enriched-finding examples are prefixed with
[!], but the unmanaged-files renderer emits each detail as an indented- <detail>bullet (audit.py:470, 497), not a[!]symbol; the text after[!]matches_format_unmanaged_detailoutput exactly, so it is purely cosmetic framing atdocs/src/content/docs/enterprise/policy-reference.md:366.
Python architect
- [nit]
_symlink_escapes_workspacere-resolvesproject_rooton every call (src/apm_cli/policy/policy_checks.py); hoistingproject_root.resolve()to a local and passing it in avoids repeated syscalls when many symlinks exist. Cosmetic -- symlinks in governance dirs are rare.
Test coverage expert
No findings. Prior nit resolved (tightened [type: agent] assertion); new test_next_action_hint_present regression-trap mutation-breaks on footer removal; all new behavioral surfaces have a dedicated assertion.
CLI logging expert
No in-scope findings. One out-of-scope deferral: warn-mode details are suppressed by the global renderer gate if not check.passed and check.details (audit.py:496), which affects all checks; fixing it crosses this PR's scope and causes no data loss (details surface in --ci/JSON).
DevX UX expert
- [nit]
packages/apm-guide/.apm/skills/apm-usage/governance.mdcould add '(null = inherit parent)' to theunmanaged_files.excludeinline comment to echo the null-transparency mental model; low priority since the reference docs already cover it.
Supply chain security expert
- [nit]
_symlink_escapes_workspacecallsproject_root.resolve()per file-symlink; resolving once at the top of_check_unmanaged_fileswould avoid repeated syscalls in repos with many symlinks. Minor perf/clarity. Guard is otherwise correct for report-only drift visibility.
OSS growth hacker
- [nit] The CHANGELOG entry is accurate and correctly framed as drift visibility, but at ~4 lines reads as implementation notes; a punchier lead sentence would convert better in release notes. Low priority.
Auth expert -- inactive
No token management, credential resolution, or remote-host authentication surface is touched by this PR.
Performance expert -- inactive
No hot path is affected; the audit is an on-demand CLI scan, not an install/resolve inner loop.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Author OpenAPM v0.1 requirement req-pl-015 (Section 6.3.5, governance
MUST) codifying the unmanaged-artifact surfacing behavior added in this
PR, resolving the Mode B spec-conformance gate honestly (spec is the
contract) rather than waiving it.
req-pl-015 governs reporting COMPLETENESS only: a governance
implementation evaluating policy over a populated primitive target tree
MUST surface every file under a managed primitive target directory that
is neither recorded in apm.lock.yaml nor matched by a configured
unmanaged_files.exclude glob, each carrying its unmanaged reason, a
dependency/MCP deny-conflict note where applicable, and a determinable
inferred primitive type; an excluded path MUST NOT be surfaced.
Enforcement stays governed by unmanaged_files.action -- this is not an
enforcement claim.
Citation ritual (all sites edited for orphan_check + linter-check-6):
- Section 6.3.5 anchor + normative prose.
- Section 6.4 merge table: unmanaged_files.exclude row (union, dedup,
parent order preserved -- matches inheritance._union).
- Section 1.3 + Appendix C count 87 -> 88 (83 MUST); Appendix C row;
Section 6.8 governance trailer; Appendix D erratum row.
- Manifest entry after req-pl-012; regenerated CONFORMANCE.{md,json}.
- New @pytest.mark.req("req-pl-015") behavioral test binding the spec
MUST to _check_unmanaged_files (reason, type, deny-conflict, exclude).
Cross-PR note: sibling spec-citation PR #1794 also edits the shared
count sites; whichever merges second needs a trivial count reconcile.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-type, additive exclude) Restructure req-pl-015 into lettered sub-clauses (a)/(b)/(c) so each obligation is individually citable; add the absent-type omission MUST; clarify the deny-conflict note is supplemental enrichment; cross-ref the exclude matcher to Section 6.5 (no new dialect pin); qualify the 6.4 merge-table exclude row as additive union (child cannot clear parent; null and [] both preserve parent); relabel the Appendix D row as a semver-zero normative addition (not an informative erratum); reserve req-pl-013/014 in the manifest for concurrent in-flight work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Apply the three zero-risk fold-now items from the apm-spec-guardian round-2 synthesis (ship_decision=fold_and_ship, shocked_meter_avg=8.0, zero blocking): state the exclude-merge dedup key is byte-exact on each pattern's UTF-8 string; remove an orphan line break in sub-clause (b); replace the em-dash in the (b) reason clause with a parenthetical. No normative, anchor, count, or conformance-test change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Spec Guardian:
|
| Panel | Stance | shocked_meter | Blocking | Recommended | Nits |
|---|---|---|---|---|---|
| swagger-openapi-editor | ship_with_followups | 8 | 0 | 0 | 1 |
| oci-distribution-editor | ship_with_followups | 8 | 0 | 1 | 1 |
| pkgmgr-registry-contract-editor | ship_with_followups | 8 | 0 | 1 | 1 |
| w3c-tag-architect | ship_with_followups | 8 | 0 | 1 | 1 |
Convergent themes
- Round-1 closure confirmed across all four panels. F1 (sub-clause (a)/(b)/(c) decomposition), F2 (absent-type omission MUST), F3 (additive-union exclude merge), F4 (matcher cross-ref to Section 6.5), F5 (Appendix D semver-zero normative-addition framing), F6 (manifest reservation of req-pl-013/014) all verified resolved.
- Report-vs-enforcement layering is clean. req-pl-015 owns reporting completeness;
unmanaged_files.action(Section 6.4) owns pass/fail. No implied enforcement. - Glob-dialect pin is a spec-wide pre-existing gap, not introduced by this PR -- the spec pins no glob grammar anywhere. Deferred (see below).
Folded now (applied at b118d360)
- [oci-rec-r2-1] Merge-table exclude row now states the dedup key is byte-exact on each pattern's UTF-8 string.
- [pkg-nit-r2-1] Removed an orphan line break in sub-clause (b).
- [tag-nit-r2-1] Replaced the em-dash in the (b) reason clause with a parenthetical.
Deferred (version-scoped, out of scope for this single-requirement patch)
Defer to v0.1.1
- Pin a glob grammar reference for all pattern fields spec-wide (swagger sw-def-r2-1, oci-defer-r2-2 -- convergent).
- Distinct reason string for symlink targets that diverge from their referent classification (oci-defer-r2-1). Current code uses
os.walk(followlinks=False)+is_file()resolve, so a symlink reason enum would drift from behavior. - Optional idempotency note: identical tree state + config MUST yield an identical surfaced set (oci-nit-r2-1).
Defer to v0.2
- Normative glob-dialect pin as a first-class spec section (pkg-rec-r2-1; pre-existing, needs broader design pass).
- Further decompose sub-clause (b) into (b-i)/(b-ii)/(b-iii) (tag-rec-r2-1; self-marked v0.2).
Rejected
- [sw-nit-r2-1] Already satisfied -- the spec body already renders the reason string with backticks (
`not tracked in apm.lock.yaml`). No-op.
Linter (11-check Wave-5)
All checks pass. orphan_check: 88 requirements aligned across anchors / manifest / Appendix C / pytest markers. Mode B: spec-concurrent short-circuit (orphan_check owns this PR). spec_conformance pytest: green.
Full per-panel round-2 returns
swagger-openapi-editor (8): All six round-1 findings confirmed folded. Count sites align at 88 (83 MUST, 5 SHOULD) across Section 1.3, Appendix C, Appendix D. Keyword discipline clean: each sub-clause binds one testable MUST/MUST NOT. Conformance test binds verbatim spec phrases -- good drift practice. Ship-quality.
oci-distribution-editor (8): Sub-clause (c) precedence ("MUST NOT be surfaced, even when it also matches a deny pattern") is an unambiguous hard override. Additive-union merge is deterministic given parent-order preservation + dedup. No content-addressing / lockfile-integrity concerns -- governance/reporting layer only. Ship.
pkgmgr-registry-contract-editor (8): Additive-union exclude semantics correctly distinguished from the deny/require tri-state (which honors explicit-[] as a clear). Report-only vs enforcement boundary crisp. Structured sub-clauses individually citable. Only forward gap is the pre-existing glob-dialect pin. Ship-quality.
w3c-tag-architect (8): Textbook separation of observation (surfacing set) from policy-decision (action). Additive-union exclude is the correct monotone-lattice choice for a security-adjacent allow-list. Semver-zero normative-addition framing is principled and does not abuse the erratum track. Ship.
Spec-citation authored under a maintainer governance call to honestly resolve the Mode B spec-conformance gate (the prior green was a fail-open self-skip). req-pl-015 is report-only completeness; enforcement remains governed by the existing unmanaged_files.action field.
… type-annotate locals apm-review-panel re-run folds (in-scope, non-blocking): - [recommended/doc-writer] policy-schema.md + policy-reference.md exclude merge rows now state additive-only: null AND [] both preserve the parent list (unlike deny/require, a child cannot clear an inherited exclude), matching the openapm-v0.1 6.4 merge-table row and _merge_unmanaged_files. - [nit/python-architect] annotate deployed/deployed_dir_prefixes locals as set[str] / list[str] in _check_unmanaged_files. Declined (noted): doc-writer (a) absolute-framing carve-out nit -- guardian already passed the absolute wording; the 'when it evaluates policy' preamble covers symlink-escape/scan-cap degraded modes; low value vs reopening blessed spec prose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Architecture sound: single-report invariant, shared matcher reuse, frozen dataclasses, clean module boundaries, correct symlink guard. (nit folded) |
| Cli Logging Expert | 0 | 0 | 0 | Output UX clean: ASCII-only detail lines, factual reason strings, lazy type tags, single actionable next-action footer. |
| Devx Ux Expert | 0 | 0 | 0 | Audit output self-resolving; exclude ergonomics follow familiar glob-list pattern; null-transparent extends-merge documented. |
| Supply Chain Security Expert | 0 | 0 | 0 | Symlink guard fail-closed; sole matcher (no parallel impl); exclude cannot weaken enforcement; spec disclaims enforcement -- honest framing. |
| Oss Growth Hacker | 0 | 0 | 0 | Strong growth signal: req-pl-015 upgrades story to spec-mandated governance; conformance binding is good contributor-funnel signal. |
| Test Coverage Expert | 0 | 0 | 0 | All six behaviors have regression-trap tests; new req-pl-015 binding test asserts real output AND spec-phrase drift guard. 28 tests green. |
| Doc Writer | 0 | 0 | 0 | Citation accurate to _check_unmanaged_files; counts internally consistent at 88. (recommended finding folded) |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Doc Writer] Reference docs (
policy-schema.md,policy-reference.md) exclude merge rows clarified: additive-only,nulland[]both preserve parent list, child cannot clear inherited exclude. -- Folded in the current head; verified against_merge_unmanaged_files. - [Python Architect]
deployed/deployed_dir_prefixeslocals atpolicy_checks.py:815-816lacked type annotations (set[str]/list[str]). -- Folded in the current head. - [Doc Writer] req-pl-015(a) "surface every file" omits two silent completeness carve-outs (symlink-escape skip, scan-cap). -- Declined: guardian passed the absolute framing at shocked_meter 8.0; the "when it evaluates policy" preamble covers degraded modes; low value vs reopening blessed spec prose.
Architecture
classDiagram
direction LR
class UnmanagedFilesPolicy {
<<ValueObject / frozen>>
+action str | None
+directories tuple | None
+exclude tuple | None
+effective_action() str
}
class DependencyPolicy {
<<ValueObject / frozen>>
+allow tuple | None
+deny tuple | None
+effective_deny() tuple
}
class McpPolicy {
<<ValueObject / frozen>>
+allow tuple | None
+deny tuple
}
class CheckResult {
<<ValueObject>>
+name str
+passed bool
+message str
+details list
}
class first_matching_pattern {
<<Pure Function>>
+__call__(name, patterns) str | None
}
class _check_unmanaged_files {
<<IOBoundary>>
}
_check_unmanaged_files ..> UnmanagedFilesPolicy : reads policy
_check_unmanaged_files ..> first_matching_pattern : exclude + deny-conflict
_check_unmanaged_files ..> CheckResult : returns
note for first_matching_pattern "Single matcher reused by allow/deny, exclude, and deny-conflict -- no duplicate impl"
note for UnmanagedFilesPolicy "Null-transparent tri-state: None=no opinion, ()=explicit empty, (...)=values"
flowchart TD
A["apm audit invokes _check_unmanaged_files"] --> B{"effective_action == ignore?"}
B -- yes --> Z1["CheckResult(passed, disabled)"]
B -- no --> C["Build deployed set from LockFile"]
C --> D["os.walk(gov_dir, followlinks=False)"]
D --> E{"file_path.is_symlink() and escapes workspace?"}
E -- yes --> D
E -- no --> H{"rel in deployed OR under dir prefix?"}
H -- yes --> D
H -- no --> I{"first_matching_pattern(rel, exclude)?"}
I -- matched --> D
I -- None --> J["classify type + deny-conflict + format detail"]
J --> M["details.append(line)"]
M --> D
D -- walk done --> N{"unmanaged_count > 0?"}
N -- no --> Z3["CheckResult(passed, clean)"]
N -- yes --> O{"effective_action == deny?"}
O -- yes --> Z4["CheckResult(failed, block)"]
O -- no --> Z5["CheckResult(passed, warn + details)"]
Recommendation
All seven active personas returned ship_now with zero blocking findings. The single recommended finding and one nit are folded in the current head. The spec citation strengthens the PR from "feature" to "spec-mandated governance behavior" and passed adversarial guardian review. CI is green on the citation-inclusive head. No reason to delay merge.
Full per-persona findings
Python Architect
- [nit] Type annotation for
deployedanddeployed_dir_prefixescould be explicit atsrc/apm_cli/policy/policy_checks.py:815-816
Narrowing these two locals (set[str]/list[str]) aligns with the module's well-typed public API. (Folded in current head.)
Cli Logging Expert
No findings.
Devx Ux Expert
No findings.
Supply Chain Security Expert
No findings.
Oss Growth Hacker
No findings.
Test Coverage Expert
- [evidence] All six Surface unmanaged artifacts in apm audit (drift visibility) #1775 behaviors carry dedicated regression-trap tests; the new
test_unmanaged_files_surfacing_completeness(@pytest.mark.req("req-pl-015")) asserts real_check_unmanaged_filesoutput AND a spec-phrase drift guard.
Proof (passed):tests/unit/policy tests/spec_conformance/test_policy_reqs.py-- 28 passed in 0.70s [Governed by policy, Secure by default]
Doc Writer
- [recommended] Reference docs exclude merge rows were silent on
[]-behavior while the nearby deny/require tri-state says[]clears parent -- a reader could wrongly infer exclude[]clears too, contradicting the new 6.4 spec row. (Folded in current head: both rows now state additive-only;nulland[]both preserve the parent list.) - [nit] req-pl-015(a) "surface every file" omits two silent completeness carve-outs (symlink-escape skip, scan-cap). (Declined: guardian passed the absolute framing; preamble covers degraded modes.)
Auth Expert -- inactive
No auth, token-management, or credential-resolution surface in this PR.
Performance Expert -- inactive
No dependency-resolution, cache-layout, or install-path performance surface in this PR.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Reconcile cross-PR normative-count collision after #1794 (req-pl-013/014) and #1801 (Mode B fail-closed CI) merged to main. Union all shared count sites to cumulative 90 (req-pl-013, req-pl-014, req-pl-015 all present; 85 MUST, 5 SHOULD). Bump req-pl-015 revision-history row to 0.1.4 to keep Appendix D monotonic. Regenerated CONFORMANCE.{json,md}. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nt reconcile + new-tip) Union CHANGELOG entries with newly-landed #1810 (ADO marketplace host) and #1770 (Antigravity target); spec count sites unchanged at cumulative 90 (req-pl-013/014/015). Regenerated policy golden snapshot to carry both the merged security.* keys and unmanaged_files.exclude. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…h-1670 Faithful union resolution: keep main's new [Unreleased] Added entries (#1793 audit unmanaged-files, #1810 ADO marketplace, #1770 antigravity target, #1794 security policy keys) AND re-insert this PR's MCP extra-passthrough + denylist entry (#1670/#1765) in Keep-a-Changelog order. All adapter/integrator denylist wiring preserved. Co-authored-by: Sergio Sisternes <sergio.sisternes@epam.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sync the 800-line/complexity tightening branch with origin/main tip 788a09a (8 commits ahead of merge-base 45843c3): SBOM export + declared-license (#1820), dompurify bump (#1789), audit-unmanaged (#1793), ADO sourceBase (#1810), Antigravity target (#1770), marketplace token (#1763), spec-conformance (#1801), declared-license and integrity keys (#1794/#1777). Conflict resolution preserves the strangler-fig extraction: HEAD's relocations into sibling _*.py modules win, with main's feature additions folded into the new homes. Notable folds: - hook_merge.py: thread container key + antigravity dispatch. - audit: route fail_on_drift + LockFile through the audit module so test monkeypatches on apm_cli.commands.audit.* still take effect. Resolve merge-introduced CI regressions under the tightened gates: - ruff complexity: _classify_primitive_type (PLR0911), validate_policy (C901/PLR0912 via _validate_security), _audit_content_scan (PLR0912 via _run_drift_detection). - file-length <=800: split spdx_data.py (_spdx_exception_ids.py), policy_checks.py (_policy_checks_unmanaged.py), pack.py render helpers (into _pack_ops.py); all re-exported for the patch contract. Local CI mirror green: ruff check/format, pylint R0801 10/10, auth-signals, file-length<=800, full unit suite 17225 passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#1834) Three independent stale-test failures surfaced in the merge queue once recently-merged PRs combined on main: - test_wave2_adapters_coverage: patched GeminiClientAdapter._get_gemini_dir, which #1770 renamed to _get_config_dir. Update the patch target. - test_wave7 test_unmanaged_file_deny: asserted exactly one detail line, but #1793 appends a single trailing next-action hint to any non-empty unmanaged-files report. Assert on the flagged-file line instead of the raw detail count. - test_commands_deep_coverage (outdated/compile): these tests passed obj={"cwd": ...}, which the commands never read -- they resolve the project via Path.cwd(). They only passed because pytest's cwd was the repo root (a valid APM project); under CI xdist a sibling test leaking os.chdir flipped their exit codes. Pin cwd with monkeypatch.chdir and assert the commands' real behavior (no-lockfile and no-content exit 1). Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
Agent primitives (skills, agents, MCP server configs) can land in a workspace through paths other than APM -- for example, configuration an agent harness installs by default. Those artifacts carry no
apm.lock.yamlentry, and teams had no single place to see what is installed that APM did not pin. The existing unmanaged-files check listed bare paths with no reason, type, or conflict context, so a user could not triage what they were looking at.This is drift / divergence visibility, not supply-chain-attack prevention:
apm.lock.yamlis hand-editable YAML, so this does not defend against a forged lockfile. It answers "what is here that APM did not put here?" honestly.What was built
This extends the existing
_check_unmanaged_filesinsrc/apm_cli/policy/policy_checks.py-- it does not add a parallel scan engine. A second overlapping "unmanaged" report on the same files causes alarm fatigue, so there remains exactly one unified unmanaged-files report. The pre-existing single tree-walk in that check was enriched in place; no secondrglobwas introduced.Each flagged file is now annotated within that single report:
not tracked in apm.lock.yaml(deliberately not "no integrity evidence" jargon).[type: skill|agent|instruction|mcp], classified lazily on only the already-flagged files, never the whole tree.matches deny rule (<pattern>)when the path matches APM's own policy (dependencies.deny/mcp.deny). Report only; APM never removes or blocks the file. Routed through a sharedfirst_matching_patternmatcher inmatcher.pythat the dependency/MCP deny-list checks were refactored to reuse -- no second matcher.Example output (ASCII only):
The
excludekeyA new
unmanaged_files.excludeglob list lets teams suppress known harness-managed paths so the check does not fire forever and become noise. Excluded paths are never reported. The field is:parser.py(must be a YAML list),inheritance.py_merge_unmanaged_filesas a union (passing the structural coverage guard from fix(policy): carry all ApmPolicy fields through inheritance merge #1791), withNonetransparent,discovery.py.Symlink guard
Traversal never follows a symlink that resolves outside the workspace (
_symlink_escapes_workspace); broken/looping links are skipped -- no traversal bomb.Test seams (strict TDD, red first)
excludecarried through the inheritance merge.Mutation-break confirmed on the deny-conflict and non-duplication tests (then restored): nulling
_unmanaged_deny_conflictfailed both deny-conflict tests; double-appending the detail failed the non-duplication test.Note on the drift tree-walk
drift.py's_walk_managedunconditionally injects a rootAGENTS.mdentry, which would contaminate the unmanaged report. Reusing the existing in-place walk inside_check_unmanaged_files(rather than calling that helper) is what satisfies the paramount "one unified report, no second scan" constraint; the symlink guard and non-duplication invariants are covered by tests.Validation evidence
All run locally, green:
uv run --extra dev ruff check src/ tests/-- All checks passeduv run --extra dev ruff format --check src/ tests/-- cleanuv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/-- 10.00/10bash scripts/lint-auth-signals.sh-- auth-signal lint cleanuv run --extra dev pytest tests/unit/policy/-- 878 passed, 1 xfailed, 17 subtests passeduv run --extra dev pytest tests/unit -k "unmanaged or audit or drift"-- 778 passedpolicy_checks.pystays well under the 2450-line guard (1238 lines).Docs
docs/src/content/docs/reference/policy-schema.md--excludefield, merge rule, enriched-output example.docs/src/content/docs/enterprise/policy-reference.md--excludesection, merge rule, enriched findings.packages/apm-guide/.apm/skills/apm-usage/governance.md--excludekey in the policy template.CHANGELOG.md--[Unreleased]additive entry.Spec citation (req-pl-015)
Per a maintainer governance call, this PR also codifies the unmanaged-artifact surfacing behavior as a normative requirement in OpenAPM v0.1, so the spec-conformance gate evaluates it honestly (the prior green was a fail-open self-skip on an unresolvable merge-base, not an evaluated pass).
governance, Section 6.3.5): a conforming governance implementation MUST, when it evaluates policy over a populated primitive target tree, surface every file under a managed primitive target directory that is neither recorded inapm.lock.yamlnor matched by a configuredunmanaged_files.excludeglob, carrying the reason it was flagged and -- where determinable -- its inferred primitive type; a path matched by anexcludeglob MUST NOT be surfaced.unmanaged_files.actionfield; the spec text keeps that boundary explicit.tests/spec_conformance/test_policy_reqs.py::test_unmanaged_files_surfacing_completeness(@pytest.mark.req("req-pl-015")).openapm-v0.1.md(6.3.5 anchor + prose, 6.4 merge-tableexcluderow, L139 count, 6.8 governance trailer, Appendix C row + total, Appendix D revision row),openapm-v0.1.requirements.ymlmanifest, and regeneratedCONFORMANCE.{json,md}.ship_decision = fold_and_ship,shocked_meter_avg = 8.0, zero blocking.Cross-PR count caveat: this PR raises the OpenAPM v0.1 normative count from 87 -> 88 and edits the shared count sites (L139, Section 6.8 governance trailer, Appendix C table + total, the requirements manifest, Appendix D revision history). Sibling spec-citation PR #1794 (req-pl-013 / req-pl-014) edits the same shared sites independently. Whichever of #1793 / #1794 merges second needs a trivial count reconcile (cumulative total -> 90, union of the added rows). The two PRs author their counts independently and do not coordinate.
Part of #1774
Closes #1775