feat(policy): walk GitLab subgroups for org policy discovery (#2753)

[Rafael Azevedo (rrazvd)]

TL;DR

GitLab org-policy auto-discovery only ever probed the top-level group (<top-level-group>/apm-policy), so a project nested under subgroups could not enforce its own governance without changing the whole organization. This PR walks the GitLab subgroup tree from the project's own group up to the top-level group and applies the closest apm-policy (closest wins), and lets extends: compose across nested namespaces. Flat <group>/<project> remotes are unchanged. Closes #2753.

Problem (WHY)

For a project at gitlab.com/acme/dept-a/team-x/my-project, discovery resolved only acme/apm-policy and discarded every intermediate subgroup:

• A single team could not scope its own policy under its subgroup — the only place APM looked was the top-level group, imposing one team's rules org-wide (or none).
• The prior docs stated the limitation outright: "GitLab discovery uses only the top-level group ... It does not search nested subgroup scopes."
• This is the accepted follow-up to [BUG] GitLab (self-hosted) org-policy auto-discovery is impossible: all candidate repo names are invalid GitLab project paths #2566 (top-level GitLab discovery, shipped in v0.29.0). Issue [FEATURE] GitLab: support subgroup-scoped policy discovery (team-level apm-policy), not only the top-level group #2753 is status/accepted.

Root cause: _parse_remote_url returned only path_parts[0] as the org, and _auto_discover passed that single org to the GitLab fetch — the full namespace was never derived.

Approach (WHAT)

Decision	Choice
Leaf selection	Walk namespaces deepest → top-level; first non-absent wins (closest)
No policy at a level	absent (404) ascends to the parent group
Closer policy the token can't read	GitLab 404s a private project the same as a missing one, so a denied closer apm-policy is skipped in favour of the next ancestor (404 == no policy, like GitHub/ADO) — a platform limitation, documented (grant the token read access).
Error / malformed / 403	Fail closed immediately — never coerced into "no policy"
Composition	Opt-in via existing extends:; now accepts nested-namespace refs
Flat / personal namespace	Probes only <group>/apm-policy — identical to prior behavior
Remote parsing	Extracted to a dedicated policy/_remote.py owner (no split authority; architecture-guard enforced)

Implementation (HOW)

File	Change
src/apm_cli/policy/_remote.py (new)	The git-remote identity family, extracted from discovery.py (which grew past the 2100-line file-length ratchet): _git_remote_origin_url (single git remote get-url origin owner), _remote_url_parts (single host+segments splitter), _parse_remote_url (refactored to consume the splitter — behavior-preserving for GitHub/GHE/ADO/visualstudio), plus _extract_org_host_port_from_git_remote / _extract_org_from_git_remote. discovery.py re-exports them, so call sites/tests are unchanged.
src/apm_cli/policy/discovery.py	New _gitlab_namespace_descending (deepest→top namespaces from the origin remote) and _gitlab_walk_candidate (the walk: first non-absent wins, absent ascends). Reads origin once and reuses it for identity + the walk.
src/apm_cli/policy/discovery.py (_extract_extends_host)	Leaf-aware pre-fetch host-pin guard: a bare single-label first segment is a namespace only on GitLab leaves (acme/dept-a/apm-policy); on GitHub/ADO leaves it is still parsed as a (cross-)host, so extends: "evil/org/repo" remains rejected (Security Finding F1 preserved).
src/apm_cli/policy/_gitlab.py	_fetch_gitlab_chain_parent accepts nested-namespace extends: refs (namespace = parts[:-1], repo = parts[-1]) after an optional leaf-host-prefix strip.
scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py, install_policy_intent.py, .apm/architecture/owners/install-deployment.json	Add a static delegation guard (install-deployment-policy-remote-origin-owner), bound to a dedicated _remote.py owner-registry record: only _remote.py may read git remote get-url origin or define the remote-URL splitter/parsers. Also re-point the existing gitlab-facade-orchestration guard at _gitlab_walk_candidate.
tests	New test_discovery.py coverage for the walk, nested extends:, and F1; an owner-guard mutation-break case in test_architecture_owner_rule_mutations.py and the frozen-inventory entry in test_architecture_runner.py for the new guard.
docs + CHANGELOG.md	Updated apm-policy.md, governance-guide.md, policy-reference.md, apm-usage/governance.md; added the Unreleased changelog entry.

Note

The GitLab REST transport and AuthResolver auth already URL-encode a full multi-segment project_path, so no transport/auth changes were needed — org="acme/dept-a" was already fetchable.

Diagram

The GitLab branch of _auto_discover: each namespace level is probed closest-first; only a clean absent ascends, every other outcome returns immediately.

flowchart TD
    A[origin remote] --> B[_gitlab_namespace_descending]
    B --> C["deepest: acme/dept-a/team-x"]
    C -->|found / error| R[return result]
    C -->|absent| D["acme/dept-a"]
    D -->|found / error| R
    D -->|absent| E["acme (top-level)"]
    E -->|found / error| R
    E -->|absent| Z[outcome = absent, clean no-op]

Loading

Trade-offs

• Up to N REST probes when no policy exists (N = subgroup depth). The common no-governance case costs a few extra REST calls; the walk short-circuits on the first hit, and logging stays at debug so --verbose is not spammed. Absent REST results are served from the existing per-namespace policy cache on warm runs; caching the absent REST outcome itself is a possible follow-up, out of scope. (The concealment Git probe, a separate path, is cached — see below.)
• GitLab conceals private projects with 404. A closer apm-policy the token cannot read is indistinguishable from a missing one, so it is skipped in favour of the next ancestor — the same 404 == no policy behaviour GitHub/ADO discovery already have. An earlier revision tried to detect this via an authenticated git ls-remote probe; it was removed after review as scope creep (it caught only a narrow case, could not be soundly cached, and false-positived on an apm-policy project containing only a README). Mitigation: grant the CI token read access to every apm-policy it should honour.
• Closest-wins is not tighten-only at the discovery layer. Anyone who can create an apm-policy project in an intermediate subgroup can publish a weaker policy that shadows a stricter ancestor for projects under it (only extends: chains enforce tighten-only). This is intrinsic to team-scoped governance; the mitigation (restrict apm-policy project creation via GitLab RBAC) is documented in the governance guide's sharp-edges list.
• extends: "org" resolves to the top-level group, not the nearest ancestor — kept consistent with GitHub/ADO semantics and documented explicitly, rather than silently redefining the shorthand per host.
• Nested-namespace vs cross-host ambiguity in extends: refs is resolved by an FQDN heuristic (a host has a . or :port); a GitLab group whose name literally contains a dot would fail closed (rejected). This is safe and rare, and the docs prescribe the host-qualified spelling (gitlab.com/acme.tools/..., with host:port when the origin uses a port) as the workaround.

Benefits

1. A team under any subgroup depth can own its apm-policy without touching org-wide governance.
2. A top-level acme/apm-policy still applies to every project with no closer policy — zero migration for existing setups.
3. Cross-level composition works via extends: (including nested-namespace parents).
4. Remote-URL splitting and the git-remote subprocess each now have a single canonical owner (architecture-rule aligned).

Validation

All CI-mirror gates pass locally on the merge base.

Lint, duplication, tests, architecture, conformance

$ uv run --extra dev ruff check src/ tests/            -> All checks passed!
$ uv run --extra dev ruff format --check src/ tests/   -> 1768 files already formatted
$ uv run --extra dev python -m pylint --disable=all --enable=R0801 \
    --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/  -> 10.00/10
$ uv run python -m pytest tests/unit/policy tests/spec_conformance/test_policy_reqs.py
    -> 1246 passed, 1 xfailed
$ uv run python -m pytest tests/unit/install -k "policy or preflight or discover"
    -> 323 passed
$ uv run python -m pytest tests/integration/test_architecture_semantic_rule_mutations.py
    -> 148 passed
$ uv run python -m pytest tests/unit/scripts/test_architecture_runner.py
    -> 72 passed
$ bash scripts/lint-architecture-boundaries.sh          -> exit 0

Scenario evidence

User-promise scenario	Proven by	APM principle
Closest subgroup policy wins	test_gitlab_subgroup_closest_wins	Governance is scoped, not all-or-nothing
Absent level ascends to parent	test_gitlab_subgroup_absent_ascends_to_parent	Deterministic discovery order
No policy anywhere → clean absent	test_gitlab_subgroup_all_absent_is_clean_absent	Missing policy is not an error
Error fails closed (first and inner level)	test_gitlab_subgroup_error_fails_closed, ..._error_at_inner_level_fails_closed	Fail closed on ambiguity
Flat / personal namespace unchanged	test_gitlab_flat_project_falls_back_to_top_level_org	No regression
Nested extends: allowed, cross-host FQDN rejected	TestValidateExtendsHostNestedNamespace	Host-pin guard (F1) intact
_parse_remote_url refactor is behavior-preserving	pre-existing TestParseRemoteUrl ADO/GHE/visualstudio cases	Single-owner refactor, no behavior drift

Important

The nested-extends: host-pin fix was surfaced by the local apm-review-panel dogfood pass (supply-chain + auth personas, independently): the original tests exercised _fetch_chain_parent directly and bypassed _validate_extends_host, masking the reject. Regression tests now drive the real validation path.

How to test

• Point a repo's origin at a nested GitLab remote (gitlab.com/acme/dept-a/team-x/my-project) and publish apm-policy.yml in acme/dept-a/apm-policy.
• Run apm audit --ci (or apm install) and confirm the acme/dept-a policy is applied (closest wins), not the top-level one.
• Remove that policy; confirm discovery falls through to acme/apm-policy.
• Remove all policies; confirm a clean absent (no warning, no fetch-failure).
• uv run python -m pytest tests/unit/policy/test_discovery.py.

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

[Copilot]

🟡 Changes recommended

Unresolved critical host-pin and moderate GitLab discovery findings block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds subgroup-aware GitLab policy discovery, selecting the closest policy and supporting nested extends: references.

Changes:

• Walks GitLab namespaces from deepest subgroup to top-level.
• Updates nested inheritance parsing and host validation.
• Adds tests, conformance updates, documentation, and changelog entries.

File summaries

File	Summary
tests/unit/policy/test_discovery.py	Adds subgroup discovery and inheritance tests.
tests/spec_conformance/test_policy_reqs.py	Adds conformance coverage.
src/apm_cli/policy/discovery.py	Implements namespace walking and shared remote parsing. Critical (1 vote): single-label host prefixes can bypass the host-pin guard for non-GitLab leaves. Moderate (3 votes): origin is read twice during GitLab discovery.
src/apm_cli/policy/_gitlab.py	Supports nested GitLab policy references. Moderate (1 vote): mismatched host-like prefixes and malformed ports are not safely rejected.
packages/apm-guide/.apm/skills/apm-usage/governance.md	Updates usage guidance. Nit (1 vote): publishing checklist still describes top-level-only policies.
docs/src/content/docs/enterprise/policy-reference.md	Updates policy reference behavior.
docs/src/content/docs/enterprise/governance-guide.md	Updates governance guidance.
docs/src/content/docs/enterprise/apm-policy.md	Documents subgroup policy resolution.
CONFORMANCE.md	Updates generated conformance count.
CONFORMANCE.json	Updates generated conformance mapping.
CHANGELOG.md	Adds the feature entry.

Review details

Suppressed comments (2)

packages/apm-guide/.apm/skills/apm-usage/governance.md:431

• The packaged usage guide still has a publishing checklist at governance.md:681-682 that says GitLab policy belongs under the top-level group only. With the subgroup walk described here, that omits the primary team-scoped deployment and leaves this user-facing guide incomplete; update the checklist to mention any ancestor namespace and closest-wins behavior.

to inherit it. A flat `<group>/<project>` remote probes only `<group>/apm-policy`.

src/apm_cli/policy/_gitlab.py:155

• For nested host-qualified refs, this branch now reaches explicit.port for any 3+ segment ref. On a non-default-port leaf, gitlab.example.test/acme/... passes hostname-only validation but is not stripped and is then fetched as namespace gitlab.example.test/acme/...; a malformed port such as gitlab.example.test:not-a-port/... instead raises ValueError because only urlsplit is caught. Treat host-like first segments whose host/port does not exactly match the leaf as invalid (and safely catch port parsing) rather than falling through.

                and explicit.port == port
            ):
                parts = parts[1:]

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Rafael Azevedo (rrazvd)]

