Skip to content

feat: compute env vars needed at init time and set up if possible#567

Merged
evanphx merged 25 commits into
mainfrom
mir-651-improve-initial-deploy-ie-setup-process
Apr 29, 2026
Merged

feat: compute env vars needed at init time and set up if possible#567
evanphx merged 25 commits into
mainfrom
mir-651-improve-initial-deploy-ie-setup-process

Conversation

@evanphx
Copy link
Copy Markdown
Contributor

@evanphx evanphx commented Feb 2, 2026

Summary

  • Environment variable detection: Ruby, Python, Node.js, Go, Rust, and Bun stacks
  • Implements elevation logic: package presence alone = "recommended", source code reference = "required"
  • Adds the ability to write an app + app_version record for the app an init time with secure env vars (like rails SECRET_KEY_BASE and RAILS_MASTER_KEY).

Changes by Stack

Python (pkg/stackbuild/python.go)

  • Package detection: psycopg2, boto3, redis, celery, sentry-sdk, stripe, sendgrid, etc.
  • Source patterns: os.getenv(), os.environ[], os.environ.get()
  • Framework defaults: DJANGO_SECRET_KEY, FLASK_SECRET_KEY

Node.js (pkg/stackbuild/node.go)

  • Package detection: pg, mongoose, @prisma/client, aws-sdk, stripe, @sentry/node, etc.
  • Source patterns: process.env.VAR, process.env['VAR']
  • File types: .js, .ts, .jsx, .tsx, .mjs, .cjs

Go (pkg/stackbuild/golang.go)

  • Module detection: lib/pq, pgx, go-redis, aws-sdk-go, sentry-go, etc.
  • Source patterns: os.Getenv(), os.LookupEnv()

Rust (pkg/stackbuild/rust.go)

  • Crate detection: sqlx (required), diesel, redis, sentry, aws-sdk-*, etc.
  • Source patterns: std::env::var(), env!(), option_env!()
  • Default: RUST_LOG=info

Bun (pkg/stackbuild/bun.go)

  • Reuses Node.js detection since Bun is npm-compatible

Shared Infrastructure (pkg/stackbuild/envdetect.go)

  • scanSourceFilesForEnvVars() with 10-second timeout
  • Skips vendor, node_modules, .git, and build directories
  • Returns partial results on timeout

Test plan

  • Added tests for all env var patterns (Python, Node, Go, Rust)
  • Added tests for all file type scanning (.js, .ts, .jsx, .tsx, .mjs, .cjs, .py, .go, .rs)
  • Added tests for elevation logic
  • Added tests for timeout behavior
  • Run make test to verify all tests pass

@evanphx evanphx requested a review from a team as a code owner February 2, 2026 04:31
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 2, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This pull request adds comprehensive environment variable detection and management across the codebase. It introduces environment variable requirement tracking for multiple language stacks (Node, Python, Ruby, Rust, Go, Bun) via new detectEnvVars pipelines and RequiredEnvVars() methods. The miren init command gains --update flag support for updating existing app configurations with detected environment variables and automatic secret generation. A new SetInitialEnvVars RPC endpoint enables batching environment variables during app initialization. The CLI deploy command now displays local environment variable detection results showing available, missing, and additional detected variables. AppConfig TOML/JSON marshaling is updated with omitempty tags, and a ParseWithoutValidation function is added for validation-bypassing configuration parsing.


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

