Repository navigation
feat(cloudflare): run the manual-OAuth catalogue re-snapshot as a workflow #1259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3d35bd0
d8909c4
8d184f8
f20cf20
c59e66e
a21de0c
a57f4c3
6fca453
fb02778
1331363
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| #!/usr/bin/env bash | ||
| # Step 4: prove the snapshot actually moved. | ||
| # | ||
| # An IDENTICAL catalogue is a FAILURE, not a pass. That is the outcome this | ||
| # whole procedure exists to prevent — `last_synced` moving while the stored | ||
| # tools do not is exactly what a flip that did nothing looks like — so it is | ||
| # the one result that must not read as success. | ||
| # | ||
| # The first version of this script compared the tool COUNT and printed a | ||
| # warning, on the reasoning that "the real check is the drift detector". That | ||
| # was wrong: the detector compares the snapshot's tool NAMES against each | ||
| # owning repository's allowlist, so it is blind to a tool whose schema or | ||
| # annotations changed under an unchanged name — and blind to nothing having | ||
| # changed at all whenever the allowlist already agrees with the old snapshot. | ||
| # A warning is also not a verdict, and the pull request describing this | ||
| # workflow claimed a failure. Two answers to one question, which is the defect | ||
| # this repository keeps finding. | ||
| # | ||
| # WHOLE tool objects, because annotations travel in the snapshot and | ||
| # `readOnlyHint` is a structural claim this platform acts on: a projection down | ||
| # to names and schemas would call a changed annotation "no change". | ||
| set -euo pipefail | ||
| # shellcheck source=.github/scripts/portal-resnapshot-lib.sh | ||
| . "$(dirname "$0")/portal-resnapshot-lib.sh" | ||
| assert_server_id | ||
| : "${BEFORE_JSON:?the pre-flip catalogue from the flip job}" | ||
| if [ ! -s "$BEFORE_JSON" ]; then | ||
| # The artifact is missing, which its own upload step tolerates on purpose: | ||
| # failing that step would skip this job, and this job is the only bound on | ||
| # the outage. So the comparison is lost and the run is red for it -- but the | ||
| # WAIT above has already happened, which is the part that mattered. | ||
| echo "::error::the pre-flip catalogue is missing, so it cannot be proved the snapshot moved. The sign-in completed (the wait above passed); compare ${SERVER}'s tools against the previous run by hand." | ||
| exit 1 | ||
| fi | ||
|
|
||
| after=$(cf); printf '%s' "$after" | ok | ||
|
|
||
| # `tools` is null for a server nobody has authorized — the state this step is | ||
| # verifying its way out of. Unguarded, jq dies with "Cannot iterate over null"; | ||
| # worse, an empty ARRAY would diff as every tool removed and read as a release | ||
| # delta. | ||
| printf '%s' "$after" | jq -e '(.result.tools | type) == "array" and (.result.tools | length) > 0' >/dev/null \ | ||
| || { echo "::error::the portal holds no tools: the sign-in did not complete"; exit 1; } | ||
|
|
||
| work=$(mktemp -d); trap 'rm -rf "$work"' EXIT | ||
| printf '%s' "$after" | jq -S '[.result.tools[]] | sort_by(.name)' > "$work/after.json" | ||
|
|
||
| before_n=$(jq 'length' "$BEFORE_JSON") | ||
| after_n=$(jq 'length' "$work/after.json") | ||
| last=$(printf '%s' "$after" | jq -r '.result.last_synced') | ||
|
|
||
| if cmp -s "$BEFORE_JSON" "$work/after.json"; then | ||
| echo "::error::the catalogue is byte-identical to the pre-flip snapshot (${after_n} tools, last_synced=${last}). The snapshot did not move: whoever signed in was shown the same tools/list, or the sign-in landed on a different server." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Names only in the summary. The full objects carry schemas, which are long and | ||
| # are already in the owning repository's allowlist; what an operator wants here | ||
| # is which tools arrived and which left. | ||
| added=$(jq -r -n --slurpfile a "$BEFORE_JSON" --slurpfile b "$work/after.json" \ | ||
| '($b[0] | map(.name)) - ($a[0] | map(.name)) | join(", ")') | ||
| removed=$(jq -r -n --slurpfile a "$BEFORE_JSON" --slurpfile b "$work/after.json" \ | ||
| '($a[0] | map(.name)) - ($b[0] | map(.name)) | join(", ")') | ||
|
|
||
| { | ||
| echo "### Portal catalogue — \`${SERVER}\`" | ||
| echo | ||
| echo "\`${before_n}\` tools before, \`${after_n}\` after. \`last_synced=${last}\`" | ||
| echo | ||
| echo "| | |" | ||
| echo "| --- | --- |" | ||
| echo "| added | ${added:-_none_} |" | ||
| echo "| removed | ${removed:-_none_} |" | ||
| [ -z "$added" ] && [ -z "$removed" ] && echo && echo "No name changed; something in a tool's schema or annotations did." | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 — still worth an explicit On the run this workflow exists for — Whether I could not execute a shell in this sandbox to settle it, so I am leaving it at P3 rather than asserting either reading — but a line whose correctness depends on which of two adjacent bash carve-outs applies, inside the one script that decides whether a production outage was worth it, should not be left to the reader: if [ -z "$added" ] && [ -z "$removed" ]; then
echo
echo "No name changed; something in a tool's schema or annotations did."
fi
} >> "$GITHUB_STEP_SUMMARY"The rest of this script is right, and it is the cleanest of the closed findings: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 — still worth an explicit On the run this workflow exists for — Whether
if [ -z "$added" ] && [ -z "$removed" ]; then
echo
echo "No name changed; something in a tool's schema or annotations did."
fi
} >> "$GITHUB_STEP_SUMMARY"The rest of this script is right, and it is the cleanest of the closed findings: |
||
| } >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| echo "catalogue moved: ${before_n} -> ${after_n} tools, last_synced=${last}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| #!/usr/bin/env bash | ||
| # The end state, checked by the same detector that found the problem — but | ||
| # scoped in its VERDICT to the server this run touched. | ||
| # | ||
| # `portal-catalogue-drift.py` is account-wide and takes no server filter, so a | ||
| # successfully re-snapshotted `api` would go red because `seerrsense` is stale | ||
| # for reasons this run never touched. Worse than noise: the alert names one | ||
| # server, so the operator reads "the re-snapshot failed" about a server whose | ||
| # re-snapshot worked. | ||
| # | ||
| # Account-wide drift is `cloudflare-drift.yml`'s job and it runs nightly. This | ||
| # gate owns one question — did THIS server end up consistent — and says the | ||
| # rest out loud rather than either failing on it or hiding it. | ||
| set -euo pipefail | ||
| # shellcheck source=.github/scripts/portal-resnapshot-lib.sh | ||
| . "$(dirname "$0")/portal-resnapshot-lib.sh" | ||
| assert_server_id | ||
|
|
||
| # PYTHONUNBUFFERED, because the evidence check reads stdout out of a stream | ||
| # merged with stderr. Python block-buffers stdout when it is not a tty and | ||
| # line-buffers stderr, so without this the `compared: ...` line can be | ||
| # interleaved with a stderr write and lose its line start -- and the anchored | ||
| # `^compared: ` then fails to match. That used to cost a spurious green; since | ||
| # the gate requires positive evidence it costs the opposite, failing a run | ||
| # where the sign-in completed and the catalogue moved. Either way the buffering | ||
| # decides the verdict, which is not something a verdict may depend on. | ||
| out=$(PYTHONUNBUFFERED=1 python3 scripts/portal-catalogue-drift.py 2>&1) && code=0 || code=$? | ||
| printf '%s\n' "$out" | ||
|
|
||
| # POSITIVE EVIDENCE FIRST, and it is what makes every branch below safe. | ||
| # | ||
| # The detector prints `compared: api=76@<last_synced>, ...` for every outcome | ||
| # that has one, clean or red. Requiring that line means this gate can only | ||
| # reach a verdict about a comparison that HAPPENED. | ||
| # | ||
| # The alternative was a list of the detector's known failure shapes -- its four | ||
| # `[2] ` bail-outs -- with everything else falling through to "consistent". | ||
| # That is a list to keep in step with another file, and it answers green for | ||
| # every shape not on it: an uncaught exception, a SyntaxError, a missing | ||
| # module, an OOM kill. The final gate of a procedure that just took a | ||
| # production server down must not report the end state good having compared | ||
| # nothing, and only positive evidence rules that out. | ||
| # THESE BRANCHES MUST EXIT NON-ZERO, and `${code:-1}` was not that. `:-` | ||
| # substitutes only on | ||
| # unset-or-empty, and on a clean detector run `code` is the STRING "0" -- so | ||
| # both branches below printed their `::error::` and exited 0, passing the step | ||
| # and reporting the end state good having compared nothing. Verbatim the | ||
| # outcome the paragraph above says positive evidence was chosen to rule out, | ||
| # and unreachable today only because of how the detector happens to order its | ||
| # own exit codes: the same coupling, moved from a grep into an exit status. | ||
| fail() { | ||
| echo "::error::$1" | ||
| [ "${code:-0}" -ne 0 ] && exit "$code" | ||
| exit 1 | ||
| } | ||
|
|
||
| compared=$(printf '%s\n' "$out" | grep -E '^compared: ' || true) | ||
| if [ -z "$compared" ]; then | ||
| fail "the catalogue detector produced no \"compared:\" line (exit ${code}): it never compared ${SERVER}, whatever else it printed. Nothing here has been verified." | ||
| fi | ||
| printf '%s\n' "$compared" | grep -qE "(^|[ ,])${SERVER}=" \ | ||
| || fail "the detector compared servers but not ${SERVER}: ${compared}" | ||
|
|
||
| if [ "$code" -eq 0 ]; then | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 — this branch is unreachable, and the gate's weakest branch is therefore its normal outcome.
if not token:
raise Undetermined(
f"{server}: {repo} is private and ${TOKEN_ENV} is not set; "
"the allowlist cannot be read")
The run then falls through to line 103, which is wrong in both halves of what it tells the operator:
The verdict happens to be right — exit 0 is correct for
|
||
| echo "catalogue drift is clean account-wide" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # GLOBAL BAIL-OUTS FIRST. The detector exits 2 before its per-server loop for | ||
| # a missing token or a portal it cannot read, and prints no `{server}:` line at | ||
| # all -- so a per-server grep finds nothing and the "red for somebody else" | ||
| # branch below would report green on a check that made no comparison. That is | ||
| # the final gate of this workflow saying the end state is good having measured | ||
| # nothing. | ||
| # | ||
| # The detector marks them itself: every global bail-out starts a line with | ||
| # `[2] `, and no per-server message does. | ||
| if printf '%s\n' "$out" | grep -qE '^\[2\] '; then | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 — this block is now unreachable, and the comment above it (lines 22-34) is the argument for deleting it rather than keeping it. Every Keeping both is not harmful, but the new comment says the point of positive evidence is that it is not "a list to keep in step with another file" — and this is that list, still here, still needing to stay in step. If it is retained as belt-and-braces, that is worth one line saying so; otherwise the reachable code is the compared-line check alone. Two smaller things in the same file while it is open:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3 — carried from the Unreachable. Every
One line, before line 27: python3 scripts/portal-catalogue-drift.py --selftest \
|| { echo "::error::the catalogue drift detector failed its own selftest; it was not run against Cloudflare and nothing here is verified"; exit 1; }Also still open from earlier reviews, stated once and not re-argued: |
||
| echo "::error::the catalogue detector could not run (exit ${code}): it never compared ${SERVER}. Fix the detector's own inputs and re-run; nothing here has been verified." | ||
| exit "$code" | ||
| fi | ||
|
|
||
| # The detector prefixes each finding with the server id it is about | ||
| # (`api: mctl_get_lifecycle_ownership is upstream but not ...`). Anchored, so a | ||
| # server id appearing inside another server's message does not match. | ||
| mine=$(printf '%s\n' "$out" | grep -E "^[[:space:]]*${SERVER}:" || true) | ||
| if [ -n "$mine" ]; then | ||
| # THE LINES, not the exit code. `$code` is a MAXIMUM across the account | ||
| # (portal-catalogue-drift.py orders `vanished > failing > undetermined > | ||
| # maintenance` and returns the loudest), so mapping it to a remedy for THIS | ||
| # server inverts exactly what the per-server grep above exists to prevent: a | ||
| # vanished `seerrsense` beside a stale `api` would print "api is missing from | ||
| # the portal entirely" about a server that is present. | ||
| # | ||
| # What is true per server is the finding text, so that is what is quoted and | ||
| # the remedy is left to it. | ||
| echo "::error::catalogue drift names ${SERVER} (account-wide exit ${code}); its own findings follow. Only a line about a MISSING tool means re-snapshot or allowlist-apply; a waiver line is a chore cloudflare-drift.yml separates precisely so nobody is sent through this recipe for it." | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 — this branch fails the run on the one exit code a successful re-snapshot guarantees. The detector gained a fifth category in main, and it is the category "the catalogue moved" produces.
So a run that does exactly what this workflow exists to do — the snapshot moves and a new tool appears — makes the detector exit 5, The README on this branch states that outcome as the expected one (
and Two things follow:
A |
||
| printf '%s\n' "$mine" | ||
| exit "$code" | ||
| fi | ||
|
|
||
| # Red for somebody else. Surfaced, not swallowed -- and not this run's verdict. | ||
| echo "::warning::catalogue drift exit ${code}, but no finding names ${SERVER}: this re-snapshot is consistent and another server is stale. cloudflare-drift.yml owns that." | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 — this branch also catches the detector failing to run at all, and calls it "this re-snapshot is consistent". Narrowing the verdict to this server is the right call, and the anchored grep matches the finding format correctly ( Each returns 2 before the per-server loop is ever entered, so
…and exits 0. The final gate reports green on a check that made no comparison, and the claim in its own warning ("another server is stale") is about a server the run never read either. A transient That is the one thing this file's own header rules out: "says the rest out loud rather than either failing on it or hiding it". Exiting 0 on an unrun detector hides it. The detector marks these itself — every global bail-out is prefixed if printf '%s\n' "$out" | grep -qE '^\[2\] '; then
echo "::error::the catalogue detector did not run (exit ${code}); this run proved the sign-in and the catalogue move, but not the end state"
exit "$code"
fiWorth pairing with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 — the The new guard keys on a marker the detector prints. That covers
…and exits 0. The final gate of a procedure that just took a production server down reports the end state good having compared nothing. Reachable without inventing much:
The asymmetry is the problem, not any single trigger: if checked:
print(f"compared: {', '.join(checked)}") # compared: api=76@2026-09-13T…, tg=…so: if ! printf '%s\n' "$out" | grep -qE "^compared: .*\b${SERVER}="; then
echo "::error::catalogue drift exit ${code} and the detector never reports comparing ${SERVER}; nothing here has been verified."
exit "$code"
fi
echo "::warning::catalogue drift exit ${code}, but no finding names ${SERVER}: this re-snapshot is consistent and another server is stale. cloudflare-drift.yml owns that."
exit 0That subsumes the |
||
| exit 0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| #!/usr/bin/env bash | ||
| # Steps 0, 0a, 0b, 1, 2 and 2a of the README recipe, in one job. | ||
| # | ||
| # ONE job on purpose. From step 1 the live server no longer holds the | ||
| # registration, so every command between the flip and the read-back that | ||
| # confirms the restoration has to run without a job boundary in it — a boundary | ||
| # is a place the run can stop with the server in a state nothing records. | ||
| set -euo pipefail | ||
| # shellcheck source=.github/scripts/portal-resnapshot-lib.sh | ||
| . "$(dirname "$0")/portal-resnapshot-lib.sh" | ||
| assert_server_id | ||
| : "${VAULT_ADDR:?}" ; : "${VAULT_TOKEN:?}" ; : "${VAULT_BACKUP_PATH:?}" | ||
|
|
||
| work=$(mktemp -d) | ||
|
|
||
| # WHETHER A MUTATION HAPPENED, recorded on the way rather than inferred after. | ||
| # | ||
| # A cancelled run — the operator hitting the button, a runner evicted — takes | ||
| # the script mid-procedure, and "the job was cancelled" says nothing about | ||
| # which side of the flip it was on. The two need opposite responses: before it, | ||
| # nothing was touched; after it, the server has no registration and the Vault | ||
| # copy is the only way back. So the marker is written BEFORE the destructive | ||
| # PUT and survives into the trap, which reports it on every exit path | ||
| # including the signals a cancellation actually arrives as. | ||
| # ON DISK, not only in a shell variable. A cancelled job kills the step, and | ||
| # the workflow's own announce step -- which is what actually reaches a human -- | ||
| # can only see `steps.flip.outcome`, which for a cancellation is `cancelled` | ||
| # whichever side of the destructive PUT it landed on. Reading the outcome | ||
| # alone, the announce told an operator "nothing was written" for a run that had | ||
| # already cleared the registration. | ||
| # | ||
| # A file survives the step and is readable by every later `if: always()` step, | ||
| # so the marker the shell keeps and the marker the workflow reports are the | ||
| # same fact. | ||
| MARKERS="${RUNNER_TEMP:-/tmp}/portal-resnapshot" | ||
| mkdir -p "$MARKERS" | ||
| rm -f "$MARKERS/flipped" "$MARKERS/restored" | ||
|
|
||
| flipped=no | ||
| on_exit() { | ||
| rc=$? | ||
| if [ "$flipped" = yes ] && [ "$restored" != yes ]; then | ||
| echo "::error::the run ended (rc=${rc}) between the flip and the restore. ${SERVER} has NO registration; restore from Vault at ${VAULT_BACKUP_PATH}." | ||
| elif [ "$flipped" = no ]; then | ||
| echo "nothing was written: the run ended before the flip" | ||
| fi | ||
| rm -rf "$work" | ||
| } | ||
| restored=no | ||
| trap on_exit EXIT | ||
| trap 'exit 130' INT TERM HUP | ||
|
|
||
| # 0. back up the registration, and refuse to go on without one. | ||
| summary=$(cf) | ||
| printf '%s' "$summary" | ok | ||
| printf '%s' "$summary" | jq -e '.result.auth_config_summary != null' >/dev/null | ||
| (umask 077; printf '%s' "$summary" | jq '.result.auth_config_summary' > "$work/summary.json") | ||
| test -s "$work/summary.json" | ||
|
|
||
| # 0a. and the catalogue, as WHOLE tool objects. Annotations travel in the | ||
| # snapshot and readOnlyHint is a structural claim this platform acts on, so | ||
| # a projection down to names and schemas would call a changed annotation | ||
| # "no change". | ||
| printf '%s' "$summary" | jq -e '(.result.tools | type) == "array" and (.result.tools | length) > 0' >/dev/null | ||
| printf '%s' "$summary" | jq -S '[.result.tools[]] | sort_by(.name)' > "$work/before.json" | ||
|
|
||
| # 0b. build the ENTIRE restoration body now, while the server still works, and | ||
| # put it somewhere that outlives this runner. A shell that dies after step | ||
| # 1 otherwise takes the only copy of the registration with it, and for | ||
| # `api` and `seerrsense` nothing else records it. | ||
| CREDS=$(jq -ce '{auth_mode, config, registration_info}' "$work/summary.json") | ||
| SECRET=$(openssl rand -hex 24); test -n "$SECRET" | ||
| (umask 077; CREDS="$CREDS" SECRET="$SECRET" \ | ||
| jq -n '{auth_type:"oauth", auth_credentials:env.CREDS, client_secret:env.SECRET}' \ | ||
| > "$work/restore.json") | ||
| test -s "$work/restore.json" | ||
| echo "::add-mask::${SECRET}" | ||
|
|
||
| # Through STDIN: the restoration body carries the whole registration and a | ||
| # live client_secret, and a command line is world-readable in /proc. | ||
| jq -n --slurpfile r "$work/restore.json" --arg run "${RUN_ID}" \ | ||
| '{data: {restore: ($r[0] | tojson), run_id: $run}}' \ | ||
| | curl -sS --fail-with-body -H "X-Vault-Token: ${VAULT_TOKEN}" \ | ||
| -H 'content-type: application/json' \ | ||
| -X POST "${VAULT_ADDR}/v1/secret/data/${VAULT_BACKUP_PATH}" --data-binary @- >/dev/null | ||
| # Read back, because a write this one depends on must not be believed on a 2xx. | ||
| curl -sS --fail-with-body -H "X-Vault-Token: ${VAULT_TOKEN}" \ | ||
| "${VAULT_ADDR}/v1/secret/data/${VAULT_BACKUP_PATH}" \ | ||
| | jq -e '.data.data.restore | fromjson | .auth_credentials != null' >/dev/null | ||
| echo "registration backed up to ${VAULT_BACKUP_PATH}" | ||
|
|
||
| # 1. flip to bearer, then READ BACK that the registration is really gone. | ||
| # Envelope FIRST: on success:false `.result` is null and | ||
| # `.result.auth_config_summary == null` is then true — a failed read | ||
| # reporting the flip as done. | ||
| echo "::warning::${SERVER} is now going offline until someone signs it back in" | ||
| flipped=yes | ||
| : > "$MARKERS/flipped" | ||
| cf -X PUT --json '{"auth_type":"bearer","auth_credentials":"resnapshot-not-a-token"}' | ok | ||
| gone=$(cf); printf '%s' "$gone" | ok | ||
| printf '%s' "$gone" | jq -e '.result.auth_config_summary == null' >/dev/null | ||
|
|
||
| # 2. restore, and read back that manual OAuth is in place and the server is | ||
| # waiting for its first authorization. | ||
| cf -X PUT --json "@$work/restore.json" | ok | ||
|
|
||
| cf | jq -e '.result.status == "waiting" and .result.auth_config_summary.auth_mode == "manual"' >/dev/null | ||
| restored=yes | ||
| : > "$MARKERS/restored" | ||
|
|
||
| # 2a. and that it is the SAME registration, field for field. status+auth_mode | ||
| # only say a manual-OAuth registration exists; this API is on record | ||
| # answering 200 while keeping a field it was told to change, and for `api` | ||
| # and `seerrsense` nothing declarative would catch a narrowed scope or a | ||
| # moved endpoint later. | ||
| # Compared by FIELD NAME, never by printing the values. A `diff -u` of the | ||
| # two blobs puts endpoints, client id and scope into the run log — readable | ||
| # by everyone with repository access, and outliving the run. The whole | ||
| # reason the backup goes to Vault rather than an artifact is that this blob | ||
| # must not land somewhere durable and broadly readable, and a log is both. | ||
| # | ||
| # A null `auth_config_summary` reports all three as changed rather than | ||
| # comparing as equal-to-nothing, which is the shape a failed read takes. | ||
| (umask 077; cf | jq -S '.result.auth_config_summary' > "$work/restored.json") | ||
| changed=$(jq -r -n --slurpfile a "$work/summary.json" --slurpfile b "$work/restored.json" ' | ||
| ($a[0] | {auth_mode, config, registration_info}) as $before | ||
| | ($b[0] // {}) as $after | ||
| | ["auth_mode", "config", "registration_info"] | ||
| | map(select(($before[.] // null) != ($after[.] // null))) | ||
| | join(", ")') | ||
| if [ -n "$changed" ]; then | ||
| echo "::error::registration changed across the flip in: ${changed}. Restore from Vault at ${VAULT_BACKUP_PATH} by hand — the values are deliberately not printed here." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # The catalogue as it was, for `verify` to prove the snapshot actually moved. | ||
| # An artifact, unlike the registration: this is tool names, schemas and | ||
| # annotations, which already live in each owning repository's | ||
| # docs/portal-allowlist.json. The registration does not. | ||
| mkdir -p "${RUNNER_TEMP:-/tmp}/portal-resnapshot" | ||
| cp "$work/before.json" "${RUNNER_TEMP:-/tmp}/portal-resnapshot/before.json" | ||
|
|
||
| # The baseline `verify` waits to move, and it has to be taken HERE. | ||
| # | ||
| # The bearer flip moves `last_synced` itself — the README records that as the | ||
| # sign it worked — so a baseline read before the flip is already stale by the | ||
| # time the restore lands, and `verify` comparing against it would find the | ||
| # value "moved" the instant the job starts. The only value that moving proves a | ||
| # SIGN-IN is the one the server holds after the restoration. | ||
| restored_synced=$(cf | jq -r '.result.last_synced // ""') | ||
| echo "last_synced_after_restore=${restored_synced}" >> "$GITHUB_OUTPUT" | ||
|
|
||
| echo "restored: waiting for a user sign-in (baseline last_synced=${restored_synced:-none})" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # shellcheck shell=bash | ||
| # Shared by the portal-resnapshot steps. Sourced, never executed. | ||
| # | ||
| # Two things here are load-bearing, both measured rather than assumed: | ||
| # | ||
| # `ok` — `--fail-with-body` catches HTTP errors, and this API answers 200 | ||
| # with `success: false`, and has been measured answering 200 while silently | ||
| # keeping a field it was told to change. The ENVELOPE is the truth, and after | ||
| # every write the server is read back rather than trusted to have changed. | ||
| # | ||
| # the token on a file descriptor — a command line is world-readable in | ||
| # /proc. The runner is single-tenant, but this file is also what an operator | ||
| # copies when running the procedure by hand, and the habit is the point. | ||
|
|
||
| # shellcheck disable=SC2120 # called with no args on read paths, with -X PUT on writes | ||
| # `--max-time`, because every caller is inside a window where the server is | ||
| # down. curl's default is no total timeout at all: a connection that hangs | ||
| # holds the step until the job's own limit, and the job that would be holding | ||
| # is the one bounding the outage. | ||
| cf() { | ||
| curl -sS --fail-with-body --max-time 45 --connect-timeout 10 \ | ||
| -K <(printf 'header = "Authorization: Bearer %s"\n' "$CF_TOKEN") \ | ||
| "https://api.cloudflare.com/client/v4/accounts/${CLOUDFLARE_ACCOUNT_ID}/access/ai-controls/mcp/servers/${SERVER}" "$@" | ||
| } | ||
|
|
||
| ok() { jq -e '.success' >/dev/null; } | ||
|
|
||
| # The server id is an input, so it is shape-checked before it reaches a URL. | ||
| # `workflow_dispatch` type:choice constrains the UI, not the API: a dispatch | ||
| # through `gh api` can send anything. | ||
| assert_server_id() { | ||
| case "$SERVER" in | ||
| api|tg|seerrsense) ;; | ||
| *) echo "::error::unknown portal server: ${SERVER}"; exit 1 ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3 — worth making this an explicit
if, because whether it is a bug depends on aset -ecarve-out that is easy to read either way.The
&&-list is the last command in the brace group, so on the run this workflow exists for —addednon-empty, which is the stated first case (mctl_get_lifecycle_ownership) —[ -z "$added" ]returns 1 and the group returns 1.Whether
set -ethen fires hinges on bash's rule that a compound command which returns non-zero because a command failed while-ewas being ignored does not cause an exit;[ -z "$added" ]is exempt as a non-final element of a&&list. On that reading the script is fine and line 70 sets the step's exit status to 0. Thed8909c4creview read it the other way and called it a P2. I could not execute a shell in this sandbox to settle it, which is itself the argument for not leaving it to the reader:The rest of this script is right, and it is the finding from the last review that is most clearly closed:
cmp -sagainst the artifact on line 45 is a real byte comparison of whole sorted tool objects, identical exits 1, and thejq -S | sort_by(.name)filters on both sides (flip.sh:28and line 39 here) are identical, so key and element ordering cannot produce a spurious difference.