Thanks for the review — addressed all four findings in e9cf62ef4.

🔴 Critical — host-pin bypass for single-label prefixes (discovery.py)
Correct catch: my _extract_extends_host relaxation was too broad. It's now leaf-aware — a bare single-label first segment is treated as a namespace only when the leaf is GitLab (where nested subgroups make acme/dept-a/apm-policy valid). On GitHub/ADO leaves a 3-segment ref is host/owner/repo, so extends: "evil/org/.github" is again parsed as a cross-host ref and rejected (F1 preserved). Regression tests added: test_single_label_first_segment_stays_a_host_on_github_leaf, test_validate_rejects_single_label_cross_host_on_github_leaf.

🟠 Moderate — host-like prefix / malformed port (_gitlab.py)
A host-like first segment (FQDN or host:port) must now match the leaf host and port exactly; otherwise the ref is rejected as invalid instead of being folded into the namespace. Port parsing is inside the try, so a malformed port (gitlab.example.test:not-a-port/...) fails closed instead of raising ValueError. Tests: test_gitlab_parent_rejects_host_prefix_with_mismatched_port, test_gitlab_parent_rejects_malformed_port_without_crashing.

🟠 Moderate — origin read twice (discovery.py)
_auto_discover now reads git remote get-url origin once and reuses it for both identity and the namespace walk — _gitlab_namespace_descending takes the already-read URL, and _extract_org_host_port_from_git_remote accepts it via a sentinel so it never re-reads. Removes the double subprocess and the TOCTOU on a changing remote. The single-subprocess contract test (tests/unit/policy/test_discovery.py, ADO) still passes.

