From a0b102aa4e2672f0418a10c56b78ca3e251bc00b Mon Sep 17 00:00:00 2001 From: okxlin <61420215+okxlin@users.noreply.github.com> Date: Fri, 13 Nov 2026 00:35:36 +0800 Subject: [PATCH] fix(renovate): keep auxiliary image updates open for review Auxiliary-only Renovate updates now stay open for manual review. Guard coverage and workflow lint passed. --- .github/scripts/renovate_sidecar_guard.py | 55 ++++++++++--------- .github/scripts/test_renovate_app_version.py | 10 ++-- .../scripts/test_renovate_sidecar_guard.py | 37 +++++++++++-- .github/workflows/renovate-sidecar-guard.yml | 40 +++----------- 4 files changed, 76 insertions(+), 66 deletions(-) diff --git a/.github/scripts/renovate_sidecar_guard.py b/.github/scripts/renovate_sidecar_guard.py index b99fac77f..8a869ebe6 100644 --- a/.github/scripts/renovate_sidecar_guard.py +++ b/.github/scripts/renovate_sidecar_guard.py @@ -161,11 +161,38 @@ def compare_compose( service_count, changed_services, primary_services, - "close", + "review", "multi-service compose changed only auxiliary services", ) +def aggregate_decisions(decisions: list[ComposeDecision], *, seen_compose: bool) -> dict: + if not seen_compose: + return { + "decision": "allow", + "reason": "no compose files changed", + "details": [], + } + + manual_review = [d for d in decisions if d.outcome == "review"] + blocking_unknown = [d for d in decisions if d.outcome == "unknown"] + if manual_review and not blocking_unknown: + summary = "; ".join( + f"{d.app}:{','.join(d.changed_services)}" for d in manual_review + ) + return { + "decision": "review", + "reason": f"auxiliary-only update needs full-stack review ({summary})", + "details": [d.__dict__ for d in decisions], + } + + return { + "decision": "allow", + "reason": "keep PR open", + "details": [d.__dict__ for d in decisions], + } + + def main() -> int: parser = argparse.ArgumentParser() parser.add_argument("--repo", required=True) @@ -221,31 +248,7 @@ def main() -> int: ) ) - if not seen_compose: - result = { - "decision": "allow", - "reason": "no compose files changed", - "details": [], - } - else: - closable = [d for d in decisions if d.outcome == "close"] - blocking_unknown = [d for d in decisions if d.outcome == "unknown"] - primary_changed = [d for d in decisions if d.outcome == "allow"] - if closable and not blocking_unknown and not primary_changed: - summary = "; ".join( - f"{d.app}:{','.join(d.changed_services)}" for d in closable - ) - result = { - "decision": "close", - "reason": f"sidecar-only Renovate update detected ({summary})", - "details": [d.__dict__ for d in decisions], - } - else: - result = { - "decision": "allow", - "reason": "keep PR open", - "details": [d.__dict__ for d in decisions], - } + result = aggregate_decisions(decisions, seen_compose=seen_compose) json.dump(result, sys.stdout, ensure_ascii=False, indent=2) sys.stdout.write("\n") diff --git a/.github/scripts/test_renovate_app_version.py b/.github/scripts/test_renovate_app_version.py index f6a100093..359e86adf 100644 --- a/.github/scripts/test_renovate_app_version.py +++ b/.github/scripts/test_renovate_app_version.py @@ -515,7 +515,7 @@ class RenovateAppVersionTests(unittest.TestCase): self.assertEqual("wait", too_soon["decision"]) self.assertEqual("limit", too_many["decision"]) - def test_retry_workflow_and_sidecar_branch_cleanup_are_guarded(self): + def test_retry_workflow_and_sidecar_updates_are_reviewed_without_branch_cleanup(self): retry_workflow = ( REPO_ROOT / ".github" / "workflows" / "renovate-rate-limit-retry.yml" ).read_text(encoding="utf-8") @@ -527,9 +527,11 @@ class RenovateAppVersionTests(unittest.TestCase): self.assertIn("actions: write", retry_workflow) self.assertIn("renovate_retry_gate.py", retry_workflow) self.assertIn("automatic retry after Docker Hub 429", retry_workflow) - self.assertIn('[[ "$head_repo" == "$REPO" ]]', sidecar_workflow) - self.assertIn('selfhosted-renovate/*|renovate/*', sidecar_workflow) - self.assertIn("git/refs/heads/$head_ref", sidecar_workflow) + self.assertIn("decision == 'review'", sidecar_workflow) + self.assertIn("gh pr comment", sidecar_workflow) + self.assertNotIn("gh pr close", sidecar_workflow) + self.assertNotIn("git/refs/heads/$head_ref", sidecar_workflow) + def test_image_tag_strips_digest_from_tagged_image(self): module = load_module() diff --git a/.github/scripts/test_renovate_sidecar_guard.py b/.github/scripts/test_renovate_sidecar_guard.py index a30103489..4b6e49db2 100644 --- a/.github/scripts/test_renovate_sidecar_guard.py +++ b/.github/scripts/test_renovate_sidecar_guard.py @@ -14,6 +14,7 @@ SPEC.loader.exec_module(GUARD) REPOSITORY_ROOT = Path(__file__).resolve().parents[2] PRIMARY_SERVICES_PATH = REPOSITORY_ROOT / ".github" / "renovate-primary-services.json" RENOVATE_CONFIG_PATH = REPOSITORY_ROOT / ".github" / "renovate-docker.json" +SIDECAR_GUARD_WORKFLOW_PATH = REPOSITORY_ROOT / ".github" / "workflows" / "renovate-sidecar-guard.yml" def compose(**images: str) -> dict: @@ -55,14 +56,14 @@ class ResolvePrimaryServicesTests(unittest.TestCase): class InferredPrimaryDecisionTests(unittest.TestCase): - def test_closes_weblate_sidecar_only_update(self) -> None: + def test_marks_weblate_sidecar_only_update_for_manual_review(self) -> None: base = compose(cache="valkey:9.1.0", database="postgres:18", weblate="weblate:latest") head = compose(cache="valkey:9.1.1", database="postgres:18", weblate="weblate:latest") primary = GUARD.resolve_primary_services("weblate", [], base, head) decision = GUARD.compare_compose("weblate", "apps/weblate/latest/docker-compose.yml", base, head, primary) - self.assertEqual(decision.outcome, "close") + self.assertEqual(decision.outcome, "review") self.assertEqual(decision.changed_services, ["cache"]) def test_allows_inferred_primary_update(self) -> None: @@ -84,6 +85,24 @@ class InferredPrimaryDecisionTests(unittest.TestCase): self.assertEqual(decision.detail, "single-service compose") +class DecisionAggregationTests(unittest.TestCase): + def test_auxiliary_only_update_emits_manual_review_decision(self) -> None: + decision = GUARD.ComposeDecision( + app="firecrawl", + path="apps/firecrawl/2.11.359/docker-compose.yml", + services_total=2, + changed_services=["firecrawl-playwright"], + primary_services=["firecrawl"], + outcome="review", + detail="multi-service compose changed only auxiliary services", + ) + + result = GUARD.aggregate_decisions([decision], seen_compose=True) + + self.assertEqual(result["decision"], "review") + self.assertIn("firecrawl:firecrawl-playwright", result["reason"]) + + class RepositoryPolicyTests(unittest.TestCase): @classmethod def setUpClass(cls) -> None: @@ -119,7 +138,7 @@ class RepositoryPolicyTests(unittest.TestCase): self.primary_services["dify"], ) - self.assertEqual(decision.outcome, "close") + self.assertEqual(decision.outcome, "review") def test_safeline_components_are_grouped_behind_management_service(self) -> None: self.assertEqual(self.primary_services["safeline"], ["safeline-mgt"]) @@ -173,7 +192,7 @@ class RepositoryPolicyTests(unittest.TestCase): self.primary_services["uuwaf"], ) - self.assertEqual(decision.outcome, "close") + self.assertEqual(decision.outcome, "review") def test_langflow_updates_require_manual_compatibility_review(self) -> None: self.assertTrue( @@ -184,5 +203,15 @@ class RepositoryPolicyTests(unittest.TestCase): ) +class WorkflowSafetyTests(unittest.TestCase): + def test_sidecar_only_updates_are_reviewed_without_closing_or_deleting_branches(self) -> None: + workflow = SIDECAR_GUARD_WORKFLOW_PATH.read_text(encoding="utf-8") + + self.assertIn("decision == 'review'", workflow) + self.assertIn("gh pr comment", workflow) + self.assertNotIn("gh pr close", workflow) + self.assertNotIn("git/refs/heads", workflow) + + if __name__ == "__main__": unittest.main() diff --git a/.github/workflows/renovate-sidecar-guard.yml b/.github/workflows/renovate-sidecar-guard.yml index 7f1d44f56..0b419535a 100644 --- a/.github/workflows/renovate-sidecar-guard.yml +++ b/.github/workflows/renovate-sidecar-guard.yml @@ -13,7 +13,7 @@ on: type: string permissions: - contents: write + contents: read pull-requests: write jobs: @@ -71,38 +71,14 @@ jobs: cat /tmp/renovate-sidecar-guard.json echo "decision=$(jq -r '.decision' /tmp/renovate-sidecar-guard.json)" >> "$GITHUB_OUTPUT" - { - echo 'reason<> "$GITHUB_OUTPUT" - - name: Comment and close sidecar-only PR - if: steps.guard.outputs.decision == 'close' + - name: Comment on auxiliary-only update for manual review + if: steps.guard.outputs.decision == 'review' env: GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number || inputs.pr_number }} run: | - pr_number="${{ github.event.pull_request.number || inputs.pr_number }}" - reason="${{ steps.guard.outputs.reason }}" - - gh pr comment "$pr_number" \ - --repo "${{ github.repository }}" \ - --body "Closing this Renovate PR automatically because it updates only auxiliary service image tags in a multi-service app and does not include any configured primary service image update.\n\nReason: ${reason}\n\nIf we decide to move this sidecar version intentionally, we can reopen or submit a maintainer PR with tested upgrade evidence." - - gh pr close "$pr_number" \ - --repo "${{ github.repository }}" - - REPO="${{ github.repository }}" - pr_head=$(gh api "repos/$REPO/pulls/$pr_number" \ - --jq '[.head.repo.full_name, .head.ref] | @tsv') - IFS=$'\t' read -r head_repo head_ref <<< "$pr_head" - - if [[ "$head_repo" == "$REPO" ]]; then - case "$head_ref" in - selfhosted-renovate/*|renovate/*) - if ! gh api --method DELETE "repos/$REPO/git/refs/heads/$head_ref"; then - echo "::notice::Renovate branch was already removed or could not be deleted: $head_ref" - fi - ;; - esac - fi + gh pr comment "$PR_NUMBER" \ + --repo "$REPO" \ + --body "Keeping this auxiliary-only image update open for manual review. Validate the complete service stack and old-to-new upgrade impact before merging. Current app-version automation derives package versions from primary service images, so it cannot represent this change as a primary-version bump."