fix: keep PR walkthrough links canonical (#3438)
This commit is contained in:
@@ -8,6 +8,8 @@ import unittest
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
WORKFLOW = REPO_ROOT / ".github" / "workflows" / "pr-walkthrough.yml"
|
||||
RECONCILE_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "pr-walkthrough-reconcile.yml"
|
||||
PREVIEW_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "preview-env.yml"
|
||||
REVIEW_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "opencode-code-review.yml"
|
||||
LINT_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "lint-action-pinning.yml"
|
||||
SETUP_OPENCODE_ACTION = REPO_ROOT / ".github" / "actions" / "setup-opencode" / "action.yml"
|
||||
@@ -35,11 +37,34 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase):
|
||||
if not WORKFLOW.is_file():
|
||||
raise AssertionError("PR walkthrough must have a dedicated workflow file")
|
||||
cls.workflow = WORKFLOW.read_text(encoding="utf-8")
|
||||
cls.reconcile_workflow = (
|
||||
RECONCILE_WORKFLOW.read_text(encoding="utf-8")
|
||||
if RECONCILE_WORKFLOW.is_file()
|
||||
else ""
|
||||
)
|
||||
cls.preview_workflow = PREVIEW_WORKFLOW.read_text(encoding="utf-8")
|
||||
cls.review_workflow = REVIEW_WORKFLOW.read_text(encoding="utf-8")
|
||||
cls.skill = (SKILL_DIR / "SKILL.md").read_text(encoding="utf-8")
|
||||
cls.generation = workflow_job(cls.workflow, "pr-walkthrough-generate")
|
||||
cls.publication = workflow_job(cls.workflow, "pr-walkthrough-publish")
|
||||
cls.link = workflow_job(cls.workflow, "pr-walkthrough-link")
|
||||
cls.reconcile = (
|
||||
workflow_job(cls.reconcile_workflow, "pr-walkthrough-reconcile")
|
||||
if cls.reconcile_workflow
|
||||
else ""
|
||||
)
|
||||
cls.preview_deploy = workflow_job(cls.preview_workflow, "deploy-same-repo")
|
||||
cls.preview_fork = workflow_job(cls.preview_workflow, "deploy-fork")
|
||||
cls.preview_cleanup = workflow_job(cls.preview_workflow, "cleanup-preview")
|
||||
cls.preview_description_same = workflow_job(
|
||||
cls.preview_workflow, "update-description-same-repo"
|
||||
)
|
||||
cls.preview_description_fork = workflow_job(
|
||||
cls.preview_workflow, "update-description-fork"
|
||||
)
|
||||
cls.preview_description_cleanup = workflow_job(
|
||||
cls.preview_workflow, "cleanup-preview-description"
|
||||
)
|
||||
cls.same_repo_review = workflow_job(cls.review_workflow, "opencode-review-same-repo")
|
||||
cls.fork_review = workflow_job(cls.review_workflow, "opencode-review-fork")
|
||||
|
||||
@@ -76,6 +101,82 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase):
|
||||
self.assertIn(condition, self.generation)
|
||||
self.assertNotIn("safe-to-test", self.generation)
|
||||
|
||||
def test_reconciliation_is_limited_to_authorized_edited_events(self) -> None:
|
||||
self.assertIn("pull_request_target:", self.reconcile_workflow)
|
||||
self.assertIn("types: [edited]", self.reconcile_workflow)
|
||||
for condition in (
|
||||
"vars.PR_WALKTHROUGH_ENABLED == 'true'",
|
||||
"github.event_name == 'pull_request_target'",
|
||||
"github.event.action == 'edited'",
|
||||
"github.event.pull_request.draft == false",
|
||||
"github.event.pull_request.head.repo.full_name == github.repository",
|
||||
"github.event.pull_request.head.repo.full_name != github.repository",
|
||||
"contains(github.event.pull_request.labels.*.name, 'safe-to-review')",
|
||||
"vars.CLAUDE_REVIEW_ALLOWLIST != ''",
|
||||
"contains(fromJSON(vars.CLAUDE_REVIEW_ALLOWLIST), github.event.pull_request.user.login)",
|
||||
):
|
||||
self.assertIn(condition, self.reconcile)
|
||||
self.assertNotIn("safe-to-test", self.reconcile)
|
||||
|
||||
def test_reconciliation_uses_trusted_read_only_publication_check(self) -> None:
|
||||
for value in (
|
||||
"ref: ${{ github.workflow_sha }}",
|
||||
"fetch-depth: 1",
|
||||
"persist-credentials: false",
|
||||
'test "$(git rev-parse HEAD)" = "$TRUSTED_SHA"',
|
||||
"WALKTHROUGH_BASE_URL: https://walkthrough.kandev.ai",
|
||||
'SHORT_HEAD_SHA="${HEAD_SHA:0:12}"',
|
||||
'PUBLIC_URL="${WALKTHROUGH_BASE_URL%/}/pr/${PR_NUMBER}/${SHORT_HEAD_SHA}.html"',
|
||||
"RESPONSE_META_PATH=pr-walkthrough-public.response",
|
||||
"curl --fail",
|
||||
"--write-out '%{http_code}\\t%{content_type}\\t%{url_effective}'",
|
||||
"response_status",
|
||||
'response_effective_url" != "$PUBLIC_URL"',
|
||||
"response_content_type",
|
||||
'test -s "$PUBLIC_HTML_PATH"',
|
||||
):
|
||||
self.assertIn(value, self.reconcile)
|
||||
self.assertNotIn("--dump-header", self.reconcile)
|
||||
|
||||
for forbidden in (
|
||||
"OPENCODE_API_KEY",
|
||||
"CLOUDFLARE_R2",
|
||||
"SPRITES_API_TOKEN",
|
||||
"opencode run",
|
||||
"go run",
|
||||
"pull_request.head.repo.full_name }}",
|
||||
):
|
||||
self.assertNotIn(forbidden, self.reconcile)
|
||||
|
||||
self.assertIn("contents: read", self.reconcile)
|
||||
self.assertIn("pull-requests: write", self.reconcile)
|
||||
self.assertIn("github.workflow_sha", self.reconcile)
|
||||
|
||||
def test_reconciliation_reuses_marker_required_race_safe_body_update(self) -> None:
|
||||
for value in (
|
||||
"concurrency:",
|
||||
"group: pr-description-${{ github.event.pull_request.number }}",
|
||||
"cancel-in-progress: false",
|
||||
"for attempt in 1 2 3; do",
|
||||
"--require-existing",
|
||||
"pr-latest-response.json",
|
||||
"jq -j '.body // \"\"'",
|
||||
"cmp -- pr-body-before pr-body-latest",
|
||||
"pr-body-expected",
|
||||
"pr-body-verified",
|
||||
"cmp -- pr-body-expected pr-body-verified",
|
||||
"gh api --method PATCH",
|
||||
"current_head_sha=",
|
||||
"verified_head_sha=",
|
||||
"pr-verified-response.json",
|
||||
"verify_status",
|
||||
):
|
||||
self.assertIn(value, self.reconcile)
|
||||
|
||||
patch_position = self.reconcile.index("gh api --method PATCH")
|
||||
public_check_position = self.reconcile.index("curl --fail")
|
||||
self.assertLess(public_check_position, patch_position)
|
||||
|
||||
def test_skill_explains_trusted_context_without_provider_names(self) -> None:
|
||||
self.assertNotIn("Kandev", self.skill)
|
||||
self.assertIn("managed runner", self.skill)
|
||||
@@ -365,12 +466,39 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase):
|
||||
|
||||
def test_link_job_retries_transient_github_api_responses(self) -> None:
|
||||
for value in (
|
||||
"pr_response_ok=false",
|
||||
"for attempt in 1 2 3; do",
|
||||
'if ! gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" > pr-response.json ||',
|
||||
"python3 -c 'import json; json.load(open(\"pr-response.json\", encoding=\"utf-8\"))'",
|
||||
'test "$pr_response_ok" = true',
|
||||
"patch_ok=false",
|
||||
'test "$patch_ok" = true',
|
||||
"pr-latest-response.json",
|
||||
"pr-verified-response.json",
|
||||
"sleep 2",
|
||||
):
|
||||
self.assertIn(value, self.link)
|
||||
|
||||
def test_description_writers_compare_fresh_bodies_and_verify_after_patch(self) -> None:
|
||||
for job in (
|
||||
self.link,
|
||||
self.preview_description_same,
|
||||
self.preview_description_fork,
|
||||
self.preview_description_cleanup,
|
||||
):
|
||||
self.assertIn("concurrency:", job)
|
||||
self.assertIn(
|
||||
"group: pr-description-${{ github.event.pull_request.number }}",
|
||||
job,
|
||||
)
|
||||
self.assertIn("cancel-in-progress: false", job)
|
||||
|
||||
for value in (
|
||||
"for attempt in 1 2 3; do",
|
||||
"pr-latest-response.json",
|
||||
"jq -j '.body // \"\"'",
|
||||
"cmp -- pr-body-before pr-body-latest",
|
||||
"pr-body-expected",
|
||||
"pr-body-verified",
|
||||
"cmp -- pr-body-expected pr-body-verified",
|
||||
"pr-verified-response.json",
|
||||
"verify_status",
|
||||
):
|
||||
self.assertIn(value, self.link)
|
||||
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
"""Contract tests for the fork preview authorization workflow."""
|
||||
|
||||
from pathlib import Path
|
||||
import re
|
||||
import unittest
|
||||
|
||||
|
||||
@@ -9,6 +10,15 @@ REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
WORKFLOW = REPO_ROOT / ".github" / "workflows" / "preview-env.yml"
|
||||
|
||||
|
||||
def workflow_job(workflow: str, name: str) -> str:
|
||||
marker = f" {name}:\n"
|
||||
_, separator, remainder = workflow.partition(marker)
|
||||
if not separator:
|
||||
raise AssertionError(f"workflow job is missing: {name}")
|
||||
next_job = re.search(r"^ [A-Za-z0-9_-]+:\n", remainder, re.MULTILINE)
|
||||
return remainder[: next_job.start()] if next_job else remainder
|
||||
|
||||
|
||||
class PreviewEnvironmentWorkflowContractTest(unittest.TestCase):
|
||||
def test_safe_to_review_and_allowlists_authorize_fork_preview_deployment(self) -> None:
|
||||
workflow = WORKFLOW.read_text(encoding="utf-8")
|
||||
@@ -30,7 +40,7 @@ class PreviewEnvironmentWorkflowContractTest(unittest.TestCase):
|
||||
"contains(fromJSON(vars.CLAUDE_REVIEW_ALLOWLIST), github.event.pull_request.user.login)",
|
||||
deploy_job,
|
||||
)
|
||||
self.assertEqual(workflow.count("persist-credentials: false"), 3)
|
||||
self.assertEqual(workflow.count("persist-credentials: false"), 6)
|
||||
|
||||
def test_safe_to_review_approval_survives_follow_up_pushes(self) -> None:
|
||||
workflow = WORKFLOW.read_text(encoding="utf-8")
|
||||
@@ -45,6 +55,32 @@ class PreviewEnvironmentWorkflowContractTest(unittest.TestCase):
|
||||
self.assertNotIn(" strip-safe-to-test:", workflow)
|
||||
self.assertNotIn("github.rest.issues.removeLabel", workflow)
|
||||
|
||||
def test_description_mutation_isolated_from_preview_lifecycle_jobs(self) -> None:
|
||||
workflow = WORKFLOW.read_text(encoding="utf-8")
|
||||
for name in ("deploy-same-repo", "deploy-fork", "cleanup-preview"):
|
||||
job = workflow_job(workflow, name)
|
||||
self.assertNotIn("concurrency:", job)
|
||||
|
||||
for name, command in (
|
||||
("update-description-same-repo", "update-description"),
|
||||
("update-description-fork", "update-description"),
|
||||
("cleanup-preview-description", "remove-description"),
|
||||
):
|
||||
job = workflow_job(workflow, name)
|
||||
self.assertIn("concurrency:", job)
|
||||
self.assertIn(
|
||||
"group: pr-description-${{ github.event.pull_request.number }}",
|
||||
job,
|
||||
)
|
||||
self.assertIn("cancel-in-progress: false", job)
|
||||
self.assertIn(f"go run ./cmd/preview {command}", job)
|
||||
|
||||
for name in ("deploy-same-repo", "deploy-fork"):
|
||||
job = workflow_job(workflow, name)
|
||||
self.assertIn("--skip-description", job)
|
||||
self.assertIn("preview_url=", job)
|
||||
self.assertIn("--skip-description", workflow_job(workflow, "cleanup-preview"))
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user