⚪ Nit — publishing checklist (governance.md)
Updated to say GitLab policy may live under any ancestor namespace (closest wins), not the top-level group only.

Local gates green: ruff check/format, pylint R0801 10/10, tests/unit/policy + tests/spec_conformance/test_policy_reqs.py (1247 passed), tests/unit/install -k "policy or preflight or discover" (323 passed), scripts/lint-architecture-boundaries.sh (exit 0).

[Copilot]

🟡 Changes recommended

The canonical-owner guard is incomplete, conformance coverage is misclassified, and dotted GitLab namespace guidance is missing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tests/spec_conformance/test_policy_reqs.py:150

• This marker overstates conformance coverage. req-pl-011 requires a registered, ordered provider list and per-project provider selection; this test only checks the implementation-specific namespace order inside one GitLab provider, which is already covered by the unit test. Remove this requirement marker/test from the conformance suite and regenerate CONFORMANCE.*, or replace it with an assertion that exercises the normative provider-registration contract.

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Balanced

[Rafael Azevedo (rrazvd)]

Addressed the second review in 7ae2ce674.

Canonical-owner guard (incomplete) — discovery.py
Added a registered static delegation guard, install-deployment-policy-remote-origin-owner (in install_policy_gitlab_and_bundle.py, wired via install_policy_intent.EXTRA_RULES). It forbids any policy-tree module other than discovery.py from reading git remote get-url origin or defining the remote-URL splitter/parsers (_remote_url_parts, _parse_remote_url, _git_remote_origin_url), so a future caller cannot reintroduce the double-read/divergent-parse. Verified it has teeth (a planted violation in another policy file is flagged; removing it returns the linter to green). This mirrors the existing install-deployment-gitlab-policy-adapter guard, which likewise ships without a standalone owner-inventory record: discovery.py already has exactly one owner record (cached-policy-shape in contracts-tooling.json) and the registry rejects both a second record selecting the same file and a cross-group guard reference, so the guard is the enforcement mechanism here.

