diff --git a/.github/workflows/flamingo-code-review.yml b/.github/workflows/flamingo-code-review.yml index 05aaa3bc..b1bb11d7 100644 --- a/.github/workflows/flamingo-code-review.yml +++ b/.github/workflows/flamingo-code-review.yml @@ -165,6 +165,11 @@ permissions: contents: read pull-requests: write checks: write + # Cache SAVE needs it. With an explicit permissions block every unlisted + # scope is 'none', so actions/cache could restore but not reserve a key and + # every run ended with "Failed to save: ... cache write denied: token has no + # writable scopes" — the corpus 304 path silently never worked. + actions: write jobs: # Consuming the label is its OWN job, gated on nothing but "that label was @@ -463,28 +468,109 @@ jobs: - name: Download the report script env: WEBHOOK_SECRET: ${{ secrets.DOC_ORCH_WEBHOOK_SECRET }} - HASH_WORKFLOW_HELPERS: "3df9f07c408d987a44e9df7ba1584f01ff57502a596f013dd3218d3b4c30fd4c" - HASH_REPORT: "479b45311e6e91e7dc36b90d6a32db57fae25b215fdcd35c2889fb5f0618df07" run: | set -euo pipefail - SCRIPTS_BASE_URL="${HUB_BASE_URL}/api/doc-orchestrator/scripts" + + SCRIPT_MANIFEST=/tmp/flamingo-script-manifest.json + # The canonical scripts surface and the pre-rename one. load_script_manifest + # picks whichever this deployment actually serves and pins SCRIPTS_BASE_URL. + CI_SCRIPTS_URL="${HUB_BASE_URL%/}/api/ci/scripts" + LEGACY_SCRIPTS_URL="${HUB_BASE_URL%/}/api/doc-orchestrator/scripts" + + # _try_manifest — 0 loaded, 1 no manifest surface there, 2 fatal. + _try_manifest() { + local base="$1" code + code=$(curl -sS -w '%{http_code}' -o "$SCRIPT_MANIFEST" \ + -H "Authorization: Bearer $WEBHOOK_SECRET" \ + "$base/manifest.json") || code="000" + + if [ "$code" = "404" ]; then rm -f "$SCRIPT_MANIFEST"; return 1; fi + if [ "$code" != "200" ]; then + echo "❌ manifest request to $base failed (HTTP $code)" + rm -f "$SCRIPT_MANIFEST" + return 2 + fi + # The digests are the TOP-LEVEL object. successResponse is the standard + # emitter but it does NOT add a wrapper — it is NextResponse.json(data) + # plus the no-store header — so there is no .data to reach through. + # A 200 that is not a manifest is how a hub which does not serve this path + # answers (the proxy rewrites unknown routes and returns HTML), so it + # means "wrong surface", not "corrupt". + if ! jq -e 'type == "object" and length > 0 and (to_entries | all(.value | type == "string"))' "$SCRIPT_MANIFEST" >/dev/null 2>&1; then + rm -f "$SCRIPT_MANIFEST" + return 1 + fi + return 0 + } + + load_script_manifest() { + # The scripts surface was renamed from /api/doc-orchestrator/scripts to the + # pipeline-neutral /api/ci/scripts (it always served BOTH pipelines). The + # workflow file ships in the repo and the routes ship with the deployment, + # so the two are one version apart in BOTH directions across the rollout. + # Probe the canonical surface, fall back to the legacy one, and let the + # winner decide SCRIPTS_BASE_URL for every download that follows. + # "cmd; rc=$?" dies under the set -euo pipefail these steps run with — + # errexit fires before rc is read and the step ends with NO output. And + # "if ! cmd; then rc=$?" is worse: inside the branch $? is the status of + # the NEGATION (0), so every failure reads as success. "|| rc=$?" is the + # one form that both suppresses errexit and preserves the real code. + local rc=0 + _try_manifest "$CI_SCRIPTS_URL" || rc=$? + if [ "$rc" = "0" ]; then + SCRIPTS_BASE_URL="$CI_SCRIPTS_URL" + echo "✅ script manifest loaded ($(jq -r 'length' "$SCRIPT_MANIFEST") scripts)" + return 0 + fi + if [ "$rc" = "2" ]; then exit 1; fi + + echo "::warning::this hub does not serve $CI_SCRIPTS_URL — falling back to the legacy $LEGACY_SCRIPTS_URL; it predates the rename" + SCRIPTS_BASE_URL="$LEGACY_SCRIPTS_URL" + + rc=0 + _try_manifest "$LEGACY_SCRIPTS_URL" || rc=$? + if [ "$rc" = "0" ]; then + echo "✅ script manifest loaded ($(jq -r 'length' "$SCRIPT_MANIFEST") scripts)" + return 0 + fi + if [ "$rc" = "2" ]; then exit 1; fi + + # Neither surface published a manifest: a hub older than the manifest + # itself. Downloads proceed UNVERIFIED and say so on every file. Any + # OTHER failure above already exited — only a genuine "no such endpoint" + # reaches here, so a hijacked or failing request can never land in this + # branch and silently disable verification. + echo "::warning::no manifest endpoint on this hub — it predates the manifest; downloads run UNVERIFIED until the hub is redeployed" + rm -f "$SCRIPT_MANIFEST" + } download_and_verify() { local script_name="$1" - local expected_hash="$2" local output_path="/tmp/$script_name" + local expected_hash="" + if [ -f "$SCRIPT_MANIFEST" ]; then + expected_hash=$(jq -r --arg n "$script_name" '.[$n] // empty' "$SCRIPT_MANIFEST") + if [ -z "$expected_hash" ]; then + echo "❌ $script_name is not in the server's script manifest!" + echo " The hub serves no such script, or it failed to read on the server." + exit 1 + fi + fi + curl -fsSL "$SCRIPTS_BASE_URL/$script_name" \ -H "Authorization: Bearer $WEBHOOK_SECRET" \ -o "$output_path" local actual_hash=$(shasum -a 256 "$output_path" | cut -d' ' -f1) - if [ "$actual_hash" != "$expected_hash" ]; then + if [ -z "$expected_hash" ]; then + echo "::warning::$script_name downloaded UNVERIFIED (no manifest; hash: ${actual_hash:0:16}...)" + elif [ "$actual_hash" != "$expected_hash" ]; then echo "❌ HASH MISMATCH for $script_name!" echo " Expected: $expected_hash" echo " Actual: $actual_hash" - echo " This could indicate tampering or an outdated hash." + echo " The download was corrupted in transit — both values come from the same deployment." exit 1 fi @@ -493,11 +579,14 @@ jobs: chmod +x "$output_path" fi - echo "✅ $script_name verified (hash: ${actual_hash:0:16}...)" + if [ -n "$expected_hash" ]; then + echo "✅ $script_name verified (hash: ${actual_hash:0:16}...)" + fi } - download_and_verify "workflow-helpers.sh" "$HASH_WORKFLOW_HELPERS" - download_and_verify "code-review-report.sh" "$HASH_REPORT" + load_script_manifest + download_and_verify "workflow-helpers.sh" + download_and_verify "code-review-report.sh" # Early liveness ping — the FIRST real step after report capability is # secured. Stamps workflow_run_id + status 'running' on the hub's run row, @@ -566,32 +655,109 @@ jobs: - name: Download and verify scripts env: WEBHOOK_SECRET: ${{ secrets.DOC_ORCH_WEBHOOK_SECRET }} - HASH_FETCH_RULES: "d4608b1ec02d429ef958c6b65d174e80ec51c2073f26a99ea0bbf58a57eb8b43" - HASH_RUN: "e4ebfe017a09ed3f783c8aea995a079be46b4add3b3f92db1918291d1b3e7412" - HASH_MINE: "c12e6fd532bab44c95e855046efef3b45940fbde87d449d5577d198a0d3c4f62" - HASH_REVIEW: "edb42c89fb9199b99e0405c71ee78520a00bc17a928bb28978ae68734f764fde" - HASH_LIB: "6575dd2e94e2d81822a0415247ad243fb03b0029aea206bbf6519f08e3d54e1f" - HASH_POST: "9d25e0ca7396a306c1dca79325aa911bd157e8bdf5449c4305e628e5c2d76eb1" run: | set -euo pipefail - SCRIPTS_BASE_URL="${HUB_BASE_URL}/api/doc-orchestrator/scripts" + + SCRIPT_MANIFEST=/tmp/flamingo-script-manifest.json + # The canonical scripts surface and the pre-rename one. load_script_manifest + # picks whichever this deployment actually serves and pins SCRIPTS_BASE_URL. + CI_SCRIPTS_URL="${HUB_BASE_URL%/}/api/ci/scripts" + LEGACY_SCRIPTS_URL="${HUB_BASE_URL%/}/api/doc-orchestrator/scripts" + + # _try_manifest — 0 loaded, 1 no manifest surface there, 2 fatal. + _try_manifest() { + local base="$1" code + code=$(curl -sS -w '%{http_code}' -o "$SCRIPT_MANIFEST" \ + -H "Authorization: Bearer $WEBHOOK_SECRET" \ + "$base/manifest.json") || code="000" + + if [ "$code" = "404" ]; then rm -f "$SCRIPT_MANIFEST"; return 1; fi + if [ "$code" != "200" ]; then + echo "❌ manifest request to $base failed (HTTP $code)" + rm -f "$SCRIPT_MANIFEST" + return 2 + fi + # The digests are the TOP-LEVEL object. successResponse is the standard + # emitter but it does NOT add a wrapper — it is NextResponse.json(data) + # plus the no-store header — so there is no .data to reach through. + # A 200 that is not a manifest is how a hub which does not serve this path + # answers (the proxy rewrites unknown routes and returns HTML), so it + # means "wrong surface", not "corrupt". + if ! jq -e 'type == "object" and length > 0 and (to_entries | all(.value | type == "string"))' "$SCRIPT_MANIFEST" >/dev/null 2>&1; then + rm -f "$SCRIPT_MANIFEST" + return 1 + fi + return 0 + } + + load_script_manifest() { + # The scripts surface was renamed from /api/doc-orchestrator/scripts to the + # pipeline-neutral /api/ci/scripts (it always served BOTH pipelines). The + # workflow file ships in the repo and the routes ship with the deployment, + # so the two are one version apart in BOTH directions across the rollout. + # Probe the canonical surface, fall back to the legacy one, and let the + # winner decide SCRIPTS_BASE_URL for every download that follows. + # "cmd; rc=$?" dies under the set -euo pipefail these steps run with — + # errexit fires before rc is read and the step ends with NO output. And + # "if ! cmd; then rc=$?" is worse: inside the branch $? is the status of + # the NEGATION (0), so every failure reads as success. "|| rc=$?" is the + # one form that both suppresses errexit and preserves the real code. + local rc=0 + _try_manifest "$CI_SCRIPTS_URL" || rc=$? + if [ "$rc" = "0" ]; then + SCRIPTS_BASE_URL="$CI_SCRIPTS_URL" + echo "✅ script manifest loaded ($(jq -r 'length' "$SCRIPT_MANIFEST") scripts)" + return 0 + fi + if [ "$rc" = "2" ]; then exit 1; fi + + echo "::warning::this hub does not serve $CI_SCRIPTS_URL — falling back to the legacy $LEGACY_SCRIPTS_URL; it predates the rename" + SCRIPTS_BASE_URL="$LEGACY_SCRIPTS_URL" + + rc=0 + _try_manifest "$LEGACY_SCRIPTS_URL" || rc=$? + if [ "$rc" = "0" ]; then + echo "✅ script manifest loaded ($(jq -r 'length' "$SCRIPT_MANIFEST") scripts)" + return 0 + fi + if [ "$rc" = "2" ]; then exit 1; fi + + # Neither surface published a manifest: a hub older than the manifest + # itself. Downloads proceed UNVERIFIED and say so on every file. Any + # OTHER failure above already exited — only a genuine "no such endpoint" + # reaches here, so a hijacked or failing request can never land in this + # branch and silently disable verification. + echo "::warning::no manifest endpoint on this hub — it predates the manifest; downloads run UNVERIFIED until the hub is redeployed" + rm -f "$SCRIPT_MANIFEST" + } download_and_verify() { local script_name="$1" - local expected_hash="$2" local output_path="/tmp/$script_name" + local expected_hash="" + if [ -f "$SCRIPT_MANIFEST" ]; then + expected_hash=$(jq -r --arg n "$script_name" '.[$n] // empty' "$SCRIPT_MANIFEST") + if [ -z "$expected_hash" ]; then + echo "❌ $script_name is not in the server's script manifest!" + echo " The hub serves no such script, or it failed to read on the server." + exit 1 + fi + fi + curl -fsSL "$SCRIPTS_BASE_URL/$script_name" \ -H "Authorization: Bearer $WEBHOOK_SECRET" \ -o "$output_path" local actual_hash=$(shasum -a 256 "$output_path" | cut -d' ' -f1) - if [ "$actual_hash" != "$expected_hash" ]; then + if [ -z "$expected_hash" ]; then + echo "::warning::$script_name downloaded UNVERIFIED (no manifest; hash: ${actual_hash:0:16}...)" + elif [ "$actual_hash" != "$expected_hash" ]; then echo "❌ HASH MISMATCH for $script_name!" echo " Expected: $expected_hash" echo " Actual: $actual_hash" - echo " This could indicate tampering or an outdated hash." + echo " The download was corrupted in transit — both values come from the same deployment." exit 1 fi @@ -600,15 +766,23 @@ jobs: chmod +x "$output_path" fi - echo "✅ $script_name verified (hash: ${actual_hash:0:16}...)" + if [ -n "$expected_hash" ]; then + echo "✅ $script_name verified (hash: ${actual_hash:0:16}...)" + fi } - download_and_verify "code-review-fetch-rules.sh" "$HASH_FETCH_RULES" - download_and_verify "code-review-run.sh" "$HASH_RUN" - download_and_verify "code-review-mine.mjs" "$HASH_MINE" - download_and_verify "code-review-review.mjs" "$HASH_REVIEW" - download_and_verify "code-review-lib.mjs" "$HASH_LIB" - download_and_verify "code-review-post.mjs" "$HASH_POST" + # Digests come from the deployment that serves the bytes, never from + # this file: a pinned hash here would be the PR head ref's while the + # scripts are main's, which is exactly how a PR used to brick its own + # review. See workflow-scripts-bootstrap.ts. + load_script_manifest + + download_and_verify "code-review-fetch-rules.sh" + download_and_verify "code-review-run.sh" + download_and_verify "code-review-mine.mjs" + download_and_verify "code-review-review.mjs" + download_and_verify "code-review-lib.mjs" + download_and_verify "code-review-post.mjs" - name: Fetch the rule corpus id: rules @@ -627,6 +801,31 @@ jobs: # Stage checkpoints are their OWN credentialed steps — the review step # deliberately never holds the webhook secret or run token (it processes # untrusted PR content), so pings happen at step boundaries, not inside. + # Say so ON THE PULL REQUEST before spending minutes in the model. Placed + # AFTER the rules step on purpose: the skip decision is known here, so a run that + # reviews nothing never leaves a placeholder to strand. The comment + # carries the summary marker, so the final "Post to the pull request" + # step UPDATES this exact comment into the verdict rather than adding a + # second one, and it is re-created (not edited) each run so the live + # review sits at the BOTTOM of the timeline instead of scrolling away + # under later discussion. Best-effort: never fails the review. + - name: Say the review is running + if: steps.rules.outputs.skip != 'true' + continue-on-error: true + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number || needs.resolve_command.outputs.pr_number }} + # Feature-DETECTED, not assumed: across a deploy boundary this workflow + # can be handed a post.mjs that predates --running, which would ignore + # the flag and run the full post pass with no findings.json — posting a + # bogus verdict mid-review. Grep first; skip quietly if unsupported. + run: | + if grep -q -- '--running' /tmp/code-review-post.mjs; then + node /tmp/code-review-post.mjs --running + else + echo "the hub's post.mjs predates --running — skipping the placeholder" + fi + - name: Report progress — reviewing env: WEBHOOK_SECRET: ${{ secrets.DOC_ORCH_WEBHOOK_SECRET }} @@ -643,7 +842,19 @@ jobs: if: steps.rules.outputs.skip != 'true' # MODE / RUN_ID arrive via the generated workflow-level env; only the # secret is scoped to the step (doc-workflow security discipline). + # The reviewer calls Claude THROUGH THE HUB (/api/ci/claude, the + # unified caller), so the step carries the hub's workflow secret and + # the target repo needs no ANTHROPIC_API_KEY of its own. + # + # ANTHROPIC_API_KEY is still passed as a TRANSITION SHIM. The scripts + # are served by the deployed hub while this workflow file ships in the + # repo, so across a deploy boundary a NEW workflow runs OLD scripts — + # and the old ones call Anthropic directly. Withdrawing the key the + # same day the proxy landed would kill every review until the hub went + # live, including the review of the pull request that moved it. Drop + # this line once the fleet is on scripts that no longer read it. env: + WEBHOOK_SECRET: ${{ secrets.DOC_ORCH_WEBHOOK_SECRET }} ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} run: /tmp/code-review-run.sh