Repository navigation
Add production previews to the observability PR preview workflow - #6172
Conversation
The workflow pushed the changed dashboards to the staging Grafana only. It now pushes them to the production Grafana also. - The workflow applies the preview to both environments in one job. The job posts one sticky comment. The comment shows each changed dashboard one time, with one link for each environment. - The workflow applies staging first. The comment step runs also when an apply step fails. If production fails, the comment keeps the staging links and shows a warning. - The new script scripts/ci-apply-preview.sh holds the steps for one environment. Staging and production run the same code. - Each Grafana token stays in its own step. The workflow writes no token to $GITHUB_ENV. - The cleanup job deletes the preview from both environments when the PR closes. - The daily sweep workflow sweeps both environments. The production IAM role trusts repo:cardstack/boxel:* with no ref condition. The role grants the three SSM parameters that the preview reads. This change needs no infrastructure work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a57d7b1649
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A code review found that the previous design gave production credentials to pull-request-controlled code. GitHub loads a `pull_request` workflow, and every script that workflow calls, from the pull request itself. A contributor could therefore change those files and read the temporary production AWS credentials, the decrypted production grafanactl token, and GRAFANA_SECRET. GitHub always loads a `workflow_run` workflow from the default branch. The production preview moves there: - observability-preview.yml keeps staging credentials only. - observability-preview-production.yml applies the production preview, and removes it when the pull request closes. It restores every script from the default branch, then overlays only packages/observability/grafanactl/resources from the pull request. Those manifests are data, and no step runs them. - render-preview.sh fails now if a manifest that it emits lacks the `pr<n>-` prefix. A crafted manifest cannot address a canonical dashboard. - The production workflow asserts the production server URL before it reads a credential. - The production sweep refuses a ref other than main. The staging workflow posts a comment for staging. The production workflow rewrites that comment with one link for each environment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Host Test Results 1 files ±0 1 suites ±0 1h 41m 0s ⏱️ - 11m 47s Results for commit dbf918e. ± Comparison against earlier commit e11a2cd. Realm Server Test Results 1 files ±0 362 suites ±0 1h 56m 4s ⏱️ - 11m 1s Results for commit dbf918e. ± Comparison against earlier commit e11a2cd. |
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖]
This review went after the trust boundary the workflow_run split rests on, the teardown paths, and the load-bearing comments the two new workflows introduce. I traced the overlay step's effect on the working tree, the resolve-the-PR lookup across merged / closed-unmerged, and the pr<n>- assertion against the rendered tree; I did not exercise either workflow end to end, and the dashboard manifests themselves are unreviewed.
The split is the right shape and the pr<n>- assertion holds — it runs over every JSON in the rendered tree, not just the changed dashboards, so a manifest added under a new resources/ subdirectory also fails closed. Three asks below; none block, but two are comments that assert more than their mechanism delivers, which is the kind of thing the next author builds on.
On your question about GRAFANA_SECRET. Substitute a dummy for the production leg. The reason isn't secret exposure — canonical production dashboards already carry the real value in a constant template variable, so anyone with production Grafana access can read it today. It's that a preview dashboard is a PR-authored artifact whose operator buttons reindex realms, complete jobs, grant realm permissions and add credit against production, with nothing in the panel saying so; the confirm dialogs read identically to the canonical dashboard's. A reviewer opening a preview to check a layout change is one click from a production mutation. A dummy still renders every panel and still runs the customCode path — the button just comes back 401, which is the visible, safe outcome. Staging keeps the real secret, so an operator button added in a PR can still be exercised for real, one environment over. Mechanically this is a branch in ci-apply-preview.sh: skip the /production/boxel/GRAFANA_SECRET read and export a placeholder when ENV_NAME=production, which also drops one SSM path from what the production role needs.
Recommendations:
- The overlay leaves the pull request's version of everything outside
.githubandpackages/observabilityon disk; the header claims onlyresourcesis taken. A--head-refoption onrender-preview.shremoves the class — see the thread on the Overlay step. - A pull request closed without merging looks like it resolves to
found=false, leaving the production preview for the daily sweep — see the thread on the Resolve step; the README's "deleted when the PR closes" wants softening either way. - The sweep's non-main guard can't stop the threat its comment names, because
workflow_dispatchruns the chosen ref's copy of the guard — see the thread on that step.
Adjacent, out of scope: prefix_uid's right-truncation at 40 characters can map two distinct canonical UIDs onto one preview UID, silently dropping a dashboard from a preview that changes both. Pre-existing, and the new assertion doesn't see it because the truncated UID still carries the prefix. Worth a collision check whenever someone next touches that function.
Generated by Claude Code
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Went after the trust boundary from the other side than the review above: not what the overlay leaves outside the two restored trees, but what a pull request can leave inside them. Also swept the production job for the usual workflow_run holes. No end-to-end run.
One finding below; the rest of what I pushed on holds. Nothing after the overlay resolves uses: ./ — install-grafanactl runs while only default-branch code is on disk, and the two later actions are SHA-pinned externals. ci-apply-preview.sh and its callees are bash, aws and grafanactl only, invoked by explicit relative path: no mise, no pnpm, no node, so the PR-controlled repo root (package.json, .mise.toml, lockfile) is never read. Every PR-derived value reaches a run: block through env: rather than ${{ }} interpolation, so there is no template-injection path through the branch name or base ref.
On approving: I am not adding one, and the reason is the open thread above rather than anything I found. A preview dashboard is a PR-authored artifact that currently receives the real production GRAFANA_SECRET, so its operator buttons reindex realms and grant permissions against production behind confirm dialogs identical to the canonical dashboard's. That wants a decision before this merges, and it is yours rather than mine.
The preview workflow runs on `pull_request`. GitHub loads that workflow, and every script it calls, from the pull request. A contributor controls the code that runs after the job assumes the role. The previous commit fenced the code. It moved the production push into a `workflow_run` workflow that GitHub loads from the default branch. That protected the credential, but it added a second workflow, and it stopped the production preview from running before merge. This commit removes the asset instead. The preview assumes boxel-observability-preview, which grants two parameters for each environment: - /<env>/grafana/grafanactl_token - /<env>/boxel-grafana/realm_server_url The worst case now equals the feature itself: write `pr<n>-` dashboards. - render-preview.sh stamps an inert placeholder into `grafana_secret`. It ignores an exported GRAFANA_SECRET, so a caller with wider credentials cannot leak the real value into a preview. Operator buttons on a preview dashboard return 401. - ci-apply-preview.sh no longer reads /<env>/boxel/GRAFANA_SECRET. - observability-preview-production.yml is deleted. One workflow applies both environments again, and one comment links both. - The daily sweep uses the same narrow role. render-preview.sh keeps the assertion that every manifest it emits carries the `pr<n>-` prefix. cardstack/infra adds the role. Merge that change first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Re-review of e25f927 only: what the narrower-role design really takes away from the pull request, and where the earlier threads now stand. I didn't do an end-to-end run.
One blocking finding. The preview role's Grafana token isn't scoped to previews, so it can still reach canonical production dashboards and GRAFANA_SECRET. Details are in the inline comment on the env: block in observability-preview.yml.
Earlier threads:
- Overlay leaves PR-added files inside
.github/packages/observability: resolved (the workflow is gone). I replied in that thread. - Overlay leaves the PR's repo root outside the restored trees: resolved (same reason).
commits/{sha}/pullsmisses closed-without-merge PRs: resolved. Cleanup now reads the PR number from thepull_requestpayload, so the README's "both previews are deleted when the PR closes" holds.- Sweep's "refuse non-main ref" guard justification: still open. The comment still says it stops a ref "whose copy of this workflow was rewritten", and the diff didn't touch that comment.
- Operator buttons on a preview acting against production: resolved as far as the preview dashboards go (placeholder secret, 401). The secret's other route in is the new finding.
A preview whose operator buttons return 401 cannot show a reviewer whether a button change works. Those buttons are part of what the preview exists to show. The preview role stays narrow. It grants three parameters for each environment, and it still excludes the Grafana database credentials: - /<env>/grafana/grafanactl_token - /<env>/boxel-grafana/realm_server_url - /<env>/boxel/GRAFANA_SECRET The operator token is weaker than it looks. apply.sh substitutes it into six canonical dashboards as a `constant` template variable, so every Grafana viewer can already read it from the dashboard JSON. A pull request that reads it from CI gains nothing that a dashboard viewer does not have. CAUTION: the buttons on a preview dashboard act on the real environment. A button clicked on a production preview performs the production mutation. render-preview.sh keeps the assertion that every manifest it emits carries the `pr<n>-` prefix. cardstack/infra adds the parameter to the role. Apply that change first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T6hzc2RPYWnjG8xPbmYhf4
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Re-review of the operator-secret change: what the README and the PR description now promise next to what the preview actually carries. I did not do an end-to-end run.
Two blocking findings. The README still describes inert previews, and the deferred fix has no ticket. The grafanactl-token thread on the role's env: block is resolved: the false "worst case" claim is gone, and since main needs no approval to merge, the PR doesn't add reach.
packages/observability/README.mdstill says the role grants two parameters and that preview buttons return 401. See the inline comment. Regression, blocking.- The description defers the durable fix: stop embedding
GRAFANA_SECRET, proxy operator calls through Grafana, then drop this grant. That needs a Linear ticket linked from the description before merge (AGENTS.md, PR Instructions). Blocking.
The preview used /<env>/grafana/grafanactl_token. That token belongs to an Admin service account, and nothing in Grafana scopes it to `pr<n>-` UIDs, so a pull request holding it could overwrite or delete any canonical dashboard. It could also read GRAFANA_SECRET without a grant, because apply.sh substitutes the live secret into six canonical dashboards and cleanup-preview.sh pulls every dashboard on each close. The preview now authenticates as `observability-preview`: Viewer, plus Admin on the `Previews` folder and nothing else. - grafanactl-env.sh takes GRAFANA_TOKEN_SSM_PATH. It still defaults to the Admin parameter, so the canonical tooling is unchanged. - apply-preview.sh and cleanup-preview.sh set that path to /<env>/grafana/preview_grafanactl_token. Both are preview-only entry points, so every caller inherits it, including the daily sweep. - render-preview.sh nests each per-PR folder under `Previews`. The scoped account can create a folder only inside that parent, so a root-level preview folder is refused. - The UID assertion now reads the manifest kind. A dashboard must sit in this pull request's own folder; a preview folder's only permitted parent is the shared `Previews` one. Measured end to end against staging with the scoped credential alone: the apply pushed one folder and five dashboards into `Previews`, the canonical Overview dashboard stayed at its existing version, and the cleanup removed all six with no errors. A pull request can still read every dashboard, and so can still read GRAFANA_SECRET. That half closes when operator calls move to a Grafana datasource proxy. The README and the config comments say so rather than implying the preview is fully fenced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T6hzc2RPYWnjG8xPbmYhf4
A review pointed out that the comment claimed more than the step delivers. `workflow_dispatch` runs the chosen ref's copy of this workflow, so it runs the chosen ref's copy of the guard too. The guard cannot stop a rewritten workflow. It does stop an operator dispatching a stale but honest branch, which is a real mistake, so the step stays. The comment now says that, and points at the control that does limit a rewritten copy: the scoped Grafana account the sweep assumes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T6hzc2RPYWnjG8xPbmYhf4
|
[Claude Code 🤖] Correcting the record on the Codex thread ( What shipped is neither of them. The exfiltration path Codex identified is closed by narrowing what the credential reaches, not by fencing the code:
Two corrections to what I wrote in that thread. I said the role would not grant I also said the worst case was "write Residual, stated plainly: a pull request can still read every dashboard, and so can still read |
What this changes
The PR preview pushed the changed dashboards to the staging Grafana only. It now pushes them to the production Grafana also, and the PR gets one comment rather than one for each environment:
The credential question
This workflow runs on
pull_request. GitHub loads it, and every script it calls, from the pull request itself. A contributor controls the code that runs after the job assumes the role. Whatever that role can read, a pull request can read.So the preview uses a dedicated role,
boxel-observability-preview, granting three parameters per environment:Not
boxel-observability-apply, which also grants the Grafana database credentials.GRAFANA_SECRETis in the list deliberately. A preview whose operator buttons return 401 cannot show a reviewer whether a button change works, and the token is already readable by every Grafana viewer:apply.shsubstitutes it into six canonical dashboards as aconstanttemplate variable, andhide=2hides it from the variable bar rather than from the dashboard JSON. Withholding it from CI protects nothing that is not already exposed. The durable fix is to stop embedding it and serve operator calls through a Grafana datasource proxy; this grant should be dropped once that lands.Caution for reviewers: preview buttons act on the real environment. A button clicked on a production preview performs the production mutation. Prefer the staging preview for exercising one.
A preview still cannot address a canonical dashboard:
render-preview.shrewrites every UID and then fails closed if any manifest lacks thepr<n>-prefix.Tests
actionlint,prettier --checkandshellcheckpass.github,contextandcoreobjects over four conditions: both environments, production failed, no dashboard changed, and both failed.render-preview.shagainst a real five-dashboard change substitutes the supplied operator token and realm-server URL, and emitspr999-UIDs for every dashboard and the folder.prefix_uidfails the render with exit 1, so the UID assertion is not vacuous.Still open
This branch is behind
main. I have not mergedmainin, because that merge touches files carrying uncommitted local work.🤖 Generated with Claude Code
https://claude.ai/code/session_01T6hzc2RPYWnjG8xPbmYhf4