Dotted GitLab namespace guidance (missing) — apm-policy.md
Added a note: a bare nested extends: ref whose first segment contains a dot (group named e.g. acme.tools) is read as a host and rejected; users must spell it host-qualified (extends: "gitlab.com/acme.tools/team/apm-policy").

Conformance coverage misclassified — test_policy_reqs.py
Agreed — removed the req-pl-011 test that only re-asserted the GitLab provider's namespace order. Per req-pl-011 §6.1.1 that order is implementation-defined, so it isn't a normative conformance property, and the unit suite already covers it. Regenerated CONFORMANCE.* (req-pl-011 back to 2). The policy-tree change is covered under Mode B by an apm-spec-waiver: trailer (the walk refines the existing req-pl-011 provider; no new normative requirement) — the detector reports WAIVED.

Local gates: ruff check/format (src/tests) clean, scripts/lint-architecture-boundaries.sh exit 0, Mode B detector exit 0 (WAIVED), tests/unit/policy + tests/spec_conformance green (the one unrelated copilot_plugins failure is pre-existing on main).

[Copilot]

🟡 Changes recommended

Architecture mutation checks will fail or pass vacuously, and policy documentation remains internally inconsistent.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 4
• Review effort level: Balanced

[Rafael Azevedo (rrazvd)]

Addressed the third review in 0eebdf871.

Vacuous facade guard + failing mutation (discovery.py)
Correct — hoisting is_gitlab renamed the branch to elif is_gitlab: and moved the adapter call into _gitlab_walk_candidate, so check_gitlab_facade_orchestration scanned a stale elif is_gitlab_hostname(host): region and passed vacuously (its mutation case install-deployment-gitlab-facade-orchestration no longer produced a violation). The check now scopes its orchestration scan to _gitlab_walk_candidate's body. Verified: test_semantic_rule_catches_its_mutation[install-deployment-gitlab-facade-orchestration] passes again.

