feat(omnigraph): stamp the storage format onto both images as a label - #358
Merged
Merged
Conversation
ol-infrastructure deploys these images by digest and has no access to this repo, so the on-disk format a given image reads was opaque to the Pulumi program. Merging agent-kit#345 shipped a format-9 image into a cluster whose config still said fmt6, and the mismatch surfaced as the cluster-apply Job dying mid-deploy (CI 2026-09-16, builds 187/188/189) rather than as a refused preview. Both Dockerfiles now declare `OMNIGRAPH_INTERNAL_SCHEMA` alongside the version, tag and digest pins and emit it as `edu.mit.ol.omnigraph.internal-schema` on the runtime stage. That gives ol-infrastructure's `validate_internal_schema_version` a third party to compare against: the image actually being deployed, not just the two committed config values. `just check-omnigraph-pins` covers the new declaration the same way it covers the other three. A label that drifts from `_OMNIGRAPH_INTERNAL_SCHEMA` is worse than no label, because the Pulumi check would then compare the stack against a number this repo does not believe and pass. Verified by building docker/omnigraph-server.Dockerfile: the image carries `edu.mit.ol.omnigraph.internal-schema: "9"`, and the `omnigraph` binary copied out of it reports storage format 9 to bin/check_omnigraph_format.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vG9XvMbsQrjxW72Jp2Bqx
… label The pin check compared the global `ARG OMNIGRAPH_INTERNAL_SCHEMA=9` across the three files but nothing else. Docker scopes a global ARG out of every stage, so deleting the runtime stage's bare re-declaration expands the label to the empty string and still builds a clean image. ol-infrastructure reads an unparseable label as "image predates the label" and skips the check, so the gate would be off with nothing failing anywhere. Verified by building: with the stage ARG removed, `check-omnigraph-pins` passed and the image carried `edu.mit.ol.omnigraph.internal-schema=""`. The check now also requires the stage re-declaration and a LABEL line emitting it from the ARG, in both Dockerfiles. Each guard was confirmed to fire. Also: witan.Dockerfile points at omnigraph-server.Dockerfile for the shared rationale rather than carrying a second copy of it, matching what it already does for OMNIGRAPH_VERSION; the comments no longer name an ol-infrastructure function that is not merged yet; and renovate.json's omnigraph rule says three declarations rather than one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vG9XvMbsQrjxW72Jp2Bqx
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The validation guards are not scoped to the runtime stage and can falsely pass for an unlabeled final image.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds storage-format labels to Omnigraph-based images so infrastructure can validate image compatibility before deployment.
Changes:
- Labels both runtime images with storage format 9.
- Extends pin validation to cover schema declarations and labels.
- Updates Renovate guidance for future upgrades.
File summaries
| File | Description |
|---|---|
docker/omnigraph-server.Dockerfile |
Adds the storage-format label. |
docker/witan.Dockerfile |
Adds the matching label to Witan. |
justfile |
Validates schema pins and label emission. |
renovate.json |
Documents required manual schema updates. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both guards grepped the whole Dockerfile, so an `ARG OMNIGRAPH_INTERNAL_SCHEMA` declared in any other stage satisfied them while the runtime stage still expanded the label to the empty string. That is the arrangement the guards exist to reject, so the file-wide grep went green on it. They now read only the `FROM ... AS runtime` block, and a file with no such stage fails rather than checking nothing. Verified by moving the bare ARG from the runtime stage into the fetch stage: caught. Reported by Copilot on #358. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vG9XvMbsQrjxW72Jp2Bqx
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.
What are the relevant tickets?
tk-make-an-armed-storage-format-migration-self-cons-f9c9c5, item 1 of 3. The other two are ol-infrastructure changes that consume this label.
Description (What does it do?)
ol-infrastructure deploys
omnigraph-serverandwitanby digest and has no access to this repo, so the on-disk storage format a given image reads is opaque to the Pulumi program. Its preview check says as much: it "does NOT verify either value against the image actually being deployed". Merging #345 shipped a format-9 image into a cluster whose config still said fmt6, and the mismatch surfaced as the cluster-apply Job dying mid-deploy (CI 2026-09-16, builds 187/188/189) rather than as a refused preview.Both Dockerfiles now declare
OMNIGRAPH_INTERNAL_SCHEMAalongside the existing version, tag and digest pins and emit it on the runtime stage asedu.mit.ol.omnigraph.internal-schema. Reading that label back out of ECR gives the Pulumi-side check a third party to compare against: the image actually being deployed.just check-omnigraph-pinscovers the new declaration the same way it covers the other three, and also covers the two lines that actually produce the label. Docker scopes a global ARG out of every stage, so a runtime stage that does not re-declare it expands${OMNIGRAPH_INTERNAL_SCHEMA}to the empty string and still builds a clean image, which ol-infrastructure reads as "predates the label" and skips. Without that second guard the gate could be switched off with nothing failing anywhere.No behaviour change to either image beyond the added label.
How can this be tested?
just check-omnigraph-pinsprintsomnigraph pins agree: 0.11.0 (tag v0.11.0, sha da192e1a0508…, format 9). Each guard was confirmed to fire by mutating the tree and restoring it:ARG OMNIGRAPH_INTERNAL_SCHEMA=8in one Dockerfile: drift failure naming all three files._OMNIGRAPH_INTERNAL_SCHEMA: the moved-or-renamed failure, rather than passing on three empty strings.ARG OMNIGRAPH_INTERNAL_SCHEMA: caught. Before this guard existed, that mutation passed the check and produced an image carryingedu.mit.ol.omnigraph.internal-schema=""(verified by building it).docker build --checkis clean on both files. Buildingdocker/omnigraph-server.Dockerfileproduces an image whosedocker image inspectshowsedu.mit.ol.omnigraph.internal-schema: "9", and theomnigraphbinary copied out of that same image reportsomnigraph 0.11.0 reads storage format 9, as declared.tobin/check_omnigraph_format.py. The label and the binary inside one image agreeing is the property the Pulumi check relies on.docker/witan.Dockerfilewas linted and its label lines checked, but not built. It uses the same ARG scoping as the one that was.🤖 Generated with Claude Code
https://claude.ai/code/session_016vG9XvMbsQrjxW72Jp2Bqx