Skip to content

Release#1764

Merged
omkar273 merged 93 commits into
mainfrom
develop
May 13, 2026
Merged

Release#1764
omkar273 merged 93 commits into
mainfrom
develop

Conversation

@omkar273

@omkar273 omkar273 commented May 12, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes

    • Safer customer listing: large result sets fetched in paginated batches.
  • New Features

    • Helm chart v1.0.0 (GA) with full templates, values and packaging.
    • Multi-architecture image publishing and enhanced CI/CD publish/deploy workflows.
    • New provisioning script for streamlined local (kind) and production installs.
    • Subscription "auto-invoice threshold": API field, scheduled billing, and end-to-end tests.
  • Documentation

    • Extensive Helm/operator docs: architecture, quickstarts, backups, secrets, migration, troubleshooting, and production readiness.

Review Change Stack

subratsahilgupta and others added 30 commits May 9, 2026 14:02
Squashed from 8 incremental commits (76040f40f8a25b) that
incrementally built the FlexPrice Helm chart:

- initial chart skeleton + deployment templates
- internal Postgres / ClickHouse / Kafka spin-up logic
- migration job + encryption key config

Subsequent commits refine and refactor on top of this foundation.
- Commit helm/values-local.yaml: all required passwords, ECR registry
  overrides, memory limits, Temporal sprig config, ingress hostnames
- Remove values-local.yaml from .gitignore (contains only dev-safe defaults)
- Add helm/docs/local-setup-troubleshooting.md: 13-issue guide covering
  every failure mode encountered (Bitnami Docker Hub removal, Temporal
  Cassandra config regression, Kafka quorum break, OOMKill loops, helm
  pending lock, OrbStack vs kind, Apple Silicon ARM64 constraints)
- Add .claude/skills/local-k8s-setup.md: comprehensive Claude skill for
  local deployment covering all edge cases, recovery procedures, and
  OrbStack/Apple Silicon nuances

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bumps chart to 1.1.0 and addresses the P0/P1/P2 items from the
go-live gap analysis so the chart can be installed by an OSS user
out of the box and used as a prod-grade template.

P0 — blocking:
- Default image to ghcr.io/flexprice/flexprice; image.tag falls back
  to .Chart.AppVersion when empty.
- Add multi-arch app-image publish workflow with SBOM, build-provenance
  attestation and Trivy HIGH/CRITICAL scan
  (.github/workflows/publish-app-image.yml).
- Bundled stateful subcharts (postgresql, kafka, redis, temporal) now
  default to enabled=false; values-local.yaml explicitly flips them on
  for dev with pinned image tags (no more :latest).
- Temporal namespace bootstrap moved from provision.sh into a Helm
  post-install/post-upgrade hook Job, driven by
  temporalConfig.bootstrapNamespaces.
- Fix two pre-existing template bugs in migration.yaml:
  - split returns a map, not a list → use splitList/index for
    clickhouse.address parsing.
  - kafkaConfig.brokers can be a comma-separated string (prod) or a YAML
    list (default) → normalize before indexing.

P1 — reliability + OSS hygiene:
- Add terminationGracePeriodSeconds + opt-in preStop sleep to api,
  consumer, worker for graceful shutdown (rebalance, HTTP drain,
  Temporal activity completion).
- Add topologySpreadConstraints (global + per-component override), empty
  default — opt-in for multi-AZ HA.
- Add helm test connectivity probe under templates/tests/.
- Pin bitnami/postgresql and bitnami/redis tags in values-local.yaml.
- Create helm/flexprice/CHANGELOG.md.

P2 — docs + knobs:
- Surface ClickHouse per-query memory limit as clickhouse.maxMemoryUsageGB
  (default 90), wired to FLEXPRICE_CLICKHOUSE_MAX_MEMORY_USAGE env.
- Add docs/ARCHITECTURE.md, docs/TROUBLESHOOTING.md, docs/MIGRATION-GUIDE.md,
  docs/PRE-SHIP-VALIDATION.md.
- Update PRODUCTION_READINESS.md to reflect completed gates.

Redis cluster-mode client branching lives on claude/redis-cluster-mode
for independent testing before merge.

Validated:
- helm lint clean for default, values-local, values-prod.example.
- helm template renders cleanly for all three profiles.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pairs with the upcoming claude/redis-cluster-mode merge. The Redis client
used to always create *redis.ClusterClient; once it branches on a toggle,
defaulting that toggle to false would break any in-flight production install
pointing at AWS ElastiCache cluster-mode-enabled / Redis Cluster.

- helm/flexprice/values.yaml: redisExtended.clusterMode: true (was false)
- helm/values-local.yaml: explicit redisExtended.clusterMode: false (bundled
  bitnami/redis is single-node)
- helm/values-prod.example.yaml: flip example to true with clarifying comment
- CHANGELOG.md: document the backward-compat default

Validated: helm lint clean; helm template renders for default, values-local,
and values-prod.example profiles.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
§6 Reliability:
- PodDisruptionBudget for consumer + worker (matches existing api PDB).
- Per-component RollingUpdate strategy: api/worker default to surge+0 for
  zero-downtime rollouts; consumer uses 0+1 to minimise consumer-group
  rebalance churn.
- topologySpreadConstraints helper already wired (opt-in).

§7 Security:
- Hardened podSecurityContext (fsGroup, runAsGroup, seccompProfile=RuntimeDefault)
  and securityContext (readOnlyRootFilesystem=true, runAsNonRoot=true,
  privileged=false, seccompProfile=RuntimeDefault). Defaults are
  PodSecurityStandard "restricted"-compatible.
- Opt-in per-workload ServiceAccounts: serviceAccount.perComponent=true
  renders one SA per api/consumer/worker with per-component annotations
  (for IRSA / Workload Identity scoping). Default remains the shared SA.
- Opt-in NetworkPolicy: ingress-only allow on api (TCP/8080 from
  ingress-nginx by default). Egress intentionally unrestricted — managed
  services rotate IPs and a CIDR allow-list is operationally brittle.