Guard-less rule without a mutation case (install_policy_intent.py)
Correct — the frozen matrix asserts exact set-equality with every guard-less rule. Gave install-deployment-policy-remote-origin-owner teeth the matrix can exercise (it now also requires discovery.py to define all three canonical read/parse helpers) and added its mutation case (def _git_remote_origin_url( -> _impl). test_matrix_covers_every_guardless_rule_exactly_once and the new case's catches_its_mutation both pass. I kept it as a guard-less semantic rule rather than an owner-registry record because discovery.py already has exactly one owner record (cached-policy-shape) and the registry rejects a second record selecting the same file — this mirrors the sibling install-deployment-gitlab-policy-adapter/-facade guards.

policy-reference.md internal inconsistency
Updated the extends value table (added the GitLab namespace/.../repo ancestor form and the top-level-group meaning of org) and the Discovery-vs-extends note (now states GitLab discovers the closest apm-policy up the subgroup tree), so the page has one consistent contract.

Dotted-group workaround incomplete for explicit ports (apm-policy.md)
Documented that when the origin uses a port, the host-qualified extends: ref must carry the exact host:port authority (e.g. gitlab.example.com:8443/acme/team/apm-policy), since GITLAB_HOST is only the hostname and _fetch_gitlab_chain_parent requires host+port to match.

Local gates: ruff (src/tests) clean, scripts/lint-architecture-boundaries.sh exit 0, mutation matrix structural + the two affected cases green.

[Rafael Azevedo (rrazvd)]

Pre-emptive hardening in 7bcbc1854 (before requesting another review): ran the full architecture suite locally and caught one more gap the earlier per-case runs missed.

tests/unit/scripts/test_architecture_runner.py::test_registered_rule_inventory_matches_frozen_semantic_contract pins a frozen set of every registered rule id. The new install-deployment-policy-remote-origin-owner guard was missing from it, so this test would have failed in CI. Added it to _EXPECTED_RULE_ID_TEXT.

Full local runs now green:

• tests/integration/test_architecture_semantic_rule_mutations.py — 148 passed
• tests/unit/scripts/test_architecture_runner.py — 72 passed
• registry / registry-conflicts / intent-guards / owner-rule-mutations / linter-entrypoint — all passed
• ruff (src/tests), scripts/lint-architecture-boundaries.sh (exit 0)

[Copilot]

🔵 Needs a closer look

The new canonical-owner rule is not registered and bound through the architecture owner registry.

Review details

Suppressed comments (1)

scripts/architecture_linter/checks/install_policy_intent.py:110

• This new canonical-owner check is registered through _semantic_rule, which assigns guard_ids=(), and there is no matching owner record in .apm/architecture/owners/install-deployment.json. As a result, the owner registry cannot validate this guard bidirectionally or ensure it executes exactly once. Register the remote-origin decision in the owner registry and bind this rule to that guard ID rather than treating it as guard-less.

    _semantic_rule(
        RULE_REMOTE_ORIGIN_OWNER,
        "Policy discovery reads and parses the git remote through one owner (discovery.py).",
        check_policy_remote_origin_owner,
    ),

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Rafael Azevedo (rrazvd)]

Addressed in fcacaa26e — bound the remote-origin guard to the owner registry as requested.

What changed

• install-deployment-policy-remote-origin-owner is now a real owner-guard: registered with guard_ids=(<id>,) (a Rule(...), not _semantic_rule), so it is a known guard the registry validates bidirectionally and requires to execute exactly once.
• It is referenced from discovery.py's existing owner record (cached-policy-shape in contracts-tooling.json). Note on the earlier back-and-forth: discovery.py can hold only one owner-registry record (selector uniqueness is global), and a guard's id-prefix is independent of the shard it's referenced from, so the remote-origin decision joins that record's guard list rather than getting a second record.
• Moved its mutation-break case from the guard-less matrix to the owner-guard matrix (test_architecture_owner_rule_mutations.py).

Verification — full architecture suite green:

• test_architecture_owner_rule_mutations.py + test_architecture_semantic_rule_mutations.py + test_architecture_runner.py + test_architecture_registry.py + test_architecture_registry_conflicts.py + test_architecture_intent_guards.py -> 603 passed
• scripts/lint-architecture-boundaries.sh -> exit 0 (guard executes once, bound to its owner record)
• The guard's mutation case fires (test_owner_rule_catches_its_guard_mutation[install-deployment-policy-remote-origin-owner]), and HEAD is clean.