@evanphx evanphx changed the title feat: extend env var detection to all language stacks feat: compute env vars needed at init time and set up if possible Feb 2, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@pkg/stackbuild/golang.go`:
- Around line 243-250: Update the EnvVarRequirement entry that defines the
GO_ENV environment variable (the struct instance added in the results slice) to
set Confidence from "recommended" to "optional"; locate the block creating
EnvVarRequirement{Name: "GO_ENV", Source: "go_core", ...} and change the
Confidence field value to "optional" to reflect that GO_ENV is
framework-specific, not a general Go convention.
🧹 Nitpick comments (12)
pkg/stackbuild/envdetect.go (2)

149-195: Timeout granularity is per-file, not per-scan.

The timeout check on line 166 occurs before processing each file, so a single large file could extend the scan beyond the 10-second limit. This is acceptable for a best-effort scan, but consider documenting this behavior.

Also, the walk error is discarded (_ = filepath.Walk). If the root directory is inaccessible, this silently returns empty results.

📝 Optional: Add debug logging for walk errors

If you have access to a logger in this context, consider logging walk failures:

-	_ = filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
+	walkErr := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return nil
 		}
 		// ... rest of function
 	})
+	// walkErr is only non-nil if the callback returns an error other than SkipDir
+	// errScanTimeout is returned on timeout, which is expected behavior
+	_ = walkErr

84-106: Consider allowing lowercase env var names.

The validation rejects lowercase names like database_url, but some systems use mixed-case or lowercase env vars. While UPPER_SNAKE_CASE is conventional, this strictness might miss legitimate env vars in some projects.

pkg/stackbuild/envdetect_test.go (1)

1272-1278: Consider adding a functional timeout test.

The current test only verifies the timeout constant value and error message. A functional test that creates many files and verifies partial results on timeout would provide stronger coverage of the timeout behavior.

cli/commands/deploy.go (1)

1031-1052: Consider extracting duplicated display logic.

The display logic for "Additional" env vars (lines 1037-1050) is nearly identical to lines 1013-1028. This could be extracted to a small helper function to reduce duplication.

♻️ Example helper extraction
func printAdditionalEnvVars(ctx *Context, vars []LocalEnvVar) {
    ctx.Printf("  %s\n", lipgloss.NewStyle().Foreground(lipgloss.Color("14")).Render("Found locally (may be relevant):"))
    for _, ev := range vars {
        valueDisplay := MaskValue(ev.Value, ev.Sensitive)
        symbol := lipgloss.NewStyle().Foreground(lipgloss.Color("14")).Render("?")
        if ev.Sensitive {
            ctx.Printf("    %s %s=%s\n", symbol, ev.Key,
                lipgloss.NewStyle().Faint(true).Render(valueDisplay))
        } else {
            ctx.Printf("    %s %s=%s\n", symbol, ev.Key, valueDisplay)
        }
    }
}
pkg/stackbuild/bun.go (1)

172-174: Consider adding Bun.env patterns for completeness.

The comment mentions that Bun supports Bun.env in addition to process.env, but the scan only uses nodeEnvPatterns which detect process.env. Consider adding Bun-specific patterns if you want to detect Bun.env.VAR usage.

♻️ Example Bun-specific patterns
// bunEnvPatterns for Bun-specific env access
var bunEnvPatterns = []*regexp.Regexp{
    // Bun.env.VAR
    regexp.MustCompile(`Bun\.env\.([A-Z][A-Z0-9_]+)`),
    // Bun.env["VAR"]
    regexp.MustCompile(`Bun\.env\[['"]([A-Z][A-Z0-9_]+)['"]\]`),
}

Then combine with nodeEnvPatterns in the scan call.

cli/commands/envdetect_test.go (1)

231-239: Consider using strings.Cut instead of manual splitEnv.

Go 1.18+ provides strings.Cut which handles this case more idiomatically.

♻️ Using strings.Cut
-// splitEnv splits an environment variable string into key and value
-func splitEnv(e string) []string {
-	for i := 0; i < len(e); i++ {
-		if e[i] == '=' {
-			return []string{e[:i], e[i+1:]}
-		}
-	}
-	return []string{e}
-}
+// splitEnv splits an environment variable string into key and value
+func splitEnv(e string) []string {
+	key, value, found := strings.Cut(e, "=")
+	if found {
+		return []string{key, value}
+	}
+	return []string{e}
+}

Note: You'll need to add "strings" to imports.

pkg/stackbuild/python.go (1)

45-49: packageEnvVarDef could be moved to envdetect.go for reuse.

This struct is defined identically in multiple stack files (golang.go uses the same pattern). Consider moving it to envdetect.go to avoid duplication.

♻️ Centralize in envdetect.go

In pkg/stackbuild/envdetect.go:

// packageEnvVarDef holds env var definition for a package/module/crate
type packageEnvVarDef struct {
    name       string
    confidence string
}

Then remove the duplicate definitions from python.go, golang.go, etc.

cli/commands/init.go (1)

180-221: Consider clarifying the priority order for env var value resolution.

The current priority for determining how to handle a required env var is:

  1. DefaultValue → written to app.toml
  2. CanGenerate → auto-generated secret stored server-side
  3. ReadFromFile → read from file, stored server-side
  4. Otherwise → manual configuration required

This order seems intentional, but if an EnvVarRequirement has both DefaultValue and CanGenerate set, the default value wins. Ensure this is the desired behavior across all stacks.

pkg/stackbuild/node.go (2)

42-58: Destructuring pattern not detected.

The regex patterns don't capture destructuring assignments like const { DATABASE_URL } = process.env;, which is a common pattern in Node.js. This is a minor limitation that could be addressed in a follow-up.

💡 Optional enhancement to detect destructuring

Add a pattern for destructuring:

// Destructuring: const { VAR1, VAR2 } = process.env
regexp.MustCompile(`\{\s*([A-Z][A-Z0-9_]+(?:\s*,\s*[A-Z][A-Z0-9_]+)*)\s*\}\s*=\s*process\.env`),

Note: This would require additional parsing to extract individual variable names from the match.


340-354: Wildcard matching code exists but no wildcard patterns are defined.

The wildcard matching logic for packages like @aws-sdk/* is implemented, but the nodePackageEnvVars map only contains exact package names (e.g., @aws-sdk/client-s3, @aws-sdk/client-dynamodb). Consider either:

  1. Adding a wildcard entry "@aws-sdk/*" to cover all AWS SDK clients, or
  2. Removing the wildcard code if exact matches are preferred
💡 Option to use wildcards for AWS SDK
 // Cloud services
-"aws-sdk":                  {{name: "AWS_ACCESS_KEY_ID", confidence: "recommended"}, {name: "AWS_SECRET_ACCESS_KEY", confidence: "recommended"}},
-"@aws-sdk/client-s3":       {{name: "AWS_ACCESS_KEY_ID", confidence: "recommended"}, {name: "AWS_SECRET_ACCESS_KEY", confidence: "recommended"}},
-"@aws-sdk/client-dynamodb": {{name: "AWS_ACCESS_KEY_ID", confidence: "recommended"}, {name: "AWS_SECRET_ACCESS_KEY", confidence: "recommended"}},
+"aws-sdk":    {{name: "AWS_ACCESS_KEY_ID", confidence: "recommended"}, {name: "AWS_SECRET_ACCESS_KEY", confidence: "recommended"}},
+"@aws-sdk/*": {{name: "AWS_ACCESS_KEY_ID", confidence: "recommended"}, {name: "AWS_SECRET_ACCESS_KEY", confidence: "recommended"}},
pkg/stackbuild/ruby.go (2)

15-44: Confidence level differs from Node.js stack.

The Ruby gem mappings use "required" confidence for most env vars, while Node.js uses "recommended". This inconsistency means Ruby apps will show more env vars as "Required" even without source code references, whereas Node.js apps will show them as "Recommended" until code usage is confirmed.

Consider aligning the confidence levels across stacks for consistency, or document the intentional difference (Ruby gems are typically more tightly coupled to their env vars).


410-446: Ruby source scanning doesn't use shared infrastructure with timeout protection.

The Node.js stack uses scanSourceFilesForEnvVars from envdetect.go which includes a 10-second timeout for large codebases. The Ruby stack implements its own scanRubySourceForEnvVars without timeout protection. This could cause hangs on very large Ruby codebases.

Consider refactoring to use the shared scanSourceFilesForEnvVars function for consistency and timeout safety.

💡 Suggested refactor to use shared scanning
-// scanRubySourceForEnvVars walks .rb files in the directory and extracts ENV usage
-func (s *RubyStack) scanRubySourceForEnvVars() []detectedEnvVar {
-	var found []detectedEnvVar
-	seen := make(map[string]bool)
-
-	_ = filepath.Walk(s.dir, func(path string, info os.FileInfo, err error) error {
-		// ... custom implementation
-	})
-
-	return found
-}
+// In detectEnvVars, replace the source scan:
+codeVars := scanSourceFilesForEnvVars(s.dir, []string{".rb"}, rubyEnvPatterns, optionalEnvPatterns)

Comment thread pkg/stackbuild/golang.go Outdated
Copy link
Copy Markdown
Contributor

@phinze phinze left a comment

Choose a reason for hiding this comment

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

Hey Evan - this is really good directional work! The goal of walking users through getting sufficient config for their app to boot is exactly right, and the env detection heuristics across stacks are solid.

After digging in, I think this PR is actually 3 things interleaved:

1. Env var detection at build/analyze time
The pkg/stackbuild/envdetect.go work - scanning source for ENV['X'], os.Getenv("X"), process.env.X patterns, inferring from Gemfile/package.json/Cargo.toml, parsing .env.sample. This is good and largely standalone.

2. Local environment cross-referencing
The cli/commands/envdetect.go work - scanning the user's shell to show "you have DATABASE_URL locally" vs "you're missing STRIPE_API_KEY". Nice UX improvement for miren analyze.

3. Pre-deploy config setup + server-side secret storage
The cli/commands/init.go changes - generating secrets, reading from files, creating App + AppVersion entities, storing values via SetEnvVar RPC. This is where it gets complicated.


The model pressure:

The third piece creates a new state: AppVersion with config but no image. The guards added throughout (if image == "") are handling this, but it reveals that we're stretching AppVersion beyond its original design.

Currently AppVersion is an immutable snapshot of "deployable state" = artifact + config bundled together. This PR wants to set config before we have an artifact, which means either:

  • AppVersion can be "not deployable" (current approach - feels like a partial object)
  • Or we need a separate place for config to live

I think it's time to split out ConfigVersion as a first-class entity:

AppVersion (immutable)
  ├── ConfigVersion: ref (can exist pre-deploy)  
  └── ImageURL: string | null (null = not deployable)

ConfigVersion (immutable)
  ├── Variables[]
  └── Services[]

This also sets us up for a proper secrets story. Right now Sensitive: true is cosmetic. If we model Source as a reference rather than a label:

Variable {
    Key:    "SECRET_KEY_BASE"
    Source: "miren:generated"  // or "vault:path" or "aws-sm:arn"
    Value:  ""                 // actual value lives in secrets backend
}

Then ConfigVersion becomes a secret manifest - declaring what's needed and where to get it. Values for sensitive vars never persist in etcd, and we have a path to external secret backends (Vault, AWS SM, etc.).


Recommendation:

Let's split this into phases:

  1. ConfigVersion model - introduce the entity, refactor AppVersion to reference it, establish the Source-as-reference pattern for secrets
  2. Env detection - land the stackbuild detection logic (this PR's good stuff) on top of the new model
  3. Init-time setup - wire up the UX for pre-deploy config, generating secrets, etc.

The detection work here is valuable and we don't want to lose it. I'm concerned that landing it on the current model will create tech debt we'll have to undo.

Can we grab some time to talk through the ConfigVersion design? I've got some ideas sketched out and would love to jam on it.

@evanphx
Copy link
Copy Markdown
Contributor Author

evanphx commented Feb 2, 2026

@phinze That all sounds right on, and actually one way that I was trending with the work (adding something that version can reference) but couldn't come up with the right abstraction. I think Config something is totally right and I like the ability to have the same config version referenced across multilpe app versions, that helps us tell the user more info (like "the last config update was on date X by person Y").

@evanphx evanphx force-pushed the mir-651-improve-initial-deploy-ie-setup-process branch from bc486ec to 50b6618 Compare April 27, 2026 17:52
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@appconfig/appconfig.go`:
- Around line 161-170: ParseWithoutValidation currently uses toml.Unmarshal
which silently drops unknown fields; keep the unknown-field guard by decoding
with a TOML decoder configured to reject unknown keys (instead of
toml.Unmarshal) so any unknown fields cause an error to be returned, then
continue to return the unmarshaled *AppConfig without calling Validate(); update
ParseWithoutValidation to use the TOML decoder API that enforces
strict/DisallowUnknownFields behavior (e.g., toml.NewDecoder /
decoder.Strict(true) or equivalent) so unknown keys are surfaced as errors when
decoding into the AppConfig struct.

In `@cli/commands/envdetect.go`:
- Around line 87-96: Update the sensitive-detection so credential-bearing
URLs/DSNs are treated as sensitive: modify the sensitivePatterns slice and/or
the looksLikeSensitive function to include URL/DSN-related checks (e.g., add
substrings like "_URL", "_URI", "_DSN", "DATABASE_URL", "REDIS_URL",
"SENTRY_DSN" to sensitivePatterns and/or detect values containing "://" or
credentials patterns like "user:pass@" in looksLikeSensitive) so keys and values
that are connection strings are masked before printing.

In `@cli/commands/init.go`:
- Around line 94-96: The update flow currently treats an env var as configured
only if its AppEnvVar.Value is non-empty, causing --update to reprocess keys
that exist but have empty values; change the logic to consider a key configured
if it exists in the existingEnvVars map regardless of Value. Specifically, when
populating existingEnvVars (variable existingEnvVars) include the env var key
unconditionally, and when deciding whether to skip or preserve a var in the
--update path (use of isUpdate and checks against AppEnvVar.Value) switch the
check to look up the key in existingEnvVars instead of checking AppEnvVar.Value
!= ""; apply this same presence-based change to the other similar checks
referenced in the file (the other occurrences around the code mentioned).

In `@controllers/deployment/launcher.go`:
- Around line 356-365: The early-return on image=="" currently returns success
and allows reconcileAppVersion to run cleanupOldVersionPools, potentially
scaling down live pools; change the no-image path to return a sentinel error
(e.g., ErrNoImageSkipPoolCreation) from the function in
controllers/deployment/launcher.go instead of returning "", nil, and update
reconcileAppVersion to detect that sentinel and skip calling
cleanupOldVersionPools for that version (treat it as "skipped, not succeeded").
Reference the image=="" check in launcher.go, the reconcileAppVersion caller,
and the cleanupOldVersionPools function when implementing the sentinel and
conditional handling.
🪄 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: f30c66b4-4183-4413-a151-c71bb26e617b

📥 Commits

Reviewing files that changed from the base of the PR and between bc486ec and 50b6618.

📒 Files selected for processing (7)
  • appconfig/appconfig.go
  • appconfig/appconfig_test.go
  • cli/commands/deploy.go
  • cli/commands/envdetect.go
  • cli/commands/envdetect_test.go
  • cli/commands/init.go
  • controllers/deployment/launcher.go
✅ Files skipped from review due to trivial changes (1)
  • appconfig/appconfig_test.go

Comment thread appconfig/appconfig.go Outdated
Comment thread cli/commands/envdetect.go
Comment thread cli/commands/init.go Outdated
Comment thread controllers/deployment/launcher.go Outdated
evanphx added 3 commits April 27, 2026 11:17
…side secrets

Enhance the miren init and deploy workflow to automatically detect required
environment variables and store secrets securely on the server instead of
in app.toml (which gets committed to git).

Environment Variable Detection:
- Add env var detection for Ruby/Rails apps by scanning Gemfile, source
  code, and config files for ENV references
- Detect common gems (pg, redis, sidekiq, stripe, sentry, etc.) and infer
  their required env vars
- Parse .env.sample/.env.example files for declared variables
- Identify Rails core requirements (SECRET_KEY_BASE, RAILS_MASTER_KEY)
- Add RAILS_ENV=production as a default for Rails apps

Server-Side Secret Storage:
- During miren init, connect to server and store detected secrets via
  app.New and app.SetEnvVar RPCs
- Generated secrets (SECRET_KEY_BASE) and file-based secrets (master.key)
  are sent to server, never written to app.toml
- Non-secret defaults (RAILS_ENV) are written to app.toml
- Graceful offline handling with clear guidance to configure later

Enhanced Deploy Analysis:
- miren deploy --analyze now cross-references detected env vars with
  local environment
- Shows which vars are available locally, which are missing, and
  additional relevant vars found in the environment
- Masks sensitive values in output

Infrastructure Hardening:
- Handle AppVersion entities with no image (pre-deploy state) gracefully
- Add validation in deployment launcher, sandbox controller, exec proxy
- Clear error messages guide users to run miren deploy first

Bug Fixes:
- Fix app.New RPC to set Project field and return proper entity ID
- Allow empty env var values in app.toml validation (secrets server-side)
…stacks

Implements environment variable detection across all language stacks following
the pattern established in the Ruby stack:

- Python: Detect vars from requirements.txt, Pipfile, pyproject.toml; scan
  *.py files for os.getenv/os.environ patterns; Django/Flask framework defaults

- Node.js: Detect vars from package.json dependencies; scan *.js, *.ts, *.jsx,
  *.tsx, *.mjs, *.cjs files for process.env patterns; NODE_ENV default

- Go: Detect vars from go.mod modules; scan *.go files for os.Getenv/os.LookupEnv
  patterns

- Rust: Detect vars from Cargo.toml crates; scan *.rs files for std::env::var
  and env! macro patterns; sqlx marked as required (compile-time check)

- Bun: Reuses Node.js detection logic since Bun is npm-compatible

Key features:
- Shared scanning infrastructure in envdetect.go with 10-second timeout
- Elevation logic: package presence = recommended, source code reference = required
- Skips vendor, node_modules, .git, and other build directories
- Comprehensive tests for all file types and patterns
GO_ENV is a Buffalo/framework convention, not a general Go convention
like NODE_ENV is for Node.js. Surfacing it as "recommended" implied a
best practice that doesn't actually apply to most Go apps. Drop the
confidence to "optional" and update the reason text.
@evanphx evanphx force-pushed the mir-651-improve-initial-deploy-ie-setup-process branch from 50b6618 to bc6a176 Compare April 27, 2026 18:21
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

♻️ Duplicate comments (1)
controllers/deployment/launcher.go (1)

238-247: ⚠️ Potential issue | 🟠 Major

No-image short-circuit is too narrow and can skip deployable service-image versions.

At Line 242, checking only ver.ImageUrl misses versions where spec.Services[*].Image is set. Those versions are deployable, but reconciliation is skipped entirely.

💡 Suggested fix
-	if ver.ImageUrl == "" {
+	hasDeployableImage := ver.ImageUrl != ""
+	if !hasDeployableImage {
+		for _, svc := range spec.Services {
+			if svc.Image != "" {
+				hasDeployableImage = true
+				break
+			}
+		}
+	}
+	if !hasDeployableImage {
 		l.Log.Info("skipping reconcile: app version has no image",
 			"app", app.ID,
 			"version", ver.Version)
 		return nil
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@controllers/deployment/launcher.go` around lines 238 - 247, The short-circuit
that skips reconciliation when ver.ImageUrl == "" is too narrow and wrongly
skips versions that have deployable images in ver.Spec.Services; update the
condition in the launcher reconcile path so you only skip when there is no
ImageUrl AND none of the services have a non-empty Image field. Concretely, in
the function/method surrounding the current block (the launcher reconcile logic
that references ver and app.ID), iterate ver.Spec.Services and check each
service.Image (or equivalent service image field) and only return nil if
ImageUrl is empty AND every service.Image is empty; otherwise proceed with
reconciliation.
🧹 Nitpick comments (2)
pkg/stackbuild/rust.go (1)

51-61: Add explicit patterns for fully-qualified std::env::var(...) forms for consistency.

rustEnvPatterns lists both env::var(...) and std::env::var(...), but rustOptionalEnvPatterns only covers the alias form. While the patterns work in practice (substring matching), adding explicit std::env::var(...) patterns improves clarity and consistency.

Suggested fix
 var rustOptionalEnvPatterns = []*regexp.Regexp{
 	// env::var("VAR").unwrap_or(...)
 	regexp.MustCompile(`env::var\(['"]([A-Z][A-Z0-9_]+)['"]\)\.unwrap_or`),
+	// std::env::var("VAR").unwrap_or(...)
+	regexp.MustCompile(`std::env::var\(['"]([A-Z][A-Z0-9_]+)['"]\)\.unwrap_or`),
 	// env::var("VAR").unwrap_or_else(...)
 	regexp.MustCompile(`env::var\(['"]([A-Z][A-Z0-9_]+)['"]\)\.unwrap_or_else`),
+	// std::env::var("VAR").unwrap_or_else(...)
+	regexp.MustCompile(`std::env::var\(['"]([A-Z][A-Z0-9_]+)['"]\)\.unwrap_or_else`),
 	// env::var("VAR").ok()
 	regexp.MustCompile(`env::var\(['"]([A-Z][A-Z0-9_]+)['"]\)\.ok\(`),
+	// std::env::var("VAR").ok()
+	regexp.MustCompile(`std::env::var\(['"]([A-Z][A-Z0-9_]+)['"]\)\.ok\(`),
 	// option_env!("VAR") is always optional
 	regexp.MustCompile(`option_env!\(['"]([A-Z][A-Z0-9_]+)['"]\)`),
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/stackbuild/rust.go` around lines 51 - 61, The rustOptionalEnvPatterns
slice currently only includes regexps for the alias form env::var(...); add
explicit patterns that match the fully-qualified std::env::var(...) forms to
mirror each existing entry (for unwrap_or, unwrap_or_else, and .ok()) so
rustOptionalEnvPatterns contains both env::var and std::env::var variants;
update the slice used by the parser (rustOptionalEnvPatterns) to include the
three additional regexps that exactly parallel the existing ones to improve
clarity and consistency.
pkg/stackbuild/envdetect_test.go (1)

882-929: Add a Node regression test for confidence and optional classification.

The Node coverage only checks extraction and file-type discovery right now. A stack-level case for a source-only process.env.API_KEY and a process.env["API_KEY"] ?? "default" case would have caught both of the Node detection bugs above.

Also applies to: 1037-1100

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/stackbuild/envdetect_test.go` around lines 882 - 929, Extend
TestNodeEnvPatterns to include regression cases that cover a source-only usage
and a nullish-coalescing default form: add a test case whose input is just a
bare source-only occurrence like `process.env.API_KEY` and another whose input
uses `process.env["API_KEY"] ?? "default"` and assert that nodeEnvPatterns still
extracts "API_KEY" from both; update the testCases slice in TestNodeEnvPatterns
(which iterates over nodeEnvPatterns and calls pattern.FindAllStringSubmatch) to
include these inputs and expected results so the detection handles both
extraction and optional/default patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/commands/envdetect.go`:
- Around line 176-212: The loop that populates result.Available and
result.Missing iterates the raw detectedKeys slice and can produce duplicates;
instead iterate over the deduplicated keys in detectedSet (or create a unique
slice from detectedSet) when checking localEnv, using the same logic with
looksLikeSensitive and LocalEnvVar, and append to
result.Available/result.Missing only once per unique key; ensure you still set
Key, Value, HasValue, Source, and Sensitive the same way as before (use
detectedSet to drive the loop so duplicates are eliminated).
- Around line 311-320: MaskValue currently returns raw strings for non-sensitive
values and when including the first two chars of sensitive values, which can let
control characters or ANSI sequences inject into logs; update MaskValue to
escape/control-normalize output before returning (apply escaping to the
direct-return branch and to the prefix used in the masked branch). Specifically,
in function MaskValue ensure any returned substring (value or value[:2]) has
control characters and ANSI escape sequences sanitized or encoded (e.g., replace
newline, carriage return, tab and strip/escape CSI/ANSI sequences) so the
function only emits safe printable characters.

In `@cli/commands/init.go`:
- Around line 282-289: When SetEnvVar succeeds inside the loop (the ac.SetEnvVar
call), also add a blank entry for that secret into appConfig.EnvVars so the
in-memory config reflects the stored secret; specifically, after
appConfig.SetEnvVar succeeds and you append to serverConfigured, insert a new
env var record (with the secret's Key and an empty Value / mark as Sensitive as
appropriate) into appConfig.EnvVars (or call the existing method that registers
env vars) so the later write/save of appConfig will persist the key as
configured and prevent re-generation on miren init --update. Reference symbols:
SetEnvVar, appConfig.EnvVars, secretsToStore, serverConfigured, secret.Key.

In `@pkg/stackbuild/bun.go`:
- Around line 223-230: detectEnvVars in BunStack currently only scans for
process.env via nodeEnvPatterns/nodeOptionalEnvPatterns and thus misses Bun.env
usage; update the scanner call in BunStack.detectEnvVars to include Bun-specific
patterns by creating or adding bunEnvPatterns (and bunOptionalEnvPatterns if
needed) that match Bun.env.FOO and Bun.env["FOO"], then pass the combined
pattern slices (e.g., append(nodeEnvPatterns, bunEnvPatterns...)) to
scanSourceFilesForEnvVars so Bun.env accesses are detected alongside
process.env. Ensure the new pattern symbol names (bunEnvPatterns,
bunOptionalEnvPatterns) are referenced where scanSourceFilesForEnvVars is
invoked.

In `@pkg/stackbuild/envdetect.go`:
- Around line 242-250: The elevateToRequired helper currently only elevates vars
inferred from other sources; update elevateToRequired(name string, sourceVars
[]detectedEnvVar) to also treat a bare non-optional code reference as required
by checking the detectedEnvVar's source/origin field: return true if any v in
sourceVars has v.name == name and !v.optional OR if v.name == name and v.source
== "source-code" (or v.origin == "source-code" depending on the actual field
name on detectedEnvVar), so direct code references (non-optional) are elevated
to required.

In `@pkg/stackbuild/node.go`:
- Around line 290-304: The loop over sourceVars currently sets all discovered
code-referenced env vars to "recommended" by default; change it so direct
process.env reads are marked "required" unless the source var is optional. In
the for loop that appends EnvVarRequirement for each v in sourceVars, set
Confidence = "required" and Reason = "Direct process.env reference in
application code" when v.optional is false, and keep "optional" when v.optional
is true; update the appended EnvVarRequirement fields in that loop (the
sourceVars processing block and the EnvVarRequirement append) accordingly.
- Around line 50-58: The nodeOptionalEnvPatterns slice is missing a regex to
match bracket-access environment variables using the nullish coalescing operator
(e.g., process.env['VAR'] ?? 'default'); add a new pattern alongside the
existing entries in nodeOptionalEnvPatterns using regexp.MustCompile with a
pattern like process\.env\[['"]([A-Z][A-Z0-9_]+)['"]\]\s*\?\? so
bracket-notation + ?? is detected as optional (update the
nodeOptionalEnvPatterns variable to include this new regexp).

In `@pkg/stackbuild/python.go`:
- Around line 538-565: The code is adding synthetic environment variables
"DJANGO_SECRET_KEY" and "FLASK_SECRET_KEY" via EnvVarRequirement when
s.hasDjango or s.hasFlask; remove those synthetic entries so we don't invent env
var names the frameworks won't read. Instead delete the blocks that append
EnvVarRequirement with Name "DJANGO_SECRET_KEY" and "FLASK_SECRET_KEY" (the two
append calls inside the s.hasDjango and s.hasFlask conditionals) and rely on the
existing source/config scanning logic to detect real env var names the app uses;
keep the DJANGO_DEBUG recommended entry if desired but do not generate
fabricated secret keys.

In `@pkg/stackbuild/ruby.go`:
- Around line 15-39: The rubyGemEnvVars map currently marks gem-detected env
vars as confidence "required" (e.g., entries for "pg", "redis", "aws-sdk-s3",
"aws-sdk-core", "sidekiq", etc.), which is too strong; update the map so that
all gem-only inferred rubyEnvVarDef confidence values are "recommended" instead
of "required" (leave any truly-proven-by-code detections unchanged if present),
modifying the rubyGemEnvVars entries to use confidence: "recommended" for the
listed gems.
- Around line 314-323: The EnvVarRequirement for RAILS_MASTER_KEY currently
always sets ReadFromFile to "config/master.key" even when
"config/credentials/production.yml.enc" was detected; update the logic in the
block that checks s.hasFile(...) so it sets ReadFromFile to the matching key
path: use "config/master.key" when "config/credentials.yml.enc" is present and
use "config/credentials/production.key" when
"config/credentials/production.yml.enc" is present (or map other per-environment
.yml.enc filenames to their corresponding .key files), keeping the
EnvVarRequirement.Name ("RAILS_MASTER_KEY"), Source ("rails_core"), Confidence,
and Reason the same.

---

Duplicate comments:
In `@controllers/deployment/launcher.go`:
- Around line 238-247: The short-circuit that skips reconciliation when
ver.ImageUrl == "" is too narrow and wrongly skips versions that have deployable
images in ver.Spec.Services; update the condition in the launcher reconcile path
so you only skip when there is no ImageUrl AND none of the services have a
non-empty Image field. Concretely, in the function/method surrounding the
current block (the launcher reconcile logic that references ver and app.ID),
iterate ver.Spec.Services and check each service.Image (or equivalent service
image field) and only return nil if ImageUrl is empty AND every service.Image is
empty; otherwise proceed with reconciliation.

---

Nitpick comments:
In `@pkg/stackbuild/envdetect_test.go`:
- Around line 882-929: Extend TestNodeEnvPatterns to include regression cases
that cover a source-only usage and a nullish-coalescing default form: add a test
case whose input is just a bare source-only occurrence like
`process.env.API_KEY` and another whose input uses `process.env["API_KEY"] ??
"default"` and assert that nodeEnvPatterns still extracts "API_KEY" from both;
update the testCases slice in TestNodeEnvPatterns (which iterates over
nodeEnvPatterns and calls pattern.FindAllStringSubmatch) to include these inputs
and expected results so the detection handles both extraction and
optional/default patterns.

In `@pkg/stackbuild/rust.go`:
- Around line 51-61: The rustOptionalEnvPatterns slice currently only includes
regexps for the alias form env::var(...); add explicit patterns that match the
fully-qualified std::env::var(...) forms to mirror each existing entry (for
unwrap_or, unwrap_or_else, and .ok()) so rustOptionalEnvPatterns contains both
env::var and std::env::var variants; update the slice used by the parser
(rustOptionalEnvPatterns) to include the three additional regexps that exactly
parallel the existing ones to improve clarity and consistency.
🪄 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: 9cbc4813-2843-4f8a-adfb-0171e1a4bfd9

📥 Commits

Reviewing files that changed from the base of the PR and between 50b6618 and bc6a176.

📒 Files selected for processing (21)
  • appconfig/appconfig.go
  • appconfig/appconfig_test.go
  • cli/commands/deploy.go
  • cli/commands/envdetect.go
  • cli/commands/envdetect_test.go
  • cli/commands/init.go
  • controllers/deployment/launcher.go
  • controllers/deployment/launcher_test.go
  • pkg/stackbuild/bun.go
  • pkg/stackbuild/envdetect.go
  • pkg/stackbuild/envdetect_test.go
  • pkg/stackbuild/golang.go
  • pkg/stackbuild/node.go
  • pkg/stackbuild/python.go
  • pkg/stackbuild/ruby.go
  • pkg/stackbuild/rust.go
  • pkg/stackbuild/stackbuild.go
  • servers/app/app.go
  • servers/build/build.go
  • servers/exec/exec.go
  • servers/exec_proxy/exec_proxy.go
✅ Files skipped from review due to trivial changes (1)
  • servers/exec/exec.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • appconfig/appconfig_test.go
  • pkg/stackbuild/stackbuild.go
  • cli/commands/deploy.go
  • appconfig/appconfig.go

Comment thread cli/commands/envdetect.go
Comment thread cli/commands/envdetect.go
Comment thread cli/commands/init.go Outdated
Comment thread pkg/stackbuild/bun.go Outdated
Comment thread pkg/stackbuild/envdetect.go
Comment thread pkg/stackbuild/node.go
Comment thread pkg/stackbuild/node.go Outdated
Comment thread pkg/stackbuild/python.go Outdated
Comment thread pkg/stackbuild/ruby.go Outdated
Comment thread pkg/stackbuild/ruby.go Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
pkg/stackbuild/bun.go (1)

256-263: Consider whether NODE_ENV should be "recommended" rather than "required".

While NODE_ENV is commonly used, marking it as "required" with a default value may cause confusion—it will always appear in the "required" output even though it has a sensible default. The Python stack uses "recommended" for DJANGO_DEBUG which is similar in nature (framework mode setting with a default).

This is a minor consistency point and not blocking.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/stackbuild/bun.go` around lines 256 - 263, Change the NODE_ENV
EnvVarRequirement from "required" to "recommended" so the Bun framework mode
variable is suggested but not forced; locate the EnvVarRequirement struct
instance that appends to results with Name: "NODE_ENV" (in the same block that
sets Source: "bun_core", Reason: "Bun/Node.js environment mode", DefaultValue:
"production") and set Confidence: "recommended" instead of "required" to match
the Python/DJANGO_DEBUG pattern.
cli/commands/init.go (1)

282-298: Secrets are set individually without transactionality.

If setting one secret succeeds but a subsequent one fails, the app ends up in a partial state. While each failure is logged, the appConfig.EnvVars only includes successfully stored secrets, which is correct. However, users may not realize some secrets weren't stored if they don't read the warnings carefully.

Consider summarizing failures at the end or exiting with a non-zero status if any secrets failed to store.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/commands/init.go` around lines 282 - 298, The loop over secretsToStore
calls ac.SetEnvVar per secret and logs individual failures but leaves the
process succeeding with partial state; modify the logic in the init flow to
collect any failed secrets (track their keys and errors when ac.SetEnvVar
returns err), don’t just log inline—after the loop inspect the failures and (a)
print a clear summary of failed keys and their errors and (b) exit non-zero or
return an error if any failures occurred so callers know the operation didn’t
fully succeed; ensure appConfig.EnvVars, serverConfigured and existingEnvVars
are only updated for successful sets (keep current updates) and avoid mutating
state for failed secrets.
pkg/stackbuild/rust.go (1)

307-318: Consider using proper TOML parsing instead of substring matching for crate detection.

Using strings.Contains(content, crate) can produce false positives when crate names appear as substrings in other dependency names or comments. For example, searching for "redis" would match dependencies like "redis-cluster". Since Cargo.toml is a structured TOML file, parsing it properly (similar to how parseUvLock() parses lock files in python.go) would provide more reliable and accurate dependency detection.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/stackbuild/rust.go` around lines 307 - 318, The current crate detection
uses substring matching on content built from s.cargoTomlContent and
readFile("Cargo.lock"), which yields false positives (e.g., "redis" matching
"redis-cluster"); replace the brittle strings.Contains check by parsing
Cargo.toml and Cargo.lock with a TOML/lockfile-aware parser: load
s.cargoTomlContent via a TOML decode to locate [dependencies],
[dev-dependencies], and any package tables to match exact crate names, and parse
Cargo.lock (similar to parseUvLock() approach) to inspect package.name entries;
then iterate rustCrateEnvVars and check exact crate names in the parsed
dependency lists (use the rustCrateEnvVars map, readFile, and the parsing code
paths) instead of substring matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/commands/envdetect.go`:
- Around line 203-212: The current branch that builds LocalEnvVar entries uses
only looksLikeSensitive(key) to set Sensitive, which lets credential-bearing
values like DSNs slip through; update the sensitivity check to also inspect the
actual value from localEnv (e.g., call or add a helper like
looksLikeSensitiveValue(value) or extend looksLikeSensitive to accept (key,
value)) and set Sensitive = true if either the key or the value indicates
sensitivity; apply the same change to the other branch that builds LocalEnvVar
around lines 237-243 so result.Available entries always consider both key and
value when deciding to mask output.
- Around line 328-333: The slice `value[:2]` is taking bytes not characters and
can cut multibyte UTF-8 runes, producing invalid output; change the code that
builds the masked prefix to convert the string to runes first (e.g., create
`runes := []rune(value)` and compute a safe prefix with `string(runes[:min(2,
len(runes))])`), then pass that prefix to `escapeForDisplay` and append the
masking dots so `escapeForDisplay` receives a valid UTF-8 string and multibyte
characters aren’t truncated.

---

Nitpick comments:
In `@cli/commands/init.go`:
- Around line 282-298: The loop over secretsToStore calls ac.SetEnvVar per
secret and logs individual failures but leaves the process succeeding with
partial state; modify the logic in the init flow to collect any failed secrets
(track their keys and errors when ac.SetEnvVar returns err), don’t just log
inline—after the loop inspect the failures and (a) print a clear summary of
failed keys and their errors and (b) exit non-zero or return an error if any
failures occurred so callers know the operation didn’t fully succeed; ensure
appConfig.EnvVars, serverConfigured and existingEnvVars are only updated for
successful sets (keep current updates) and avoid mutating state for failed
secrets.

In `@pkg/stackbuild/bun.go`:
- Around line 256-263: Change the NODE_ENV EnvVarRequirement from "required" to
"recommended" so the Bun framework mode variable is suggested but not forced;
locate the EnvVarRequirement struct instance that appends to results with Name:
"NODE_ENV" (in the same block that sets Source: "bun_core", Reason: "Bun/Node.js
environment mode", DefaultValue: "production") and set Confidence: "recommended"
instead of "required" to match the Python/DJANGO_DEBUG pattern.

In `@pkg/stackbuild/rust.go`:
- Around line 307-318: The current crate detection uses substring matching on
content built from s.cargoTomlContent and readFile("Cargo.lock"), which yields
false positives (e.g., "redis" matching "redis-cluster"); replace the brittle
strings.Contains check by parsing Cargo.toml and Cargo.lock with a
TOML/lockfile-aware parser: load s.cargoTomlContent via a TOML decode to locate
[dependencies], [dev-dependencies], and any package tables to match exact crate
names, and parse Cargo.lock (similar to parseUvLock() approach) to inspect
package.name entries; then iterate rustCrateEnvVars and check exact crate names
in the parsed dependency lists (use the rustCrateEnvVars map, readFile, and the
parsing code paths) instead of substring matching.
🪄 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: 9358306c-7423-4a94-974c-e93ef142ec83

📥 Commits

Reviewing files that changed from the base of the PR and between bc6a176 and 60668c7.

📒 Files selected for processing (11)
  • cli/commands/envdetect.go
  • cli/commands/envdetect_test.go
  • cli/commands/init.go
  • docs/docs/command/init.md
  • pkg/stackbuild/bun.go
  • pkg/stackbuild/envdetect_test.go
  • pkg/stackbuild/golang.go
  • pkg/stackbuild/node.go
  • pkg/stackbuild/python.go
  • pkg/stackbuild/ruby.go
  • pkg/stackbuild/rust.go
✅ Files skipped from review due to trivial changes (1)
  • docs/docs/command/init.md
🚧 Files skipped from review as they are similar to previous changes (4)
  • pkg/stackbuild/envdetect_test.go
  • pkg/stackbuild/golang.go
  • pkg/stackbuild/ruby.go
  • pkg/stackbuild/node.go

Comment thread cli/commands/envdetect.go
Comment thread cli/commands/envdetect.go
@evanphx evanphx requested a review from phinze April 28, 2026 18:06
evanphx added 14 commits April 28, 2026 11:13
The init --update flow reads app.toml, modifies it in memory, and
writes it back. Using toml.Unmarshal silently dropped any field the
struct didn't know about, so an unknown key in the existing file
would disappear on the next update.

Switch to a configured decoder with DisallowUnknownFields() so
those keys surface as errors instead of being lost.
`miren init --update` was checking AppEnvVar.Value to decide whether a
key was already configured. With server-side secrets, a key declared
with an empty value is the expected steady state — so the update flow
kept treating those keys as new and re-appended defaults to app.toml
on every run.

Switch existingEnvVars to a presence-only map so any declared key
short-circuits regardless of its in-file value.
DATABASE_URL, REDIS_URL, MONGODB_URI, SENTRY_DSN and similar keys
typically embed credentials in their values (postgres://user:pass@host/db,
https://key@sentry.io/proj). The previous looksLikeSensitive only
matched on KEY/SECRET/PASSWORD/TOKEN/CREDENTIAL/PRIVATE/AUTH substrings,
so `miren deploy --analyze` was printing full DSNs to terminals and CI
logs.

Treat any *_DSN suffix as sensitive, plus *_URL/*_URI when prefixed by
a known credential-bearing service (DATABASE/DB/REDIS/POSTGRES/MYSQL/
MONGO/ELASTICSEARCH/RABBITMQ/AMQP/CLOUDINARY/etc.). Plain WEBHOOK_URL
and APP_HOST stay unmasked.
After successfully storing a secret server-side via SetEnvVar, the
key was never written to app.toml. A subsequent `miren init --update`
would treat the key as missing and run the generator again, rotating
secrets like SECRET_KEY_BASE on every update.

Append a value-less, sensitive entry for each successfully stored
secret so future --update runs see the key as already configured.
Source-only env var references were defaulting to "recommended", the
same confidence we use for package-inferred guesses. That's wrong:
package presence is a heuristic ("you might use this gem's connection
string"), but a direct ENV['X'] / process.env.X read is a hard
requirement — the app won't boot without the var.

Bump the source-only fallback to "required" (keeping "optional" when
a default is detected) across node, bun, python, rust, golang, and
ruby stacks. Also extend the Node optional-env regex to recognize
bracket-notation `process.env['X'] ?? default`, which previously left
those vars classified as required.

Test fixtures that locked in the prior "recommended" behavior are
updated to assert "required".
DJANGO_SECRET_KEY and FLASK_SECRET_KEY were being marked as required
core framework variables, but neither name is read automatically:
Django requires settings.py to wire SECRET_KEY = os.environ[...] and
Flask only honors FLASK_-prefixed names when the app explicitly calls
app.config.from_prefixed_env(). The chosen name is app-specific.

Drop both synthetic entries. Source-code and .env.sample scanning
already pick up whatever names the app actually reads.
The rubyGemEnvVars map was hardcoding pg/redis/aws-sdk/sidekiq/etc.
inferences as required env vars. Gem presence doesn't prove the app
reads DATABASE_URL/REDIS_URL/AWS creds from the environment — many
apps use database.yml, hard-coded clients, or non-standard names.

Demote all gem-only entries to recommended. Source-code scanning
already elevates them to required when the actual ENV read is
detected, so apps that do use these names still get the strong
classification.
A Rails app with config/credentials/production.yml.enc was being
told to read config/master.key — but environment-scoped credential
files use environment-scoped key files (production.key for
production.yml.enc). The init flow would either fail to find the
key or pick up an unrelated master key.

Map each detected credentials file to its matching key file, and
prefer the env-scoped credentials file when both are present.
BunStack only scanned for process.env.X / process.env["X"]. Bun apps
that use the Bun-specific Bun.env accessor went entirely undetected,
so the analyze/init flows missed required env vars in idiomatic
Bun code.

Add bunEnvPatterns / bunOptionalEnvPatterns mirroring the Node ones
for Bun.env (dot, bracket, ||, ??), and combine them with the Node
patterns when scanning Bun source.
MaskValue returned env values verbatim for non-sensitive vars and
for the 2-character prefix of sensitive vars. A value containing
\n, \r, or an ANSI escape sequence could break out of its line in
`miren deploy --analyze` output and CI logs, corrupting layout or
spoofing other lines.

Replace control characters (and the ESC byte that introduces ANSI
sequences) with their escaped textual form before emission. The
prefix shown for sensitive values gets the same treatment so a
secret beginning with ESC can't bypass masking.
The analyzer can surface the same key from multiple sources (e.g.
package-based plus source-based detection). DetectLocalEnvVars
iterated the raw slice, so duplicate keys produced duplicate rows
in `miren deploy --analyze`.

Track keys we've already emitted in a set so each key appears at
most once in either Available or Missing.
The init-time flow will set up a ConfigVersion before the first
deploy rather than creating an AppVersion-without-image, so the
launcher no longer needs to defensively skip pool creation for
no-image versions. Drop the guard and let the original
"image not found" containerd path surface any genuinely bad state.
The init flow was calling SetEnvVar once per generated secret. Each
call created a fresh ConfigVersion + AppVersion and set the AppVersion
active — even though no image had been built yet. That produced a
parade of no-image AppVersions and required the launcher to defensively
skip them.

Wire up the schema's pre-existing app.initial_config field instead:

- New Crud.setInitialEnvVars RPC: takes the same vars list, creates a
  ConfigVersion entity, and points app.initial_config at it. No
  AppVersion is created and active_version stays empty. Subsequent
  calls merge into the existing initial ConfigVersion. Rejects calls
  once an AppVersion exists (caller should use SetEnvVars at that
  point).
- Builder.nextVersion now seeds currentCfg from app.initial_config when
  there is no active version, so the first deploy's AppVersion picks
  up init-time secrets and any other staged config.
- cli/commands/init.go batches all generated secrets into a single
  SetInitialEnvVars call instead of per-secret SetEnvVar calls.

Adds server-side tests for the new RPC: creates initial CV without an
AppVersion, merges across calls, and rejects after deploy.
@evanphx evanphx force-pushed the mir-651-improve-initial-deploy-ie-setup-process branch from 60668c7 to 191bed9 Compare April 28, 2026 23:40
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

♻️ Duplicate comments (2)
cli/commands/envdetect.go (2)

203-212: ⚠️ Potential issue | 🟠 Major

Use the env value when deciding whether output is sensitive.

The current flow only looks at the key name, so a generic key like SERVICE_URL can still print a credential-bearing DSN verbatim if the name misses the heuristic. Please combine key- and value-based checks before populating Available and Additional.

Also applies to: 236-243, 296-317

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/commands/envdetect.go` around lines 203 - 212, The code only checks keys
for sensitivity; update the logic in the blocks that populate result.Available
and result.Additional (where LocalEnvVar structs are created) to combine key-
and value-based heuristics: call the existing looksLikeSensitive(key) and also
run a value-based check (implement a helper like looksLikeSensitiveValue(value)
or extend looksLikeSensitive to accept value) and set Sensitive = keySensitive
|| valueSensitive; use the combined boolean wherever isSensitive is used
(including the "both" and Additional branches referenced around
looksLikeSensitive(key) and localEnv[key]) so both key and value influence
masking.

328-333: ⚠️ Potential issue | 🟡 Minor

Slice masked prefixes by rune, not by byte.

value[:2] can cut through a multibyte UTF-8 character and feed invalid text into escapeForDisplay. Convert to []rune before taking the visible prefix.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/commands/envdetect.go` around lines 328 - 333, Convert the byte-sliced
prefix to a rune-safe slice before passing to escapeForDisplay: turn value into
r := []rune(value), compute a rune-safe prefix like prefix := string(r[:min(2,
len(r))]) and return escapeForDisplay(prefix) + "••••••••"; also ensure the
length check (currently len(value) > 8) is updated to operate on rune count
(len(r) > 8) so multibyte characters are handled correctly and you never slice
mid-rune when calling escapeForDisplay.
🧹 Nitpick comments (1)
pkg/stackbuild/envdetect_test.go (1)

932-974: Add stack-level regression coverage for promotion and optionality.

These tests validate regex extraction, but they won’t catch GO_ENV / RUST_LOG staying weak when directly referenced, or plain os.LookupEnv being treated as optional. A small temp-dir GoStack / RustStack integration test around detectEnvVars() would lock those rules in.

Also applies to: 976-1036, 1294-1310

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/stackbuild/envdetect_test.go` around lines 932 - 974, Add a small
integration-style test that writes minimal Go and Rust files into a temp dir and
runs detectEnvVars (e.g., create a TestGoEnvIntegration/TestRustEnvIntegration
alongside TestGoEnvPatterns) to ensure stack-level rules (promotion and required
vs optional) are enforced: create a Go file that directly references GO_ENV and
uses os.LookupEnv for another var and assert detectEnvVars (or the GoStack
detection flow) promotes GO_ENV and marks the os.LookupEnv result as optional;
do the same for Rust with RUST_LOG to verify promotion; reference goEnvPatterns
and detectEnvVars/GoStack/RustStack in the test to locate the code paths being
exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/app/envvar.go`:
- Around line 271-299: The code path that updates service-scoped env vars fails
when spec.Services is empty because it only updates existing services and
returns "service not found"; update the logic in the function handling env var
staging (the block using spec.Services, svcFound, envFound) so that when no
matching service is found you append a new service entry to spec.Services with
Name set to service and Env initialized with a
core_v1alpha.ConfigSpecServicesEnv constructed from v.Key, v.Value, v.Sensitive
and Source "manual" (instead of immediately returning an error); keep existing
behavior for updating existing env entries (the inner loop that sets
spec.Services[i].Env[j]) and only return an error if adding the new service is
impossible for some other reason.
- Around line 233-315: The current read-merge-write around appRec.InitialConfig
is racy: after ec.Get and before ec.Update another actor can change
ActiveVersion or InitialConfig; switch to an optimistic compare-and-swap loop
that re-reads the app, verifies ActiveVersion is still empty and the
previously-read InitialConfig hasn't changed, then attempts an update and
retries on conflict. Concretely, keep building the new ConfigVersion
(configVer/cvName) as you do, then in a loop call ec.Get to obtain fresh appRec,
check appRec.ActiveVersion == "" and appRec.InitialConfig is unchanged from the
value you used to build spec (or simply ensure it is still empty if you based
spec on no initial config), set appRec.InitialConfig = cvid and call an update
method that performs a conditional update (use ec.CompareAndSwap /
ec.UpdateIfUnchanged / equivalent on the client) or, if only blind Update
exists, detect conflict by comparing the app record before/after and retry; on
persistent conflict return an error. Ensure you reference and replace uses of
ec.Get and ec.Update around appRec.ActiveVersion and appRec.InitialConfig to
this CAS/retry flow.

In `@pkg/stackbuild/golang.go`:
- Around line 48-52: The current goOptionalEnvPatterns blanket-matches any
os.LookupEnv call and wrongly treats all Lookups as optional; update
goOptionalEnvPatterns to only match LookupEnv when a fallback is actually used
(e.g., patterns where the returned ok is tested and a default is assigned).
Remove the broad regexp.MustCompile(`os\.LookupEnv\(`) entry and replace it with
more specific regexes that detect usage like
`(?s)(?:\w+,\s*ok\s*:=\s*os\.LookupEnv\([^)]*\)).*?\bif\s*!ok\b` or
`(?s)os\.LookupEnv\([^)]*\).*?[:=].*?default` (i.e., only mark as optional when
code tests ok or assigns a fallback); reference the goOptionalEnvPatterns
variable and the regexp.MustCompile entries when making the change.

In `@servers/app/app.go`:
- Around line 84-95: The Get call's error is not discriminated: if r.EC.Get
returns an error other than cond.ErrNotFound the code falls through and
incorrectly tries to Create; update the logic around r.EC.Get to: on nil error
keep existing behavior (SetId and return), if the error is cond.ErrNotFound
proceed to set defaults (appRec.Project) and call r.EC.Create, otherwise
immediately return the original Get error; reference r.EC.Get, cond.ErrNotFound,
r.EC.Create and appRec when making this change.

---

Duplicate comments:
In `@cli/commands/envdetect.go`:
- Around line 203-212: The code only checks keys for sensitivity; update the
logic in the blocks that populate result.Available and result.Additional (where
LocalEnvVar structs are created) to combine key- and value-based heuristics:
call the existing looksLikeSensitive(key) and also run a value-based check
(implement a helper like looksLikeSensitiveValue(value) or extend
looksLikeSensitive to accept value) and set Sensitive = keySensitive ||
valueSensitive; use the combined boolean wherever isSensitive is used (including
the "both" and Additional branches referenced around looksLikeSensitive(key) and
localEnv[key]) so both key and value influence masking.
- Around line 328-333: Convert the byte-sliced prefix to a rune-safe slice
before passing to escapeForDisplay: turn value into r := []rune(value), compute
a rune-safe prefix like prefix := string(r[:min(2, len(r))]) and return
escapeForDisplay(prefix) + "••••••••"; also ensure the length check (currently
len(value) > 8) is updated to operate on rune count (len(r) > 8) so multibyte
characters are handled correctly and you never slice mid-rune when calling
escapeForDisplay.

---

Nitpick comments:
In `@pkg/stackbuild/envdetect_test.go`:
- Around line 932-974: Add a small integration-style test that writes minimal Go
and Rust files into a temp dir and runs detectEnvVars (e.g., create a
TestGoEnvIntegration/TestRustEnvIntegration alongside TestGoEnvPatterns) to
ensure stack-level rules (promotion and required vs optional) are enforced:
create a Go file that directly references GO_ENV and uses os.LookupEnv for
another var and assert detectEnvVars (or the GoStack detection flow) promotes
GO_ENV and marks the os.LookupEnv result as optional; do the same for Rust with
RUST_LOG to verify promotion; reference goEnvPatterns and
detectEnvVars/GoStack/RustStack in the test to locate the code paths being
exercised.
🪄 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: 3ada6e59-f6c0-4a08-8cdd-911cc4fd450d

📥 Commits

Reviewing files that changed from the base of the PR and between 60668c7 and 191bed9.

📒 Files selected for processing (19)
  • api/app/app_v1alpha/rpc.gen.go
  • api/app/envvar.go
  • api/app/rpc.yml
  • appconfig/appconfig.go
  • appconfig/appconfig_test.go
  • cli/commands/envdetect.go
  • cli/commands/envdetect_test.go
  • cli/commands/init.go
  • docs/docs/command/init.md
  • pkg/stackbuild/bun.go
  • pkg/stackbuild/envdetect_test.go
  • pkg/stackbuild/golang.go
  • pkg/stackbuild/node.go
  • pkg/stackbuild/python.go
  • pkg/stackbuild/ruby.go
  • pkg/stackbuild/rust.go
  • servers/app/app.go
  • servers/app/app_test.go
  • servers/build/build.go
✅ Files skipped from review due to trivial changes (4)
  • docs/docs/command/init.md
  • pkg/stackbuild/ruby.go
  • pkg/stackbuild/bun.go
  • cli/commands/envdetect_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • pkg/stackbuild/python.go
  • pkg/stackbuild/node.go
  • cli/commands/init.go

Comment thread api/app/envvar.go Outdated
Comment thread api/app/envvar.go Outdated
Comment thread pkg/stackbuild/golang.go Outdated
Comment thread pkg/stackbuild/golang.go
Comment thread pkg/stackbuild/rust.go Outdated
Comment thread servers/app/app.go
evanphx added 4 commits April 28, 2026 16:52
With init-time config now staged on app.initial_config, every
AppVersion is created by the build flow with a real image. Drop the
defensive empty-string checks in imageMatchesAppVersion and the
exec_proxy sandbox spec builder, plus the test case that pinned the
empty-image behavior.
Two related fixes to api/app/envvar.go:

- SetInitialEnvVars previously did a blind Update on the App entity
  after merging. Two parallel `miren init` invocations or a deploy
  slipping in between Get and Update could silently drop staged vars
  or land after activation. Wrap the read/merge/write in a CAS loop
  using EAC.Replace + entity revision; on conflict, re-read and retry
  up to maxAttempts before surfacing an error. Orphaned ConfigVersions
  on retry are correctness-safe (immutable, content-addressable).

- The shared merge logic (now extracted as mergeIntoSpec, used by both
  SetEnvVars and SetInitialEnvVars) previously errored when staging a
  service-scoped var against a service that had no entry in spec yet.
  That made the very first per-service env set on a fresh app or a
  service whose entries are otherwise filled in only at build time
  always fail. Append a new ConfigSpecServices entry instead, which
  matches the global Variables behavior. DeleteEnvVar still errors
  for unknown services since you can't delete from nothing.

Tests: replaced two service-not-found assertions with assertions
that the service entry gets created, plus a new SetInitialEnvVars
case that stages a service-scoped var on a fresh app.
CrudNew was treating any Get error as "app does not exist" and falling
through to Create. A real lookup failure (network, store down, etc.)
would produce a confusing follow-up Create attempt instead of being
surfaced.

Check errors.Is(err, cond.ErrNotFound{}) explicitly and return the
original error when it isn't, so genuine failures don't masquerade
as create operations.
Three related fixes to env var classification:

- pkg/stackbuild/golang.go goOptionalEnvPatterns blanket-matched any
  os.LookupEnv("VAR") call. LookupEnv is also commonly used to
  distinguish "unset" from "empty" for hard requirements, so the
  blanket pattern silently downgraded those to optional. Replace
  with narrow patterns that only match when a fallback is visible
  on the same line (cmp.Or). LookupEnv on its own now classifies
  as required, matching its real-world usage.

- pkg/stackbuild/golang.go GO_ENV was seeded as optional and
  pkg/stackbuild/rust.go RUST_LOG as recommended, but the source-
  derived loop only adds *missing* names, so a direct os.Getenv /
  std::env::var read for one of these never upgraded the seeded
  entry. Run elevateToRequired against the source scan at seed
  time and bump the confidence + reason when matched.

- pkg/stackbuild/python.go DJANGO_DEBUG had the same shape; apply
  the same elevation pattern for consistency.
@evanphx
Copy link
Copy Markdown
Contributor Author

evanphx commented Apr 29, 2026

Ok, back at this change.

We're rebased the work on top of main, which picks up the ConfigVersion change.

With that, there are broadly 2 concerns: env var detection and improving the init time story. I considered splitting this into 2 PRs, but that would mean the env var detection would just be part of the analyze output, and it's really there to help drive user interaction at init time.

So, I left them pulled together because init is really based on top of env var, and env var detection isn't much without usage at init time.

Copy link
Copy Markdown
Contributor

@phinze phinze left a comment

Choose a reason for hiding this comment

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

Claude and Paul back for round two. The architecture concern from last time is fully resolved. Staging on app.initial_config lets all the if image == "" guards go away, which is the cleanest possible signal the model is sitting where it should. The CAS retry on SetInitialEnvVars and the env classification sweep both look right, and the coderabbit pile has been mostly folded in.

Main thing missing is docs. The user-facing surface changed a lot here (server-side secrets, required vs recommended, --update, --analyze cross-referencing) and the only doc change is one auto-generated flag line. A walk-through in getting-started.md and the secrets-on-server split in app-configuration.md would go a long way; the narrative is kind of the whole feature.

One small follow-up worth filing: init --update post-deploy hits a confusing error. Trending toward terraform-init-style idempotence would be nice polish, not a blocker.

evanphx added 2 commits April 29, 2026 13:38
miren init now scans the project for required env vars and stages
generated/file-backed/default values server-side before the first
deploy. Document what gets detected, how confidence is assigned, what
ends up in app.toml vs. on the server, and the --update workflow.
Also reword the brief mention of init in deployment.md to point at the
new section, and add a changelog entry.
Talk about init's pre-set values the same way users already think
about miren config set, instead of leaking ConfigVersion / "server-
side" terminology. Same behavior, friendlier prose.
@evanphx
Copy link
Copy Markdown
Contributor Author

evanphx commented Apr 29, 2026

@phinze opt'd to just pull init --update out for now. You're right, it's a bit confusing, semantically. A user can delete their app.toml and run init if they really want the new functionality.

evanphx added 2 commits April 29, 2026 13:54
The two-mode behavior was confusing — users had to remember when to
pass --update vs. when not to, the flag's effect on detection was
subtle, and the only real benefit (re-detecting after adding a gem)
is fine to handle by deleting app.toml and re-running init.

Restore the original behavior: if app.toml already exists, init
errors out. The detection + secret pre-set path stays intact for
fresh inits.

Also drop appconfig.ParseWithoutValidation, which was added to
support --update's read-modify-write flow and has no remaining
callers.
@evanphx evanphx merged commit dccd14f into main Apr 29, 2026
14 checks passed
@evanphx evanphx deleted the mir-651-improve-initial-deploy-ie-setup-process branch April 29, 2026 21:20
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.

2 participants