§13 CI:
- New .github/workflows/helm-validate.yml: helm lint + helm template |
  kubeconform across default/values-local/values-prod.example profiles, plus
  helm install --dry-run against a kind cluster. Triggers on PR + main push
  paths under helm/**.
- renovate.json: weekly grouped PRs for Helm subchart deps, image tags, and
  GitHub Actions; auto-merge for trusted patch/minor on base images; majors
  held for human review; vulnerability alerts run on demand.

§9 Observability deliberately skipped — app doesn't yet expose /metrics;
ServiceMonitor + dashboards land once that endpoint exists.

PRODUCTION_READINESS.md and CHANGELOG.md updated to reflect new state.

Validated: helm lint clean, helm template renders cleanly for default,
values-local, and values-prod.example profiles.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same fix as test/ghcr branch — helm dependency build needs the
subchart repos declared in Chart.yaml to be known via `helm repo add`,
even when the tarballs are already vendored under charts/.

Without this the PR validate workflow errors:
  no repository definition for https://charts.bitnami.com/bitnami,
  https://go.temporal.io/helm-charts. Please add the missing repos via
  'helm repo add'

Adds the two repos and `helm repo update` before both `helm dependency
build` call sites (lint-and-template job + install-dry-run-kind job).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same root cause as the test/ghcr fix: the default GITHUB_TOKEN gets 403
on blob HEAD when pushing to an org-owned GHCR package that isn't linked
to this specific repo with Write. Validated on test/ghcr branch where
swapping to a classic PAT (write:packages, read:packages) unblocked the
push.

Apply the same swap to the two tag-triggered prod workflows so the next
v* tag actually publishes:

- publish-app-image.yml: docker/login-action uses
  secrets.GHCR_PUBLISH_TOKEN (was secrets.GITHUB_TOKEN); username
  pulls from vars.GHCR_USERNAME if set, else github.actor.
- publish-helm-chart.yml: helm registry login uses
  secrets.GHCR_PUBLISH_TOKEN with the same username fallback.

Required GitHub repo configuration:
- Settings → Secrets → Actions → GHCR_PUBLISH_TOKEN
    classic PAT with write:packages + read:packages
    (SSO-authorized for the org if SAML is on)
- Settings → Variables → Actions → GHCR_USERNAME (optional override)

Long-term path (post-test): link the GHCR packages to this repo with
Write access via Packages settings → Manage Actions access; that lets
the default GITHUB_TOKEN push and we can drop the PAT.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…faults

Chart code changes to unblock common OSS install scenarios:

- migration.images.{postgres,busybox,clickhouse}: externalise the three
  hardcoded image refs in templates/jobs/migration.yaml. Operators on a
  private registry mirror can now retarget without forking. New helper
  `flexprice.migrationImage` keeps the rendered ref shape backwards-compat.

- consumer.keda.*: optional KEDA ScaledObject template for Kafka
  consumer-lag-based autoscaling
  (templates/autoscaling/keda-consumer.yaml). Off by default. When on,
  operator should flip consumer.autoscaling.enabled=false to stop the HPA
  fighting KEDA for the same Deployment.

- values-prod.example.yaml: scrub placeholder hostnames to
  .example.invalid so users don't accidentally try real-looking strings.
  Also flip the example kafka SASL mechanism to SCRAM-SHA-512 — MSK IAM is
  NOT wired in the app today (internal/kafka/base.go only handles SCRAM).

- Chart.yaml: add annotations.licenses=AGPL-3.0-only and a category for
  Artifact Hub discoverability; expand the kubeVersion comment to explain
  the 1.24 floor decision.

Validated: helm lint clean; helm template renders cleanly for default,
values-local, and values-prod.example profiles.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tal)

Adds a complete docs/ tree so any OSS user can take the chart from zero
to a running install on their platform. Single canonical EKS walkthrough
+ deltas for the rest, per the agreed scope.

New docs (helm/docs/):
- README.md — entry point / TOC
- PREREQUISITES.md — kube version, ingress controller, cert-manager,
  storage classes, pre-flight checklist
- SECRETS.md — minimum-viable Secret snippet, full key inventory, External
  Secrets Operator pattern, Postgres verify-full / RDS CA mount recipe
- CONFIGURATION-REFERENCE.md — every prod-relevant values knob grouped
  by purpose (image, workloads, data services, secrets, ingress,
  observability, security, NetworkPolicy, migration)
- AWS-IAM.md — IRSA trust policy + three workload-specific permission
  policies (api / consumer / worker); EKS Pod Identity alternative
- TEMPORAL-GUIDE.md — Cloud vs self-hosted vs bundled subchart side-by-side
- BACKUPS.md — per-service backup strategy (managed = nothing to do;
  self-hosted = pgdump/clickhouse-backup CronJob skeletons)
- EKS-QUICKSTART.md — full end-to-end walkthrough: RDS + ClickHouse Cloud
  + MSK (SCRAM, not IAM — app limitation) + ElastiCache + Temporal Cloud
  + ingress-nginx + cert-manager + IRSA
- PLATFORMS.md — service-mapping table + deltas for GKE / AKS / bare-metal
  / local dev

Also polishes helm/flexprice/README.md: adds a Quickstart block at the
top with copy-paste Secret creation, values-prod.example flow, and
links to the per-platform quickstarts.

Calls out two app-side gaps the docs need to flag for now:
- MSK IAM SASL is not wired (internal/kafka/base.go only handles SCRAM)
- CORS is hardcoded to "*" in internal/rest/middleware/cors.go

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…able.sql

The file was added in 2f42978 ("(local) Make it idempotent") but is not
FlexPrice schema:

- defines `default.billing_entries` with voice-AI columns (voice_id,
  num_channels, byok, data_interface, model_name) — looks like a paste
  from a different project's workspace.
- uses SharedMergeTree engine which is ClickHouse Cloud only — breaks on
  Altinity or standalone CH clusters that the chart supports.
- hardcodes the `default.` database, ignoring clickhouse.database config.
- zero references from Go code (grep confirms billing_entries is unused).

The chart's migration Job iterates every *.sql in migrations/clickhouse/
so shipping this file would attempt the CREATE TABLE on every install —
either silently noise on ClickHouse Cloud or hard failure on
self-hosted topologies.
The refactor introduced in 2f65c61 ("(local) Make it idempotent") was
left half-finished and has real bugs:

1. Mid-file orphaned `IF EXISTS ... END IF;` block (lines 49–66) with no
   enclosing `DO $$ ... END $$;` wrapper → psql syntax error.
2. Dropped the Default Tenant insert while still referencing
   tenant_id='00000000-...' from users/environments/secrets → FK violation.
3. Dropped the Sandbox environment row (only Production seeded now).
4. Hardcoded a `sk_local_flexprice_test_key` API key with a fixed hash
   into a seed file that ships with the chart — anyone running the seed
   gets a known-secret API key.
5. Removed the table-existence guards that the file's own header comment
   relies on for the docker-entrypoint-initdb.d/ first-init path.

The helm chart's migration Job seeds inline (templates/jobs/migration.yaml
§8) and does NOT read this file, so reverting is safe for helm installs.
docker-compose local dev relies on this file via /docker-entrypoint-initdb.d/
and is restored to the working upstream/main version.
PR 3 (feat/dockerfile-local-and-its-container, d21923f) extends the
mainline Dockerfile to build the migrate binary and ship the migrations/
directory — the only reasons Dockerfile.local existed. Now that the
mainline image satisfies the chart's runtime contract, remove the
duplicate Dockerfile.local and point provision.sh at Dockerfile.

This commit will become a no-op once PR 3 merges into upstream/main and
the helm branch is rebased. Including it here keeps the helm branch
self-consistent and tested locally in the meantime.
Both are local-tooling artifacts that shouldn't ship with the helm chart:

- .claude/skills/local-k8s-setup.md — internal Claude skill notes for
  local kind/OrbStack bring-up; lives in personal tooling, not in the
  shipped chart.
- renovate.json — config experimental; deferring its addition until we
  have explicit team agreement on Renovate adoption strategy.
The required guards in templates/app/secret.yaml already fail-fast on
empty critical keys (auth.secret, postgres.password, clickhouse.password,
secrets.encryptionKey). Removing the plaintext "flexprice123" defaults
from values.yaml means a bare 'helm install ./flexprice' now correctly
errors out instead of silently installing with an insecure password.

Dev defaults live only in values-local.yaml. Prod consumers must set
secrets.existingSecret or provide values via -f overrides.

Also fix publish-helm-chart.yml lint/template steps which used bare
helm calls — they now pass --set secrets.existingSecret=test to satisfy
the required guards without needing a full values file override.

Breaking change for anyone who was installing without -f overrides.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace x.x.x floats in Chart.yaml with the exact versions already in
Chart.lock. This makes the declared versions visible to Renovate so it
can propose bumps via PR; previously only the lock was pinned.

No behavior change at install time — Chart.lock was already exact.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add 'chart-v*' tag trigger to publish-helm-chart.yml so chart
  releases can be cut without an app version bump.
- Branch/app-tag triggers now only publish when Chart.yaml version
  changed (avoids re-publishing the same chart on every helm/**
  commit).
- Bump chart 1.1.0 → 1.2.0 for the secrets-defaults breaking change
  in this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
(1) Add `set -euo pipefail` to the ECR-login `run:` block; replace the
    `if [ $? -eq 0 ]` anti-pattern (which defeats pipefail) with a
    proper `if cmd; then` form so pipeline failures are caught.

(2) Wrap bare `steps.<id>.outputs.<name>` references in `if:` conditions
    with `${{ }}` for canonical GitHub Actions style; remove the
    step-level `if:` guard on the "Check if chart version changed" step.

(3) The `changed` step now always runs and emits `publish=true`
    immediately for `chart-tag` and `manual` triggers (instead of being
    skipped and relying on fragile `|| trigger == ...` fallbacks in every
    downstream `if:`). Publish steps now only need
    `steps.changed.outputs.publish == 'true'` — one clear signal.

(nit) Move `## [Unreleased]` above `## [1.2.0]` in CHANGELOG.md to
      match Keep-a-Changelog convention; add `[1.2.0]` reference link.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…aveat

Several bucket items were already done in code but still marked open
in PRODUCTION_READINESS.md (migration hooks, idempotency). Flip the
checkboxes to reflect reality.

Add Schema-and-rollback caveat to MIGRATION-GUIDE.md so operators
know that helm rollback alone doesn't reverse Ent or ClickHouse
schema migrations.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Side-effect of `helm dependency update` in Unit B (commit 753b38d).
Only digest + timestamp changed; dependency versions are identical.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
omkar273 and others added 20 commits May 13, 2026 13:05
…ive time and streamline invoice creation logic
…cessAutoInvoiceThresholdSubscription and simplify error handling in ProcessThresholdBilling
…oldSubscription by updating transaction error check
…fixtures

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r quality issues

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
 feat(oauth): add additional ZohoBooks scopes for settings management
feat(billing): add progressive billing via auto_invoice_threshold

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
internal/api/dto/subscription_test.go (1)

86-124: ⚡ Quick win

Prefer a table-driven shape for the multi-scenario threshold test.

This test covers multiple scenarios well, but converting it to a table-driven loop would align with the test convention used in this repo and reduce repetition.

As per coding guidelines "**/*_test.go: Use table-driven tests to test multiple scenarios in a single test function".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/dto/subscription_test.go` around lines 86 - 124, Replace the
multiple t.Run blocks in
TestCreateSubscriptionRequestValidate_AutoInvoiceThreshold with a single
table-driven loop: define a slice of test cases (name, optional
AutoInvoiceThreshold value, expectError bool, expectMessage string) then iterate
calling t.Run(tc.name, func(t *testing.T){ set req :=
baseCreateSubscriptionRequest(); assign tc.AutoInvoiceThreshold to
req.AutoInvoiceThreshold when non-nil; call req.Validate(); assert error
presence matches expectError and, when expectMessage is set, assert
strings.Contains(strings.ToLower(err.Error()), tc.expectMessage) }); keep
references to TestCreateSubscriptionRequestValidate_AutoInvoiceThreshold,
baseCreateSubscriptionRequest, req.Validate, and the AutoInvoiceThreshold field
so the behavior and assertions (including the negative case checking for
"auto_invoice_threshold") remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@ent/schema/subscription.go`:
- Around line 205-211: Add a database index for the new auto_invoice_threshold
field to support efficient threshold-based billing scans: update the
Subscription schema where field.Other("auto_invoice_threshold",
decimal.Decimal{}) is defined and add an Index that includes
"auto_invoice_threshold" (create a descriptive name like
idx_subscription_auto_invoice_threshold). If you expect queries to also filter
by status/tenant during cron scans, add a composite index on
("auto_invoice_threshold", "status") or ("auto_invoice_threshold", "tenant_id")
as appropriate to your query patterns; ensure the index is declared using the
schema's Index(...) builder so Ent will generate the migration.

In `@internal/repository/ent/subscription.go`:
- Around line 1165-1211: GetSubscriptionsWithAutoInvoiceThreshold is missing the
subscription type filter, causing inherited/parent subscriptions to be returned;
update the Ent query in GetSubscriptionsWithAutoInvoiceThreshold to include
subscription.SubscriptionTypeEQ(types.SubscriptionTypeStandalone) in the
Where(...) clause (alongside the existing
TenantID/EnvironmentID/Status/SubscriptionStatus/AutoInvoiceThreshold filters)
so the repository matches the in-memory store behavior.

In `@internal/service/subscription_threshold_billing_test.go`:
- Around line 272-314: The test inserts follow-up events at
t1.Add(1*time.Minute) which can be in the future relative to the immediate
second call to ProcessAutoInvoiceThresholdBilling, so the second run sees an
empty window; change the follow-up insert timestamp to be slightly before the
second run (e.g. t1.Add(-1*time.Minute) or t1.Add(-1*time.Second)) so the events
are inside the new-period window when calling ProcessAutoInvoiceThresholdBilling
again; update the call site that uses s.insertEvents(500, t1.Add(1*time.Minute))
in TestThresholdBilling_PeriodAdvanceResetsUsageWindow to use a past timestamp
(referencing s.insertEvents and ProcessAutoInvoiceThresholdBilling) so the
second invocation actually evaluates the new period's usage.

In `@internal/service/subscription.go`:
- Around line 7721-7727: The call to invoiceService.CreateSubscriptionInvoice is
reusing the period-end renewal path (ReferencePointPeriodEnd and
types.InvoiceFlowRenewal) for a mid-period threshold hit, which causes
recurring/advance items to be invoiced incorrectly; change this to a dedicated
threshold-invoice flow by invoking CreateSubscriptionInvoice with a reference
point and flow that represent threshold billing (e.g., a new
ReferencePointThreshold or ReferencePointNow and a new/explicit
types.InvoiceFlowThreshold) or add a boolean/flag to CreateSubscriptionInvoice
to restrict invoicing to accrued usage since sub.CurrentPeriodStart only; update
any callers and enum values (ReferencePointPeriodEnd, types.InvoiceFlowRenewal)
accordingly so threshold hits do not trigger advance/recurring charges.
- Around line 7719-7736: The code ignores the updated subscription returned by
invoiceService.CreateSubscriptionInvoice and instead persists the original sub,
which can overwrite status/payment changes; capture the returned subscription
(replace the blank identifier "_" with a variable like updatedSub), use
updatedSub when advancing CurrentPeriodStart (set updatedSub.CurrentPeriodStart
= effectiveTime) and pass updatedSub to s.SubRepo.Update(ctx, updatedSub) so the
transaction persists the latest subscription state from
CreateSubscriptionInvoice.

---

Nitpick comments:
In `@internal/api/dto/subscription_test.go`:
- Around line 86-124: Replace the multiple t.Run blocks in
TestCreateSubscriptionRequestValidate_AutoInvoiceThreshold with a single
table-driven loop: define a slice of test cases (name, optional
AutoInvoiceThreshold value, expectError bool, expectMessage string) then iterate
calling t.Run(tc.name, func(t *testing.T){ set req :=
baseCreateSubscriptionRequest(); assign tc.AutoInvoiceThreshold to
req.AutoInvoiceThreshold when non-nil; call req.Validate(); assert error
presence matches expectError and, when expectMessage is set, assert
strings.Contains(strings.ToLower(err.Error()), tc.expectMessage) }); keep
references to TestCreateSubscriptionRequestValidate_AutoInvoiceThreshold,
baseCreateSubscriptionRequest, req.Validate, and the AutoInvoiceThreshold field
so the behavior and assertions (including the negative case checking for
"auto_invoice_threshold") remain identical.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1b0e6406-3c80-43f4-b313-736ff8df22fb

📥 Commits

Reviewing files that changed from the base of the PR and between e65e1c3 and ec564b7.

📒 Files selected for processing (37)
  • docs/superpowers/specs/2026-05-07-trial-start-invoice-design.md
  • ent/migrate/schema.go
  • ent/mutation.go
  • ent/schema/subscription.go
  • ent/subscription.go
  • ent/subscription/subscription.go
  • ent/subscription/where.go
  • ent/subscription_create.go
  • ent/subscription_update.go
  • internal/api/dto/subscription.go
  • internal/api/dto/subscription_test.go
  • internal/domain/subscription/model.go
  • internal/domain/subscription/model_test.go
  • internal/domain/subscription/repository.go
  • internal/interfaces/service.go
  • internal/repository/ent/subscription.go
  • internal/service/line_item_proration_test.go
  • internal/service/price_test.go
  • internal/service/subscription.go
  • internal/service/subscription_line_item_test.go
  • internal/service/subscription_modification.go
  • internal/service/subscription_modification_test.go
  • internal/service/subscription_test.go
  • internal/service/subscription_threshold_billing_test.go
  • internal/service/subscription_trial_payment_matrix_test.go
  • internal/service/subscription_trial_test.go
  • internal/temporal/activities/cron/subscription_activities.go
  • internal/temporal/models/cron.go
  • internal/temporal/registration.go
  • internal/temporal/service/schedules.go
  • internal/temporal/workflows/cron/auto_invoice_threshold_billing_workflow.go
  • internal/testutil/base_service_suite.go
  • internal/testutil/inmemory_subscription_store.go
  • internal/testutil/inmemory_subscription_store_test.go
  • internal/types/invoice.go
  • internal/types/schedule.go
  • internal/types/schedule_test.go
💤 Files with no reviewable changes (1)
  • docs/superpowers/specs/2026-05-07-trial-start-invoice-design.md
✅ Files skipped from review due to trivial changes (7)
  • internal/service/subscription_trial_payment_matrix_test.go
  • ent/subscription/subscription.go
  • ent/subscription_update.go
  • ent/subscription.go
  • ent/subscription/where.go
  • ent/migrate/schema.go
  • ent/mutation.go

Comment on lines +205 to +211
field.Other("auto_invoice_threshold", decimal.Decimal{}).
Optional().
Nillable().
SchemaType(map[string]string{
"postgres": "decimal(20,6)",
}).
Comment("Threshold usage amount (in subscription currency) that triggers an intermediate invoice. Overrides plan-level threshold when set."),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add an index for threshold-based billing scans.

The new auto_invoice_threshold field is introduced for recurring billing checks, but there’s no index to support threshold filtering. This can degrade cron performance as subscriptions grow.

💡 Suggested schema index addition
 func (Subscription) Indexes() []ent.Index {
 	return []ent.Index{
 		// Common query patterns from repository layer
 		index.Fields("tenant_id", "environment_id", "customer_id", "status").
 			Annotations(entsql.IndexWhere("status = 'published'")),
 		index.Fields("tenant_id", "environment_id", "plan_id", "status"),
 		index.Fields("tenant_id", "environment_id", "subscription_status", "status"),
 		// For billing period updates
 		index.Fields("tenant_id", "environment_id", "current_period_end", "subscription_status", "status"),
+		// For auto-invoice-threshold candidate scans
+		index.Fields("tenant_id", "environment_id", "auto_invoice_threshold", "subscription_status", "status").
+			Annotations(entsql.IndexWhere("auto_invoice_threshold IS NOT NULL AND auto_invoice_threshold > 0 AND status = 'published'")),
 	}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
field.Other("auto_invoice_threshold", decimal.Decimal{}).
Optional().
Nillable().
SchemaType(map[string]string{
"postgres": "decimal(20,6)",
}).
Comment("Threshold usage amount (in subscription currency) that triggers an intermediate invoice. Overrides plan-level threshold when set."),
func (Subscription) Indexes() []ent.Index {
return []ent.Index{
// Common query patterns from repository layer
index.Fields("tenant_id", "environment_id", "customer_id", "status").
Annotations(entsql.IndexWhere("status = 'published'")),
index.Fields("tenant_id", "environment_id", "plan_id", "status"),
index.Fields("tenant_id", "environment_id", "subscription_status", "status"),
// For billing period updates
index.Fields("tenant_id", "environment_id", "current_period_end", "subscription_status", "status"),
// For auto-invoice-threshold candidate scans
index.Fields("tenant_id", "environment_id", "auto_invoice_threshold", "subscription_status", "status").
Annotations(entsql.IndexWhere("auto_invoice_threshold IS NOT NULL AND auto_invoice_threshold > 0 AND status = 'published'")),
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ent/schema/subscription.go` around lines 205 - 211, Add a database index for
the new auto_invoice_threshold field to support efficient threshold-based
billing scans: update the Subscription schema where
field.Other("auto_invoice_threshold", decimal.Decimal{}) is defined and add an
Index that includes "auto_invoice_threshold" (create a descriptive name like
idx_subscription_auto_invoice_threshold). If you expect queries to also filter
by status/tenant during cron scans, add a composite index on
("auto_invoice_threshold", "status") or ("auto_invoice_threshold", "tenant_id")
as appropriate to your query patterns; ensure the index is declared using the
schema's Index(...) builder so Ent will generate the migration.