[Copilot]

🟡 Changes recommended

Concealed GitLab 404s can select a weaker ancestor policy, and the ownership guard does not fully enforce single-reader delegation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 2
• Review effort level: Balanced

[Rafael Azevedo (rrazvd)]

Addressed both findings in 4de20be38.

Concealed GitLab 404 -> weaker ancestor (fail-open)
Real concern, thanks. GitLab returns 404 both for a missing project and for a private one the token cannot read, so the walk could silently downgrade a denied closer team policy to a weaker ancestor. Fix: before applying an ancestor over skipped closer levels, _gitlab.first_concealed_closer_policy verifies via authenticated Git whether any skipped closer apm-policy project actually exists; if one is confirmed, discovery fails closed (cache_miss_fetch_fail) naming the concealed project, instead of applying the weaker ancestor. The common inheritance case (a team with no closer project) is unaffected -- Git can't confirm a non-existent project, so the walk ascends normally, and the all-absent path never probes. The residual case (a token blind to the project over both REST and Git) is indistinguishable from genuinely absent and still ascends; that's a GitLab-platform limitation, now documented with a caution note advising read access for every honoured apm-policy project. Kept _gitlab_project_state_via_git inside the adapter so the facade-orchestration guard still holds. Tests: test_gitlab_subgroup_concealed_closer_policy_fails_closed, TestFirstConcealedCloserPolicy, and the ascend test now asserts the skipped levels are verified.

Origin-read guard didn't enforce single delegation
Correct -- the guard only required the three helper names to exist, so a second subprocess.run([... "remote", "get-url", "origin"]) elsewhere in discovery.py would have passed. Added an exact-count clause (the origin-read argv must appear exactly once in the owner) and switched the owner-guard mutation to introduce a duplicate read, so the single-reader invariant is now enforced and proven by the mutation matrix.

Verification (full local runs): owner-guard + guard-less mutation matrices, runner, tests/unit/policy, tests/spec_conformance/test_policy_reqs.py, install-side policy -> 2139 passed; ruff/format clean; scripts/lint-architecture-boundaries.sh exit 0.

[Rafael Azevedo (rrazvd)]

Ran the repo's apm-review-panel dogfood (7 personas) over the full branch diff before requesting another review; it surfaced real issues, fixed in af821d5f5.

Fail-open by default (devx, blocking) — the concealed-closer result returned cache_miss_fetch_fail, which honours policy.fetch_failure_default (default warn), so a concealed private closer policy silently proceeded with no enforcement — contradicting the doc's "fails closed." Fixed: it now returns incomplete_chain, which always fails closed regardless of the knob, and the CLI surfaces the specific "closer policy concealed — grant read access" message instead of the generic "check connectivity" copy. Added a gate-level test asserting it blocks under fetch_failure_default=warn.

Uncached git probe on the hot path (performance, blocking) — the concealment git ls-remote probe ran uncached on every install in the common "team inherits the org policy" path (N-1 network round-trips). Fixed: each probe verdict is cached per (host, namespace, repo) under the policy cache (same TTL); --no-cache bypasses it. Tests cover cache-hit (probes once) and no-cache-bypass.

Closest-first coverage gap (test-coverage, blocking) — first_concealed_closer_policy was only tested with a match at index 0. Added middle-level and among-many-confirmed tests.

Docs (devx + doc-writer) — reworded the concealed-policy caution to state the unconditional block; added a governance-guide "sharp edges" bullet covering both the closest-wins shadowing risk (a closer subgroup policy can weaken a stricter ancestor — restrict apm-policy project creation via GitLab RBAC) and the concealed-blocks-install behavior; added the skill note.

Panel also confirmed no blocking issues in: the leaf-aware F1 host-pin, the _remote.py extraction (faithful, no auth/behavior drift), fail-closed semantics across the walk, credential routing in the git probe, and internal doc consistency.

Local gates green: ruff/format, pylint R0801, tests/unit/policy (1235), install-side policy (323), owner+guardless mutation matrices, arch linter, Mode B (WAIVED), file-length guard, YAML/relative_to/auth-signals. The full unit suite's only failures were pre-existing/environmental (xdist-parallelism flakes in unrelated test_uninstall_* / windows-diagnostics files; they pass in isolation).

[Copilot]

🟡 Changes recommended

Concealment handling can bypass checks for stale policies, perform network access in cache-only mode, and cache indeterminate authentication failures as absence.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/apm_cli/policy/discovery.py:1017

