refactor(compliance): consolidate contributor check CLIs - #3876
refactor(compliance): consolidate contributor check CLIs#3876Carlos Hernandez (carloshvp) wants to merge 2 commits into
Conversation
Signed-off-by: Carlos Hernandez-Vaquero <carloshvp@gmail.com>
Signed-off-by: Carlos Hernandez-Vaquero <carloshvp@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 3876 in repo microsoft/agent-governance-toolkit |
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
Consolidating these is clearly right and long overdue. One thing about the change is not what the
title says it is, and it decides how this should be reviewed.
This is not a move, it is a merge of two divergent implementations. Measured on main:
scripts/contributor_check.py 1475 lines
agent_compliance/cli/contributor_check.py 1076 lines
diff between the two main copies ~605 changed lines
and this PR's consolidated file is 1700 lines, with ~1238 changed lines against the packaged copy
it replaces. So the two copies had drifted substantially and the result is a third thing that matches
neither original. That is the correct destination, but "refactor" and "consolidate" both imply
behaviour preservation, and behaviour preservation is the one property a merge of two divergent forks
does not get for free.
Why that matters more here than it would elsewhere. scripts/contributor_check.py is what the
contributor-check workflow runs, through
scripts/contributor_check_action.py, and its risk output becomes the
needs-review:LOW/MEDIUM/HIGH/UNKNOWN label applied to every incoming PR. Those labels are already
imperfect in ways people have noticed. Changing which of two implementations produces them, in a PR
whose stated purpose is deduplication, means any resulting change in labelling would be attributed to
the wrong cause for weeks.
What I would want before merging is not more code but evidence: run both main copies and the
consolidated one against the same set of, say, a dozen real logins spanning LOW through HIGH, and
paste the risk values in a table. If all three agree, the merge is proven and this becomes an easy
approval. If they diverge, that is worth knowing regardless, because it means the workflow and the
agt-contributor-check console script have been giving different answers to the same question.
Two things I want to credit specifically.
Adding agent-governance-python/agent-compliance/src/agent_compliance to the workflow's
sparse-checkout is easy to forget and would have produced a confusing ModuleNotFoundError in CI
only, not locally. Catching it in the same PR is the difference between this landing and this being
reverted.
The shim shape is right. sys.modules[__name__] = _implementation after defining main keeps both
python scripts/contributor_check.py and import credential_audit working, so existing callers and
existing tests do not need touching. test_repository_shim_is_packaged_module asserting
shim is packaged pins exactly the property that makes it safe, which is the identity of the module
object rather than the equality of its behaviour.
Sequencing with #3812. That PR fixes a real pagination bug in _search, editing both
scripts/credential_audit.py and the packaged copy. Your shim deletes the implementation from the
scripts/ one. Landing #3812 first is the cheaper order: it is 160 lines against 3,200, and you then
drop its now-redundant scripts/ hunk on rebase. The other way round, #3812 has to be re-cut.
One small gap. The [tool.setuptools.package-data] addition covers cli/*.json, which picks up
contributor_check_allowlist.json. Worth confirming that the packaged wheel actually contains it, for
example with python -m build and a unzip -l on the result, since a missing data file in a wheel
fails at runtime for installed users and never in CI, where the source tree is present.
Nothing blocking on the direction.
|
Follow-up evidence requested in review, run on 2026-09-03 against exact PR head For each login, I called
The consolidated implementation matches the current workflow script in 12 of 12 cases across LOW, MEDIUM, and HIGH. The two mismatches are between the already-divergent Wheel evidence from the same exact head:
|
Summary
agent_compliance.clithe single source for contributor and credential checksThe canonical contributor checker keeps the package's retry and future-timestamp fixes and gains the repository checker's org-aware and domain-aware hardening.
cluster_detect.pyandcontributor_check_action.pyremain repository-local tools.Validation
Closes #3571