Comment on lines +1165 to +1211
// GetSubscriptionsWithAutoInvoiceThreshold returns active, published subscriptions (paginated)
// where auto_invoice_threshold is set directly on the subscription.
func (r *subscriptionRepository) GetSubscriptionsWithAutoInvoiceThreshold(ctx context.Context, limit, offset int) ([]*domainSub.Subscription, error) {
tenantID := types.GetTenantID(ctx)
envID := types.GetEnvironmentID(ctx)

span := StartRepositorySpan(ctx, "subscription", "get_subscriptions_with_auto_invoice_threshold", map[string]interface{}{
"tenant_id": tenantID,
"environment_id": envID,
"limit": limit,
"offset": offset,
})
defer FinishSpan(span)

subs, err := r.client.Reader(ctx).Subscription.Query().
Where(
subscription.TenantID(tenantID),
subscription.EnvironmentID(envID),
subscription.Status(string(types.StatusPublished)),
subscription.SubscriptionStatusEQ(types.SubscriptionStatusActive),
subscription.AutoInvoiceThresholdNotNil(),
subscription.AutoInvoiceThresholdGT(decimal.Zero),
).
Order(ent.Asc(subscription.FieldID)).
Limit(limit).
Offset(offset).
All(ctx)
if err != nil {
SetSpanError(span, err)
return nil, ierr.WithError(err).
WithHint("Failed to fetch threshold subscriptions").
Mark(ierr.ErrDatabase)
}

if len(subs) == 0 {
SetSpanSuccess(span)
return []*domainSub.Subscription{}, nil
}

result := make([]*domainSub.Subscription, len(subs))
for i, sub := range subs {
result[i] = domainSub.GetSubscriptionFromEnt(sub)
}

SetSpanSuccess(span)
return result, nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Missing subscription type filter in Ent repository.

The in-memory test store implementation at internal/testutil/inmemory_subscription_store.go:553 filters for SubscriptionTypeStandalone:

if sub.SubscriptionType != types.SubscriptionTypeStandalone || sub.Status != types.StatusPublished {
    continue
}

However, this Ent repository implementation does not include a similar filter. This means the Ent implementation would return parent/inherited subscriptions with thresholds, while the test store would not—leading to test/production behavioral drift.

🔧 Proposed fix to add subscription type filter
 	subs, err := r.client.Reader(ctx).Subscription.Query().
 		Where(
 			subscription.TenantID(tenantID),
 			subscription.EnvironmentID(envID),
 			subscription.Status(string(types.StatusPublished)),
 			subscription.SubscriptionStatusEQ(types.SubscriptionStatusActive),
+			subscription.SubscriptionTypeEQ(types.SubscriptionTypeStandalone),
 			subscription.AutoInvoiceThresholdNotNil(),
 			subscription.AutoInvoiceThresholdGT(decimal.Zero),
 		).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/repository/ent/subscription.go` around lines 1165 - 1211,
GetSubscriptionsWithAutoInvoiceThreshold is missing the subscription type
filter, causing inherited/parent subscriptions to be returned; update the Ent
query in GetSubscriptionsWithAutoInvoiceThreshold to include
subscription.SubscriptionTypeEQ(types.SubscriptionTypeStandalone) in the
Where(...) clause (alongside the existing
TenantID/EnvironmentID/Status/SubscriptionStatus/AutoInvoiceThreshold filters)
so the repository matches the in-memory store behavior.

Comment on lines +272 to +314
func (s *SubscriptionThresholdBillingTestSuite) TestThresholdBilling_PeriodAdvanceResetsUsageWindow() {
ctx := s.GetContext()

// Period 1: 1001 events -> $10.01, crosses threshold.
// Timestamp 1 hour before now, inside the original period (now-7d .. now).
s.insertEvents(1001, s.testData.now.Add(-1*time.Hour))

result1, err := s.service.ProcessAutoInvoiceThresholdBilling(ctx)
s.NoError(err)
s.Equal(1, result1.TotalInvoiced, "first run should invoice subA")

// Record the new CurrentPeriodStart (T1 approx time of first run).
reloadedAfterFirst, err := s.GetStores().SubscriptionRepo.Get(ctx, s.testData.subA.ID)
s.NoError(err)
t1 := reloadedAfterFirst.CurrentPeriodStart

// Period 2: only 500 events -> $5.00, below the $10 threshold.
// Timestamps are 1 minute after T1 to ensure they fall inside the new window.
s.insertEvents(500, t1.Add(1*time.Minute))

result2, err := s.service.ProcessAutoInvoiceThresholdBilling(ctx)
s.NoError(err)
s.Require().NotNil(result2)

// Second run must skip subA: $5 usage < $10 threshold.
s.Equal(1, result2.TotalChecked)
s.Equal(0, result2.TotalInvoiced)
s.Equal(1, result2.TotalSkipped)
s.Equal(0, result2.TotalFailed)

// Still exactly one invoice in the store (the one from the first run).
filter := types.NewNoLimitInvoiceFilter()
filter.SubscriptionID = s.testData.subA.ID
invoices, err := s.GetStores().InvoiceRepo.List(ctx, filter)
s.NoError(err)
s.Len(invoices, 1, "no second invoice should have been created")

// CurrentPeriodStart must not have moved again.
reloadedAfterSecond, err := s.GetStores().SubscriptionRepo.Get(ctx, s.testData.subA.ID)
s.NoError(err)
s.True(reloadedAfterSecond.CurrentPeriodStart.Equal(t1),
"CurrentPeriodStart should remain at T1 after the skipped second run")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The second run is exercising an empty window, not the new-period window.

Lines 288-290 insert the follow-up events at t1.Add(1 * time.Minute), but the second ProcessAutoInvoiceThresholdBilling call runs immediately afterward. In practice those events are usually still in the future, so this test passes because the second run sees no current-period usage, not because the first period was correctly cut off.

Suggested fix
-	// Period 2: only 500 events -> $5.00, below the $10 threshold.
-	// Timestamps are 1 minute after T1 to ensure they fall inside the new window.
-	s.insertEvents(500, t1.Add(1*time.Minute))
+	// Period 2: only 500 events -> $5.00, below the $10 threshold.
+	// Use a timestamp after T1 but not in the future relative to the second run.
+	ts := time.Now().UTC()
+	if ts.Before(t1) {
+		ts = t1
+	}
+	s.insertEvents(500, ts)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s *SubscriptionThresholdBillingTestSuite) TestThresholdBilling_PeriodAdvanceResetsUsageWindow() {
ctx := s.GetContext()
// Period 1: 1001 events -> $10.01, crosses threshold.
// Timestamp 1 hour before now, inside the original period (now-7d .. now).
s.insertEvents(1001, s.testData.now.Add(-1*time.Hour))
result1, err := s.service.ProcessAutoInvoiceThresholdBilling(ctx)
s.NoError(err)
s.Equal(1, result1.TotalInvoiced, "first run should invoice subA")
// Record the new CurrentPeriodStart (T1 approx time of first run).
reloadedAfterFirst, err := s.GetStores().SubscriptionRepo.Get(ctx, s.testData.subA.ID)
s.NoError(err)
t1 := reloadedAfterFirst.CurrentPeriodStart
// Period 2: only 500 events -> $5.00, below the $10 threshold.
// Timestamps are 1 minute after T1 to ensure they fall inside the new window.
s.insertEvents(500, t1.Add(1*time.Minute))
result2, err := s.service.ProcessAutoInvoiceThresholdBilling(ctx)
s.NoError(err)
s.Require().NotNil(result2)
// Second run must skip subA: $5 usage < $10 threshold.
s.Equal(1, result2.TotalChecked)
s.Equal(0, result2.TotalInvoiced)
s.Equal(1, result2.TotalSkipped)
s.Equal(0, result2.TotalFailed)
// Still exactly one invoice in the store (the one from the first run).
filter := types.NewNoLimitInvoiceFilter()
filter.SubscriptionID = s.testData.subA.ID
invoices, err := s.GetStores().InvoiceRepo.List(ctx, filter)
s.NoError(err)
s.Len(invoices, 1, "no second invoice should have been created")
// CurrentPeriodStart must not have moved again.
reloadedAfterSecond, err := s.GetStores().SubscriptionRepo.Get(ctx, s.testData.subA.ID)
s.NoError(err)
s.True(reloadedAfterSecond.CurrentPeriodStart.Equal(t1),
"CurrentPeriodStart should remain at T1 after the skipped second run")
}
func (s *SubscriptionThresholdBillingTestSuite) TestThresholdBilling_PeriodAdvanceResetsUsageWindow() {
ctx := s.GetContext()
// Period 1: 1001 events -> $10.01, crosses threshold.
// Timestamp 1 hour before now, inside the original period (now-7d .. now).
s.insertEvents(1001, s.testData.now.Add(-1*time.Hour))
result1, err := s.service.ProcessAutoInvoiceThresholdBilling(ctx)
s.NoError(err)
s.Equal(1, result1.TotalInvoiced, "first run should invoice subA")
// Record the new CurrentPeriodStart (T1 approx time of first run).
reloadedAfterFirst, err := s.GetStores().SubscriptionRepo.Get(ctx, s.testData.subA.ID)
s.NoError(err)
t1 := reloadedAfterFirst.CurrentPeriodStart
// Period 2: only 500 events -> $5.00, below the $10 threshold.
// Use a timestamp after T1 but not in the future relative to the second run.
ts := time.Now().UTC()
if ts.Before(t1) {
ts = t1
}
s.insertEvents(500, ts)
result2, err := s.service.ProcessAutoInvoiceThresholdBilling(ctx)
s.NoError(err)
s.Require().NotNil(result2)
// Second run must skip subA: $5 usage < $10 threshold.
s.Equal(1, result2.TotalChecked)
s.Equal(0, result2.TotalInvoiced)
s.Equal(1, result2.TotalSkipped)
s.Equal(0, result2.TotalFailed)
// Still exactly one invoice in the store (the one from the first run).
filter := types.NewNoLimitInvoiceFilter()
filter.SubscriptionID = s.testData.subA.ID
invoices, err := s.GetStores().InvoiceRepo.List(ctx, filter)
s.NoError(err)
s.Len(invoices, 1, "no second invoice should have been created")
// CurrentPeriodStart must not have moved again.
reloadedAfterSecond, err := s.GetStores().SubscriptionRepo.Get(ctx, s.testData.subA.ID)
s.NoError(err)
s.True(reloadedAfterSecond.CurrentPeriodStart.Equal(t1),
"CurrentPeriodStart should remain at T1 after the skipped second run")
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/subscription_threshold_billing_test.go` around lines 272 -
314, The test inserts follow-up events at t1.Add(1*time.Minute) which can be in
the future relative to the immediate second call to
ProcessAutoInvoiceThresholdBilling, so the second run sees an empty window;
change the follow-up insert timestamp to be slightly before the second run (e.g.
t1.Add(-1*time.Minute) or t1.Add(-1*time.Second)) so the events are inside the
new-period window when calling ProcessAutoInvoiceThresholdBilling again; update
the call site that uses s.insertEvents(500, t1.Add(1*time.Minute)) in
TestThresholdBilling_PeriodAdvanceResetsUsageWindow to use a past timestamp
(referencing s.insertEvents and ProcessAutoInvoiceThresholdBilling) so the
second invocation actually evaluates the new period's usage.

Comment on lines +7719 to +7736
if err := s.DB.WithTx(ctx, func(ctx context.Context) error {

inv, _, err = invoiceService.CreateSubscriptionInvoice(ctx, &dto.CreateSubscriptionInvoiceRequest{
SubscriptionID: sub.ID,
PeriodStart: sub.CurrentPeriodStart,
PeriodEnd: effectiveTime,
ReferencePoint: types.ReferencePointPeriodEnd,
BillingReason: types.InvoiceBillingReasonAutoInvoiceThreshold,
}, paymentParams, types.InvoiceFlowRenewal, false)

if err != nil {
return err
}

// Advance current_period_start only after successful invoice creation.
sub.CurrentPeriodStart = effectiveTime
if err := s.SubRepo.Update(ctx, sub); err != nil {
return err

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use the subscription returned by CreateSubscriptionInvoice before writing it back.

The rest of this file preserves updatedSub from CreateSubscriptionInvoice before any follow-up SubRepo.Update. Here you ignore that return and then persist the old sub, which can clobber status/payment changes made during invoice creation in the same transaction.

Suggested fix
-	var inv *dto.InvoiceResponse
+	var (
+		inv        *dto.InvoiceResponse
+		updatedSub *subscription.Subscription
+	)
 	if err := s.DB.WithTx(ctx, func(ctx context.Context) error {
 
-		inv, _, err = invoiceService.CreateSubscriptionInvoice(ctx, &dto.CreateSubscriptionInvoiceRequest{
+		inv, updatedSub, err = invoiceService.CreateSubscriptionInvoice(ctx, &dto.CreateSubscriptionInvoiceRequest{
 			SubscriptionID: sub.ID,
 			PeriodStart:    sub.CurrentPeriodStart,
 			PeriodEnd:      effectiveTime,
 			ReferencePoint: types.ReferencePointPeriodEnd,
 			BillingReason:  types.InvoiceBillingReasonAutoInvoiceThreshold,
 		}, paymentParams, types.InvoiceFlowRenewal, false)
 
 		if err != nil {
 			return err
 		}
 
+		if updatedSub != nil {
+			sub = updatedSub
+		}
+
 		// Advance current_period_start only after successful invoice creation.
 		sub.CurrentPeriodStart = effectiveTime
 		if err := s.SubRepo.Update(ctx, sub); err != nil {
 			return err
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/subscription.go` around lines 7719 - 7736, The code ignores
the updated subscription returned by invoiceService.CreateSubscriptionInvoice
and instead persists the original sub, which can overwrite status/payment
changes; capture the returned subscription (replace the blank identifier "_"
with a variable like updatedSub), use updatedSub when advancing
CurrentPeriodStart (set updatedSub.CurrentPeriodStart = effectiveTime) and pass
updatedSub to s.SubRepo.Update(ctx, updatedSub) so the transaction persists the
latest subscription state from CreateSubscriptionInvoice.

Comment on lines +7721 to +7727
inv, _, err = invoiceService.CreateSubscriptionInvoice(ctx, &dto.CreateSubscriptionInvoiceRequest{
SubscriptionID: sub.ID,
PeriodStart: sub.CurrentPeriodStart,
PeriodEnd: effectiveTime,
ReferencePoint: types.ReferencePointPeriodEnd,
BillingReason: types.InvoiceBillingReasonAutoInvoiceThreshold,
}, paymentParams, types.InvoiceFlowRenewal, false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't reuse the normal period-end renewal path for threshold billing.

Line 2949 already documents this CreateSubscriptionInvoice(... ReferencePointPeriodEnd ...) flow as generating both arrear and advance charges at period end. Calling the same path here mid-period means a threshold hit can invoice recurring fixed/advance items before the real renewal boundary, not just the accrued usage since CurrentPeriodStart.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/service/subscription.go` around lines 7721 - 7727, The call to
invoiceService.CreateSubscriptionInvoice is reusing the period-end renewal path
(ReferencePointPeriodEnd and types.InvoiceFlowRenewal) for a mid-period
threshold hit, which causes recurring/advance items to be invoiced incorrectly;
change this to a dedicated threshold-invoice flow by invoking
CreateSubscriptionInvoice with a reference point and flow that represent
threshold billing (e.g., a new ReferencePointThreshold or ReferencePointNow and
a new/explicit types.InvoiceFlowThreshold) or add a boolean/flag to
CreateSubscriptionInvoice to restrict invoicing to accrued usage since
sub.CurrentPeriodStart only; update any callers and enum values
(ReferencePointPeriodEnd, types.InvoiceFlowRenewal) accordingly so threshold
hits do not trigger advance/recurring charges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/deploy.yml:
- Around line 86-89: The workflow currently always echoes "${GHCR_IMAGE}:latest"
for IS_RELEASE pushes; change the conditional around the latest tag so it only
emits "${GHCR_IMAGE}:latest" when GITHUB_REF_NAME is a stable semver tag (e.g.,
matches ^v[0-9]+(\.[0-9]+)*$ or does not contain a prerelease separator like '-'
), otherwise skip echoing the latest tag; update the block that prints tags
(referencing GHCR_IMAGE and GITHUB_REF_NAME) to add a shell test (grep -E or [[
]] pattern) that excludes prerelease names such as v1.2.3-rc1 before printing
the latest tag.
- Line 101: Replace the docker build action reference to a v7 release to avoid
Node 16 incompatibility: update the uses entry `uses:
docker/build-push-action@v4` to `uses: docker/build-push-action@v7` (or the
specific tag `@v7.1.0`), then verify the workflow step(s) that call
`docker/build-push-action` still work with the v7 inputs/outputs and adjust any
renamed parameters if needed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e9787ca2-a52c-45c2-9674-34a10fa7a247

📥 Commits

Reviewing files that changed from the base of the PR and between ec564b7 and 2fb6156.

📒 Files selected for processing (3)
  • .github/workflows/deploy.yml
  • internal/api/v1/subscription.go
  • internal/types/temporal.go
✅ Files skipped from review due to trivial changes (1)
  • internal/api/v1/subscription.go

Comment on lines +86 to +89
if [ "$IS_RELEASE" = "true" ]; then
echo "${GHCR_IMAGE}:${GITHUB_SHA}"
echo "${GHCR_IMAGE}:${GITHUB_REF_NAME}"
echo "${GHCR_IMAGE}:latest"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Consider filtering pre-releases from the latest tag.

The workflow currently tags every v* push as latest in GHCR. If you publish pre-release versions (e.g., v1.2.3-rc1, v1.2.3-beta), those would incorrectly overwrite the latest tag.

🏷️ Proposed fix to exclude pre-releases from latest tag
            if [ "$IS_RELEASE" = "true" ]; then
              echo "${GHCR_IMAGE}:${GITHUB_SHA}"
              echo "${GHCR_IMAGE}:${GITHUB_REF_NAME}"
-             echo "${GHCR_IMAGE}:latest"
+             # Only tag as latest if not a pre-release (no hyphen in tag name)
+             if [[ ! "$GITHUB_REF_NAME" =~ - ]]; then
+               echo "${GHCR_IMAGE}:latest"
+             fi
            fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if [ "$IS_RELEASE" = "true" ]; then
echo "${GHCR_IMAGE}:${GITHUB_SHA}"
echo "${GHCR_IMAGE}:${GITHUB_REF_NAME}"
echo "${GHCR_IMAGE}:latest"
if [ "$IS_RELEASE" = "true" ]; then
echo "${GHCR_IMAGE}:${GITHUB_SHA}"
echo "${GHCR_IMAGE}:${GITHUB_REF_NAME}"
# Only tag as latest if not a pre-release (no hyphen in tag name)
if [[ ! "$GITHUB_REF_NAME" =~ - ]]; then
echo "${GHCR_IMAGE}:latest"
fi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/deploy.yml around lines 86 - 89, The workflow currently
always echoes "${GHCR_IMAGE}:latest" for IS_RELEASE pushes; change the
conditional around the latest tag so it only emits "${GHCR_IMAGE}:latest" when
GITHUB_REF_NAME is a stable semver tag (e.g., matches ^v[0-9]+(\.[0-9]+)*$ or
does not contain a prerelease separator like '-' ), otherwise skip echoing the
latest tag; update the block that prints tags (referencing GHCR_IMAGE and
GITHUB_REF_NAME) to add a shell test (grep -E or [[ ]] pattern) that excludes
prerelease names such as v1.2.3-rc1 before printing the latest tag.

fi

- name: Build & push image
uses: docker/build-push-action@v4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check the latest version of docker/build-push-action

# Fetch the latest release tag from the GitHub API
gh api repos/docker/build-push-action/releases/latest --jq '.tag_name'

Repository: flexprice/flexprice

Length of output: 70


🏁 Script executed:

cd flexprice/flexprice && cat -n .github/workflows/deploy.yml | sed -n '95,115p'

Repository: flexprice/flexprice

Length of output: 133


🏁 Script executed:

git ls-files .github/workflows/deploy.yml

Repository: flexprice/flexprice

Length of output: 92


🏁 Script executed:

cat -n .github/workflows/deploy.yml | sed -n '95,115p'

Repository: flexprice/flexprice

Length of output: 891


🏁 Script executed:

# Check the GitHub Actions warning/error details for this action
# Let's look at the action.yml or any GitHub documentation about version requirements

# First, let's see if there are any other workflow files or documentation that might indicate version constraints
fd -e yml -e yaml .github/ | head -20

Repository: flexprice/flexprice

Length of output: 401


🏁 Script executed:

fd -e yml -e yaml . .github/

Repository: flexprice/flexprice

Length of output: 528


🏁 Script executed:

rg "docker/build-push-action" .github/

Repository: flexprice/flexprice

Length of output: 216


🏁 Script executed:

# Search for any GitHub Actions warnings or documentation about docker/build-push-action@v4
# Let's check the action's releases and migration guide

web_search "docker/build-push-action v4 GitHub Actions compatibility deprecation"

Repository: flexprice/flexprice

Length of output: 114


🌐 Web query:

docker/build-push-action v4 deprecated GitHub Actions compatibility

💡 Result:

docker/build-push-action v4 is deprecated and no longer recommended for use in GitHub Actions workflows. The official repository and documentation recommend upgrading to the latest version, which is v7.0.0 as of March 2026 [1][2][3][4][5][6][7]. Older versions like v4 predate major updates, including Node.js runtime upgrades (e.g., from Node 16 in earlier versions to Node 20/24 in later ones) [8][9][10], removal of deprecated environment variables and legacy tools in v7 [1][11], and changes in inputs/outputs in related actions [12][13]. GitHub has deprecated older Node runtimes (e.g., Node 12 for v2/v3, Node 20 nearing deprecation), making v4 incompatible with current GitHub-hosted runners that require newer Actions Runner versions for Node 24 support in v7 [1][12][8]. Workflows using v4 may fail due to runtime deprecations or missing features like improved Buildx support [14][15]. Update your workflow to use docker/build-push-action@v7 alongside companion actions like docker/login-action@v4, docker/setup-qemu-action@v4, and docker/setup-buildx-action@v4 [2][4][5]. Example: name: ci on: push jobs: docker: runs-on: ubuntu-latest steps: - name: Login to Docker Hub uses: docker/login-action@v4 with: username: ${{ vars.DOCKERHUB_USERNAME }} password: ${{ secrets.DOCKERHUB_TOKEN }} - name: Set up QEMU uses: docker/setup-qemu-action@v4 - name: Set up Docker Buildx uses: docker/setup-buildx-action@v4 - name: Build and push uses: docker/build-push-action@v7 with: push: true tags: user/app:latest [2][4]

Citations:


Upgrade docker/build-push-action to v7 or later.

docker/build-push-action@v4 uses Node.js 16, which is deprecated on current GitHub Actions runners that require Node 20+. This version incompatibility will cause workflow failures. Upgrade to the latest version (currently v7.1.0).

🧰 Tools
🪛 actionlint (1.7.12)

[error] 101-101: the runner of "docker/build-push-action@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/deploy.yml at line 101, Replace the docker build action
reference to a v7 release to avoid Node 16 incompatibility: update the uses
entry `uses: docker/build-push-action@v4` to `uses: docker/build-push-action@v7`
(or the specific tag `@v7.1.0`), then verify the workflow step(s) that call
`docker/build-push-action` still work with the v7 inputs/outputs and adjust any
renamed parameters if needed.

@omkar273
omkar273 merged commit f1eb8e7 into main May 13, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants