Repository navigation
Conversation
…kflow Closes the runnable half of #1258. The runbook existed (#1241) and the detector existed (#1242), and between them a green detector and a red one both ended at "an operator reads a README". The shape is decided by one constraint, quoted from the Cloudflare docs: a manual-OAuth server "remain[s] in Waiting status until a user completes upstream OAuth", and no API completes it. So the middle of the procedure is a person in a browser, and between the flip and that sign-in the server is DOWN for every portal user. That is why the obvious design — one job, one approval at the top — would be worse than the runbook it replaces. The runbook has an operator at a terminal with the restoration body already built, so the window is minutes; a CI job that flips and then ends leaves the server in Waiting until somebody notices, and nothing bounds it. So: approval before the destructive half, and a bounded MEASUREMENT after it rather than a second approval. #1258 proposed a second gate meaning "I have signed in"; `status: ready` with a moved `last_synced` is the same claim, measured instead of asserted, and it fails on a 30-minute timeout with a telegram alert — which an approval nobody clicks does not. `last_synced` as well as `status`, because it is the only field this API moves on a real re-authorization. Three jobs. `preflight` runs unprivileged and prints the state, so the approval is given against something visible rather than an intention — the same split cloudflare-apply.yml makes between plan and apply. `flip` takes the approval and does steps 0 through 2a without a job boundary in them: from step 1 the live server no longer holds the registration, and a boundary is a place the run can stop with the server in a state nothing records. `verify` waits, diffs and runs the detector. Load-bearing details carried over from the recipe rather than reinvented: every response is checked on its envelope, because this API answers 200 with `success: false` and has been measured answering 200 while silently keeping a field it was told to change; the catalogue is compared as WHOLE tool objects, because annotations travel in the snapshot and `readOnlyHint` is a structural claim this platform acts on; and an IDENTICAL catalogue is a failure, not a pass, since that is the outcome the whole procedure exists to prevent. The registration goes to Vault before the flip, and the run proves that path is writable first — a workflow that assumed the policy exists would discover otherwise immediately after the flip, the one moment there is no way back. Not a workflow artifact: that publishes endpoints, client id and scope into the repository's artifact store, where it outlives the run. Two things this deliberately does NOT do: - It does not apply the tool allowlists. Those live with the servers that register the tools, because "this tool is safe to expose" belongs in the same diff as the tool — the rule this directory's README already states. A new tool arriving still needs its owning repository's `scripts/portal-allowlist-apply.sh`, and the final drift check is what says so rather than this workflow guessing. - It does not roll back. After step 1 the registration is gone and "restore" IS step 2, which lands in Waiting, not Ready. There is no path back to Ready without a human sign-in, and the docs say so rather than leaving the first failed run to discover it. Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @mashkovd's task in 5m 23s —— View job Review posted — changes requested
1 P1, 4 P2, 3 P3. Full review is on the Files changed tab. The shape is right, and the reasoning in the header comments is the good kind — preflight before the approval, no job boundary inside the destructive window, a Vault canary that fails closed rather than asserting a policy, a measurement instead of a second approval. The findings are almost all about places where the code does not do what the comment above it says it does. P1The final gate can never pass. P2Three of the four converge on one failure mode: the server flipped, and nothing bounding how long it stays down — the exact thing the file header argues this design exists to prevent.
P3
Checked and clear
On the design departure from #1258: measuring
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - name: Catalogue drift must be green | ||
| run: | | ||
| set -euo pipefail | ||
| python3 scripts/portal-catalogue-drift.py || { |
There was a problem hiding this comment.
P1 — this step can never pass: portal-catalogue-drift.py reads CLOUDFLARE_API_TOKEN, and the verify job only sets CF_TOKEN.
scripts/portal-catalogue-drift.py:809:
token = os.environ.get("CLOUDFLARE_API_TOKEN")
if not token:
print("[2] CLOUDFLARE_API_TOKEN is not set", file=sys.stderr)
return 2verify sets only CF_TOKEN: ${{ secrets.CF_PORTAL_READ_TOKEN }} (line 160). Every other call site wires the name the script actually reads — cloudflare-drift.yml:133 and cloudflare-plan.yml:371 both set CLOUDFLARE_API_TOKEN from CF_PORTAL_READ_TOKEN. So this exits 2 before it makes a single API call, and the error handler prints catalogue drift exit 2; if new tools arrived, run scripts/portal-allowlist-apply.sh ... — a message that sends the operator to the wrong remedy.
The consequence is not cosmetic. This is the step the PR body calls "the final drift check is what says so rather than this workflow guessing", and it is the only thing in verify that looks at the allowlist. Every run of this workflow ends red here, after the server has been taken down and signed back in, with a message pointing at allowlists. The first real run reads as "the re-snapshot failed".
env:
CF_TOKEN: ${{ secrets.CF_PORTAL_READ_TOKEN }}
CLOUDFLARE_API_TOKEN: ${{ secrets.CF_PORTAL_READ_TOKEN }}
| <b>Portal re-snapshot</b> — <code>${{ inputs.server }}</code> | ||
| ${{ job.status == 'success' | ||
| && 'Server is in <b>Waiting</b> and unusable until someone signs it out and back in on the portal server-selection page. Do it now — the verify job is already counting down.' | ||
| || 'Flip job ended <b>' }}${{ job.status == 'success' && '' || format('{0}</b>. Check whether the registration was restored before touching anything else.', job.status) }} |
There was a problem hiding this comment.
P2 — the && '' || idiom does not do what it looks like: '' is falsy in GitHub expressions, so the second interpolation always falls through to format(...).
On a successful flip:
- first expression →
true && 'Server is in <b>Waiting</b>…'→ the Waiting text. Correct. - second expression →
true && ''→'', which is falsy, so||takes the right branch →format('{0}</b>. Check whether the registration was restored…', 'success').
The message an operator actually receives on the happy path is:
Server is in Waiting and unusable until someone signs it out and back in on the portal server-selection page. Do it now — the verify job is already counting down.success</b>. Check whether the registration was restored before touching anything else.
A dangling </b> and a sentence telling them to go check whether the restore worked — on the run where it did. This is the one notification in the whole design that carries the "go sign in now" instruction, and its trailing half contradicts its leading half.
The empty-string trick has no safe spelling here. Build the whole body in one expression instead:
${{ job.status == 'success'
&& 'Server is in <b>Waiting</b> and unusable until someone signs it out and back in on the portal server-selection page. Do it now — the verify job is already counting down.'
|| format('Flip job ended <b>{0}</b>. Check whether the registration was restored before touching anything else.', job.status) }}… its happy one Four scenarios traced, and two of them did not hold. **An identical catalogue passed.** The pull request said "an identical catalogue is a FAILURE, not a pass"; the script compared the tool COUNT and printed `::warning::`, justified by "the real check is the drift detector". That justification is false: the detector compares snapshot 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. And a warning is not a verdict. Two answers to one question — the defect this repository keeps finding. It is a byte comparison now, against the pre-flip catalogue published as an artifact from the flip job, and an identical one exits 1. An artifact, unlike the registration: tool names, schemas and annotations already live in each owning repository's `docs/portal-allowlist.json`. The summary names what arrived and what left rather than dumping schemas. **The `last_synced` baseline was taken before the flip, which the flip moves.** The bearer flip moves it itself — the README records that as the sign it worked — so the wait was comparing against a value that had already changed, finding it "moved" the instant the job started. The only value whose movement proves a SIGN-IN is the one the server holds after the restoration, so the flip job publishes that and `verify` waits on it. The preflight job now publishes no outputs at all: everything it reads is stale by the time anything compares against it, and it exists to be read before the approval. Two leaks closed on the way: - Step 2a compared the registrations with `diff -u`, which puts endpoints, client id and scope into the run log — readable by everyone with repository access and outliving the run. That is the exact property the Vault backup exists to avoid, and a log is durable and broadly readable too. It compares by FIELD NAME now and says which field differs, never the value. 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. - The Vault login body carried the OIDC assertion on the command line, and the backup write carried the whole registration plus a live `client_secret`. Both go through stdin now — a command line is world-readable in /proc, which is the habit the `cf` helper already follows for the API token. And the flip alert told an operator the wrong thing. It had two outcomes where there are three: a failure BEFORE the flip step (the Vault canary refused) touched nothing, a failure INSIDE it may have left the registration cleared, and collapsing them into "the job failed" sends someone to look for damage that does not exist — or not to look for damage that does. `steps.flip.outcome` separates them, and the dangerous branch names the Vault path. The four scenarios now end: canary refusal → red, nothing written, alert says so; restore answering 200/success:false → red, alert names the backup; no sign-in in 30 minutes → red with the server named as unusable; ready but the catalogue unmoved → red on the byte comparison. None of the four prints the registration. Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
|
Audited against the four failure scenarios rather than the happy path, as asked. Two of them did not hold, and one was a contradiction between this PR's own description and the script. "ready appeared, but the catalogue did not change" was a The Two leaks closed, both under the "no registration blob" requirement:
The flip alert had two outcomes where there are three. A failure before the flip step (canary refused) touched nothing; a failure inside it may have left the registration cleared. "The job failed" sends an operator to look for damage that does not exist, or not to look for damage that does. Where the four land now:
None of the four prints the registration. On the lifecycle side — agreed, and that is why #384 has not been merged despite three APPROVED verdicts: each time the unfiltered comment check found something the verdict summary did not carry. Clean review first, then the production dry run, then the line-by-line report against the store and Temporal, and only then #385. |
|
Claude finished @mashkovd's task in 6m 59s —— View job Review —
|
| echo "\`${TOOLS_BEFORE}\` tools before, \`${count}\` after." | ||
| } >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| if [ "$count" -eq "$TOOLS_BEFORE" ]; then |
There was a problem hiding this comment.
P2 — this script does not do what its own header, the workflow comment above it, and the PR description all say it does. Two claims, neither implemented:
"An IDENTICAL catalogue is a FAILURE, not a pass." Lines 29–36 make it a ::warning:: and the script exits 0. The inline comment at line 31 concedes this ("so this is a warning here") and defers to the drift detector — but the header at lines 4–6 says "it is the one outcome that must not read as success", and portal-resnapshot.yml:177 repeats it, as does the PR body. Given the P1 on the drift step, the deferred-to check does not currently run at all; and even once it does, its own docstring says it detects "names and closedness", not schema content — so a re-snapshot dispatched for mctl-telegram#637-style schema drift that silently did not move ends the run green with one warning line.
"Whole tool objects, because annotations travel in the snapshot." Nothing here compares tool objects. TOOLS_BEFORE is an integer — preflight.sh:16 publishes jq '(.result.tools // []) | length' — so line 29 compares two counts. A changed readOnlyHint, a tool replaced one-for-one, a rewritten inputSchema: all identical counts, all "no change". That is precisely the projection the comment says it is avoiding, only coarser.
The pre-image needed to do it properly is computed, and then thrown away: flip.sh:28 writes $work/before.json as sorted whole tool objects into a mktemp -d with trap 'rm -rf "$work"' EXIT, and nothing ever reads it. Dead code today, but it is also the fix — that array has to cross the job boundary for this check to mean what it says.
Two ways out, and either is fine as long as the comments match the code:
- Carry it across. Put the sorted
toolsarray in the same Vault path the restore body already goes to — it is already proven writable and already outlives the runner — read it back here, anddiffit. Then "identical catalogue is a failure" is a real comparison and can be a real failure. - Or keep the count check, and rewrite lines 3–6 and
portal-resnapshot.yml:177-181to say "the count moved", and drop thebefore.jsoncomputation fromflip.sh.
What should not ship is the third state: a comment block asserting a structural guarantee over a count comparison that cannot deliver it.
| . "$(dirname "$0")/portal-resnapshot-lib.sh" | ||
| assert_server_id | ||
|
|
||
| : "${LAST_SYNCED_BEFORE:?}" |
There was a problem hiding this comment.
P2 — ${LAST_SYNCED_BEFORE:?} fires on an empty value as well as an unset one, and preflight.sh is happy to publish an empty one. The result is that the bounded wait — the thing that replaces #1258's second approval gate — dies instantly, and it dies after the flip.
The path:
preflight.sh:17—last=$(... | jq -r '.result.last_synced // ""'), so an absent or nulllast_syncedbecomes"".preflight.sh:36—echo "last_synced=${last}"writes an empty output. Nothing between it and here rejects that; the only gates in preflight areauth_mode == manualandtools > 0.portal-resnapshot.yml:174—LAST_SYNCED_BEFORE: ${{ needs.preflight.outputs.last_synced }}→ the empty string.- here —
:?treats null the same as unset →LAST_SYNCED_BEFORE: parameter null or not set, exit 1, first line of the job.
So: approval given, server flipped to bearer and back, now sitting in Waiting and unusable — and the job whose entire purpose is to bound that outage exits before its first poll. No countdown, no 30-minute timeout, no telegram (the failure() notify at line 204 does fire, but it fires immediately and says "the server may still be in Waiting", which is not the same as the alert an actual timeout would produce). The outage is then bounded by nothing, which is the exact failure mode the header comment of this file argues against.
A server with tools but no last_synced is not far-fetched: it is what the whole procedure is about — a snapshot taken once and never touched by the sync that would set that field.
Fix it in preflight.sh, before the approval, alongside the auth_mode and tools checks — that is where every other "this server is not a candidate" refusal already lives, and it is the only place where refusing costs nothing:
if [ -z "$last" ]; then
echo "::error::${SERVER} has no last_synced; the verify job has no baseline to measure the sign-in against"
exit 1
fi
|
|
||
| verify: | ||
| name: verify ${{ inputs.server }} | ||
| needs: [preflight, flip] |
There was a problem hiding this comment.
P2 — needs: [preflight, flip] with no condition means verify is skipped whenever the flip job's conclusion is anything but success, and the last step in flip is a network call to api.telegram.org that this workflow does not control.
flip ends with the Announce step at line 141, if: always(), no continue-on-error. If the flip itself succeeded but the telegram POST fails — a rotated TELEGRAM_BOT_TOKEN, a wrong chat id, a 5xx from Telegram, a runner network blip — the job concludes failure. verify is then skipped, and so is its failure() notify along with it (a skipped job runs no steps).
The state that leaves behind is the worst one this design has: the server is flipped, sitting in Waiting, unusable for every portal user — and both mechanisms meant to bound that are gone. No telegram (it just failed), and no bounded wait (the job that does it never starts). The outage ends when a human notices, which is the specific outcome the file header argues the runbook does better than a naive workflow.
The notification is an announcement, not a gate. It should not be able to veto the measurement:
- name: Announce the sign-in window
if: always()
continue-on-error: true
uses: appleboy/telegram-action@...Worth pairing with if: always() && needs.flip.result != 'skipped' on verify — after an approval has been granted and the flip step has run, "measure what state the server is actually in" is the right behaviour under almost any conclusion, including cancelled (which the concurrency block at line 53 already goes out of its way to make survivable).
| preflight: | ||
| name: preflight ${{ inputs.server }} | ||
| runs-on: ubuntu-latest | ||
| if: github.ref == 'refs/heads/main' |
There was a problem hiding this comment.
P3 — this guard makes a misdirected dispatch look like a clean run rather than a refusal.
workflow_dispatch from a non-main ref skips preflight; flip and verify then skip on needs, and a run where every job is skipped reports success in the Actions UI and to the API. Someone testing the workflow from this branch gets a green check and a portal server that was never touched — the same reading as a successful re-snapshot.
A step that fails with a reason is louder than a job-level if that vanishes. Either drop the guard (the cloudflare-apply environment can carry a deployment-branch rule, which is where this constraint belongs anyway) or make it explicit:
- name: Refuse a dispatch from a non-default ref
if: github.ref != 'refs/heads/main'
run: |
echo "::error::dispatch this from main; ${{ github.ref }} is not the reviewed copy of the scripts this run would execute"
exit 1The second form also states the actual reason, which is not obvious from the bare condition: the scripts are read from the checkout, so the ref decides what code runs against a production credential.
|
|
||
| elapsed=$(( $(date -u +%s) - started )) | ||
| if [ "$elapsed" -ge "$DEADLINE_S" ]; then | ||
| echo "::error::${SERVER} is still status=${status} after ${elapsed}s; it is UNUSABLE until someone signs it out and back in on the portal server-selection page" |
There was a problem hiding this comment.
P3 — the timeout message asserts one diagnosis for two very different states.
now=$(cf) || true at line 21 swallows every transport and envelope failure, and line 22's // "unreadable" turns it into a status string. An expired CF_PORTAL_READ_TOKEN, a Cloudflare 5xx, or a DNS failure all loop silently for 30 minutes and then print "${SERVER} is still status=unreadable after 1800s; it is UNUSABLE until someone signs it out and back in" — sending an operator to the portal UI to fix a server that may well be ready already.
Cheap to separate, since the value is already in hand:
if [ "$status" = "unreadable" ]; then
echo "::error::cannot read ${SERVER} after ${elapsed}s — this is a read failure, not a verdict on the server; check CF_PORTAL_READ_TOKEN and the API before touching the portal"
exit 1
fiWorth considering a consecutive-unreadable counter rather than the deadline: 90 unreadable polls in a row is a broken credential long before it is a slow human, and failing at poll 5 gives back 29 minutes of a live outage.
| # the detector is what says so rather than this file guessing. | ||
| - name: Catalogue drift must be green | ||
| run: | | ||
| set -euo pipefail |
There was a problem hiding this comment.
P3 — the other two call sites run --selftest before trusting this detector; this one does not.
validate-manifests.yml:291 and cloudflare-drift.yml:364 both gate on it, and cloudflare-drift.yml spells out why in its surrounding block: a detector that fails its own selftest has not been run against Cloudflare at all, and its exit code means nothing. Here the code goes straight into a case-less handler that reports any non-zero as "if new tools arrived, run scripts/portal-allowlist-apply.sh" — so a broken detector produces a confident, wrong remedy at the most expensive moment in the run.
scripts/portal-catalogue-drift.py --selftest
python3 scripts/portal-catalogue-drift.py || { ... }Also worth mirroring PYTHONUNBUFFERED=1 from cloudflare-drift.yml:380 — that comment records that the "known stale:" lines are printed first and get held to the end of the block without it, which is exactly the ordering an operator reading this step needs.
(Note the two spellings of the invocation across the repo: scripts/portal-catalogue-drift.py here vs "$GITHUB_WORKSPACE/scripts/..." there. Both work from the default working directory; just flagging the inconsistency.)
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P1, 4 P2, 3 P3.
The shape is right — preflight before the approval, no job boundary inside the destructive window, a Vault canary that fails closed, a measurement instead of a second approval. The findings are about the parts that do not match what the file says they do.
P1 — verify sets CF_TOKEN, but portal-catalogue-drift.py reads CLOUDFLARE_API_TOKEN. The final gate exits 2 before its first API call, on every run, after the outage has already happened.
P2 — (a) the && '' || Telegram ternary: '' is falsy in GH expressions, so the success message ends with a dangling </b> and "check whether the registration was restored"; (b) diff.sh compares tool counts, not whole tool objects, and treats an identical catalogue as a warning — three comment blocks and the PR body say otherwise, and the before.json that would make it true is computed in flip.sh:28 and deleted unread; (c) an empty last_synced passes preflight but kills wait.sh on ${LAST_SYNCED_BEFORE:?} — after the flip, removing the only bound on the outage; (d) the announce step can fail the flip job and skip verify entirely, same consequence.
(b), (c) and (d) all land in the same place: the server flipped and nothing bounding how long it stays down. That is the failure mode the header comment argues this design exists to prevent, so they are worth closing before the first run.
Verified unrelated: api/tg/seerrsense are real portal server ids (mcp-servers.tf:49); the random client_secret in flip.sh:35 is the README recipe, not a bug; script modes are 755 with lib.sh correctly 644.
Reviewed at 3d35bd0. The head has since moved to d8909c4 — findings not re-checked against it.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness, concurrency, credential-isolation, and recovery issues block safe approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a manually dispatched workflow for Cloudflare manual-OAuth catalogue re-snapshots, including Vault backup, approval, restoration, sign-in polling, and verification.
Changes:
- Added workflow orchestration and notifications.
- Added shared scripts for preflight, Vault access, flipping, waiting, and diffing.
- Documented operational usage and constraints.
File summaries
| File | Description |
|---|---|
infrastructure/cloudflare/portal/README.md |
Documents workflow usage and operational constraints. |
.github/workflows/portal-resnapshot.yml |
Orchestrates approvals, notifications, and verification. |
.github/scripts/portal-resnapshot-wait.sh |
Polls for OAuth completion. |
.github/scripts/portal-resnapshot-vault-canary.sh |
Validates Vault access. |
.github/scripts/portal-resnapshot-preflight.sh |
Validates initial server state. |
.github/scripts/portal-resnapshot-lib.sh |
Provides shared API helpers. |
.github/scripts/portal-resnapshot-flip.sh |
Handles backup, flipping, and restoration. |
.github/scripts/portal-resnapshot-diff.sh |
Verifies catalogue changes. |
Review details
Suppressed comments (2)
.github/scripts/portal-resnapshot-flip.sh:20
- The environment approval covers the preflight state, but this job never verifies that the live summary is still that approved state before the destructive PUTs. A long approval wait or a concurrent Terraform/manual write can change the registration or catalogue, and this script will silently back up and flip the new state. Carry a digest or whole-state comparison from preflight and refuse on mismatch, as
cloudflare-apply.ymldoes.
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")
.github/workflows/portal-resnapshot.yml:205
- The verifier's
CF_TOKENis job-level, so this third-party Telegram action inherits the Cloudflare read credential even though it only sends an alert. Isolate the notifier from the verification job or scope the Cloudflare token only to the polling and diff steps.
SERVER: ${{ inputs.server }}
- Files reviewed: 8/8 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| concurrency: | ||
| # Per server, and never cancelled. A run cancelled between the flip and the | ||
| # restore leaves the server with no registration at all — the one state this | ||
| # procedure has no automatic way out of. | ||
| group: portal-resnapshot-${{ inputs.server }} |
| environment: cloudflare-apply | ||
| env: | ||
| CF_TOKEN: ${{ secrets.CF_APPLY_TOKEN_PORTAL }} | ||
| VAULT_ADDR: https://vault.mctl.ai |
| # click: nobody is going to be prompted. | ||
| - name: Announce the sign-in window | ||
| if: always() | ||
| uses: appleboy/telegram-action@390cf7345777cd821dba28c72f436a78bce19cc5 # master 2026-05 |
| - name: Catalogue drift must be green | ||
| run: | | ||
| set -euo pipefail | ||
| python3 scripts/portal-catalogue-drift.py || { |
| if [ "$count" -eq "$TOOLS_BEFORE" ]; then | ||
| # Same COUNT is not the same catalogue -- a tool can be replaced, or only its | ||
| # schema or annotations changed -- so this is a warning here and the real | ||
| # check is the byte comparison the drift detector makes against the | ||
| # allowlist. What is NOT tolerable is nothing having changed at all, and |
|
|
||
| # 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 |
| curl -sS --fail-with-body \ | ||
| -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}" "$@" |
| <b>Portal re-snapshot</b> — <code>${{ inputs.server }}</code> | ||
| ${{ job.status == 'success' | ||
| && 'Server is in <b>Waiting</b> and unusable until someone signs it out and back in on the portal server-selection page. Do it now — the verify job is already counting down.' | ||
| || 'Flip job ended <b>' }}${{ job.status == 'success' && '' || format('{0}</b>. Check whether the registration was restored before touching anything else.', job.status) }} |
… the outage bound Three findings from the review on 3d35bd0 that survived the failure-path audit in d8909c4. All three end in the same place — the server flipped and nothing bounding how long it stays down — which is the failure mode this design's header comment says it exists to prevent. **P1 — the final gate could never run.** `verify` sets `CF_TOKEN`, which is what the `cf` helper in the shell scripts reads; `portal-catalogue-drift.py` reads `CLOUDFLARE_API_TOKEN` and exits 2 before its first API call without it. So the drift check failed on every run, for a reason having nothing to do with the catalogue, after the outage had already happened. Two consumers, two names, and both have to be fed. **An empty `last_synced` killed the wait.** `: "${LAST_SYNCED_BEFORE:?}"` treats empty as fatal, and empty is a real answer — the API can return no `last_synced`. It would have died on the script's first line, after the flip, removing the only bound on the outage for the sake of a value the loop can do without: an empty baseline just means any non-empty `last_synced` counts as movement, which is the correct reading. The variable being UNSET is a different thing — a wiring error — so that is what is refused, and refused with a message rather than `set -u` reporting it mid-wait. **A failing notifier took the job with it.** The announce step is `if: always()`, so a Telegram outage failed `flip`, and `verify` — which `needs: flip` — was skipped. Losing the announcement is bad; losing the bound because the announcement failed is the thing the bound exists for. `continue-on-error: true`. The other two findings from that review were already closed in d8909c4: the `&& '' ||` ternary is three independent expressions now, and `diff.sh` does a byte comparison against the pre-flip catalogue published as an artifact instead of comparing counts and warning. Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
|
The review landed on Already closed in Three survived, and all three end in the same place: the server flipped and nothing bounding how long it stays down. P1 is real and was invisible to every reading I did. The empty The notifier taking the job. Your framing of (b), (c) and (d) as one finding is the useful part: I had been auditing those paths individually and each looked survivable on its own. What they share is that the bound on the outage is a single point of failure, and three unrelated things could remove it.
|
|
Claude finished @mashkovd's task in 10m 6s —— View job Review —
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1 (P1)
Finding 2 (P2)
Finding 3 (P2)
Finding 4 (P2)
Finding 5 (P3)
|
| environment: cloudflare-apply | ||
| env: | ||
| CF_TOKEN: ${{ secrets.CF_APPLY_TOKEN_PORTAL }} | ||
| VAULT_ADDR: https://vault.mctl.ai |
There was a problem hiding this comment.
P1 — vault.mctl.ai is not this platform's Vault. Every dispatch dies at the canary, before the flip.
The Vault Ingress is declared at platform-gitops/bootstrap/templates/core-infra/vault.yaml:107-112, and its only host is secrets.mctl.ai. So does every other consumer in this repo:
.github/workflows/build-image.yaml:153—VAULT_ADDR="https://secrets.mctl.ai", the existing GitHub Actions OIDC loginplatform-gitops/bootstrap/templates/mctl-platform/mctl-api.yaml:40,mctl-portal.yaml:30platform-gitops/platform-skills/catalog/mctl-platform/references/vault.md:9- twenty
vault-cleanup.yamlfiles infrastructure/cloudflare/zones/mctl-ai/firewall.tf:25enumerates the zone's hosts and listssecrets.mctl.ai;vault.mctl.aiappears in no.tf, no manifest, and nowhere else in the repository except this line.
portal-resnapshot-vault-canary.sh is the first step in flip, and its first network call is POST ${VAULT_ADDR}/v1/auth/jwt/login. So the sequence is: an operator reads preflight, commits to an outage window, approves cloudflare-apply — and the job dies on a DNS/TLS failure before anything is written.
It fails closed, which is the design working. But it fails closed on every run, so the workflow as merged can never complete once.
build-image.yaml:153 carries a comment about exactly this class of bug (#1010): a Vault address that reads like configuration while not being the one in use.
VAULT_ADDR: https://secrets.mctl.ai
| started=$(date -u +%s) | ||
| while :; do | ||
| now=$(cf) || true | ||
| status=$(printf '%s' "$now" | jq -r '.result.status // "unreadable"') |
There was a problem hiding this comment.
P2 — the two guards protecting the wait loop from a bad read are both unreachable, and one of them kills the loop outright. Unchanged since the d8909c4c review; this commit fixed the LAST_SYNCED_BEFORE half of the same block but not this.
now=$(cf) || true on line 32 says the intent is to ride out a failed read. What actually decides is the parse on lines 33–34.
Connection failure / DNS / TLS. curl writes nothing to stdout, so now="". printf '%s' "" | jq -r '.result.status // "unreadable"' gets zero JSON values on stdin, so jq emits no output and exits 0. status is "", not "unreadable" — // never runs, because there was no input document for it to be an alternative in. The log line reads waiting: status= last_synced=none, and the timeout message on line 46 reads api is still status= after 1800s.
Non-JSON body. --fail-with-body prints the body and exits non-zero on a 5xx, so an HTML error page from the edge lands in now — and || true lets it through. jq then exits 5 on the parse error, pipefail propagates it to the command substitution, and under set -e the failing assignment ends the script.
That second one is the consequential path: one 502 from api.cloudflare.com at any point in the thirty minutes and the bounded wait stops at that second, mid-sign-in. verify goes red, the failure() Telegram says "the server may still be in Waiting", and the bound this job exists to provide is gone — after the flip, which is the failure mode the file header argues against.
Normalise before parsing:
now=$(cf) || now=""
printf '%s' "$now" | jq -e . >/dev/null 2>&1 || now='{}'
status=$(printf '%s' "$now" | jq -r '.result.status // "unreadable"')With that, both paths reach "unreadable", the log says so, and the loop keeps counting.
| echo "::add-mask::${token}" | ||
| echo "VAULT_TOKEN=${token}" >> "$GITHUB_ENV" | ||
|
|
||
| path="platform/portal-resnapshot/${SERVER}" |
There was a problem hiding this comment.
P2 — the canary writes to the exact path the flip-failure alert tells the operator to recover from, so the natural reaction to a failed run destroys the backup it names.
Line 31 sets path="platform/portal-resnapshot/${SERVER}" and line 36 POSTs {"data": {"canary": …}} there. portal-resnapshot-flip.sh:48 later POSTs the registration to the same path via VAULT_BACKUP_PATH. Within one run the ordering is correct — canary, then registration. Across runs it is not, because KV v2's data endpoint replaces the secret rather than merging: the new version contains canary and nothing else.
The sequence that costs the registration:
- Run A's flip fails inside the flip step. Telegram (
portal-resnapshot.yml:189) says: "The backup is in Vault atsecret/platform/portal-resnapshot/api— restore it by hand before anything else." - The operator does the obvious thing and re-dispatches, or a second person does while the first is reading the alert.
- Run B's canary is the first step in
flip. It writes{canary: …}to that path. The current version of the path the alert just named now holds no registration. - Run B's flip step fails early — say the
cfread onflip.sh:17— before writing its own backup.
KV v2 keeps version N-1, but nothing in the workflow, the alert or the README mentions ?version=, and the alert's wording ("the backup is in Vault at …") reads as a path, not a version history. With max_versions at whatever the mount was created with, repeated re-dispatches age the real backup out.
The fix is a sibling key, so the canary never occupies the recovery path:
path="platform/portal-resnapshot/${SERVER}"
canary_path="platform/portal-resnapshot/${SERVER}/canary"…writing and reading back $canary_path while still exporting VAULT_BACKUP_PATH=${path}. That keeps the canary a real write under the same policy prefix (worth confirming the mctl-gitops policy is written as platform/portal-resnapshot/* rather than an exact-path rule — the PR body lists the policy as a prerequisite, so this is the moment to specify which shape it needs).
| <b>Portal re-snapshot</b> — <code>${{ inputs.server }}</code> | ||
| ${{ steps.flip.outcome == 'success' && 'Server is in <b>Waiting</b> and unusable until someone signs it out and back in on the portal server-selection page. Do it now — verify is already counting down.' || '' }} | ||
| ${{ steps.flip.outcome == 'failure' && format('<b>Failed during the flip.</b> The registration may be cleared and the server unusable. The backup is in Vault at <code>secret/platform/portal-resnapshot/{0}</code> — restore it by hand before anything else.', inputs.server) || '' }} | ||
| ${{ (steps.flip.outcome != 'success' && steps.flip.outcome != 'failure') && 'Stopped <b>before</b> the flip; nothing was written and the server is untouched.' || '' }} |
There was a problem hiding this comment.
P2 — cancelled lands in the "nothing was written" branch, which is the single most dangerous misreport this workflow can produce. Unchanged since the d8909c4c review.
The three-way split is built by elimination: line 188 matches success, line 189 matches failure, and this line takes everything else. The comment at lines 168–169 names two values — "skipped when an earlier step failed, failure when this one did" — but steps.<id>.outcome has four, and always() is specifically the condition that still runs on cancellation.
So a run cancelled by a person between the bearer PUT on flip.sh:60 and the restore on line 66 — the state this file's own concurrency comment (lines 54–56) calls "the one state this procedure has no automatic way out of" — announces:
Stopped before the flip; nothing was written and the server is untouched.
cancel-in-progress: false stops Actions from cancelling the run. It does not stop the Cancel button, and an operator watching a flip that looks stuck is exactly who reaches for it.
Key on the safe case explicitly and let everything unrecognised fall into the dangerous branch, which is the direction the uncertainty should point:
${{ steps.flip.outcome == 'skipped' && 'Stopped <b>before</b> the flip; nothing was written and the server is untouched.' || '' }}
${{ (steps.flip.outcome != 'success' && steps.flip.outcome != 'skipped') && format('Flip ended <b>{0}</b> — assume the registration may be cleared. Backup: <code>secret/platform/portal-resnapshot/{1}</code>.', steps.flip.outcome, inputs.server) || '' }}That collapses lines 189 and 190 into one, covers failure and cancelled with the same instruction (they need the same one), and makes any future outcome value default to "go look".
|
|
||
| # shellcheck disable=SC2120 # called with no args on read paths, with -X PUT on writes | ||
| cf() { | ||
| curl -sS --fail-with-body \ |
There was a problem hiding this comment.
P2 — cf has no --max-time and no --connect-timeout, so the "bounded" wait is not bounded.
portal-resnapshot-wait.sh checks its deadline on line 44, after cf returns on line 32. curl has no default total timeout, and its default connect timeout is 300s per attempt; a connection that opens and then stalls without RST or FIN blocks the loop for as long as the peer keeps it. The 1800s deadline is only ever evaluated between polls.
The file header of wait.sh says the wait "fails on a timeout — which an approval nobody clicks does not", and the workflow header (lines 31–35) makes the same argument as the reason a measurement replaces #1258's second approval. A hang makes the measurement behave exactly like the unclicked approval: it sits there with the server unusable, and the only thing that ends it is the runner's default 6-hour job limit.
Cheap and local to the helper, so every caller gets it:
curl -sS --fail-with-body --connect-timeout 10 --max-time 30 \30s is comfortably above a normal Cloudflare API response and well under the 20s poll interval's intent. Worth pairing with timeout-minutes on the three jobs — verify in particular, where 35 would make the outage bound a property of the workflow rather than of a curl flag.
| # one would carry the OIDC assertion. The same habit the `cf` helper follows | ||
| # for the API token, and the reason it is worth following in a script an | ||
| # operator also copies. | ||
| token=$(jq -n --arg jwt "$jwt" '{role: "mctl-gitops", jwt: $jwt}' \ |
There was a problem hiding this comment.
P3 — the role and the audience both differ from the only Vault OIDC login this repository already runs, and the PR body lists only the policy as a prerequisite.
Here: role: "mctl-gitops", and line 16 requests &audience=vault.
.github/workflows/build-image.yaml — the existing GitHub Actions → Vault login in this repo — uses role: "github-actions" (line 176) and audience=https%3A%2F%2Fgithub.com%2Fmctlhq (line 166). CLAUDE.md states the same: "GitHub Actions authenticates to Vault with OIDC JWT (id-token: write + auth/jwt role github-actions)".
That is three things that have to exist in Vault, not one: the role mctl-gitops, a bound_audiences on it that accepts vault, and the policy on platform/portal-resnapshot/*. The PR body's "Before this can run" section names the policy and the role but not the audience, and bound_audiences is the one that produces a login failure that reads like a policy problem.
If there is no reason to diverge, matching github-actions/https://github.com/mctlhq reuses a JWT role configuration that is known to work and leaves only the policy to add. If mctl-gitops is deliberate — a narrower role so this repository's write capability is not attached to the shared one — that reasoning is worth a line here, since every other comment in these scripts carries its why.
(Paired with the P1 on VAULT_ADDR: with all four of endpoint, role, audience and policy unverified, the canary is measuring four things at once and reporting one.)
| # announcement is bad; losing the bound because the announcement failed | ||
| # is the outcome this whole design exists to prevent. | ||
| continue-on-error: true | ||
| uses: appleboy/telegram-action@390cf7345777cd821dba28c72f436a78bce19cc5 # master 2026-05 |
There was a problem hiding this comment.
P3 — this third-party action runs inside flip and therefore inherits both of the run's write credentials, for a step that only needs to reach Telegram.
CF_TOKEN: ${{ secrets.CF_APPLY_TOKEN_PORTAL }}is job-level (line 104), so it is in this step's environment.portal-resnapshot-vault-canary.sh:29writesVAULT_TOKEN=…to$GITHUB_ENV, which puts a live Vault token with write access toplatform/portal-resnapshot/*into every subsequent step of the job — including this one.
appleboy/telegram-action is a container action, so the runner passes the step environment into the container. The action is SHA-pinned (good, and consistent with the rest of the repo), so this is defence in depth rather than a live hole — but it sits oddly next to the care taken everywhere else in this change: the API token rides a curl config fd rather than argv, the Vault login body goes through stdin, and the registration goes to Vault specifically to keep it out of the artifact store.
Two ways to close it, either fine:
- Move the announcement to its own job with
needs: flip,if: always(), and noCF_TOKEN/VAULT_ADDR. It has all thesteps.flip.outcomeinformation it needs via a job output. - Or keep it here and stop the Vault token outliving its use: have the flip step re-derive it, or append
VAULT_TOKEN=to$GITHUB_ENVas the flip script's last act. That does not cover the failure paths, which is why the separate job is the cleaner of the two.
| preflight: | ||
| name: preflight ${{ inputs.server }} | ||
| runs-on: ubuntu-latest | ||
| if: github.ref == 'refs/heads/main' |
There was a problem hiding this comment.
P3 — a dispatch from a non-main ref reports success with all three jobs skipped. Unchanged since the 3d35bd05 review.
preflight skips, flip and verify skip on needs, and a run whose every job is skipped is green in the Actions UI and green to the API. For a workflow whose only purpose is a destructive operation, "I ran it and it was green" reading as "nothing happened" is the wrong default — it is indistinguishable from a successful re-snapshot at a glance, and this is precisely how it will be first exercised (someone testing it from this branch).
A step that fails with a reason is louder than a job-level if that vanishes:
- name: Refuse a dispatch from a non-default ref
if: github.ref != 'refs/heads/main'
run: |
echo "::error::dispatch this from main; ${{ github.ref }} is not the reviewed copy of the scripts this run would execute"
exit 1It also states the reason, which is not recoverable from the bare condition: the six scripts come out of the checkout, so the ref decides what code runs against CF_APPLY_TOKEN_PORTAL and a Vault write token. Alternatively, put the constraint where it belongs — a deployment-branch rule on the cloudflare-apply environment — and drop the if entirely.
|
Claude finished @mashkovd's task in 6m 16s —— View job Review —
|
| compared=$(printf '%s\n' "$out" | grep -E '^compared: ' || true) | ||
| if [ -z "$compared" ]; then | ||
| echo "::error::the catalogue detector produced no \"compared:\" line (exit ${code}): it never compared ${SERVER}, whatever else it printed. Nothing here has been verified." | ||
| exit "${code:-1}" |
There was a problem hiding this comment.
P2 — both new guards can exit 0. ${code:-1} substitutes only when code is unset or empty, and on the one path that matters code is the string 0, which is neither.
Line 19 always assigns: ... && code=0 || code=$?. So on a clean detector run code=0, and:
exit "${code:-1}" # -> exit 0Both branches in this block are error branches. Line 37 prints ::error::... Nothing here has been verified. and line 41 prints ::error::the detector compared servers but not ${SERVER} — and a GitHub ::error:: annotation does not fail a step. The step goes green, verify goes green, and the final gate of a procedure that just took a production server down reports the end state good having compared nothing. That is verbatim the outcome the comment at lines 22–34 says positive evidence was chosen to rule out.
Reachability, honestly. Today the detector covers both:
- no
compared:line with exit 0 needscheckedempty, and an emptyserversraisesUndeterminedatportal-catalogue-drift.py:145(exit 2) while a per-server throw lands inundetermined(exit 2). - a
compared:line that omits${SERVER}with exit 0 needs the server absent fromportal_servers()and absent fromOWNERS, sinceexpected_missing()would otherwise make it exit 4. A portal server that is not inOWNERSthrows inowner_allowlist()(:165) and lands inundetermined, exit 2.
So the guard is correct today because of how the other file orders its exit codes — which is the coupling the comment above says it was written to avoid. The guard should not depend on the detector never returning 0 in a shape it did not anticipate; that is the same list-to-keep-in-step, relocated from a grep into an exit status.
fail() { [ "${code:-0}" -ne 0 ] && exit "$code" || exit 1; }or simply exit 1 on both, since neither branch is reporting the detector findings — it is reporting that there are none to report.
| - name: Refuse to wait on a server with no registration | ||
| if: needs.flip.outputs.restored != 'yes' | ||
| run: | | ||
| echo "::error::${{ inputs.server }} was flipped and NOT restored: the registration is cleared and no sign-in is possible. Restore from Vault at secret/platform/portal-resnapshot/${{ inputs.server }} by hand." |
There was a problem hiding this comment.
P2 — the only run: block in this file that interpolates inputs.server into a shell script, and the premise this repository states is that the value cannot be trusted.
portal-resnapshot-lib.sh:28-30:
The server id is an input, so it is shape-checked before it reaches a URL.
workflow_dispatchtype:choice constrains the UI, not the API: a dispatch throughgh apican send anything.
Every other shell consumer in this change honours that: inputs.server goes into env: SERVER (lines 82, 159, 166, 283, 305, 317) and each of the seven scripts calls assert_server_id before the value reaches a URL. This line puts it straight into the text of a run: script, where GitHub substitutes it before bash sees the file. A value shaped like
api"; curl -s https://…/x | sh; echo "
executes in a job that still holds CF_PORTAL_READ_TOKEN.
Two consistent answers, and either is fine — what is not fine is holding both at once:
- If
type: choicereally is enforced by the dispatch API, say so once andassert_server_idin six scripts is redundant. - If it is not — which is what the file says — this line has to go through the environment like every other one:
env:
SERVER: ${{ inputs.server }}
run: |
echo "::error::${SERVER} was flipped and NOT restored: the registration is cleared and no sign-in is possible. Restore from Vault at secret/platform/portal-resnapshot/${SERVER} by hand."
exit 1Note the marks step directly above already does exactly this — its run: block contains no ${{ }} at all, and the reasoning is the same one.
Separately, and worth pairing with the fix: refusing here is right and refusing before the wait is right. The 1800s delay is gone. What did not move is the message the operator actually reads — see the note on line 333.
| format: html | ||
| message: | | ||
| <b>Portal re-snapshot FAILED to verify</b> — <code>${{ inputs.server }}</code> | ||
| The server may still be in <b>Waiting</b>, i.e. unusable. |
There was a problem hiding this comment.
P2 — this commit fixed the timing of the wrong message and left the message. The cleared-registration case now fails in seconds instead of at +1800s, and then sends this:
Portal re-snapshot FAILED to verify —
api
The server may still be in Waiting, i.e. unusable.
On the path the new refusal step at line 274 exists for, the server is not in Waiting. It has no registration at all, nobody can sign into it, and the recovery is the Vault copy. "May still be in Waiting" reads as "the sign-in has not happened yet", which sends an operator to the portal server-selection page — the one action that cannot help.
The job-output comment at lines 99-106 names this exact harm as the thing being fixed:
... fires "may still be in Waiting, i.e. unusable", which contradicts the announce correct "the registration is cleared, restore it by hand" and arrives thirty minutes later, putting the wrong message at the bottom of the channel.
Both halves of that sentence were true, and only the second half was addressed. The announce still fires first from flip with the correct instruction, and this still fires after it with the contradicting one — just 30 minutes sooner. The message at the bottom of the channel is still the wrong one.
The value that separates them is already a job output as of this commit:
message: |
<b>Portal re-snapshot FAILED to verify</b> — <code>${{ inputs.server }}</code>
${{ needs.flip.outputs.restored == yes && The server may still be in <b>Waiting</b>, i.e. unusable — check whether the sign-in completed. || }}
${{ needs.flip.outputs.restored != yes && format(The registration is <b>CLEARED</b>, not Waiting: no sign-in is possible. Restore from Vault at <code>secret/platform/portal-resnapshot/{0}</code>., inputs.server) || }}
<a href="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}">run</a>Worth noting while here that this alert also asserts Waiting for the two other ways verify goes red — diff.sh finding a byte-identical catalogue and the drift gate — and in both of those the sign-in demonstrably completed, because the wait above them passed. steps.wait.outcome is in hand and distinguishes all three.
| # | ||
| # 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.
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 [2] line in the detector is printed by a bail-out that return 2 before the per-server loop: portal-catalogue-drift.py:811 (no token), :822 and :825 (stdin), :828 (no account), :837 and :841 (portal unreadable). None of them reaches the if checked: print(f"compared: ...") at :916, so every one of them produces no compared: line and is caught by the new guard at line 36 first. Nothing can arrive here with a [2] line in $out.
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:
- Still no
--selftest.cloudflare-drift.yml:305-309writes down why every other call site gates on it: a detector that fails its own selftest was not run against Cloudflare, so its exit code means nothing — and here that exit code is the last word on whether an outage was worth it. Raised at8d184f81,f20cf20aandc59e66e9; stating it once more and dropping it. PYTHONUNBUFFERED=1. The new guard anchors on^compared:, and that line goes to stdout while every finding goes to stderr. With stdout block-buffered and stderr unbuffered into the same pipe, a stdout flush that lands on a buffer boundary rather than a line boundary can split the line — and a splitcompared:line fails the anchor, which now means a hard::error::on a run that was fine. Oneenv:entry removes the possibility.
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit FindingsFinding 1 (P1)
Finding 2 (P2)
Finding 3 (P2)
Finding 4 (P3)
|
| id: wait | ||
| env: | ||
| SERVER: ${{ inputs.server }} | ||
| LAST_SYNCED_BEFORE: ${{ needs.flip.outputs.last_synced }} |
There was a problem hiding this comment.
P3 — carried from the a21de0c4 review and narrowed by the new refusal step above, not closed by it.
The refusal removes the restored=no paths from this job entirely, which is right. What remains is the set where restored=yes but flip.sh still did not reach its last line — step 2a exiting at :130 on a changed registration, the cp at :140, or the baseline cf at :149 itself failing on a 45s --max-time. Those are exactly the cases the if: at line 258 was widened to admit, and on all of them last_synced_after_restore is never written, so this env is set-but-empty.
wait.sh:23 accepts that deliberately and correctly. The consequence is that the condition at wait.sh:53 degrades to status = ready plus any non-empty last_synced, and wait.sh:50-52 is explicit that status alone is not trusted:
a server can read
readyoff the pre-flip snapshot in a race, andlast_syncedis the only field this API moves on a real re-authorization
Not load-bearing today, because flip.sh:106 asserts the restore landed in waiting, so ready still requires a real sign-in. It is worth a line because the check gets quietly weaker on precisely the runs that already went wrong, and the pass line prints (was ) without saying so. One ::warning:: at the top of the wait when the baseline is empty costs nothing and keeps the log honest.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 3 P2, 3 P3 at a57f4c39.
Closed in this commit, verified against the repository rather than the comment. restored is now a job output and the cleared-registration case is refused at the top of verify instead of polling a server nobody can sign into for 1800s — the right seam, and the announce keeps saying the right thing on that path. The drift gate now requires positive evidence: portal-catalogue-drift.py:916 prints compared: api=76@... on every outcome that has one, the grep at :35 is anchored correctly against that format, and the per-server check at :40 matches ^api= and , tg= without matching inside another message. Dropping the case "$code" remedy map in favour of quoting the finding lines is right for the stated reason — :918-928 really is a maximum across the account, so the code cannot name a remedy for one server.
P2 x3.
portal-resnapshot-drift-gate.sh:38and:42— both new guardsexit "${code:-1}", and:-substitutes only on unset-or-empty. On a clean detector runcodeis the string0, so both error branches exit 0: the::error::annotation prints, the step passes, and the final gate reports the end state good having compared nothing. That is verbatim the outcome the comment at:22-34says positive evidence was chosen to rule out. Today it is unreachable only because of how the detector orders its own exit codes — which is the coupling that comment says it was avoiding, relocated from a grep into an exit status.portal-resnapshot.yml:277— the new refusal step is the onlyrun:block in the file that interpolatesinputs.serverinto the script text. Every other shell consumer goes throughenv: SERVERplusassert_server_id, on the premiseportal-resnapshot-lib.sh:29-30states explicitly:type: choiceconstrains the UI, not the API. Themarksstep immediately above has no${{ }}in itsrun:at all. Either the premise holds and this line needs the same treatment, or it does not and sixassert_server_idcalls are redundant — the file currently holds both answers.portal-resnapshot.yml:333— the timing of the wrong message was fixed and the message was not. On the cleared-registration path thefailure()alert still says "The server may still be in Waiting, i.e. unusable", which reads as "the sign-in has not happened yet" and sends an operator to the portal UI when the registration is gone and the recovery is Vault. The job-output comment at:99-106names putting the wrong message at the bottom of the channel as the harm; it still lands after the announce, 30 minutes sooner.needs.flip.outputs.restoredbecame an output in this very commit and separates them. Same alert also asserts Waiting for the two failures that can only occur after the wait passed.
P3 x3: the [2] block at :59-62 is now unreachable, since every [2] bail-out returns before the compared: line is printed and the new guard catches it first — harmless, but it is the list the comment above argues against keeping; no PYTHONUNBUFFERED=1 on a gate that now anchors on a stdout line interleaved with stderr; LAST_SYNCED_BEFORE is still empty on the restored=yes paths where flip.sh did not reach :150, degrading the wait to ready plus any non-empty value, silently.
Carried, stated once and not re-argued: no --selftest before a detector whose exit code is the last word on whether an outage was worth it; a non-main dispatch as a green no-op with every job skipped; no lock against cloudflare-apply-portal, which manages tg auth_credentials; diff.sh:74 trailing && list.
One thing worth stating so it does not get "fixed". The automated review calls jq -n "{auth_credentials: env.CREDS}" at flip.sh:74 a P1 because env.CREDS yields a string. It is not a bug, and the repository says so twice: the README step 0b records the measured shape — auth_credentials is auth_mode + config + registration_info as one JSON-encoded string — and flip.sh:99 sends a bare string on the same field for the bearer flip. Adding fromjson would break the restore at the one moment there is no way back.
Checked and correct. Envelope before every field read, including flip.sh:100-101 and :123-129. assert_server_id ahead of every URL interpolation in all seven scripts. Markers written either side of the destructive PUT with the INT TERM HUP to EXIT trap pair. cmp -s over character-identical jq -S ... sort_by(.name) filters at flip.sh:65 and diff.sh:46, identical exiting 1, missing artifact red rather than quiet. 2a comparing by field name, so no log carries endpoint, client id or scope. The canary on _canary/<server>, off the recovery path. Vault address, role and audience matching build-image.yaml. CLOUDFLARE_ACCOUNT_ID from the workflow env. Preflight publishing no outputs and refusing auth_mode != manual and tools == 0 before the approval. The VAULT_TOKEN comment at :121-125 is now accurate about what $GITHUB_ENV can and cannot scope, which is the better of the two fixes available.
The design is right, and the departure from #1258 — measuring ready plus a moved last_synced instead of a second approval — remains the better call.
Reviewed at
a57f4c39by reading the pinned diff.shellcheckand shell execution are permission-gated in this sandbox, which thediff.sh:74note has stated as a limit on that finding sincef20cf20a; the${code:-1}finding does not depend on execution, since:-never substitutes for the string0.
…ert must name the state
Three P2s, and the first undid the commit before it.
**`exit "${code:-1}"` exits 0 on a clean detector run.** `:-` substitutes only
on unset-or-empty, and `code` there is the string `0` — so both new evidence
branches printed their `::error::` and passed the step. The final gate reported
the end state good having compared nothing, which is verbatim the outcome the
paragraph above them says positive evidence was chosen to rule out. Unreachable
today only because of how the detector happens to order its own exit codes:
the same coupling that paragraph claims to have removed, relocated from a grep
into an exit status. A `fail()` helper preserves a real code and substitutes 1
for zero; both branches are exercised at both codes.
**The refusal step interpolated `inputs.server` into the script text.** Every
other shell consumer in the file goes through `env: SERVER` and
`assert_server_id`, on the premise `portal-resnapshot-lib.sh` states outright:
`type: choice` constrains the UI, not the API, and a dispatch through `gh api`
can send anything. `${{ }}` is substituted before bash parses the line, so the
file was holding two answers to whether that premise is true. It holds one now.
**The cleared-registration alert had its timing fixed and its wording left.**
It still said "the server may still be in Waiting, i.e. unusable" — which reads
as "nobody has signed in yet" and sends an operator to the portal UI when the
registration is gone and the recovery is Vault. And the steps after the wait
can only fail once the sign-in has already happened, so asserting Waiting for
them is false too.
Three states now, off `needs.flip.outputs.restored` — which became an output in
the previous commit — and `steps.wait.outcome`: cleared, still-waiting, and
signed-in-but-a-later-check-failed. The last one says explicitly that no portal
action is needed, because the default reading of a re-snapshot alert is that
somebody must go and click something.
Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
|
Claude finished @mashkovd's task in 4m 42s —— View job Review —
|
| message: | | ||
| <b>Portal re-snapshot FAILED to verify</b> — <code>${{ inputs.server }}</code> | ||
| ${{ needs.flip.outputs.restored != 'yes' && format('The registration is <b>cleared</b>, not merely unauthorized — a sign-in cannot fix it. Restore from Vault at <code>secret/platform/portal-resnapshot/{0}</code>.', inputs.server) || '' }} | ||
| ${{ (needs.flip.outputs.restored == 'yes' && steps.wait.outcome == 'failure') && 'The server is still in <b>Waiting</b> and unusable: nobody completed the upstream sign-in, or it could not be read. The run says which.' || '' }} |
There was a problem hiding this comment.
P2 — the comment above says THREE states, and the partition has four. The fourth is the one where the server is down, and it prints nothing.
steps.wait.outcome is success or failure only when the step actually ran. With restored == 'yes', the actions/checkout at line 263 is the one thing ahead of it; if that fails, the wait is skipped and outcome is skipped. Then:
- line 350 —
restored != 'yes'→ false - line 351 —
outcome == 'failure'→ false - line 352 —
outcome == 'success'→ false
and the message that reaches the operator is the header, three blank lines, and the run link.
That is the state with the most at stake in the whole design: the flip succeeded, the server is sitting in Waiting and unusable for every portal user, the announce from flip has already said "verify is already counting down" — and verify died before it counted anything. The only message that arrives afterwards says nothing about the server and carries no instruction. Every other branch in this file is written so that uncertainty points at "go look"; this one points at nothing.
Line 352 is the branch with a positive claim to defend ("No portal action needed"), so it is the one that should require positive evidence. Line 351 should be the default:
${{ (needs.flip.outputs.restored == 'yes' && steps.wait.outcome != 'success') && 'The server is still in <b>Waiting</b> and unusable: nobody completed the upstream sign-in, it could not be read, or the wait never ran. The run says which.' || '' }}One operator changed, and the three branches then cover outcome exhaustively — skipped, an unset step, and any value GitHub adds later all land on the sentence that is true of them: the server is in Waiting and nothing is bounding it.
(cancelled is correctly out of scope here — if: failure() does not run on a cancelled job — which is precisely why the flip announce keys on always() plus the on-disk markers instead. That split is right.)
| # 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. | ||
| # NON-ZERO, and `${code:-1}` was not that. `:-` substitutes only on |
There was a problem hiding this comment.
P3 — the fix is right and the comment describing it lost its opening clause, so it reads as a continuation of the paragraph it is not part of.
Line 34 ends the positive-evidence argument with "…and only positive evidence rules that out." Line 35 then begins:
NON-ZERO, and
${code:-1}was not that.
with no blank # between them and no subject. Read top to bottom it parses as one sentence — "only positive evidence rules that out. NON-ZERO, and ${code:-1} was not that." — and the thing that has to be non-zero (the exit status of the branches below) is never named. The rest of the paragraph is good and says exactly the right thing; it is the first six words that went missing, presumably a heading like AND THE EXIT HAS TO BE NON-ZERO.
Worth fixing because these comments are the argument, not decoration — every other block in these seven scripts states its claim before defending it, and this is the one that records why fail() exists at all.
The code itself is correct, including the part that is easy to get wrong: [ "${code:-0}" -ne 0 ] && exit "$code" failing when code is 0 is a non-final element of an && list, so set -e does not fire and the unconditional exit 1 below it runs. Both paths leave non-zero, which is the whole point of the change.
| . "$(dirname "$0")/portal-resnapshot-lib.sh" | ||
| assert_server_id | ||
|
|
||
| out=$(python3 scripts/portal-catalogue-drift.py 2>&1) && code=0 || code=$? |
There was a problem hiding this comment.
P3 — still no PYTHONUNBUFFERED=1, and this commit inverted what a split compared: line costs.
The gate anchors on ^compared: , which portal-catalogue-drift.py:916 prints to stdout, while every finding and every [2] bail-out goes to stderr (:810, :821, :824, :827, :836, :840, and the four heading blocks). $(… 2>&1) is not a tty, so CPython block-buffers stdout and line-buffers stderr into the same pipe: stdout content is held until a buffer boundary or exit, and a flush that lands mid-line splits it with stderr text in between.
Before this commit that produced a false green — exit "${code:-1}" with code=0. Now fail() exits non-zero, so the same interleaving turns a run where the sign-in completed, the catalogue moved and the account is clean into a red verify with ::error::the catalogue detector produced no "compared:" line (exit 0), which is a sentence that cannot be true. Lower probability than the bug just fixed, and higher blast radius per occurrence, since it fails the gate on a run that went perfectly.
env:
PYTHONUNBUFFERED: '1'on the calling step (portal-resnapshot.yml:325) removes the possibility rather than reasoning about buffer sizes — and it is what cloudflare-drift.yml:374-380 already does, for the related reason that the ordering of these two streams is what an operator reads.
Two carried notes on this file while it is open, stated once and not re-argued:
- The
[2]block at lines 69-72 is now unreachable: every[2]bail-out returns before:916prints thecompared:line, so the guard at line 49 catches all of them first. Harmless, but it is exactly the "list to keep in step with another file" that lines 28-34 argue against keeping. - Still no
--selftest, whichcloudflare-drift.yml:305-309gates on because a detector that fails its own selftest was never run against Cloudflare — and here its exit code is the last word on whether an outage was worth it.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 2 P3 at 6fca4536.
Closed in this commit, verified against the repository rather than the comment. Both evidence branches in the drift gate now exit non-zero, and fail() is correct in the part that is easy to get wrong: [ "${code:-0}" -ne 0 ] && exit "$code" failing when code is the string 0 is a non-final element of an && list, so set -e does not fire and the unconditional exit 1 below it runs — both paths leave non-zero, which was the whole finding. The refusal step at :274 takes inputs.server through env: like every other shell consumer in the file, so the file no longer holds two answers to whether type: choice is enforceable. And the failure() alert no longer asserts Waiting on the cleared-registration path or on the two checks that can only fail once the sign-in has already happened — three genuinely different instructions where there was one.
P2 x1. portal-resnapshot.yml:351 — the comment says three states and the partition has four. steps.wait.outcome is success or failure only when the step ran; with restored == 'yes' a failed actions/checkout at :263 skips it, and none of :350, :351, :352 match. The operator gets the header, three blank lines and a link — for a server that is in Waiting, unusable, with the flip announce having already said "verify is already counting down" and verify having died before it counted. :352 is the branch with a positive claim to defend, so it is the one that should require evidence; != 'success' on :351 makes the three exhaustive and is true on every value it newly catches.
P3 x2. The new comment at drift-gate.sh:35 lost its opening clause, so "NON-ZERO, and ${code:-1} was not that" reads as a continuation of the previous sentence and never names the thing that has to be non-zero — in a file where the comments are the argument. And still no PYTHONUNBUFFERED=1 on the gate, which now costs the opposite of what it did: the anchored ^compared: line is stdout (block-buffered) captured with 2>&1 alongside stderr, and a split line used to exit 0 and now exits non-zero, failing a run where the sign-in completed and the catalogue moved.
Carried, stated once and not re-argued: the [2] block at drift-gate.sh:69-72 is now unreachable, since every [2] bail-out returns before the compared: line is printed; no --selftest; LAST_SYNCED_BEFORE still empty on the restored=yes paths where flip.sh did not reach :150, degrading the wait to ready plus any non-empty value; a non-main dispatch as a green no-op; no lock against cloudflare-apply-portal, which manages tg's auth_credentials; diff.sh:74's trailing && list — I tried again to settle it here and both shell execution and shellcheck are permission-gated, which is itself the argument for an explicit if.
One thing worth stating so it does not get "fixed". The automated review on this PR calls jq -n '{auth_credentials: env.CREDS}' at flip.sh:74 a P1 because env.CREDS yields a string rather than an object. It is not a bug, and the repository says so twice: the portal README's step 0b records the measured shape — "auth_credentials is auth_mode + config + registration_info as one JSON-encoded string" — and flip.sh:99 sends a bare string on the same field for the bearer flip. Adding fromjson would break the restore at the one moment there is no way back.
Checked and correct. Envelope before every field read, including flip.sh:100-101 where the ordering decides whether a failed read can report the flip as done, and :123-129 where a null auth_config_summary reports all three fields as changed rather than comparing as equal-to-nothing. assert_server_id ahead of every URL interpolation in all seven scripts, and now ahead of the only run: block that had bypassed it. Markers written either side of the destructive PUT with the INT TERM HUP to EXIT trap pair, and verify gated on flipped while the cleared case is refused at the top. cmp -s over character-identical jq -S ... sort_by(.name) filters at flip.sh:65 and diff.sh:46, identical exiting 1, missing artifact red rather than quiet. 2a comparing by field name, so no log carries endpoint, client id or scope. The canary on _canary/<server>, off the recovery path. Vault address, role and audience matching build-image.yaml. CLOUDFLARE_ACCOUNT_ID from the workflow env, so the bare detector invocation cannot block on stdin. Preflight publishing no outputs and refusing auth_mode != manual and tools == 0 before the approval, where refusing is free.
The design is right, and the departure from #1258 — measuring ready plus a moved last_synced instead of a second approval — remains the better call.
Reviewed at
6fca4536by reading the pinned diff675a2ece...6fca4536. Shell execution andshellcheckare permission-gated in this sandbox; neither finding above depends on running anything.
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1 (P1)
Finding 2 (P1)
Finding 3 (P2)
Finding 4 (P3)
|
…g decide the verdict **The comment said three states and the partition had four.** `steps.wait.outcome` is `success` or `failure` only when the step RAN: a failed checkout skips it and leaves `skipped`, and with `== 'failure'` none of the three branches matched. The operator got a header, three blank lines and a link — for a server sitting in `Waiting`, unusable, after the flip announce had already said the countdown was running and `verify` had died before it counted. `!= 'success'` on the middle branch makes them exhaustive and is true on every value it newly catches. The branch carrying a POSITIVE claim — "the sign-in completed, no portal action needed" — is the one that must require evidence, so it keeps `== 'success'`. **`PYTHONUNBUFFERED=1` on the drift gate**, and the reason inverted under the previous commit. 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 the `compared: …` line can be interleaved with a stderr write and lose its line start, after which the anchored `^compared: ` does not 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. And the comment defending the `fail()` helper had lost its opening clause, so it read as a continuation of the previous sentence and never named what has to be non-zero — in a file where the comments are the argument. Claude-Session: https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC
|
Claude encountered an error after 1m 51s —— View job Review —
|
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
⏳ The Claude reviewer hit the shared usage limit and could not finish. This is transient and is not a code issue. Re-run with |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1: P1 —
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Finding 1 (P1):
|
|
Claude finished @mashkovd's task in 6m 46s —— View job Review —
|
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Findings1.
|
| # A server with no registration has nothing to back up, and the flip would | ||
| # destroy nothing and restore nothing — it would just leave it as it was while | ||
| # reporting success. | ||
| if [ "$mode" != "manual" ]; then |
There was a problem hiding this comment.
P1 — this check refuses every portal server as merged, so no dispatch of this workflow can get past preflight. Nothing on the portal is in manual OAuth mode any more.
infrastructure/cloudflare/portal/mcp-servers.tf:3-9, on the base of this PR:
Every server here is registered by dynamic client registration (DCR, the dashboard's "Automatic (recommended)"):
auth_type = "oauth"with noauth_credentialsand noclient_secret. …tgandapiwere the last two manual servers; both moved on 2026-09-26 (#1363)
seerrsense went the same way earlier (:199 — "registered … on 2026-09-10 in manual mode, and moved to automatic (DCR)"). All six resources carry no auth_credentials and no client_secret, which :5-6 says is exactly what keeps a server out of manual mode.
And the README this PR edits says it in the sentence directly above the section the PR adds (infrastructure/cloudflare/portal/README.md:386-388):
For a server in manual OAuth mode — none today;
tgandapiwere the last, until 2026-09-26 — that snapshot is taken once …
So cf returns auth_config_summary: null for all three choices, mode is none, and this line exits 1 with "is auth_mode=none, not manual; this procedure is only for manual-OAuth servers". The refusal is correct — it is the preflight working — but it is the outcome on every run, which makes the workflow unrunnable rather than merely untested.
Three places state the opposite and would need to change with it:
.github/workflows/portal-resnapshot.yml:9-16— "For a server in manual OAuth mode — all three of ours — that snapshot is taken ONCE". Not true of any of them now.README.md:417-420, added here — "portal-resnapshot.ymlis what runs this, and it is the thing to dispatch", immediately beneath the line saying there is nothing to dispatch it against.- The PR body's first intended run:
server: api, to pick upmctl_get_lifecycle_ownership. Under DCR the background sync refreshes the catalogue, which is the reason portal: seerrsense, api and coolify-mcp under mcp-servers.tf in automatic (DCR) mode #1363 moved them;api's catalogue is no longer frozen.
This is not a defect in the code written here — it is main moving underneath the branch, and the merge at 13313632 brought it in without reconciling it. The choices look like:
- Merge it as a dormant tool, with the header and the new README section saying plainly that no server is in manual mode today, that the workflow exists for the day one is re-registered that way, and that preflight will refuse all three until then. The seven scripts are the artefact worth keeping either way.
- Close it, and let the README recipe stand as the historical record it already is.
What should not ship is the third state: a workflow whose own documentation tells an operator to dispatch it, and which refuses on its first step for a reason the same README already records.
| # | ||
| # 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.
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.
scripts/portal-catalogue-drift.py at the base of this PR has a lagging list and return 5 (:1176-1177), fed by catalogue_file_lag() (:316-337), which compares the live catalogue against the committed infrastructure/cloudflare/portal/allowlists/catalogue.json that mcp-portal.tf builds updated_tools from. Its line is {server}: -prefixed:
api: catalogue.json differs from the live catalogue (last_synced …) -- synced but not committed: ['mctl_get_lifecycle_ownership']; committed but not synced: none
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, mine matches on this line, and the gate exits 5. verify goes red and the failure() Telegram fires, after the outage and after the sign-in.
The README on this branch states that outcome as the expected one (:330-337):
When a server re-syncs (a tool added or removed upstream, or after the re-snapshot below): the nightly
portalplan incloudflare-drift.ymlgoes red;scripts/portal-catalogue-drift.pyexits 5 and names the difference; the fix is a PR that updates the server's list incatalogue.jsonto the portal's order.
and cloudflare-drift.yml:466-476 already has the branch that says the right thing — "The portal is fine: a server re-synced and allowlists/catalogue.json … was not updated … Do NOT re-snapshot."
Two things follow:
- Exit 5 with only
laggingfindings for this server is this workflow's success state, not a failure. It should print the follow-up — updatecatalogue.jsonin a PR, and the owning repo's allowlist first if the new tool has no decision — and exit 0. - The remedy sentence on line 97 is a two-category list for a five-category detector: "Only a line about a MISSING tool means re-snapshot or allowlist-apply; a waiver line is a chore". There is now a third shape, and quoting the finding does not help here because the finding's own text is correct while the sentence above it offers two wrong remedies.
A catalogue.json differs/not in catalogue.json line is also distinguishable from the rest without re-introducing the exit-code mapping this block deliberately dropped — the heading it prints under (the committed catalogue no longer matches the portal, :1140-1142) is on stdout in the same captured stream.
| 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.
P2 — this branch is unreachable, and the gate's weakest branch is therefore its normal outcome. ALLOWLIST_TOKEN is not set on the step, and one of the six upstreams is private.
scripts/portal-catalogue-drift.py:117 — PRIVATE_OWNERS = {"projects"} — and allowlist_source() at :244-250:
if not token:
raise Undetermined(
f"{server}: {repo} is private and ${TOKEN_ENV} is not set; "
"the allowlist cannot be read")TOKEN_ENV is ALLOWLIST_TOKEN (:122), and portal-resnapshot.yml:325-330 sets SERVER and CLOUDFLARE_API_TOKEN only. So projects raises on every invocation of this gate, lands in undetermined, and the detector returns at least 2 (:1178-1179). code is never 0, so line 64 and its "catalogue drift is clean account-wide" can never run.
The run then falls through to line 103, which is wrong in both halves of what it tells the operator:
catalogue drift exit 2, but no finding names
api: this re-snapshot is consistent and another server is stale.cloudflare-drift.ymlowns that.
projects is not stale — its allowlist could not be read. And cloudflare-drift.yml does not own it, because that workflow mints the token first (:378-394, an actions/create-github-app-token scoped to projects-mcp with contents: read) and so never sees this state at all. The nightly is green on the thing this gate reports as somebody else's staleness.
The verdict happens to be right — exit 0 is correct for api — but it is right by the fallback rather than by the positive-evidence path the comment at :30-42 argues the whole design rests on, and the message sends the reader to a workflow that cannot reproduce it.
cloudflare-drift.yml:378-394 is the recipe; the comment there records why GITHUB_TOKEN will not do. Mirroring it here needs the two AGENTS_APP_* secrets in this job — worth weighing against the alternative of narrowing the gate to one server, which the file's own opening paragraph says is the question it owns.
| description: 'Portal server to re-snapshot' | ||
| required: true | ||
| type: choice | ||
| options: [api, tg, seerrsense] |
There was a problem hiding this comment.
P3 — three of the portal's six servers, and the second place that has to change if manual mode ever returns.
scripts/portal-catalogue-drift.py:99-106 now lists six: tg, api, seerrsense, projects, alice, coolify, and mcp-servers.tf has a resource for each. This options: list and assert_server_id in portal-resnapshot-lib.sh:32-35 both hard-code the original three.
Not wrong today — none of the six is in manual mode (see the preflight note), so the correct list is empty and three is as good as six. It matters for whichever resolution the P1 takes: if a server is ever re-registered manually, it is as likely to be one of the new three, and the shape check and the dropdown are two files apart with no test tying either to OWNERS or to mcp-servers.tf.
Cheapest durable form is to stop maintaining the list by hand: assert_server_id could read the ids out of infrastructure/cloudflare/portal/allowlists/catalogue.json, which is already committed, already validated by scripts/validate-portal-allowlists.py, and already the file this procedure's follow-up PR edits. The dropdown cannot be generated, but a mismatch between it and the shape check would then be caught by the shape check rather than by a reader.
| # | ||
| # 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.
P3 — carried from the 6fca4536 review and confirmed still unreachable, plus one new reason the missing --selftest now costs more.
Unreachable. Every [2] line is printed by a bail-out that returns before the per-server loop — portal-catalogue-drift.py:1020, :1040, :1043, :1046, :1055, :1058 — so none of them reaches the if checked: print(f"compared: …") at :1165. The guard at line 58 catches all six first. Harmless, but the comment at :36-42 argues against keeping precisely this kind of list in step with another file, and the merge just moved every one of those line numbers.
--selftest. Main's cloudflare-drift.yml:406-412 now runs it and gives it its own reported state — code=2 with the heading "the catalogue drift detector itself failed its selftest" and the note "It was not run against Cloudflare." The selftest also grew in the merge (:805-811 now covers "an uncompared server does not condemn its waiver"). This gate takes the detector's exit code as the last word on whether a production outage was worth it, and still does not ask the detector whether it works.
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: portal-resnapshot-diff.sh:74's trailing && list (writing a scratch file and running bash/shellcheck are both permission-gated in this sandbox, which four reviews running is itself the argument for an explicit if); a dispatch from a non-main ref reporting success with every job skipped; and no lock against cloudflare-apply-portal — now less pressing, since mcp-servers.tf carries no auth_credentials for any server and so the apply no longer writes the field this workflow flips.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P1, 2 P2, 2 P3 at 13313632.
Closed in fb02778f, verified against the repository rather than the comment. The failure() alert's three branches are exhaustive now: != 'success' on the Waiting branch catches skipped, so a failed actions/checkout no longer sends a header, three blank lines and a link for a server sitting in Waiting, and the branch keeping == 'success' is the one carrying the positive claim — the right way round. PYTHONUNBUFFERED=1 on the detector means the anchored ^compared: line cannot lose its line start to a stderr interleave, so buffering no longer decides the verdict. And the comment at drift-gate.sh:43 has its opening clause back.
Both of those were the last outstanding findings on the code the branch wrote. What this review is about is the merge. 13313632 pulled main in, and main changed three things this workflow is built on.
P1 — preflight.sh:22 refuses every portal server, so no dispatch can get past it. Nothing is in manual OAuth mode any more. mcp-servers.tf:3-9 on this branch's own base: "Every server here is registered by dynamic client registration … tg and api were the last two manual servers; both moved on 2026-09-26 (#1363)"; seerrsense moved 2026-09-10 (:199). All six resources carry no auth_credentials and no client_secret, which :5-6 says is what keeps a server out of manual mode. The README this PR edits says it in the sentence directly above the section the PR adds (:386-388): "For a server in manual OAuth mode — none today". So mode is none, preflight exits 1, every run. The refusal is the preflight working; it is also the only outcome available. Three places still assert the opposite — the workflow header at :9-16 ("all three of ours"), the added README section at :417-420 ("it is the thing to dispatch"), and the PR body's first intended run. Merge it as an explicitly dormant tool or close it; what should not ship is documentation telling an operator to dispatch a workflow that refuses on its first step for a reason the same README records.
P2 — the drift gate fails on the one exit code a successful re-snapshot guarantees. Main added lagging and return 5 (portal-catalogue-drift.py:316-337, :1176-1177): when a server re-syncs, the committed allowlists/catalogue.json that mcp-portal.tf builds updated_tools from no longer matches the live catalogue, and the detector prints a {server}: catalogue.json differs from the live catalogue … line. mine matches it, so drift-gate.sh:86-99 exits 5 and fires the failure() Telegram — after the outage and the sign-in — with a remedy sentence naming two categories for what is now a five-category detector, neither of them the right one. The README states this as the expected post-re-snapshot outcome (:330-337, "or after the re-snapshot below … the fix is a PR that updates the server's list in catalogue.json") and cloudflare-drift.yml:466-476 already has the branch that says "The portal is fine … Do NOT re-snapshot." Exit 5 with only lag findings for this server is this workflow's success state plus a follow-up PR, not a failure.
P2 — ALLOWLIST_TOKEN is unset, so drift-gate.sh:64 is unreachable and the fallback misreports. PRIVATE_OWNERS = {"projects"} (:117) and allowlist_source (:244-250) raises Undetermined without the token, so projects lands in undetermined on every invocation and the detector returns at least 2 (:1178-1179). "catalogue drift is clean account-wide" can therefore never print, and an otherwise-perfect account resolves through line 103: "another server is stale. cloudflare-drift.yml owns that." projects is not stale — its allowlist could not be read — and the nightly does not own it, because it mints the token first (cloudflare-drift.yml:378-394) and never sees this state. The verdict is right by the fallback rather than by the positive-evidence path :30-42 says the design rests on.
P3 ×2: the type: choice options and assert_server_id hard-code three of the portal's now-six servers, two files apart with nothing tying either to OWNERS or mcp-servers.tf; and the [2] block at drift-gate.sh:78-81 is confirmed still unreachable — all six bail-outs return before the compared: line — in a file whose own comment argues against keeping that kind of list in step with another file, every line number of which the merge just moved. Still no --selftest, which main's cloudflare-drift.yml:406-412 now runs and reports as its own state.
Carried, stated once and not re-argued: diff.sh:74's trailing && list — writing a scratch file and running bash/shellcheck are both permission-gated here, and four reviews failing to settle it is itself the argument for an explicit if; a non-main dispatch as a green no-op; no lock against cloudflare-apply-portal, which is now less pressing since no mcp-servers.tf resource writes auth_credentials at all.
One thing worth stating so it does not get "fixed". The automated review on this PR calls jq -n '{auth_credentials: env.CREDS}' at flip.sh:74 a P1 because env.CREDS yields a string rather than an object. It is not a bug: the README's step 0b records the measured shape — "auth_credentials is auth_mode + config + registration_info as one JSON-encoded string" — and flip.sh:99 sends a bare string on the same field for the bearer flip. fromjson would break the restore at the one moment there is no way back.
Checked and correct, unchanged. Envelope before every field read, including flip.sh:100-101 and :123-129. assert_server_id ahead of every URL interpolation in all seven scripts and in the one run: block that had bypassed it. Markers either side of the destructive PUT with the INT TERM HUP → EXIT trap pair; verify gated on flipped and the cleared case refused at the top. cmp -s over character-identical jq -S … sort_by(.name) filters, identical exiting 1, missing artifact red rather than quiet. 2a comparing by field name, so no log carries endpoint, client id or scope. The canary on _canary/<server>, off the recovery path. Vault address, role and audience matching build-image.yaml. CLOUDFLARE_ACCOUNT_ID from the workflow env, so the bare detector invocation cannot block on stdin — still true against the merged detector (:1046).
The design argument holds and the departure from #1258 remains the better call. The question this merge raises is not whether the workflow is well built; it is whether there is anything left for it to run against.
Reviewed at
13313632by reading33b2c119...13313632— the PR's own nine files — and cross-checking them against the mergedscripts/portal-catalogue-drift.py,cloudflare-drift.yml,mcp-servers.tfand the portal README at that SHA. Shell execution andshellcheckare permission-gated in this sandbox, which thediff.sh:74note states as a limit on that finding; none of the findings above depends on running anything.
Closes the runnable half of #1258.
The runbook existed (#1241) and the detector existed (#1242), and between them a green detector and a red one both ended at "an operator reads a README". This is the thing that runs.
The constraint that decides the shape
Quoted from the Cloudflare docs, not inferred: a manual-OAuth server "remain[s] in Waiting status until a user completes upstream OAuth", and no API completes it. So the middle of the procedure is a person in a browser, and between the flip and that sign-in the server is down for every portal user.
That is why the obvious design — one job, one approval at the top — would be worse than the runbook it replaces. The runbook has an operator sitting at a terminal with the restoration body already built, so the window is minutes. A CI job that flips and then ends leaves the server in Waiting until somebody notices: the approver clicks Approve and walks away, and nothing bounds the outage.
What changed from the issue
#1258 asked for two approval gates. This has one, and a measurement.
The second gate was to mean "I have signed in".
status: readywith a movedlast_syncedis the same claim — measured rather than asserted, and it fails on a 30-minute timeout with a telegram alert, which an approval nobody clicks does not.last_syncedas well asstatus, because it is the only field this API moves on a real re-authorization; the README already records that as the single honest signal.That is a deliberate departure from the issue I filed, not an omission.
Three jobs
preflightflipcloudflare-applyenvironment approvalverifypreflightruns unprivileged and before the approval, the same splitcloudflare-apply.ymlmakes between plan and apply: an approval given before the current state is known approves an intention rather than a change.flipdoes steps 0 → 2a without a job boundary in them. From step 1 the live server no longer holds the registration, and a boundary is a place the run can stop with the server in a state nothing records.Details carried over rather than reinvented
200withsuccess: false, and has been measured answering200while silently keeping a field it was told to change.readOnlyHintis a structural claim this platform acts on, so a projection down to names and schemas would call a changed annotation "no change".last_syncedsays — the outcome the whole procedure exists to prevent.secret/platform/portal-resnapshot/<server>) before the flip, and the run proves that path is writable first. A workflow that assumed the policy exists would discover otherwise immediately after the flip, which is the one moment there is no way back. Not a workflow artifact: that publishes endpoints, client id and scope into the repository's artifact store, where it outlives the run.type: choiceconstrains the UI, not the API.Two things this deliberately does not do
It does not apply the tool allowlists. Those live with the servers that register the tools, because "this tool is safe to expose" belongs in the same diff as the tool — the rule this directory's README already states. A new tool still needs its owning repository's
scripts/portal-allowlist-apply.sh, and the final drift check is what says so rather than this workflow guessing. This is narrower than #1258's acceptance criterion, and it is narrower on purpose.It does not roll back. After step 1 the registration is gone, and "restore" is step 2, which lands in
Waiting, notReady. There is no path back to Ready without a human sign-in. Said out loud here and in the README, so the first failed run is not met with "why didn't it roll back".Before this can run
secret/platform/portal-resnapshot/<server>must be writable by this repository's Vault OIDC role (mctl-gitops). The canary step measures it and fails closed, so a missing policy costs a red run rather than a lost registration.First intended run:
server: api, to pick upmctl_get_lifecycle_ownership— upstream 76, portal 75 since 2026-09-13, which is what run 34860686859 reported.Long-term removal of the need for any of this is the DCR spike, mctlhq/.github#76; it is not a reason to defer this.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RUQTbQvUQjwbDFd7Nq9bvC