Repository navigation
Point subgraph-deploy at Ormi instead of Goldsky - #399
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
💤 Files with no reviewable changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe deployment task now uses an Ormi probe to check subgraph versions before deployment. The ChangesOrmi deployment
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant DeploymentTask
participant subgraph_deploy
participant rainix_static
participant OrmiQueryEndpoint
participant GraphCLI
participant OrmiDeploymentEndpoint
DeploymentTask->>subgraph_deploy: invoke deployment
subgraph_deploy->>rainix_static: probe subgraph name and version
rainix_static->>OrmiQueryEndpoint: POST _meta query
OrmiQueryEndpoint-->>rainix_static: deployed, missing, or failure response
rainix_static-->>subgraph_deploy: exit status
subgraph_deploy->>GraphCLI: deploy confirmed missing version
GraphCLI->>OrmiDeploymentEndpoint: deploy subgraph
Merge Risk: 🟡 Moderate · up to Resolve the deployment credential exposure before relying on this workflow in production CI. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The deployment flow limits credential inheritance and refuses deployment when the version check is inconclusive. No introduced security vulnerability was established, but production credential permissions, runner isolation, and concurrent deployment behavior remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 76.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
findolor
left a comment
findolor
left a comment
There was a problem hiding this comment.
Please address the two findings below before merging.
Local validation passed: all nine focused Bats tests, ShellCheck, the Nix deployment-task build, and mocked dry runs covering skip, deploy, and failure paths. No live deployment was attempted.
Address review on #399. - Tracing is off for the whole task, so ORMI_DEPLOY_KEY is never printed by set -x, including in the presence check. - The deploy key is removed from the environment of npm ci, git, the probe and graph build; only graph deploy receives it. - The skip decision moves from bash/jq into `rainix-static ormi-probe`. It deploys only on a confirmed-missing response; transport and HTTP errors (retried), unrecognised bodies and a wrong query base fail the task. - The probe validates the query base (https only) and the name/version labels before building the URL. - Task orchestration moves to lib/subgraph-deploy.sh with injectable tools, with bats tests for the skip/deploy/fail paths and key exposure. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pushed eac4e05, which fixes both review findings (replies on each thread). A security pass over the PR found the following; the first three are fixed in that commit.
Verification: |
|
@rain-marvin review |
|
🔎 Reviewing |
![rain-marvin[bot]](https://avatars.githubusercontent.com/in/5077017?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Opus 5.5 (Claude 1)
This PR moves the shared subgraph-deploy task from Goldsky to Ormi. It builds with the Graph CLI and deploys with ORMI_DEPLOY_KEY, SUBGRAPH_NAME and ORMI_QUERY_BASE. A new rainix-static ormi-probe subcommand decides whether a version is already live. It skips on a confirmed live answer, deploys only on Ormi's exact "missing" body, and fails the task on anything else. The deploy logic now lives in lib/subgraph-deploy.sh, with tracing off and the key removed from the environment of the other tools.
Overall read: good. Both earlier threads (trace leak, probe failure falling through to a deploy) are fixed properly. The probe classification is strict and well tested in Rust. I found no blockers. Three minor points: the new bats suite is never run by CI; the claim that only graph deploy sees the key does not fully hold, because graph deploy compiles the subgraph again; and one failed or unsynced version stops the deploy of every network after it. The cutover order with raindex#2894 is already noted in the description: raindex deploys fail between the two merges.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@rain-marvin review |
|
🔎 Reviewing |
There was a problem hiding this comment.
Claude Opus 5.5 (Claude 1)
This PR moves the subgraph-deploy task from Goldsky to Ormi. A new rainix-static ormi-probe subcommand checks if each <name>/<version> is already live. Only a confirmed-missing answer (exit 10) leads to graph deploy. Any other probe result, or a failed deploy, skips that network and makes the task fail at the end. The deploy key is copied into a local and unset from the environment before anything runs, tracing is off, and only the hardcoded Ormi deploy endpoint gets the key.
All five earlier threads are fixed at 28c1ac8, and the fixes hold. The panel found no blocker. There are two minor points: a broken or empty networks.json makes the task pass while it deploys nothing, and raindex deploys will fail in the time between this merge and the raindex migration. The documented residual exposure (the key in graph deploy argv, and trust in the subgraph's npm dependencies) is a fair trade. Note that unset also does not remove the key from /proc/<pid>/environ of the task shell or its ancestors, so the trust statement in the comment is the real boundary. The PR description test plan still names only subgraph-deploy-version.test.bats. It could also list the new subgraph-deploy.test.bats suite and the Rust probe tests.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@CodeRabbit review |
|
New deploys were still landing on Goldsky, which is how the Robinhood-era copies appeared. The Goldsky CLI stays on PATH because other repos still call it directly. Co-authored-by: Cursor <cursoragent@cursor.com>
Address review on #399. - Tracing is off for the whole task, so ORMI_DEPLOY_KEY is never printed by set -x, including in the presence check. - The deploy key is removed from the environment of npm ci, git, the probe and graph build; only graph deploy receives it. - The skip decision moves from bash/jq into `rainix-static ormi-probe`. It deploys only on a confirmed-missing response; transport and HTTP errors (retried), unrecognised bodies and a wrong query base fail the task. - The probe validates the query base (https only) and the name/version labels before building the URL. - Task orchestration moves to lib/subgraph-deploy.sh with injectable tools, with bats tests for the skip/deploy/fail paths and key exposure. Co-authored-by: Cursor <cursoragent@cursor.com>
- default-shell-test now runs subgraph-deploy.test.bats, so the key-leak and probe-gating regressions are caught. - graph deploy compiles the manifest itself, so drop the redundant graph build and pass --network to graph deploy. The key is moved out of the environment up front so no tool inherits it; the comment now states what is and is not protected (graph deploy's toolchain and argv). - A probe that cannot answer (e.g. a failed or syncing version) or a failed deploy no longer stops the remaining networks: it is recorded, never deployed on a failed probe, and the task fails at the end naming it. Co-authored-by: Cursor <cursoragent@cursor.com>
Its live consumers now deploy with graph deploy to Ormi, and subgraph-deploy never called it. Drop the derivation, its place in subgraph-shell and the default shell, the check-shell smoke test, the README pin, and the sol-shell slim assertion that only existed because it was in the default shell. Co-authored-by: Cursor <cursoragent@cursor.com>
95a2ff5 to
e2477a9
Compare
|
@rain-marvin review |
|
🔎 Reviewing |
There was a problem hiding this comment.
This PR moves the subgraph-deploy task from Goldsky to Ormi. A new rainix-static ormi-probe subcommand decides whether a version is already live, and only a confirmed-missing answer leads to graph deploy. The deploy key is taken out of the environment and kept out of trace output. The Goldsky CLI is removed from the shells, CI, README and slim test.
Overall this is in good shape. The probe accepts only Ormi's two known answers and refuses everything else. Retries cover only transport errors, 429 and 5xx. A failed probe or deploy marks that network as not done, and the task fails at the end. The bats suite now runs in CI. No new blocker was found.
One earlier point is still open in the code: the networks.json enumeration fix from a resolved thread is not in this head (comment below). There is also one small hardening note on the graph launcher. Merge order with rainlanguage/raindex#2894 still applies, as the description says.
claude-opus-5-5 · high · 17 min
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
## Summary - Deploy subgraph now passes `ORMI_DEPLOY_KEY`, `SUBGRAPH_NAME=raindex`, and the public Ormi query base into `subgraph-deploy`. - The Robinhood chain slug in `subgraph/networks.json` is `robinhood`, matching the network Ormi indexes. `graph build --network` writes that slug into the manifest, and the DecimalFloat lookup uses the same string. The deployment name for that chain is `raindex-robinhood`. - Depends on rainlanguage/rainix#399. Merge that one first. This workflow tracks rainix `main`, so dispatching before that merge still runs the Goldsky task while this workflow no longer supplies `GOLDSKY_TOKEN`. - `CI_GOLDSKY_TOKEN` is no longer read. Leave the secret in place until a dispatch has landed on Ormi, then remove it. Kais still needs to set the `ORMI_DEPLOY_KEY` GitHub secret from Vault `secret/infra/rain/ormi` (`deploy_key`) before a dispatch can succeed. Part of [DEVOPS-366](https://linear.app/makeitrain/issue/DEVOPS-366/raindex-ci-rainix-deploy-new-subgraphs-to-ormi-instead-of-goldsky). ## Test plan - [x] prettier and the other pre-commit hooks on the slug change - [ ] Merge the rainix PR, set `ORMI_DEPLOY_KEY`, then dispatch this workflow - [ ] Confirm each deployment is on Ormi, including `raindex-robinhood`, and the run log has no `api.goldsky.com` call <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Updates** * Updated subgraph deployment to use the Raindex configuration and Ormi query service. * Updated the supported network name to `robinhood`; its deployed contract address and indexing start point remain unchanged. * Subgraph data for this network continues to use the same contract deployment and indexing start point under the updated network name. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary
subgraph-deploynow builds with the Graph CLI and deploys to Ormi (subgraph.api.ormilabs.com) usingORMI_DEPLOY_KEY,SUBGRAPH_NAME, andORMI_QUERY_BASE.subgraph-shelland the default shell, thecheck-shellsmoke test, the README pin, and the sol-shell slim assertion. Its consumers now deploy withgraph deployto Ormi (gildlab/offchainAssetVault-subgraph, Deploy subgraphs to Ormi cyclofinance/cyclo.subgraph#66), and raindex runssubgraph-deploy.S01-Issuer/st0x-rewardsstill callsgoldskyand is left alone.Merge order
Merge this before rainlanguage/raindex#2894. That workflow calls
nix develop github:rainlanguage/rainix#subgraph-shell, which tracks rainixmain.Ormi already indexes Robinhood Chain. The chain slug is
robinhood(see the raindex PR), so this task deploys it asraindex-robinhoodalong with the other networks innetworks.json.Test plan
bats test/bats/task/subgraph-deploy.test.batsandsubgraph-deploy-version.test.bats;cargo testinrainix-static(ormi-probe)ORMI_DEPLOY_KEYexists on raindex, dispatch Deploy subgraph and confirm the deployment lands on OrmiPart of DEVOPS-366.
Summary by CodeRabbit