Repository navigation
contributor-check returns UNKNOWN on transient failure with no retry, producing false needs-review flags #27
Description
Activity
Second round of false flags, and they are all on one person.
Every open issue carrying
needs-review:UNKNOWNright now was lywinged's: trace-spec#124, #128, #138, #142 and trace-tests#53. Removed all five.Worth recording because it sharpens the cost. Three of those four trace-spec issues were accurate enough to become PRs today (#146, #147, #148), and #138 found a real signature-verification defect in trace-verify. The check is applying its most suspicious label, on every submission, to the most productive external contributor this project has, purely because a transient lookup outranks LOW under the fail-closed ordering.
That is now five false flags across two rounds, versus one correctly-computed HIGH on integrations#46. Items 1 and 2 in the description are still the fix; this comment is only to note the pattern is recurring rather than one-off, and that it lands on the same account every time.
Fix transient contributor-check failures and retry handling. Removing individual UNKNOWN labels does not resolve the underlying bot behavior.
One detail on the root cause, because it makes the fix smaller than the description suggests.
scripts/contributor_check.py does have a retry loop, for attempt in range(3) at line 82. It only covers one case. A 403 retries on Retry-After, a 404 returns None, and everything else reaches raise on the first attempt. Two gaps follow:
- 5xx is not retried, so a GitHub 502 exits immediately.
- URLError is not caught. Line 31 imports HTTPError only, and URLError does not appear in the file at all, so the timeout=15 on line 84 raises straight through the loop. A socket timeout is the likeliest source of what you saw.
The script then exits non-zero, _run_check in contributor_check_action.py reads empty stdout, json.loads throws, and the blanket except Exception returns UNKNOWN. So the retry has to live in _api. By the time it reaches the action wrapper the subprocess is already gone.
The packaged copy is already right. agent-governance-python/agent-compliance/src/agent_compliance/cli/contributor_check.py has _RETRY_MAX_ATTEMPTS at line 76, a full-jitter _retry_sleep_seconds() at 81, and three branches: 403 on Retry-After clamped to 5 to 60, 5xx with backoff, and URLError with backoff. Porting _api and those two helpers over is most of item 1.
Mine is the first row in your table, so if you would rather this came from someone else that is fair. It is assigned to you, so tell me if you are already on it, otherwise I can open the AGT PR.
Mayur, there are already two related PRs: microsoft/agent-governance-toolkit#3838 adds retries around the subprocess, and #3876 consolidates the scripts onto the packaged checker. Could you check whether #3876 covers the failures you identified and add any missing regression cases there? Our action still pins the old revision, so updating that pin and checking the sparse checkout will also be needed once the fix lands.
Yes, #3876 covers it. The packaged _api it consolidates onto keeps all three branches, so 5xx retries with backoff and URLError is caught, which were the two gaps. Line numbers are on the PR.
One thing it introduces for us, worth catching before it merges. The new scripts/contributor_check.py is a shim that imports the package from agent-governance-python/agent-compliance/src. Their PR adds that path to their own workflow's sparse-checkout. Ours checks out scripts and nothing else, so advancing the pin past it means the shim imports a package that is not on disk, and _run_check returns UNKNOWN on every run rather than on a blip.
So the pin bump and the sparse-checkout widening are one change, not two.
Neither PR touches item 2 in your description, and that is the one I would rather see land. An import failure and a rate limit still produce the same label.
Happy to write the regression test for the scripts-only checkout against their branch.
Mayur, please add the regression test against #3876: show the scripts-only checkout failing and the expanded checkout working. Our pin update must include the package source in the same change.
Keep item 2 open separately. Infrastructure failures should produce an operational error without a contributor-risk label; they must not silently pass the check either. The retry and consolidation PRs do not resolve that distinction.
Ran it down against #3876's head, b246a69a. The scripts-only checkout does not just fail, it fails into the exact label this issue is about.
scripts/contributor_check.py is now a shim. It does sys.path.insert(0, ...) on parents[1] / "agent-governance-python" / "agent-compliance" / "src" and then importlib.import_module("agent_compliance.cli.contributor_check"). scripts/contributor_check_allowlist.json is removed in the same PR, having moved into the package.
Under sparse-checkout: scripts that path is not on disk, so the import raises and the subprocess exits non-zero with empty stdout.
_run_check in scripts/contributor_check_action.py calls subprocess.run without check=True and never reads result.returncode. It goes straight to json.loads(result.stdout), which raises on the empty string, and the bare except Exception returns "UNKNOWN". _aggregate_risk is fail-closed, so UNKNOWN outranks LOW and the label lands.
So bumping the pin past #3876 without changing the checkout reproduces this issue on every run rather than on an API blip. Same symptom, opposite cause: deterministic, not transient.
The pin update needs this in the same change, as you said:
sparse-checkout: | scripts agent-governance-python/agent-compliance/src
The shim resolves by sys.path rather than by an installed package, so the source tree is enough and no install step is needed.
On item 2, the line that conflates the two states is the except Exception: return "UNKNOWN" in _run_check. A missing module and a rate-limited probe are not the same fact, and the comment in _run_check already says UNKNOWN is "reserved for checks that errored or could not be determined". Reading returncode is what lets an infrastructure failure exit as an operational error rather than a contributor-risk level, and it is also what stops it passing silently. Agreed it stays separate from the retry and consolidation PRs.
Regression test to follow against b246a69a: scripts-only asserting UNKNOWN, expanded asserting a computed profile.
@Mayur021 AGT#3876 was closed as stale on 26 September without merging, and AGT main still has no shim, so our pin at
359a6b8with thescripts-only checkout is unaffected and no sparse-checkout change is needed. Your analysis stands if that consolidation comes back.Item 2 is the live fix. Since those scripts live in AGT and we only pin them, please open it here: vendor
contributor_check.pyandcontributor_check_action.pyinto this repo at359a6b8(MIT, keep the header and note the source commit), pointaction.ymlat the vendored copy, and make_run_checkreadreturncodeso an infrastructure failure exits as an operational error with no risk label. Include the scripts-only-style failure as the regression test. Aim for 10 October.- added a commit that references this issue
on Sep 30, 2026 Reopening: #46 landed item 2; item 1, the bounded retry in
_api, is still open.Reacted by Mayur AgnihotriItem 1 runs into the integrity contract #46 just set up, so flagging the choice before I write it.
The retry has to go in _api in contributor_check.py, and that file is one of the four the vendor README records as byte-identical to upstream, with the vendor-integrity job checking its sha256. Patching it fails that job by design.
Unless you would rather handle it another way, I will declare it a second local modification: the README gets a Local modifications section covering both files, contributor_check.py comes out of the checksum list, the other three stay pinned. Then offer the same patch to AGT so the divergence retires whenever we re-vendor.
Upstream on its own is not a route right now. AGT#3838 and AGT#3876 were both closed unmerged on 26 September, AGT#3571 has not moved since 4 August, and _api in AGT's scripts/ copy on main is still the 403-only loop. The packaged copy still has the three branches to port from.
@Mayur021 go ahead as a declared second modification, with the other three files still hash-pinned. Skip the upstream offer to AGT: we are not investing there, so this copy is ours now. Keep the retry bounded and limited to transient failures, so a genuine 404 or a rate-limit answer still reports as itself.
- added a commit that references this issue
on Oct 2, 2026
Summary
contributor-checkreturnsProfile: UNKNOWNon transient failures with no retry. Because the risk ordering is deliberately fail-closed,UNKNOWNoutranksLOW, so a momentary GitHub API blip labels an established contributor as more suspicious than a genuinely low-risk one. This has already produced three false flags on real submissions.Observed
Three issues from long-standing accounts were labeled
needs-review:UNKNOWN, all reportingProfile: UNKNOWN, Credential: LOW:None of these are plausible risk signals. For contrast, the same action returned a real, correctly-computed
Profile: HIGHon integrations#46, so the check works when the lookup succeeds.I have removed the misleading label from all three. They should be re-run once this is fixed.
Root cause
.github/actions/contributor-check/action.ymldoes not run its own logic. It checks outmicrosoft/agent-governance-toolkitat pinned ref359a6b8cf453f95d6bc9caf932057e1e78ccffd9(main as of 2026-07-31), sparse-checkoutscripts, and executes_agt/scripts/contributor_check_action.py.In that file:
Any exception at all, including a timeout or a single failed API call, collapses to
UNKNOWN. There is no retry.The fail-closed ordering is intentional and correct in principle:
The problem is that nothing tries hard enough before giving up, so fail-closed fires on noise.
Why this is not fixed by microsoft/agent-governance-toolkit#3571
AGT#3571 tracks
contributor_check.pyandcredential_audit.pyhaving diverged into two copies. The exponential-backoff retry added in AGT#2196 went to the packagedagent_compliance/cli/copy only, and was never backported toscripts/.This action consumes the
scripts/copy, which is the one without the retry. The only backoff-adjacent code inscripts/contributor_check.pyis a singleRetry-Afterheader read onHTTPError(line 88), not a general retry loop.So the two are related but distinct: resolving AGT#3571 upstream may fix this, but only if the reconciliation lands the retry in
scripts/and this action's pinned ref is advanced past it. Today it is pinned to 2026-07-31.Suggested fix
UNKNOWN, soUNKNOWNmeans "checked and could not determine" rather than "one call failed".UNKNOWNfromERRORin the output, so a genuine indeterminate result and an infrastructure failure are not the same label. Only the former deserves aneeds-reviewlabel.scripts/at a pinned SHA. That couples our contributor gating to an upstream repo's internal script layout and to whichever copy of a known-diverged pair happens to be there. Vendoring the checker, or depending on the published package instead, would decouple us.Items 1 and 2 are the fix. Item 3 is worth a separate decision.