🐛 Don't let chart-sync overwrite production edits to charts a branch created - #6766
Open
pabloarosado wants to merge 22 commits into
Open
🐛 Don't let chart-sync overwrite production edits to charts a branch created#6766pabloarosado wants to merge 22 commits into
pabloarosado wants to merge 22 commits into
Conversation
Contributor
|
Quick links (staging server):
Login: chart-diff: ✅No charts for review.data-diff: ✅ no differencesNo differences found. Automatically updated datasets matching excess_mortality|covid|fluid|flunet|country_profile|garden/ihme_gbd/2019/gbd_risk are not included. Run locally with Edited: 2026-08-27 18:21:42 UTC |
…ated A chart created on a staging branch has no counterpart in the target when it is reviewed, so its approval records no target version and, unlike an existing chart's, never goes stale. The target's own ETL creates the chart after the merge; if someone edits it in the target admin before chart-sync runs, the approval still matches and the sync overwrites them with no warning. Compare the two authored layers before updating a chart matched across environments: any key the target's layer carries and the source's doesn't is an edit made there, and syncing would drop it. Refuse and notify instead. Keys both sides carry are left alone — the source wins there by design, and that is the change the reviewer approved. Reproduced end to end against a staging server with a copy of production as the target: chart-sync offered to overwrite the target's edit before this change and reports it instead afterwards, while a new chart with an untouched target still syncs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pabloarosado
force-pushed
the
chart-sync-protect-new-charts
branch
from
August 26, 2026 19:36
c0030fa to
7685474
Compare
The guard reused the pending-chart Slack message, which describes a different situation: nothing is stale here, the approval is valid, and what stops the sync is an edit made in the target's own admin. Give it its own message, naming the fields that would have been dropped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds protection against chart-sync overwriting target-side edits made after an ETL-authored chart is created.
Changes:
- Loads charts’ authored patch configurations.
- Detects target-only patch fields before syncing.
- Adds conflict notifications and regression tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
etl/grapher/model.py |
Loads authored chart layers. |
apps/wizard/app_pages/chart_diff/chart_diff.py |
Detects fields lost during sync. |
apps/chart_sync/cli.py |
Blocks unsafe syncs and notifies Slack. |
tests/test_grapher_model.py |
Tests authored-layer loading. |
tests/apps/wizard/app_pages/chart_diff/test_chart_diff.py |
Tests lost-field detection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 441376a859
ℹ️ 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".
The unit tests cover the comparison functions; they cannot cover what chart-diff lists and what chart-sync writes against a real grapher database, which is where the bugs in this area have been. This runs the six situations end to end on a staging server, against a copy of its own database standing in for production. Manual: it needs Tailscale and mutates the staging database (it creates and drops a copy, edits one chart and restores it, creates and deletes another). Not a pytest integration test for that reason -- `make test-integration` also runs on staging servers in CI, where this would fire unattended against someone's data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two gaps found in review: Nested fields were compared as whole objects, so a target holding `yAxis.min` and a source holding `yAxis.max` looked like a shared `yAxis` and the target's half was dropped silently. Compare field by field and report dotted paths. Tags were not considered at all. `set_tags` replaces the target's set wholesale, and grapher's `setChartTags` rewrites `chart_tags` without touching `charts.updatedAt`, so a tag added in the target moves no timestamp and nothing else would notice it going missing. Treat a target-only tag as something the sync would drop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A field deleted in SOURCE is indistinguishable, from the target's side, from one added in TARGET: both leave the target holding something the source lacks. So a deliberate removal on a chart that already exists in the target trips the guard. That fails safe -- the sync is refused and the field named -- but it needs a way through, and `--ignore-conflicts` already means exactly "sync approved charts even when conflicts are detected". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The escape hatch had no test, and neither did the case that motivated it. S5 now also runs the sync with --ignore-conflicts and expects it to go through, and S6 deletes a field on staging that the target still has, checks that the sync is refused, and checks that the override gets past it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deleting the field changed the source chart, which invalidated the earlier approval, so chart-sync reported the chart as pending and never reached the guard the check was about. Approve again after the deletion, which is the real order of events anyway: make the change, then approve it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A new ETL chart is created as a draft, and publishing it is only possible through the admin. So publishing on staging is the ordinary workflow, and it is what makes the staging copy differ from the one the target's ETL builds. Using it as S5's setup, instead of a hand-typed footnote, tests the case that will actually occur. S6 needs a field on both sides to delete, so it now sets one up explicitly, standing in for a field that an earlier sync had carried over. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The scenario reads as though deleting a field on a branch gets refused, which is not true of ordinary work: on any later branch the chart carries the target's own id, so the guard never runs and a deletion syncs like anything else. Only the staging server that created the chart can reach it, and only if someone runs chart-sync there a second time by hand. Name it accordingly and explain why it exists at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The staging admin restarts from time to time, and calls to it return 502. When that killed a run mid-way, the test chart stayed on staging, the next run copied it into the production stand-in, and S5 then measured a chart that already existed there -- reporting a failure that said nothing about the code. Remove any leftover test chart before taking the copy. Approving is now one helper that says plainly when a chart is not listed, instead of an IndexError several steps after the real problem. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The script tested the situations this PR changes and the corner cases around
them, but not the ones people meet daily. Added:
S7 a dataset update: the data changes, the chart's settings don't. Listed for
review, and chart-sync writes nothing, since the data reaches the target
through the target's own ETL run.
S8 the same for indicator metadata.
S9 the chart is repointed at a different indicator, as a re-versioned dataset
does. A settings change like any other: listed, and synced.
S10 a chart made by hand on staging. Chart-sync creates it in the target
instead of updating one, a path none of the other scenarios touch.
Each scenario now prints its number, elapsed time and a rough estimate of what
is left, since a full run takes several minutes and most of it is silent.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
S7 asserted that a data-only change is not listed as a settings change, but by then the chart carried the settings changes S2, S3 and S4 had made, so it was listed as both and the check failed on the leftovers rather than on the behaviour. Reset the chart on both sides first, and check that reset held. S10's hand-made chart was rejected by the admin with "Schema missing": a config sent to the chart API needs $schema, which the ETL push path fills in but the create path does not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sees one chart-sync said nothing at all for a chart it would create: the update path logs an event, the create path only counted. A dry run therefore reported nothing for new charts, which is when you most want to know. Log it, symmetrically. S7 and S8 changed only the indicator's checksum, and chart-diff never showed the chart. Two filters it has to get past first: the dataset's dataEditedAt must be newer than the staging server, and the indicator's dataset must appear in the branch's git diff, so that charts are not listed merely because the branch lags master. The first is database state and is now set; the second is the working tree, so it is stood in for, and the comment says that filter is not covered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
S7 was called "a dataset update", which describes S9. The everyday update bumps a dataset to a new version folder, which mints new indicator ids; the chart then plots a different id, and chart-diff shows a settings change, not a data change -- the new indicator has no counterpart in the target, so the data comparison drops it, since it joins the two sides on catalog path. S7 is the other kind: a dataset whose version does not move, so the indicator keeps its id and only its numbers change. Real for `latest` datasets and for a corrected source re-snapshotted into the same version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Chart-diff does two different things with a chart. If it changed only because ETL changed its data or metadata, there is nothing to approve or reject -- the change reaches the target through the target's own ETL run either way -- so it asks whether you reviewed it. If its settings changed, or it is new, someone has to decide whether to copy those settings, so it offers approve/reject. The scenarios described this in prose but never checked it, and the distinction is what a data scientist actually sees. Assert it where it is the point: a settings change and a new indicator id ask for approval, a data or metadata change is review-only. Every listed chart now prints its control. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The cleanup was meant to remove both test charts, but that edit never landed -- it was applied without checking it matched, so the hand-made chart survived every run. The next run then failed at S10 with "this chart slug is in use", which says nothing about the behaviour under test. Delete both charts at the end, and at startup remove leftovers by slug as well as by UUID, so a run that dies mid-way cannot break the following one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copying a chart config file copies its chart_config_id, which is the chart's identity in grapher. Both steps then own that chart: each build overwrites the other's config and flips charts.etlConfigCatalogPath to whichever ran last. Nothing errors, so the symptom is a live chart quietly changing depending on build order. Found by doing exactly that: a copied config retitled banning-of-chick-culling on a staging server and took ownership of it, while the preview 404'd because the slug comes from the file name and the identity comes from the UUID. Grapher cannot catch this, since each push is a valid API call on its own, so scan the step configs and assert the declared UUIDs are unique. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The error told people to hand-run a python one-liner to mint a UUID, or to write a JOIN against the grapher DB to find an existing chart's. `etl chart-config-id` does both, and writes the result into the file, which the one-liner does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two fields in a chart config YAML can be edited with no effect and no signal: - topic_tags apply only when ETL creates the chart. On a chart ETL adopted, tags are admin-managed and the push ignores them. - the slug is derived from the file's short name, but grapher excludes slug from what an ETL layer may contribute, so an existing chart keeps the slug it already had. Renaming the config file looks like it renames the chart and doesn't. Both behaviours are right; the silence isn't. Log a warning in each case, naming what was asked for, what happened instead, and why. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pabloarosado
marked this pull request as ready for review
August 27, 2026 18:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human summary
This PR fixes an edge case and adds a few warnings to cover uncommon cases. It also adds some unit tests and a script with further tests to check the behavior of chart sync in various scenarios (from common situations to edge cases).
Stacked on #6511 "Charts as first-class citizens in ETL, addressed by chart config UUID". ETL only.
The hole
Chart-diff already protects an existing chart from being overwritten: the approval records the target chart's version, and chart-sync re-checks it, so an edit made in production after you approved makes the approval stale and the sync is refused.
A chart created on a branch has no such baseline. At review time production has no such chart, so the approval is stored with no target version, and for a chart matched across environments that value is never filled in. After the merge, production's own ETL creates the chart (same config UUID, its own numeric id). If someone edits it in the production admin before chart-sync runs, the approval still matches, no conflict is raised, and the sync overwrites them silently.
The fix
Before updating a chart matched across environments, compare what each side authored. Anything the target carries and the source does not is an edit made in the target's admin that syncing would drop: refuse, and name it. A chart the ETL has just created carries only the bootstrap
slug, so the normal case is unaffected.Two things are compared:
charts.patchConfigId's row), field by field rather than as whole objects, since each side can hold a different corner of the same object (yAxis.minin the target,yAxis.maxin the source);set_tagsreplaces the target's set wholesale and grapher'ssetChartTagsrewriteschart_tagswithout touchingcharts.updatedAt, so a tag added in the target moves no timestamp and nothing else would notice it vanish.A field both sides carry is deliberately not reported: the source wins there by design, and that difference is the change the reviewer approved.
--ignore-conflictsoverrides the guard, for the case where a field deliberately deleted on the branch is indistinguishable, from the target's side, from one added there.Also in here
Found while testing the above, each small and each with its own commit:
chart_config_id. Copying a chart config file copies the chart's identity, so both steps own that chart and each build overwrites the other. Found by doing it by accident: a copied config retitled a live chart on staging and took ownership of it.topic_tagsapply only when ETL creates the chart, and an existing chart's slug cannot be changed from ETL. Both behaviours are right; the silence was not.chart_config_iderror now points atetl chart-config-id, which writes the value into the file, instead of a python one-liner and a hand-written SQL join.Verified end to end
scripts/chart_sync_scenarios.pyruns against a staging server with a copy of its own database standing in for production. Eleven scenarios, 37 checks, all passing on this branch:--ignore-conflictsgets throughThe S5 control matters as much as S5: with production untouched the new chart still syncs, so the guard does not block the normal path. S7 to S9 assert which control chart-diff offers, since that distinction is what a data scientist actually sees.
The script is manual, not a pytest integration test: it needs Tailscale and it mutates the staging database.
make test-integrationalso runs on staging servers in CI, where this would fire unattended against someone's data.Unit tests cover the comparison function (bootstrap layer loses nothing, a target-side edit is reported, bookkeeping and shared keys ignored, nested fields, missing layers) and that
load_patch_configreads the authored row rather than the rendered one. Each was checked to fail when the code it covers is deliberately broken.Not covered
setChartTagsmoves no timestamp, a tag added in production after you approve is deleted by the sync for any chart, not only the case fixed here. Out of scope, worth its own look.dataEditedAtis set for real; the git-changed-paths filter is monkeypatched, and the comment says so.