feat(gate): configurable advisory check-runs so a stuck external status never freezes the gate (#4372) - #5793
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5793 +/- ##
=======================================
Coverage ? 95.15%
=======================================
Files ? 589
Lines ? 46686
Branches ? 14915
=======================================
Hits ? 44424
Misses ? 1511
Partials ? 751
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-15 02:18:56 UTC
⏸️ Suggested Action - Manual Review Review summary Nits — 5 non-blocking
Review context
Contributor next steps
Signal definitions
[BETA] Chat with GittensoryAsk Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
…us never freezes the gate (JSONbored#4372) Adds a per-repo `gate.advisoryCheckRuns` field: a list of `{ name, appSlug }` third-party check-runs (a security scanner, a contributor-trust analyzer, a CLA bot) to treat as advisory. A durable non-standard conclusion (most often a terminal `action_required` only a human can clear in that app's own UI) today freezes gittensory's gate forever — the check never turns green, so the PR is held pending with no review, label, or signal. A listed check-run — matched by name AND the trusted producing app slug, the same spoof-resistant pattern as `cla.checkRunAppSlug` — is excluded from the CI aggregate entirely (never gates pass/fail, never counts as "still running"), so it can never block or stall the gate. It is not silently swallowed either: a non-passing conclusion routes the PR to the existing manual-review hold with the triggering check/app named in the label reason and public comment. Config-driven and generic: no vendor name is hardcoded in any behavior-triggering path. Empty/omitted list is byte-identical to today for every repo that doesn't opt in. Wires the field through the engine parser, resolved settings, resolver, OpenAPI, the CI aggregate (REST + GraphQL paths) and its cache keys, the disposition planner, and the executor/approval-queue re-checks, with docs in both example configs and full line+branch test coverage.
…-end (JSONbored#4372) Address the review's one flagged design tradeoff: advisoryHoldDetails is not a persisted cache column, so a durable cache HIT reconstructs it as []. Add a test pinning both halves of why that is safe — deserialize returns [] on a hit, and invalidateCiStateCache (called app-agnostically by an advisory check_run's own `completed` webhook, before re-review) clears the entry so the next disposition read misses and re-fetches live, where the hold is re-derived. Tighten the deserialize comment to cite the exact invalidation handler and covering tests, so the guarantee is verifiable from the diff rather than asserted only in prose.
cea71bf to
1b33341
Compare
Summary
Adds a per-repo
gate.advisoryCheckRunsconfig field so a self-host operator can declare any third-party check-run — a security scanner, a contributor-trust analyzer, a license/CLA bot — as advisory. Fixes #4372: a durable, non-standard check conclusion (most commonly a terminalaction_requiredthat only a human can clear in the app's own UI) currently freezes gittensory's gate — the check never turns green, so the PR is held pending forever with no review, no label, no signal to the maintainer.A listed check-run (matched by name and trusted producing app slug — a name-only match is spoofable and ignored, exactly like
cla.checkRunAppSlug):success/neutral/skipped) routes the PR to the existing manual-review hold with the triggering check/app/conclusion named in the label reason and public comment, so a maintainer can act on the signal their installed app raised.Entirely config-driven and generic — no vendor name is hardcoded in any behavior-triggering path; the list comes wholly from
.loopover.yml. Empty/omitted ⇒ byte-identical behavior for every repo that doesn't opt in.What's wired (config-as-code parity — all in this PR)
packages/loopover-engine/src/focus-manifest.ts):FocusManifestGateConfig.advisoryCheckRuns, anormalizeOptionalAdvisoryCheckRunsvalidator (list of{ name, appSlug }, both required —appSlugmandatory because name-only matching is spoofable — capped at 16, invalid entries dropped with warnings), defaultnull, the any-gate-configured check, and thegateConfigToJsonround-trip. Type mirror inpredicted-gate-types.ts.RepositorySettings.advisoryCheckRuns(src/types.ts+ the enginemanifest-deps-types.tsmirror); resolver merge insrc/signals/focus-manifest.ts.src/openapi/schemas.tszod field + regeneratedapps/loopover-ui/public/openapi.json.src/github/backfill.ts): a spoof-resistantmatchAdvisoryCheckRunmatcher; the reducer excludes a matched run fromciState/hasPendingand, when completed non-passing, records it in a newadvisoryHoldDetailsfield. Threaded through both the REST and GraphQL aggregate paths so they stay byte-identical.src/settings/agent-actions.ts):advisoryCheckHoldfolds intoheldForManualReview(never a close), with the check/app surfaced in the manual-review label's reason + comment.ci-resolution.ts(4 entry points) → the aggregate, and is folded into both the request-memo and durable cache keys so a config change never serves a stale aggregate. The executor + approval-queue pre-merge re-checks apply the same exclusion, so planning and execution can't disagree..loopover.yml.example+ the byte-identicalconfig/examples/loopover.full.ymltwin.Scope
CONTRIBUTING.md; nosite//CNAME/lovable.Closes #4372.Validation
git diff --checknpm run typecheck(0 errors, engine rebuilt)npm run test:coverage— full suite green; every changedsrc/**+packages/loopover-engine/src/**line and branch covered (new dedicated tests below).npm run docs:drift-check, config-template parity, focus-manifest exhaustiveness — all green.npm run ui:openapi:check(regenerated artifact committed).npm run test:workers/ UI checks — the change touches no Worker binding orapps/loopover-uicode; the OpenAPI artifact is regenerated and committed.New tests: engine parse (valid/round-trip/resolve/cap/drop-missing-fields), CI-aggregate exclusion (completed non-pass → held; completed pass → no hold; in-progress → excluded; wrong app slug → not advisory; unconfigured → unchanged), the manual-review routing (holds, names the check, never closes, empty-array merges), and the cache-key fingerprint (config change → refetch; order-independent). All use synthetic app/check names, never a real vendor.
Safety
sanitizePublicComment), no compensation/optimization language.apps/loopover-uichange).Notes — requirement #4 (zero-required-contexts
isRequired()) is scoped out with rationale#4372's requirement #4 asks to also fix the general case where a repo with no branch-protection required contexts treats every check as implicitly required. This PR deliberately does not change that global
isRequired()behavior, for two reasons: (a) for the exact repos in the issue's evidence, a configured advisory check is now excluded entirely beforeisRequired()is ever consulted, so this feature already resolves their symptom; (b) changing the fold-all default for non-advisory checks is a separate, higher-risk behavior change to the CI hot path that deserves its own PR and test matrix rather than being bundled here. Flagging it explicitly here (not dropping it) so it can be tracked as a scoped follow-up: the generalisRequired()behavior for repos with zero configured required contexts is unchanged by this PR and remains open for a dedicated change.Closes #4372