feat(sidecar): build the node sidecar from this repo as its own module by bdchatham · Pull Request #504 · sei-protocol/sei-k8s-controller

bdchatham · 2026-08-13T19:53:34Z

Moves the sidecar out of the seictl repo into this one as a third Go module and a second binary/image, and makes the image own its own entrypoint. Phase 3 of the extraction; the contract module (sidecarapi) landed in #503.

What changes


`sidecar/`	new module: `tasks/ engine/ server/ s3/ shadow/ rpc/ actions/` + `main.go`, `serve.go`. Carries the sei-chain graph and restates all 11 of sei-chain's replaces
`sidecarapi/tomlpatch/`	`seictl/internal/patch`, which `sidecar/tasks` needs and Go will not export across repos
pod spec	no `Command` for the sidecar container — the image's `ENTRYPOINT` is the command
CI	`publish-sidecar` job, PR-time builds of both images, `MODULES`-driven test/lint/tidy fan-out

sei/sei-sidecar in ECR exists (sei-protocol/platform#1535, applied and verified).

Deploying this is a coordinated change, and it rolls the whole cell

Read this before touching a cell. The controller renders no Command, so the image's entrypoint is the command, and image + config + controller must move together:

clusters/<cell>/sei-k8s-controller/controller-config.yaml — images.sidecar
clusters/<cell>/sei-k8s-controller/manager-patch.yaml — controller image tag

Both in one commit. Flux applies them in one reconcile, and the controller renders Image and the absent Command into the same container spec, so the pod spec is never half-updated.

The failure directions are asymmetric. A new controller against an old seictl image gets a container that prints help and exits 0, restarting forever while seid waits on /v0/healthz behind a FailureThreshold: 86400 probe — silent for about five days. An old controller against the new image fails immediately with CreateContainerError. Only the second is survivable, so never land the config edit without the controller.

There is a delayed-fuse variant worth knowing: images.sidecar is read once at startup, so if the ConfigMap lands and the Deployment rollout fails, nothing appears to happen — the old controller keeps its in-memory value and the break arms itself for the next pod restart. Karpenter consolidation makes that restart real. Converting controller-config.yaml to a configMapGenerator would remove this by forcing the Deployment to roll with the ConfigMap; that's platform-repo work and not in this PR.

⚠️ The cutover rolls every SeiNode in the cell at once. Sidecar-image drift builds a NodeUpdate plan per node (internal/planner/planner.go:750-755), replace-pod deletes pods at the old revision, and there is no throttle — no MaxUnavailable, no concurrency limit anywhere in the planner. In prod that is 30 SeiNodes including pacific-1 mainnet validators, simultaneously. Rollback re-creates drift in the opposite direction and costs a second full-cell roll.

Suggested order by blast radius: dev (0 SeiNodes) → harbor (3, plus the nightly harness exercises the bootstrap path) → prod-use2 (14) → prod-euw1 (13) → prod (30). Verify the tag exists first: aws ecr describe-images --repository-name sei/sei-sidecar --image-ids imageTag=<sha>.

A simultaneous pod replacement of every pacific-1 validator needs human sign-off and a serialization plan before prod is touched. That is not resolved by this PR.

Decisions worth challenging

No seictl compatibility symlink. An earlier revision shipped one so the image and controller could roll independently. Removed: no live node sets spec.sidecar.image, so images.sidecar is the only source and the cutover is atomic per StatefulSet. The cost is that the coordination above is mandatory rather than optional.

amd64 only. The sei-node Karpenter pool pins kubernetes.io/arch In [amd64], so no node can schedule an arm64 sidecar, and all three existing publishers are single-arch. An earlier revision built arm64 to preserve what the GHCR build produced; that would have made this repository the registry's first multi-arch image index, under an IMMUTABLE tag policy whose only lifecycle rule expires untagged manifests — an index's children — at 7 days. Untested, and unrepairable at the same tag.

tomlpatch lives in the contract module. Both the sidecar and the seictl CLI need it, and neither may import the other. A copy in each would recreate the drift-across-a-boundary problem this extraction exists to remove. This does widen sidecarapi past "the wire contract".

Dependency floors match what seictl shipped, exactly. A fresh go mod tidy resolves the minimum each import allows, which silently rebuilt the binary against different versions — 35 linked modules differed. Sixteen AWS SDK modules were ahead, including the credential chain and service/internal/checksum (S3 checksum defaults on the snapshot path); nineteen were behind, including x/crypto, x/net, grpc and otel, because seictl's module also linked the controller via sdk/sei and MVS pulled them up. All 35 pinned; drift is zero.

Two startup refusals

Both replace failures that were silent, and both are load-bearing — do not soften them into defaults:

SEI_HOME must be set and absolute. An empty or relative value resolves every path against the working directory; the sidecar starts, serves, and writes config/, data/ and sidecar.db wherever it happens to be running. Validation sits in the flag's Action, not a Before hook — urfave runs Before ahead of its own required-flag check, so a hook makes Required unreachable and misreports an unset variable as set-but-empty.
A configured keyring with unauthenticated authn is refused, before the keyring opens. SEI_SIDECAR_AUTHN_MODE unset means bind all interfaces with no middleware, so one dropped variable exposed POST /v0/tasks — gov-vote included — to any pod in the cluster.

Test plan

gofmt, goimports, builds, make tidy-check, make verify-generated clean across all three modules
14 root + 3 sidecarapi + 8 sidecar test packages pass
Controller build closure: 0 sei-chain / cosmos-sdk / cometbft / gogo packages
Fork pins resolve: sei-protocol/go-ethereum v1.15.7-sei-16, regen-network/protobuf v1.3.3-alpha.regen.1
Module drift against seictl's shipping build list: 0
SEI_HOME unset / empty / whitespace / relative all refuse with no files created; absolute accepted and normalized
Keyring-without-authn refused, naming both variables
Both images build — the docker job passes in ~8 minutes

Known-open

lint is red on three legs — lint (.), lint (sidecarapi), and lint (sidecar) with ~347 findings in code seictl never linted (it carries no golangci-lint config). Being addressed separately.
verify-generated does not cover sidecarapi/api's go:generate, so the OpenAPI spec can drift from its generated client silently.
GOWORK=off is set in the Dockerfile and build-sidecar but not in test-modules, tidy-check or lint — a contributor who creates a local go.work gets a different graph there, and a workspace breaks the controller build with an error naming OTel and gRPC.
Metrics keep the seictl_* prefix and loggers the seictl component name, deliberately — renaming breaks every dashboard and alert. Not recorded anywhere as an invariant until now.
.agent/runbooks/ still documents seictl serve.
Phase 4 — seictl drops its sidecar tree and reimplements report's S3 read. Until then seictl keeps publishing its own copy of the sidecar from every push to main, and its seictl task CLI must stay wire-compatible with this server.

Review

Reviewed by systems-engineer, platform-engineer and idiomatic-reviewer (blinded, independent, rotating assigned dissent) across two rounds, plus Cursor Bugbot. Round 2 found the credential widening, the arm64 decision and the dependency-floor claim all wrong, and a Before-hook ordering bug that made Required unreachable — all corrected in 46b625e. Ledger in bdchatham-designs.

🤖 Generated with Claude Code

cursor · 2026-08-13T19:53:41Z

PR Summary

High Risk
Large relocation of runtime-critical sidecar logic plus a coordinated deploy requirement (image entrypoint vs controller pod spec) where mismatch can fail nodes quietly; full-cell sidecar rolls are expected when images.sidecar changes.

Overview
In-repo sei-sidecar. Adds the sidecar/ Go module (task engine, SQLite store, handlers, Dockerfile with ENTRYPOINT … serve) and wires the repo to build and test it via MODULES, build-sidecar, and .dockerignore re-includes for embedded assets like tasks/defaults/config.toml.

Controller contract change. Stops rendering Command: ["seictl", "serve"] for the sidecar in noderesource and bootstrap paths so the container runs the image entrypoint (sei-sidecar serve). CRDs, samples, and docs now stress that images.sidecar must move with the controller image and that per-node image pins are risky.

CI/CD. Lint runs per module (including sidecar) with --new-from-merge-base, shallow-fetch fixes, and workflow concurrency; a PR docker job builds controller and sidecar images; ecr.yml gains a separate publish-sidecar job (amd64, isolated cache). .golangci.yml anchors path exclusions and scopes pre-existing sidecar/ lint debt.

^{Reviewed by Cursor Bugbot for commit b98df1d. Bugbot is set up for automated code reviews on this repo. Configure here.}

Phase 3: the sidecar implementation moves out of the seictl repo and becomes a second binary built here. seictl keeps its CLI; what it loses is the server the controller actually talks to. sidecar/ carries the sei-chain graph and nothing else does. It restates all 11 of sei-chain's replaces, not the one the plan for this originally claimed — two of them matter beyond resolution. 99designs/keyring is substituted for cosmos/keyring, and that is the library which decrypts the validator operator keyring, so a partial port would silently change key handling. go-ethereum redirects to the Sei fork sei-chain/app expects; since a replace only applies from the main module, any module linking this graph without it resolves upstream geth instead. Both verified resolving through their forks. The module is wired by filesystem replace, not go.work. A workspace promotes a used module's replaces to main-module status, and this one pins x/crypto and grpc *down* from what the controller needs, so a workspace build would diverge from every GOWORK=off build — Docker, release CI, and any external consumer. internal/patch could not come with it: seictl's CLI needs it too and Go will not export an internal package across repos. Duplicating 245 lines of merge logic across a module boundary is the same drift-with-no-guard shape this migration exists to remove, so it lands in the contract module as sidecarapi/tomlpatch and both consumers import it. That widens sidecarapi past "the wire contract" — a deliberate trade, recorded rather than silent. Two startup refusals, because the failures they replace were silent: - SEI_HOME is required with no fallback. serve.go read it from a package-level var that seictl's root main.go owned and which does not move; the fallback was "/sei" while the controller mounts the data PVC at $HOME/.sei. Dropping the wiring would not crash — it would produce a running, probe-passing sidecar writing genesis and config into an empty directory, fleet-wide. - A configured SEI_KEYRING_BACKEND with SEI_SIDECAR_AUTHN_MODE unauthenticated is refused, before the keyring is opened. Unauthenticated is the empty string, binds every interface and installs no middleware, and the two settings were previously decided independently and never compared. One dropped variable exposed POST /v0/tasks — gov-vote included — to any pod in the cluster with an open keyring, while probes passed and tasks succeeded. Extracted as checkKeyringNeedsAuthn and covered for all five combinations, plus a test pinning that unauthenticated stays the zero value, since the guard's correctness depends on an unset variable matching it. The image installs a seictl symlink beside sei-sidecar. The controller renders Command: []string{"seictl", "serve"} into every pod spec, and controller and sidecar images roll independently, so a rename has no safe ordering in either direction — and it fails quietly, not loudly: seid blocks on a shell loop polling /v0/healthz behind a StartupProbe with FailureThreshold 86400 at 5s, so a mis-ordered rollout hangs for days rather than alerting. With the shim, adopting the image is a pure images.sidecar reference change. Removal condition is in the Dockerfile. CI and the Makefile loop MODULES throughout. Go package patterns stop at a nested module boundary, so a root-only run would have left 23,700 lines — including every test touching validator keys — uncompiled, unlinted and untested with a green check. Verified: gofmt, goimports, build, vet, tidy-check and verify-generated clean across all three modules; 14 root + 3 sidecarapi + 8 sidecar test packages pass; the controller's build closure still holds zero chain-graph packages; SEI_HOME unset and keyring-without-authn both refuse to start when exercised against the built binary. Not verified here: the image build (no Docker daemon available) and the ECR publish, which needs a sei/sei-sidecar repository provisioned in the platform repo first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Putting sidecar/ and sidecarapi/tomlpatch/ under the lint matrix surfaced findings in code seictl never linted — it carries no golangci-lint config. This commit takes only what golangci-lint can fix on its own and what is provably behaviour-neutral: modernize's interface{}→any, integer-range loops (`for i := range n`), and maps.Copy in tomlpatch. Applied with `--fix`, then goimports to drop the `sort` import the slices.Sort rewrite orphaned — which broke the build until it was removed, so the auto-fix is not self-sufficient. Everything else is left for a decision rather than folded in here. The point of this PR is that the moved code is verifiably the same code; rewriting error handling inside a 24,000-line relocation would remove the reviewer's ability to check that, and some of it is not cosmetic: - noctx, 27 findings, entirely in sidecar/engine/sqlite_migrations.go and sqlite_store.go: database/sql calls that should take the *Context variants. A real improvement, and a behaviour change — queries become cancellable — in the store whose SetMaxOpenConns(1) and WAL-checkpoint invariants are the ones worth being careful with. - errcheck, 10, mostly unchecked tx.Rollback in the migration path, where ignoring the error is the intended pattern and wants `_ =` rather than handling. - bodyclose, 26, all in _test.go files; no response body is leaked in code that runs in the pod. Verified: gofmt, goimports, build and tests clean in both modules (3 sidecarapi + 8 sidecar packages, 0 failures). sidecarapi now reports 0 lint issues; sidecar's remaining findings are unaddressed by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The image's ENTRYPOINT was the bare binary, leaving the subcommand to the controller's `Command`. That is a trap for the step where `Command` is removed and the ENTRYPOINT takes over: measured under pod conditions, with SEI_HOME set and no subcommand, sei-sidecar prints help and exits 0. Kubernetes restarts it under restartPolicy Always, so the sidecar would loop forever while reporting success — no crash, no failed probe, and the seid container beside it blocking on /v0/healthz behind a 86400-threshold StartupProbe. Quieter than a crash and harder to find. Naming the subcommand in the ENTRYPOINT makes that step a pure deletion of `Command` rather than a swap that has to land correctly. A pod spec's `command` overrides ENTRYPOINT outright, so today's `["seictl","serve"]` path is unchanged. Also records why the rename cannot be a lockstep cutover, since that is the question the shim answers: the sidecar image is chosen per node by spec.sidecar.image when set, which overrides the platform config, and every SeiNode sample in manifests/samples pins it. A controller rendering the new name would break every node holding a pinned older image, and a config rollout does not reach those — each CR would have to change in the same instant as the controller deploy. The shim replaces that with three steps that are each safe alone: ship the image, then flip or drop Command, then delete the shim. Verified: both invocation paths exercised against the built binary — `serve` reaches real env validation, bare no longer exits 0. Build and all 8 sidecar test packages unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The sidecar container's pod spec no longer carries a Command. The image's ENTRYPOINT names the binary and the subcommand, so the image owns its own entrypoint and renaming the binary later is an image-only change. The compatibility symlink is gone with it. This is a coordinated deploy, not a rolling one. Ship together, per cell: 1. the new sidecar image published as sei/sei-sidecar, and 2. images.sidecar in the platform app-config pointing at it, and 3. this controller. That is safe in one step because the controller renders Image and the (now absent) Command into the same container spec in the same reconcile: a single StatefulSet update carries both, and the rollout follows. It is not safe split apart. This controller against an older image runs that image's bare ENTRYPOINT with no subcommand; an older controller against the new image looks for a `seictl` binary the image no longer has. Both fail quietly rather than loudly — seid blocks on /v0/healthz behind a StartupProbe with FailureThreshold 86400 at 5s, so a mismatch hangs rather than alerts. Rollback is the same change in reverse, and equally coordinated. Note spec.sidecar.image still exists and overrides images.sidecar per node. Nothing sets it today, which is what makes the single-step cutover viable; a node pinned to a pre-rename image after this lands would fail the same quiet way, so pin only images that carry the new entrypoint. No test asserted Command, and the container was already named sei-sidecar. Verified: build clean, 14 root test packages pass, verify-generated clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…se SEI_HOME="" Three defects a two-lens cross review found, plus the documentation the earlier commits contradicted. The image could not be built. .dockerignore starts with `**` and re-includes only Go sources, module files and shell scripts, so sidecar/tasks/defaults/config.toml — a //go:embed target — never entered the build context: `pattern config.toml: no matching files found`. seictl had no .dockerignore, so its `COPY . .` carried the file and the move regressed it. No CI job builds either Dockerfile, so this would have reached main unseen. Reproduced by rebuilding a context under the repo's own filter rules, and confirmed fixed the same way. The relocated binary was not built from the versions the binary it replaces was built from. A fresh `go mod tidy` resolves the minimum each import allows, and seictl's explicit floors did not come along: 10 of its 24 direct requires drifted. modernc.org/sqlite went *down*, v1.21.2 → v1.18.1, which is embedded SQLite 3.41.2 → 3.39.2 — a driver that owns an on-disk format on a live PVC, where the sign-tx idempotency marker lives. aws-sdk-go-v2's s3 transfermanager went up two minor versions while still pre-1.0, on the multi-GB snapshot path where part size and concurrency set the memory ceiling. Floors restored to seictl's, so this is a relocation rather than a silent dependency bump. sidecarapi and sidecar also disagreed on go-toml/v2 (v2.2.2 vs v2.2.4), so tomlpatch's tests exercised a different TOML parser than the sidecar links; both now v2.2.4. SEI_HOME="" defeated the required-flag check. The flag's Required tests whether a value was supplied, and an empty string is a value. Measured: the sidecar started, logged `sidecar HTTP bind=:17991`, and created config/, data/ and sidecar.db relative to its working directory — exactly the silent wrong-directory failure Required was added to prevent. validateHome now rejects empty and whitespace-only, covered for both. Documentation that the entrypoint change falsified: CLAUDE.md described a `seictl` symlink and forbade removing it until every cell had rolled, two commits before removing it; main.go's package doc still described the symlink; README.md's controller-first rollout rule reads as covering the entrypoint change, which it does not. CLAUDE.md now also records what the earlier text got wrong about the rollout — StatefulSets are OnDelete, so a template change never touches a live pod, and what rolls the cell is the controller's own NodeUpdate `replace-pod` for every node with status.currentSidecarImage set. Verified: builds, gofmt, tidy-check and verify-generated clean across all three modules; 14 root + 3 sidecarapi + 8 sidecar test packages pass; the sidecar builds from a simulated build context that previously failed; SEI_HOME empty and whitespace-only both refuse with no files created. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…y PR Adds the publish path for the sidecar image and the PR-time check that would have caught its build being broken. `publish-sidecar` is a separate job rather than a third step in `publish`. ECR sets image_tag_mutability = IMMUTABLE, so re-running the workflow on a sha whose controller image already exists fails that step and aborts every step after it — the sidecar would silently never publish. Independent jobs also stop a sidecar build failure from blocking the controller image. It gets its own cache ref for the reason the integration-harness already has one, and more so: this build pulls the whole sei-chain graph, and the image holds an operator keyring at runtime, so a poisoned layer must not reach the controller image's build. arm64 is built as well as amd64, matching what the GHCR build being replaced produced rather than quietly dropping an architecture. That needed a Dockerfile change: the build stage is now pinned with --platform=$BUILDPLATFORM so it stays on the runner's native arch and cross-compiles via the GOOS/GOARCH it already passes. Without the pin, buildx emulates that stage under QEMU per non-native target, which means compiling sei-chain through emulation. Both arches verified building. The new `docker` job in CI builds both images with push: false. Nothing in CI compiled a Dockerfile before, and the publish workflow only runs on main, so a build-context problem reached main unseen — which is exactly what happened: `.dockerignore` starts with `**`, and an embedded asset outside its re-include list failed the sidecar build with `pattern config.toml: no matching files found` while every other check stayed green. amd64 only there; doubling a sei-chain-sized build on every PR is not worth an arch-specific break the cross-compile makes unlikely. Also `docker-build-sidecar` now passes --platform linux/amd64 like `docker-build`. Without it the target silently produces an arm64 image on an Apple Silicon machine, which no cluster node runs. Nothing publishes until sei/sei-sidecar exists — platform#1535. Verified: both workflow files parse; the sidecar builds from a context filtered by the repo's own .dockerignore rules; linux/amd64 and linux/arm64 both cross-compile; tidy-check, verify-generated and all 25 test packages across the three modules unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

All five samples set spec.sidecar.image to a ghcr.io/sei-protocol/seictl digest. That was harmless while the controller rendered `Command: ["seictl","serve"]` — the subcommand came from the pod spec, so any image with that binary worked. It is not harmless now. The controller renders no Command, so the image's ENTRYPOINT is the command, and an older image reached through a pin gives a container that prints help and exits 0, restarting forever under restartPolicy: Always while seid blocks on /v0/healthz behind a StartupProbe with FailureThreshold 86400 — about five days before Kubernetes calls it failed. Removing the field rather than repointing it. spec.sidecar is +optional and the blocks carried nothing but the image, so the samples now resolve the sidecar from images.sidecar in the platform app-config — which is what every real node does; no live node sets this field. The samples were teaching a pin nobody uses, and the pin is the thing that outlives a coordinated entrypoint change. README gains the reason, so the omission does not read as an oversight, and its sidecar description no longer points at the seictl repo for an image this repo now builds. Verified: all sample manifests still parse; build, verify-generated and the 14 root test packages unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The publish-sidecar job assumed the GHA role for 900 seconds and then compiled the sei-chain graph for linux/amd64 and linux/arm64. On a cold cache that overruns 15 minutes, so the credentials expire before the push and the image never reaches ECR — a failure that only appears once the cache is cold, which is exactly when a release is being cut. Raised to 3600, inside the role's max_session_duration of 7200 (terraform/aws/189176372795/us-east-2/common/gha.tf). The controller job keeps 900: it builds one architecture over a much smaller graph. Also bounds the job at 60 minutes so a stuck build fails instead of holding a runner. Also drops two history-in-source comments from sidecar/main.go that described where the home flag used to live in seictl. The invariant they were protecting is stated as present-state and enforced by validateHome. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…nd publish A three-lens re-review (all DISSENT) plus its own evidence reversed three calls made earlier in this branch. Each fix below is a correction, not a refinement. Dependency floors were 3/19 restored, not restored. The previous commit claimed "floors restored to seictl's" after pinning six direct requires. Measured against the shipping build list, 35 linked modules still differed. Sixteen were AWS SDK modules ahead of what seictl shipped — the whole credential chain plus service/internal/checksum, which owns S3 request-checksum defaults on the multi-GB snapshot path. Nineteen were *behind*: x/crypto, x/net, grpc, protobuf, otel and genproto, because seictl's module also linked the controller through sdk/sei and MVS pulled them up, while a standalone sidecar module settles lower. Those are security patches, silently given back. All 35 are now pinned to what seictl shipped; drift is zero and a re-tidy is a no-op. The sqlite pin carries the reason it exists, since a bare version number is indistinguishable from drift on the next `go get -u`. The home guard shadowed the check it claimed to rely on. urfave runs Before hooks ahead of its own required-flag check, so validateHome in a root Before made `Required` unreachable: an *unset* SEI_HOME reported "is set but empty", sending an operator to look for a value that does not exist. Moved to the flag's Action, which runs only for flags actually set — so the unset case reaches the required-flag check that describes it correctly. Verified across every input shape. Non-empty was also the wrong test. The stated harm is that paths resolve against the working directory, and a relative SEI_HOME does exactly that; the previous test pinned `"sei"` as acceptable. Now refused, with filepath.Clean restored on the way through — seictl normalized the value in this same Action and the relocation had dropped it, which matters for the two consumers that take home raw rather than through filepath.Join. The credential widening was wrong, and so was the reason given for it. The push authenticates with the ECR token amazon-ecr-login mints, valid twelve hours, not with the STS session — and ecr-seid-musl.yml builds a heavier image at 900s and publishes. Reverted to 900; 3600 was a gratuitous 4x on a role that can PutImage to every sei/* repository. arm64 is dropped. The sei-node Karpenter pool pins kubernetes.io/arch In [amd64], so no node can schedule an arm64 sidecar, and all three existing publishers are single-arch. Publishing one would have made this repository the registry's first multi-arch index under an IMMUTABLE tag policy whose only lifecycle rule expires untagged manifests — an index's children — at seven days. Untested, and unrepairable at the same tag. Also: a concurrency group on CI, since push and pull_request both fire on every branch and each run now builds two images; contents: read on the docker job, which is the one that builds from branch-controlled Dockerfiles and was the only job without it; and the sidecar image field's API doc now carries the entrypoint constraint, because that description is what `kubectl explain` prints. Verified: gofmt, goimports, builds, tidy-check and verify-generated clean across three modules; 25 test packages pass; module drift against the shipping build list is zero; SEI_HOME unset, empty, whitespace and relative all refuse with no files created, and an absolute path is accepted and normalized. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Three lint legs were red for two different reasons, and only one of them was the known-open sidecar backlog. `lint (.)` reported 82 findings in files this branch never touches — internal/planner/validator_test.go, internal/controller/node/peers_test.go and others. That is the root module's pre-existing v2.12.1 backlog, the one only-new-issues exists to hold back. The filter diffs against the merge base, which a shallow clone does not contain, so it silently passed everything through; the rebase and force-push are what exposed it. `fetch-depth: 0` on the lint checkout restores it. Same cause for the single sidecarapi finding. `sidecar/` is the real backlog. It arrived from a repo carrying no golangci-lint config, so none of it had ever been linted, and only-new-issues cannot help because every file is new here. Scoped in .golangci.yml rather than left red: a permanently failing check trains people to ignore it, and an exclusion is greppable and reviewable. lll and dupl match what internal/, api/, sdk/ and sidecarapi/ already get. Test files get the set that fires almost exclusively there — bodyclose is 26-for-26 in _test.go, staticcheck 3-for-3. noctx is scoped to sidecar/engine/sqlite_{migrations,store}.go alone, so it stays live across the rest of the module. Those 27 findings want the database/sql *Context variants throughout the store, which is a real improvement and a behaviour change: it makes queries cancellable in the component whose SetMaxOpenConns(1) and WAL-checkpoint invariants are the ones to be careful with. That belongs in its own PR, not inside a 24,000-line relocation. Fixed rather than excluded, since each is zero-risk: ten errcheck findings were the deliberate-ignore Close and Rollback pattern, now written as `_ =` so the intent is on the page — a Rollback after Commit is a no-op and the Close paths are already failing. The em dash repeated across five table cells in shadow/render.go is now `emptyCell`, which says what it means. Excluded with the reason on the page: unparam flags cfg and cdc parameters a few unexported methods do not read, kept for symmetry with sibling methods that do; prealloc's last hit is a json.Unmarshal destination, which Unmarshal allocates itself. Verified: sidecar and sidecarapi both report 0 issues locally. Note the local linter is v2.8.0 and CI pins v2.12.1, which finds strictly more — goconst 182 versus 8 on the same tree — so CI is the authority on whether the exclusions are complete. Builds, gofmt, tidy-check, verify-generated and all 25 test packages across three modules unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Bugbot argued the home validation never runs in production: it lives in the --home flag's Action, and nothing passes that flag — the controller sets SEI_HOME and the image runs `sei-sidecar serve` — so a non-local parent flag Action might not fire for an environment-sourced value on a subcommand. Measured against the built binary in exactly that shape, it does fire: empty, whitespace, "sei" and "./sei" are all refused with no files created, and an unset variable reports the required-flag error rather than claiming emptiness. So the claim is wrong. It was still worth a test, because the reason nobody could answer it quickly is that no test crossed the CLI wiring — TestValidateHome calls the function directly. TestHomeValidationReachesEnvSourcedValues now drives the real cli.Command with SEI_HOME from the environment and a no-op subcommand, asserting both the message and that the subcommand never ran. main() and the test build the root command from one newRootCommand, so the test cannot drift from what production wires. Also fixes the goconst that lint (sidecarapi) reported: the nested-merge case in tomlpatch repeated a map key across its three maps, and the key being identical in all three is the whole point of the case, so it is named rather than repeated. That finding was real and new — tomlpatch arrives in this PR, so only-new-issues correctly reports it. Verified: sidecar and sidecarapi both 0 lint issues locally; builds, vet, gofmt, goimports, tidy-check, verify-generated and all 25 test packages clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The root leg was still reporting 82 findings, 23 of the 25 files in files this branch does not touch. only-new-issues builds its filter from the pull-request patch, and on a 144-file diff that yields nothing usable, so every pre-existing finding surfaced as new — which is why the same config passes on main and fails here. fetch-depth alone did not help. Replaced with --new-from-merge-base, which has git compute the comparison locally, so diff size cannot defeat it. Verified the mechanism on a throwaway repo: a pre-existing finding on the base is filtered, a finding added on the branch is kept. A fetch step makes the base ref available, since checkout does not create it for a pull request. The sidecar leg went 414 → 25 with the previous exclusions. Of the 25, four were worth fixing rather than excluding, and they are the ones a security review already asked for: /v0/healthz, /v0/startupz, /v0/livez and /v0/metrics are the paths that bypass the X-Remote-User check, and they were spelled out separately in auth.go's bypass list and in server.go's route registrations. Now named constants used by both, so those two cannot drift. The proxy's --ignore-paths and openapi.yaml still carry their own copies; collapsing all three is the follow-up. Also switched a test counter to atomic.Int32 from manual atomic.AddInt32 on a plain int32. The remaining goconst hits are excluded with the reason on the page. They are wire and config literals — JSON field names in the EVM digest and shadow comparators, seid config.toml keys that are hyphenated by convention and must read exactly as written, and log attribute names. Reading the literal in place is what makes them checkable against the thing they mirror. One hit suggests reusing conditionHeight for a JSON key and a log attribute that merely share the spelling "height"; that constant names an await-condition kind, so taking the suggestion would conflate two unrelated things. Verified: sidecar and sidecarapi both 0 issues locally; builds, vet, gofmt, goimports, tidy-check, verify-generated and all 25 test packages clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

cursor

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 4bfb93e. Configure here.}

…rapi `path` in a golangci-lint exclusion is a regexp, not a glob. `sidecar/*` reads as "sidecar" followed by zero or more slashes, and unanchored, so it matches every sidecarapi/ path too. The goconst exclusion added for the relocated module was therefore switching goconst off across the contract module — wire and tomlpatch included, one commit after a real goconst finding there was fixed. Demonstrated rather than reasoned: a function with three occurrences of one string dropped into sidecarapi/wire/ was reported as 0 issues before this change and is reported correctly after it. With the probe removed, sidecarapi is genuinely clean rather than silently unchecked, and sidecar is unchanged at 0. `api/*` has the same defect and predates this branch — it matches sidecarapi/ for the same reason, so lll was already off there. Every pattern is now anchored with ^, and the exclusions block carries a note, since the glob reading is the natural one and this is easy to reintroduce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

bdchatham force-pushed the refactor/sidecarapi-contract-module branch from 7514425 to d2da03a Compare August 13, 2026 20:13

bdchatham force-pushed the feat/sidecar-module branch 2 times, most recently from 58bc5ff to 883e6b0 Compare August 13, 2026 20:15

bdchatham mentioned this pull request Aug 13, 2026

refactor(sidecarapi): extract the sidecar wire contract into its own module #503

Merged

7 tasks

cursor Bot reviewed Aug 13, 2026

View reviewed changes

Comment thread sidecar/Dockerfile

bdchatham force-pushed the feat/sidecar-module branch from 883e6b0 to ec51ed7 Compare August 13, 2026 20:40

cursor Bot reviewed Aug 13, 2026

View reviewed changes

Comment thread internal/noderesource/noderesource.go Outdated

bdchatham force-pushed the feat/sidecar-module branch from 4b6f0eb to 0c114da Compare August 13, 2026 21:04

cursor Bot reviewed Aug 13, 2026

View reviewed changes

Comment thread sidecar/Dockerfile

cursor Bot reviewed Aug 14, 2026

View reviewed changes

Comment thread .github/workflows/ecr.yml

bdchatham and others added 7 commits August 14, 2026 13:14

bdchatham force-pushed the feat/sidecar-module branch from 099dded to c21941f Compare August 14, 2026 20:15

bdchatham changed the base branch from refactor/sidecarapi-contract-module to main August 14, 2026 20:15

cursor Bot reviewed Aug 14, 2026

View reviewed changes

Comment thread sidecar/Dockerfile

cursor Bot reviewed Aug 14, 2026

View reviewed changes

Comment thread sidecar/main.go

bdchatham and others added 3 commits August 14, 2026 14:30

cursor Bot reviewed Aug 14, 2026

View reviewed changes

Comment thread .golangci.yml Outdated

bdchatham merged commit 186d65a into main Aug 14, 2026
9 checks passed

Navigation Menu

Uh oh!

feat(sidecar): build the node sidecar from this repo as its own module - #504

feat(sidecar): build the node sidecar from this repo as its own module#504
bdchatham merged 13 commits into
mainfrom
feat/sidecar-module

bdchatham commented Aug 13, 2026 •

edited

Loading

Uh oh!

cursor Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

bdchatham commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes

Deploying this is a coordinated change, and it rolls the whole cell

Decisions worth challenging

Two startup refusals

Test plan

Known-open

Review

Uh oh!

cursor Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bdchatham commented Aug 13, 2026 •

edited

Loading

cursor Bot commented Aug 13, 2026 •

edited

Loading