• The concealment check is skipped when an ancestor policy is returned as cached_stale. Both cache-only lookup and an online refresh failure can return that outcome with a usable policy, so this path applies the stale ancestor after closer 404s without checking whether a closer policy was concealed. Gate this on the presence of a policy rather than only found/empty.

            if skipped and result.outcome in {"found", "empty"}:

• Files reviewed: 16/16 changed files
• Comments generated: 3
• Review effort level: Balanced

[Rafael Azevedo (rrazvd)]

Addressed all four findings in 7e7b1f4d6.

Caching indeterminate as absent (blocking, _gitlab.py) — correct and important. _gitlab_project_state_via_git returns None for a missing project AND for auth/network/timeout failures alike, so caching an absent verdict let a transient failure be remembered as "confirmed absent" for the TTL and apply a weaker ancestor. Fixed: only the definitive present verdict is cached now; None is never cached. Since absence is never definitive, the common inherit-from-org path re-probes each install — that cost is accepted (documented in the trade-offs) rather than ship an unsound negative cache.

Network access in cache-only mode (blocking, discovery.py) — cache_only is the offline local-bundle contract; the probe could still git ls-remote. Fixed: cache_only is threaded into first_concealed_closer_policy, which never probes in that mode — a cached present fails closed, and an unverifiable level fails closed deterministically instead of reaching out. Test: test_cache_only_never_probes_and_fails_closed_when_unverifiable.

cached_stale ancestor bypassed the concealment check (suppressed) — the gate was outcome in {found, empty}, but a cached_stale ancestor also carries a usable policy and would be applied without checking for a concealed closer. Fixed: gate on result.policy is not None.

Rule description named the wrong owner — updated the install-deployment-policy-remote-origin-owner description to say policy/_remote.py (matching the registry record, guard, and definitions).

Verification: ruff/format, arch linter (exit 0), tests/unit/policy (1236) green; new tests cover present-cached / indeterminate-not-cached / cache-only-fails-closed / closest-among-many.

[Copilot]

🟡 Changes recommended

Concealment detection can falsely block valid inheritance, miss fallback credentials, and diverges from documented fail-closed behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py:126

• The owner registry names _extract_org_host_port_from_git_remote as part of the canonical remote-identity owner, but this guard neither requires that definition in _remote.py nor bans a duplicate definition elsewhere. As written, that durable identity decision can be split without the architecture linter noticing. Include this helper in the guarded definition set/count and add a mutation case proving duplicate ownership is rejected.

• Files reviewed: 16/16 changed files
• Comments generated: 6
• Review effort level: Balanced

[Rafael Azevedo (rrazvd)]

Rather than fix the fifth round of findings on the concealed-policy detection, I removed the mechanism entirely (9f4231450) — it was scope creep beyond #2753 and, as the reviews established, not worth its complexity.

Why it's gone

• It only caught a narrow case (project git-readable but the policy file 404s).
• It could not be soundly negative-cached: git ls-remote can't distinguish a missing project from an auth/network failure, so a transient error would have been cached as "absent."
• This round's finding was decisive: the probe checks project existence, but a project can exist with only a README — so it false-positived and blocked inheritance from a valid ancestor, worse than the ambiguity it targeted.

What replaces it
A GitLab absent (404) simply ascends to the next ancestor — the same 404 == no policy at this level behaviour GitHub and ADO discovery already have, and strictly better than the pre-PR single-level behaviour (an ancestor policy now applies where nothing did before). The GitLab 404-conceals-private-projects ambiguity is documented as a platform limitation (grant the CI token read access), matching how the docs already treat 404s elsewhere.

This resolves all four findings from the last review by removal, and drops ~290 lines (net diff 1135 → 844 insertions). The core of the PR is unchanged: subgroup walk, nested extends:, leaf-aware host-pin (F1), and the _remote.py extraction + its owner-guard.

Local gates green: ruff/format, arch linter (exit 0), tests/unit/policy (1227) + spec (1246 combined), install-side policy (323), owner + facade mutation cases. discovery.py is 2063 lines (well under the ratchet).

[Copilot]

🔵 Needs a closer look

The canonical-owner guard omits one function named by its registry contract, leaving the architecture boundary partially unenforced.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

scripts/architecture_linter/checks/install_policy_gitlab_and_bundle.py:129

• The owner registry names _extract_org_host_port_from_git_remote as part of this canonical decision, but this guard neither counts nor bans that definition. A second identity extractor in another policy module would therefore pass the static boundary check, leaving the registered owner only partially enforced. Include the extractor in the guarded definition set and update the expected count.
  src/apm_cli/policy/_remote.py:8
