refactor(cli): single source of truth for DHI image tags + Renovate manager by Aureliolo · Pull Request #1723 · Aureliolo/synthorg

Aureliolo · 2026-05-03T09:48:51Z

Summary

Hoists the DHI image-tag literals (dhi.io/postgres:18-debian13, dhi.io/nats:2.12-debian13) into a single source of truth so a future Renovate bump propagates to every call site instead of a single line.

cli/internal/config/state.go keeps the canonical DefaultPostgresImageTag / DefaultNATSImageTag constants and now carries // renovate: datasource=docker depName=dhi.io/{postgres,nats} annotations
cli/internal/verify/dhi.go package vars derive from those constants instead of duplicating the literal
cli/cmd/start.go thirdPartyImages() builds image refs from the constants
cli/internal/verify/dhi_test.go, cli/cmd/init_postgres_test.go, cli/internal/compose/generate_test.go assertions reference the constants (so a Renovate bump that only updates one location fails the test instead of silently dropping verification)
cli/cmd/update_test.go left alone — those fixtures are intentional historical compose.yml snapshots used to test upgrade transitions, not current state
renovate.json adds a customManager for the new annotations on Default*ImageTag so Renovate watches the constants, grouped under the same depName=dhi.io/{nats,postgres} as the existing dhiPinnedIndexDigests map manager — both bump together
Doc references in cli/CLAUDE.md, docs/reference/cli-persistence-backends.md, README.md now point at the SoT constant location instead of duplicating the literal version, so prose stays valid across future bumps

Why

Unblocks #1698 (Container deps update). That PR's Renovate run only updated the dhiPinnedIndexDigests map line in cli/internal/verify/dhi.go, leaving 9 other call sites still hardcoding 2.12-debian13. TestDHIPinnedIndexDigest and the cross-platform CLI Test jobs failed as a result. With this refactor, every literal lives in one place; the Renovate update on #1698 (or its replacement) will sweep all consumers in one diff.

Validation plan

After this merges:

Comment @renovatebot rebase on PR chore: Update Container dependencies #1698
Confirm the resulting diff bumps all dhi.io/nats:2.12-debian13 references (not just one map line)
CI on chore: Update Container dependencies #1698 should now go green (CLI tests included)

If any location is still missed, the Renovate regex needs another iteration — but the test gate (TestDHIPinnedIndexDigest deriving its key from the SoT constant) makes any drift a loud test failure rather than silent verification loss at runtime.

Test plan

go -C cli vet ./... ✓
go -C cli build ./... ✓
go -C cli test ./... — full suite passes, including TestDHIPinnedIndexDigest and TestParseDHIRef after the constant-reference refactor
Pre-push gate: golangci-lint + go vet + go test all green

Review coverage

Pre-reviewed by 4 agents:

docs-consistency — 4 findings, all addressed (docs now reference the SoT instead of the literal)
comment-quality-rot — clean (the doc-block in dhi.go and the test rationale comment in dhi_test.go explain WHY, not origin/review context; commit-body #1698 reference is justificatory git-log venue)
go-reviewer — clean / APPROVE (no verify→config→verify cycle, init-order safe, drift detection enforced at test time)
go-conventions-enforcer — clean (file sizes within budget, vars are correctly vars not consts because Configure() mutates them at runtime)

ghost · 2026-05-03T09:49:04Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

coderabbitai · 2026-05-03T09:49:24Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4520ccdb-ccc3-4c80-9bf6-f2dff8c5f4f4

📥 Commits

Reviewing files that changed from the base of the PR and between 94f89ad and f844820.

📒 Files selected for processing (1)

.claude/skills/review-dep-pr/SKILL.md

Walkthrough

Updated runtime and test code to derive DHI Postgres and NATS image tags from cli/internal/config/state.go (DefaultPostgresImageTag and DefaultNATSImageTag) instead of hardcoded tag strings. Documentation and CLI help were updated to reference the config constants. Tests and compose-generation assertions were adjusted to use those constants. Added inline // renovate: metadata on the constants and a Renovate customManagers regex entry to track them. No exported APIs or runtime behavior values were changed.

Suggested labels

type:infra, dependencies

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main change: refactoring DHI image tags into a single source of truth and adding Renovate manager support.
Description check	✅ Passed	The description is thorough and directly related to the changeset, explaining the rationale, validation plan, and review coverage for consolidating image tag constants and Renovate integration.
Docstring Coverage	✅ Passed	Docstring coverage is 40.00% which is sufficient. The required threshold is 40.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Review rate limit: 2/5 reviews remaining, refill in 34 minutes and 52 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

gemini-code-assist

Code Review

This pull request centralizes the default image tags for Postgres and NATS into constants within cli/internal/config/state.go and configures a Renovate custom manager to keep them updated. It also updates various tests and documentation to reference these new constants. A critical issue was identified in cli/cmd/start.go where the thirdPartyImages function ignores user-provided image tag overrides, potentially leading to security verification bypasses when custom tags are used.

gemini-code-assist · 2026-05-03T09:50:49Z

 	if state.PersistenceBackend == "postgres" {
-		images = append(images, thirdPartyImage{"postgres", "dhi.io/postgres:18-debian13"})
+		images = append(images, thirdPartyImage{"postgres", "dhi.io/postgres:" + config.DefaultPostgresImageTag})
 	}
 	if state.BusBackend == "nats" {
-		images = append(images, thirdPartyImage{"nats", "dhi.io/nats:2.12-debian13"})
+		images = append(images, thirdPartyImage{"nats", "dhi.io/nats:" + config.DefaultNATSImageTag})
 	}


The thirdPartyImages function now hardcodes the use of config.DefaultPostgresImageTag and config.DefaultNATSImageTag, which ignores any user-provided overrides in state.PostgresImageTag or state.NATSImageTag (e.g., set via environment variables or config.json).

This is a significant issue because thirdPartyImages is used in hasDHIDigests to determine if verification can be skipped based on the cache. If an operator provides a custom image tag, the CLI will check the cache for the default image tag instead. If the default tag is cached, the CLI will proceed to pull and run the custom image without any verification, potentially bypassing security checks. The function should respect the values in the state object, falling back to the defaults only when the state fields are empty.

if state.PersistenceBackend == "postgres" { tag := state.PostgresImageTag if tag == "" { tag = config.DefaultPostgresImageTag } images = append(images, thirdPartyImage{"postgres", "dhi.io/postgres:" + tag}) } if state.BusBackend == "nats" { tag := state.NATSImageTag if tag == "" { tag = config.DefaultNATSImageTag } images = append(images, thirdPartyImage{"nats", "dhi.io/nats:" + tag}) }

…anager Default*ImageTag in cli/internal/config/state.go is now the only place the postgres + nats DHI tags are spelled out. verify/dhi.go vars, start.go's third-party image list, and the dhi_test.go fixtures all derive from it. Update_test.go fixtures stay literal because they represent historical compose.yml snapshots used to test upgrade transitions, not current state. Adds a Renovate customManager that watches the new // renovate: annotations on Default*ImageTag so a future tag bump propagates to both the constant and the dhiPinnedIndexDigests map line in one PR (grouped under depName=dhi.io/{nats,postgres}). The previous PR #1698 missed 9 call sites because the existing manager only updated the digest map line.

cli/CLAUDE.md (Persistence Backends + env-vars), README.md (intro), and docs/reference/cli-persistence-backends.md were each duplicating the literal dhi.io/postgres:18-debian13 tag. Replaced with prose pointing at DefaultPostgresImageTag in cli/internal/config/state.go (Renovate- managed) so the prose stays valid across future tag bumps. Pre-reviewed by 4 agents (docs-consistency, comment-quality-rot, go-reviewer, go-conventions-enforcer); 4 docs-consistency findings addressed.

… merge Adds a new mandatory 'Approve with rationale' step shared by all three merge paths (Merge as-is, Improve and merge, Fix CI and merge). Every merge now goes through gh pr review NN --approve --body ... first, recording the bump-type decision, a 2-4 bullet changelog digest splitting relevant vs reviewed-but-not-relevant items, and any deferred follow-ups. Without this, merged-by-Renovate PRs leave no audit trail of why they were accepted; squash messages get rewritten by maintainers and don't surface in the PR conversation thread, so the approval review is the canonical venue.

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

.claude/skills/review-dep-pr/SKILL.md (1)

358-370: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify that approval applies to lockfile-only and wave-based merge flows.

The lockfile-only batch description (line 358) and wave-based parallel description (line 361) both describe merging PRs but don't explicitly reference the approval step added below at line 368. While line 368 states "MANDATORY before any merge" (which should apply universally), line 370's phrasing "Every merge path below funnels through this step first" creates ambiguity—the lockfile-only and wave-based flows are described above that line.

A reader executing the lockfile-only flow might be unsure whether approval is required or if those PRs skip the approval step.

📝 Suggested clarifications

Option 1 (preferred): Add a brief parenthetical in the lockfile-only and wave-based descriptions:

-Lockfile-only batch (Phase 7 skipped). When Phase 6 detected only lockfile-only overlaps (per the Phase 5 trigger rule, Phase 7 was skipped), there is no user-supplied strategy to apply. Use the implicit lockfile-race default: treat all PRs in the batch as parallel-safe; merge each eligible PR immediately (or queue with `--auto` if a final required check is still pending);
+Lockfile-only batch (Phase 7 skipped). When Phase 6 detected only lockfile-only overlaps (per the Phase 5 trigger rule, Phase 7 was skipped), there is no user-supplied strategy to apply. Use the implicit lockfile-race default: treat all PRs in the batch as parallel-safe; merge each eligible PR immediately (or queue with `--auto` if a final required check is still pending, following the "Merge as-is" flow including approval);

And for the wave-based description:

-- **Wave-based parallel**: process Wave 1 PRs first, all with `--auto`/immediate as appropriate, then for each subsequent wave wait for prior merges to land, trigger a rebase on the next wave's PRs, wait for CI, then merge.
+- **Wave-based parallel**: process Wave 1 PRs first (via the "Merge as-is" flow including approval), all with `--auto`/immediate as appropriate, then for each subsequent wave wait for prior merges to land, trigger a rebase on the next wave's PRs, wait for CI, then merge.

Option 2: Change "below" to make the scope universal:

-**Every merge path below funnels through this step first.**
+**Every merge operation in this phase funnels through this step first.**

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.claude/skills/review-dep-pr/SKILL.md around lines 358 - 370, The
documentation is ambiguous about whether the "Approve with rationale (MANDATORY
before any merge)" step applies to the "Lockfile-only batch" and "Wave-based
parallel" flows; update the text so the approval requirement is explicit for
those paths by either adding a short parenthetical to the "Lockfile-only batch"
and "Wave-based parallel" paragraphs (e.g., "(approve with rationale before
merging per 'Approve with rationale (MANDATORY before any merge)')") or by
changing the sentence "Every merge path below funnels through this step first"
to a universal phrasing like "Every merge path in this document funnels through
this step first," and ensure the header "Approve with rationale (MANDATORY
before any merge)" remains immediately before the approval instructions so
readers see it applies to all flows.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In @.claude/skills/review-dep-pr/SKILL.md:
- Around line 358-370: The documentation is ambiguous about whether the "Approve
with rationale (MANDATORY before any merge)" step applies to the "Lockfile-only
batch" and "Wave-based parallel" flows; update the text so the approval
requirement is explicit for those paths by either adding a short parenthetical
to the "Lockfile-only batch" and "Wave-based parallel" paragraphs (e.g.,
"(approve with rationale before merging per 'Approve with rationale (MANDATORY
before any merge)')") or by changing the sentence "Every merge path below
funnels through this step first" to a universal phrasing like "Every merge path
in this document funnels through this step first," and ensure the header
"Approve with rationale (MANDATORY before any merge)" remains immediately before
the approval instructions so readers see it applies to all flows.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9d1a8a69-8198-4085-a52a-50cc4192937c

📥 Commits

Reviewing files that changed from the base of the PR and between 2060f7c and 20ad458.

📒 Files selected for processing (11)

.claude/skills/review-dep-pr/SKILL.md
README.md
cli/CLAUDE.md
cli/cmd/init_postgres_test.go
cli/cmd/start.go
cli/internal/compose/generate_test.go
cli/internal/config/state.go
cli/internal/verify/dhi.go
cli/internal/verify/dhi_test.go
docs/reference/cli-persistence-backends.md
renovate.json

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: Lighthouse Site
GitHub Check: Build Web Assets (melange)
GitHub Check: CLI Test (macos-latest)
GitHub Check: CLI Test (ubuntu-latest)
GitHub Check: CLI Test (windows-latest)
GitHub Check: CLI Lint
GitHub Check: CLI Bench Regression
GitHub Check: Build Preview
GitHub Check: Analyze (python)
GitHub Check: Analyze (javascript-typescript)
GitHub Check: Analyze (go)

🧰 Additional context used

📓 Path-based instructions (11)

cli/**/*.go