• _git_remote_origin_url is defined in this module, not owned by discovery.py. Saying that discovery.py has its own reader contradicts both the implementation and the new owner registry, making the canonical ownership documentation ambiguous.

• Files reviewed: 16/16 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Rafael Azevedo (rrazvd)]

Addressed the latest Copilot review.

Owner-guard vs owner-registry mismatch (install_policy_gitlab_and_bundle.py): the _remote.py owner record names _extract_org_host_port_from_git_remote as part of the canonical git-remote decision, but the static guard install-deployment-policy-remote-origin-owner only counted/banned three defs -- a second identity extractor added elsewhere would have slipped through. Fixed in 4851ba7ac: added the extractor to _REMOTE_PARSER_DEFS and bumped the expected count to 4. Verified the guard has teeth (renaming the extractor now fails with found 3), the arch linter passes clean at HEAD, and the owner-guard mutation case still fires.

The three other inline comments in the previous round referenced the concealed-policy detection code, which was removed entirely in the simplification pass -- they're moot.

[Rafael Azevedo (rrazvd)]

🏛️ apm-review-panel (local dogfood, working tree)

Ran the multi-persona panel on the branch diff. Consolidated recommendation: APPROVE-WITH-NITS — unanimous, no REQUEST-CHANGES.

Reviewer	Verdict
supply-chain-security	APPROVE-WITH-NITS — host-pin holds vs cross-host credential routing; hash_mismatch/incomplete_chain remain unconditionally fail-closed; fetch surface bounded (namespace-depth × 1)
python-architect	APPROVE-WITH-NITS — _remote.py extraction is the right seam; single-owner boundary is real and dual-guarded
auth-expert	APPROVE-WITH-NITS — credential never routed by attacker-influenceable host/ref; walk pins host+port to the project's own remote; extends chain validates host before fetch
devx-ux-expert	APPROVE-WITH-NITS + 1 REQUIRED (visibility)
test-coverage	APPROVE-WITH-NITS — user-visible promise defended by behavioral tests; owner-guard mutant provably killed

Addressed in 97767eb9c

• REQUIRED (devx-ux): the closest-wins walk was invisible at default verbosity. A project silently picking up a subgroup policy instead of the top-level one produced no output in warn mode. Added a subgroup_scoped flag to PolicyFetchResult, set in the walk when the winning namespace is below the top-level group, threaded through outcome_routing, so the Policy: <source> line now shows at info even in non-verbose warn mode. Top-level resolutions keep the prior silence.
• Coverage (test-coverage): added the positive host-pin port branch (matching non-null port strips + fetches), an empty-outcome short-circuit at a deep subgroup (README-only apm-policy must shadow a real ancestor, not fall through), and an end-to-end wiring test running the real _gitlab_namespace_descending from a nested origin. Extended the walk tests to assert subgroup_scoped.

Considered, not applied

• python-architect advisory to narrow the two except Exception clauses in _remote.py: test_https_url_parse_exception_returns_none pins the broad catch as intentional cross-version defensiveness, so the premise (urlparse only raises ValueError) doesn't hold. Clarified the comment instead.

Advisory / deferred (documented, non-blocking)

• closest-wins is not tighten-only at discovery level, and GitLab 404 conceals private projects → both are real trust-boundary shifts but honestly documented with the RBAC mitigation. No silent drift from the security model.
• Latency of deep-nesting walks and the CHANGELOG lead-line / extends: "org" doc parenthetical are left as doc nits.

Verification: ruff check + format clean; targeted suites 318 passed; architecture linter exit 0; owner-guard mutation case still kills its mutant.

[Copilot]

🟡 Changes recommended

The remote-reader extraction drops existing exception handling, and the new tests need URL-assertion and chain-level coverage corrections.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/apm_cli/policy/_remote.py:50

• Preserve the extracted reader's previous ValueError handling here. The old implementation converted ValueError from the git subprocess setup into a clean “no remote” result; _auto_discover now calls this helper directly, so the same exception can escape policy discovery instead of returning no_git_remote.
  tests/unit/policy/test_discovery.py:1646
• This calls the guard directly, so it still does not cover the production chain path that previously let tests bypass _validate_extends_host. Add a discover_policy_with_chain regression test with a nested GitLab extends: leaf and mocked adapter fetches, verifying that validation derives the GitLab leaf host and routes the nested namespace without attempting a foreign host.

• Files reviewed: 19/19 changed files
• Comments generated: 1
• Review effort level: Balanced