feat: environment values and named environments (#53)#127
Merged
michaelmcnees merged 12 commits intomainfrom Apr 7, 2026
Merged
feat: environment values and named environments (#53)#127michaelmcnees merged 12 commits intomainfrom
michaelmcnees merged 12 commits intomainfrom
Conversation
Implements Task 1-3 for issue #53 (environment variables and promotion). Adds values file parsing with unknown-key validation and env key normalization, plus a three-layer MergeInputs helper (defaults < values file < inline flags). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Inserts a values-file precedence layer between config env and MANTLE_ENV_* OS variables (config < values < MANTLE_ENV_*), exposed via SetValuesEnv. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add --values <file> flag to both `mantle run` and `mantle plan`. The run command merges values file inputs (lower priority than --input flags) and applies env overrides via CEL.SetValuesEnv. The plan command validates the values file early for fast failure feedback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds migration 018 creating the environments table with team-scoped unique name constraint, and implements Store with Create/Get/List/Delete backed by testcontainers integration tests (all 5 passing). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements `mantle env` (create/list/show/delete) backed by the environment.Store, and adds --env to `mantle run` (4-source input merge: named env < values file < --input flags) and `mantle plan` (existence validation only). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fy plan help text - Extend MergeInputs to 4 layers (defaults < env < values < inline) and update all call sites in values_test.go; add TestMergeInputs_FourLayerPrecedence - Refactor run.go to use workflow.MergeInputs instead of manual merge loops - Add environment name validation (^[a-z][a-z0-9-]*$) in Store.Create with test - Clarify plan --values and --env flag descriptions to note they do not affect diff Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds values-file parsing and input/env merging, a DB-backed named environment store with CLI management, wires --values/--env into run/plan to merge inputs, and extends the CEL Evaluator with SetValuesEnv to layer env as config → values → OS MANTLE_ENV_*. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI (mantle)
participant WF as Workflow (LoadValues / MergeInputs)
participant DB as Database (environments)
participant Eng as Engine (Executor)
participant CEL as CEL (Evaluator)
CLI->>WF: LoadValues(--values)?
WF-->>CLI: Values{Inputs,Env}
CLI->>DB: Get(--env name)?
DB-->>CLI: Environment{Inputs,Env}
CLI->>WF: MergeInputs(defaults, envInputs, valuesInputs, inlineInputs)
WF-->>CLI: merged inputs
CLI->>Eng: ExecuteWithOptions(merged inputs)
Eng->>CEL: SetConfigEnv(config env)
Eng->>CEL: SetValuesEnv(values + named env overlay)
Note right of CEL: OS MANTLE_ENV_* are also read (highest precedence)
Eng->>CEL: Eval(step expressions using env.<KEY>)
CEL-->>Eng: evaluated results
Eng-->>CLI: execution complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/engine/internal/workflow/values.go (1)
47-54: Potential silent data loss during env key normalization.When normalizing env keys to uppercase, if the input contains both
api_keyandAPI_KEY, one value will silently overwrite the other. Consider detecting and warning about (or rejecting) conflicting keys.🔧 Proposed fix to detect key collisions
// Normalize env keys to uppercase (matches config.go behavior). if len(vals.Env) > 0 { normalized := make(map[string]string, len(vals.Env)) for k, v := range vals.Env { - normalized[strings.ToUpper(k)] = v + upper := strings.ToUpper(k) + if existing, ok := normalized[upper]; ok && existing != v { + return nil, fmt.Errorf("duplicate env key after normalization: %q and another key both normalize to %q", k, upper) + } + normalized[upper] = v } vals.Env = normalized }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/workflow/values.go` around lines 47 - 54, Normalizing env keys with strings.ToUpper into vals.Env can silently overwrite values when both lowercase and uppercase variants exist (e.g., "api_key" and "API_KEY"); update the normalization in values.go to detect collisions by checking if normalized key already exists in the new map before assigning, and handle conflicts by either returning an error or emitting a clear warning (include both conflicting original keys and their values in the message) so the caller can resolve duplicates instead of losing data; reference the vals.Env variable, the normalized map creation, and strings.ToUpper usage when implementing the check.packages/engine/internal/cli/run.go (1)
79-89: Consider early values-file validation for consistency withplan.go.In
plan.go, the values file is validated before opening the database connection (fail-fast). Here, it's loaded after the DB and environment resolution. For consistency and to avoid unnecessary DB connections when the values file is invalid, consider moving this block beforedb.Open.♻️ Suggested reordering
+ // Load values file early so we fail fast on bad input. + var valuesInputs map[string]any + var valuesEnv map[string]string + if valuesFile != "" { + vals, valErr := workflow.LoadValues(valuesFile) + if valErr != nil { + return fmt.Errorf("loading values file: %w", valErr) + } + valuesInputs = vals.Inputs + valuesEnv = vals.Env + } + database, err := db.Open(cfg.Database) if err != nil { return fmt.Errorf("failed to connect to database: %w", err) } defer database.Close()Then remove the duplicate block at lines 79-89.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/run.go` around lines 79 - 89, The values-file loading block (using valuesFile and workflow.LoadValues) should be moved earlier to validate the values file before opening the database (db.Open) so we fail-fast like plan.go; relocate the existing valuesFile handling to precede the DB/env resolution and then remove the duplicate block in run.go (the vals, valErr -> valuesInputs/valuesEnv section) to avoid redundant loading and unnecessary DB connections.packages/engine/internal/cli/env.go (1)
117-129: Consider sorting keys for deterministic output.Map iteration order in Go is non-deterministic. For better UX and testability, consider sorting the keys before printing.
♻️ Suggested fix for deterministic output
+import "sort" if len(env.Inputs) > 0 { fmt.Fprintln(cmd.OutOrStdout(), "\nInputs:") + inputKeys := make([]string, 0, len(env.Inputs)) + for k := range env.Inputs { + inputKeys = append(inputKeys, k) + } + sort.Strings(inputKeys) - for k, v := range env.Inputs { + for _, k := range inputKeys { + v := env.Inputs[k] fmt.Fprintf(cmd.OutOrStdout(), " %s: %v\n", k, v) } } if len(env.Env) > 0 { fmt.Fprintln(cmd.OutOrStdout(), "\nEnv:") + envKeys := make([]string, 0, len(env.Env)) + for k := range env.Env { + envKeys = append(envKeys, k) + } + sort.Strings(envKeys) - for k, v := range env.Env { + for _, k := range envKeys { + v := env.Env[k] fmt.Fprintf(cmd.OutOrStdout(), " %s: %s\n", k, v) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/env.go` around lines 117 - 129, The output loops over env.Inputs and env.Env (in the printing code using fmt.Fprintf and fmt.Fprintln) rely on Go map iteration which is non-deterministic; to make output deterministic and testable, collect the map keys for env.Inputs and env.Env into slices, sort them with sort.Strings, and then iterate the sorted key slices when printing instead of ranging directly over the maps (preserve the existing formatting and use the same fmt.Fprintf calls inside the sorted loops).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/engine/internal/cli/env.go`:
- Around line 135-155: The delete command currently calls store.Delete(...) but
does not emit an audit event; update the Store to accept an AuditEmitter
dependency (add an AuditEmitter field on the Store struct and wire it in via
newEnvStore), then after a successful call to Store.Delete(ctx, name) inside
newEnvDeleteCommand call the emitter (e.g., store.AuditEmitter.Emit or
EmitEvent) with the command context, the operation name ("environment.delete"),
target resource identifier (args[0]) and any relevant metadata (actor,
timestamp, outcome) so every state-changing delete emits an audit event; ensure
nil-checking of store.AuditEmitter so the code remains safe if not provided.
---
Nitpick comments:
In `@packages/engine/internal/cli/env.go`:
- Around line 117-129: The output loops over env.Inputs and env.Env (in the
printing code using fmt.Fprintf and fmt.Fprintln) rely on Go map iteration which
is non-deterministic; to make output deterministic and testable, collect the map
keys for env.Inputs and env.Env into slices, sort them with sort.Strings, and
then iterate the sorted key slices when printing instead of ranging directly
over the maps (preserve the existing formatting and use the same fmt.Fprintf
calls inside the sorted loops).
In `@packages/engine/internal/cli/run.go`:
- Around line 79-89: The values-file loading block (using valuesFile and
workflow.LoadValues) should be moved earlier to validate the values file before
opening the database (db.Open) so we fail-fast like plan.go; relocate the
existing valuesFile handling to precede the DB/env resolution and then remove
the duplicate block in run.go (the vals, valErr -> valuesInputs/valuesEnv
section) to avoid redundant loading and unnecessary DB connections.
In `@packages/engine/internal/workflow/values.go`:
- Around line 47-54: Normalizing env keys with strings.ToUpper into vals.Env can
silently overwrite values when both lowercase and uppercase variants exist
(e.g., "api_key" and "API_KEY"); update the normalization in values.go to detect
collisions by checking if normalized key already exists in the new map before
assigning, and handle conflicts by either returning an error or emitting a clear
warning (include both conflicting original keys and their values in the message)
so the caller can resolve duplicates instead of losing data; reference the
vals.Env variable, the normalized map creation, and strings.ToUpper usage when
implementing the check.
🪄 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: a58e3b56-3b00-456f-a22c-8781d87e8ef4
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
packages/engine/internal/cel/cel.gopackages/engine/internal/cel/cel_test.gopackages/engine/internal/cel/cel_values_test.gopackages/engine/internal/cli/env.gopackages/engine/internal/cli/plan.gopackages/engine/internal/cli/root.gopackages/engine/internal/cli/run.gopackages/engine/internal/db/migrations/018_environments.sqlpackages/engine/internal/environment/store.gopackages/engine/internal/environment/store_test.gopackages/engine/internal/workflow/values.gopackages/engine/internal/workflow/values_test.go
Comment on lines
+135
to
+155
| func newEnvDeleteCommand() *cobra.Command { | ||
| return &cobra.Command{ | ||
| Use: "delete <name>", | ||
| Short: "Delete an environment", | ||
| Args: cobra.ExactArgs(1), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| store, cleanup, err := newEnvStore(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer cleanup() | ||
|
|
||
| if err := store.Delete(cmd.Context(), args[0]); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fmt.Fprintf(cmd.OutOrStdout(), "Deleted environment %q\n", args[0]) | ||
| return nil | ||
| }, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if environment.Store.Delete emits audit events
rg -n -A 15 'func.*Delete.*context' packages/engine/internal/environment/store.go | head -40Repository: dvflw/mantle
Length of output: 554
🏁 Script executed:
cat -n packages/engine/internal/environment/store.go | sed -n '125,160p'Repository: dvflw/mantle
Length of output: 709
🏁 Script executed:
rg -n -B 5 -A 10 'Audit.*Delete|EmitAudit.*Delete' packages/engine/internal/environment/Repository: dvflw/mantle
Length of output: 38
🏁 Script executed:
rg -n -B 2 -A 20 'func.*Store.*Create.*ctx context' packages/engine/internal/environment/store.go | head -50Repository: dvflw/mantle
Length of output: 843
🏁 Script executed:
rg -n -B 5 -A 15 'type Store struct' packages/engine/internal/environment/store.goRepository: dvflw/mantle
Length of output: 750
🏁 Script executed:
rg -n -A 35 'func.*Store.*Create.*ctx context' packages/engine/internal/environment/store.go | head -70Repository: dvflw/mantle
Length of output: 1245
🏁 Script executed:
rg -n 'AuditEmitter|EmitAudit' packages/engine/internal/environment/store.goRepository: dvflw/mantle
Length of output: 38
🏁 Script executed:
rg -n 'type AuditEmitter|interface.*AuditEmitter' packages/engine/internal/Repository: dvflw/mantle
Length of output: 38
🏁 Script executed:
rg -n 'AuditEmitter' packages/engine/internal/ | head -20Repository: dvflw/mantle
Length of output: 38
🏁 Script executed:
rg -r 'AuditEmitter' packages/ --type goRepository: dvflw/mantle
Length of output: 38
🏁 Script executed:
cat -n CLAUDE.md | head -50Repository: dvflw/mantle
Length of output: 2632
Emit audit event for delete operation.
The Delete method performs a state-changing operation but does not emit an audit event. Per the architecture principle ("Audit from day one — every state-changing operation emits an audit event via the AuditEmitter interface"), this operation must emit an audit event via the AuditEmitter interface. The Store struct will need an AuditEmitter dependency injected to support this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/engine/internal/cli/env.go` around lines 135 - 155, The delete
command currently calls store.Delete(...) but does not emit an audit event;
update the Store to accept an AuditEmitter dependency (add an AuditEmitter field
on the Store struct and wire it in via newEnvStore), then after a successful
call to Store.Delete(ctx, name) inside newEnvDeleteCommand call the emitter
(e.g., store.AuditEmitter.Emit or EmitEvent) with the command context, the
operation name ("environment.delete"), target resource identifier (args[0]) and
any relevant metadata (actor, timestamp, outcome) so every state-changing delete
emits an audit event; ensure nil-checking of store.AuditEmitter so the code
remains safe if not provided.
Collaborator
Author
There was a problem hiding this comment.
Pushing back on this one. secret.Store — the closest analog — does not have an AuditEmitter. Audit events in the CLI layer are emitted by commands that modify execution state (cancel.go, rotate_key.go), not by CRUD stores. Adding an AuditEmitter to environment.Store alone would be inconsistent.
If we want audit coverage for all CRUD operations (secrets, environments, etc.), that should be a separate cross-cutting concern. Happy to file an issue for it.
Contributor
There was a problem hiding this comment.
@michaelmcnees, that's a fair point — applying audit emission only to environment.Store while secret.Store and other CRUD stores are unadjusted would be inconsistent, and the existing pattern of emitting audit events in execution-state commands (not stores) is a reasonable architectural boundary.
If you'd like, I can open a GitHub issue to track cross-cutting audit coverage for all CRUD stores (secrets, environments, etc.) as a follow-up. Just say the word.
✏️ Learnings added
Learnt from: michaelmcnees
Repo: dvflw/mantle PR: 127
File: packages/engine/internal/cli/env.go:146-166
Timestamp: 2026-04-06T04:47:39.490Z
Learning: In the Mantle project, audit events are emitted at the CLI command layer (e.g., `cancel.go`, `rotate_key.go`) only for execution-state-changing operations, not inside CRUD stores (e.g., `secret.Store`, `environment.Store`). Do not flag missing audit event emission in CRUD store methods; that is a separate cross-cutting concern planned for a future milestone.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T05:02:25.695Z
Learning: Applies to packages/engine/cmd/**/*.go : Use Cobra for CLI framework implementation.
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T05:02:25.695Z
Learning: Applies to packages/engine/**/*.go : Configuration should use mantle.yaml with CLI flag and env var overrides.
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T04:56:56.455Z
Learning: Applies to packages/engine/internal/**/*.go : Every state-changing operation must emit an audit event via the AuditEmitter interface.
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T05:02:25.695Z
Learning: Applies to packages/engine/**/*.go : Every state-changing operation must emit an audit event via the AuditEmitter interface.
Learnt from: michaelmcnees
Repo: dvflw/mantle PR: 0
File: :0-0
Timestamp: 2026-03-25T12:58:26.665Z
Learning: In the Mantle project, the audit emitter is a `NoopEmitter` in V1. Rollback audit events (e.g., `ActionArtifactDeleted`) are intentionally deferred to V1.1 when the Postgres-backed auditor is implemented. Do not flag missing rollback audit events as a review issue until then.
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T05:02:25.695Z
Learning: Applies to packages/engine/**/*.go : Engine development happens in `packages/engine/`. Run `go test ./...` and `go build ./cmd/mantle` from that directory.
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T04:56:56.455Z
Learning: Applies to packages/engine/**/*.go : Engine development happens in `packages/engine/`. Run `go test ./...` and `go build ./cmd/mantle` from that directory.
Learnt from: CR
Repo: dvflw/mantle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-27T04:56:56.455Z
Learning: Applies to mantle.yaml : Use mantle.yaml for configuration with CLI flag and env var overrides.
Learnt from: michaelmcnees
Repo: dvflw/mantle PR: 45
File: internal/connector/email_receive.go:126-135
Timestamp: 2026-03-25T19:44:56.827Z
Learning: When reviewing Go code that uses go-imap v2 `github.com/emersion/go-imap/v2/imapclient.Client.Store(numSet, store, options)`, do not require a separate `UIDStore` method call. The method decides between `UID STORE` and plain `STORE` at runtime based on `numSet`’s concrete type: passing an `imap.UIDSet` should be treated as correct and will send `UID STORE`, while `imap.SeqSet` will send plain `STORE`. Only flag `client.Store` usage if `numSet` is of an unexpected type or the call signature is otherwise incorrect.
- Sort map keys in `env show` output for deterministic display - Move values file loading before DB open in run.go (fail-fast) - Detect env key collisions during uppercase normalization in values.go Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
Review feedback addressed in f2943f6Nitpick: sorted map output in Nitpick: fail-fast values loading in Nitpick: env key collision detection in Inline: audit events on |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/engine/internal/workflow/values_test.go (1)
205-233: Consider renaming test to reflect actual coverage.This test is named
FullPrecedencebut passesnilforenvInputs, testing only 3 of the 4 layers. The actual 4-layer test isTestMergeInputs_FourLayerPrecedencebelow.📝 Suggested rename
-func TestMergeInputs_FullPrecedence(t *testing.T) { +func TestMergeInputs_ThreeLayerPrecedence(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/workflow/values_test.go` around lines 205 - 233, The test TestMergeInputs_FullPrecedence is misnamed because it passes nil for envInputs and therefore only verifies three layers; either rename it to reflect three-layer coverage (e.g., TestMergeInputs_ThreeLayerPrecedence) or modify the test to include a non-nil envInputs map and assertions to verify that envInputs fit into the precedence chain; update the test name or add an envInputs variable and corresponding expectation checks so the test's behavior matches its name, referencing MergeInputs, TestMergeInputs_FullPrecedence and TestMergeInputs_FourLayerPrecedence to keep naming consistent with the existing four-layer test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/engine/internal/workflow/values_test.go`:
- Around line 205-233: The test TestMergeInputs_FullPrecedence is misnamed
because it passes nil for envInputs and therefore only verifies three layers;
either rename it to reflect three-layer coverage (e.g.,
TestMergeInputs_ThreeLayerPrecedence) or modify the test to include a non-nil
envInputs map and assertions to verify that envInputs fit into the precedence
chain; update the test name or add an envInputs variable and corresponding
expectation checks so the test's behavior matches its name, referencing
MergeInputs, TestMergeInputs_FullPrecedence and
TestMergeInputs_FourLayerPrecedence to keep naming consistent with the existing
four-layer test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7075aff7-e804-4a86-9a10-332f5ed7f604
📒 Files selected for processing (4)
packages/engine/internal/cli/env.gopackages/engine/internal/cli/run.gopackages/engine/internal/workflow/values.gopackages/engine/internal/workflow/values_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/engine/internal/workflow/values.go
- packages/engine/internal/cli/env.go
7 tasks
The test passes nil for envInputs, exercising only 3 of the 4 merge layers. Rename to accurately reflect actual coverage; the full 4-layer test already exists as TestMergeInputs_FourLayerPrecedence. https://claude.ai/code/session_01ScMbDdk1f4m7HGyjfTrmv1
Collaborator
Author
|
@coderabbitai Addressed your feedback — renamed Generated by Claude Code |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3095
The test naming is now precise and the coverage is complete. LGTM! 🧠 Learnings used |
Resolves CodeRabbit docstring coverage warning on PR #127. All six functions in cli/env.go now have Go doc comments, bringing coverage from ~17% to 100% for that file. https://claude.ai/code/session_01E2yrXXhJW7Jt7ZPt7JYMWF
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/engine/internal/cli/env.go (1)
96-101: Consider adding timezone indicator to timestamp output.The timestamp format
"2006-01-02 15:04:05"doesn't indicate the timezone. Users in different timezones viewing the same environment may find this confusing.🔧 Optional: Add UTC suffix or use RFC3339
for _, e := range envs { - fmt.Fprintf(w, "%s\t%s\n", e.Name, e.CreatedAt.Format("2006-01-02 15:04:05")) + fmt.Fprintf(w, "%s\t%s\n", e.Name, e.CreatedAt.UTC().Format("2006-01-02 15:04:05 UTC")) }Alternatively, use
time.RFC3339for an unambiguous ISO format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/engine/internal/cli/env.go` around lines 96 - 101, The printed timestamps in the env listing (inside the loop that calls fmt.Fprintf with e.CreatedAt.Format("2006-01-02 15:04:05")) lack timezone information; update that formatting to include timezone or use an unambiguous RFC3339 format (e.g., use time.RFC3339 or append the timezone with "MST"/offset) and ensure you convert to the desired zone (e.g., e.CreatedAt.UTC() if you want UTC) before formatting so the output in fmt.Fprintf shows the timezone/offset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/engine/internal/cli/env.go`:
- Around line 96-101: The printed timestamps in the env listing (inside the loop
that calls fmt.Fprintf with e.CreatedAt.Format("2006-01-02 15:04:05")) lack
timezone information; update that formatting to include timezone or use an
unambiguous RFC3339 format (e.g., use time.RFC3339 or append the timezone with
"MST"/offset) and ensure you convert to the desired zone (e.g.,
e.CreatedAt.UTC() if you want UTC) before formatting so the output in
fmt.Fprintf shows the timezone/offset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8dbf3bd-e4ba-48ae-87f4-5d77606978c6
📒 Files selected for processing (1)
packages/engine/internal/cli/env.go
- Add UTC indicator to env list timestamp output - Add missing godoc comments to credentials, health, db, and metrics packages https://claude.ai/code/session_01KiE3FJpfF6XE2y3Tyth2r3
Collaborator
Author
|
@coderabbitai The latest commit (e37f3fc) addresses your remaining feedback:
Please re-review when you get a chance. Generated by Claude Code |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 910 🏁 Script executed: Length of output: 1546
However, after checking the four packages mentioned for docstring coverage, the
No // Package health provides HTTP handlers for liveness and readiness probes.
package healthCould you double-check that commit 🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/engine/internal/cli/env.go`:
- Around line 77-79: The env list command currently allows positional arguments;
update the cobra.Command instantiation for the "list" command (the struct with
Use: "list", Short: "List all environments", RunE: ...) to set Args to
cobra.NoArgs so the command rejects any extra positional arguments; locate the
anonymous command object and add the Args: cobra.NoArgs field alongside
Use/Short/RunE to enforce zero args.
🪄 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: 35f62fbc-72ff-4e74-badd-c4274cbb36b7
📒 Files selected for processing (5)
packages/engine/internal/api/health/health.gopackages/engine/internal/cli/credentials.gopackages/engine/internal/cli/env.gopackages/engine/internal/db/db.gopackages/engine/internal/metrics/metrics.go
✅ Files skipped from review due to trivial changes (4)
- packages/engine/internal/api/health/health.go
- packages/engine/internal/db/db.go
- packages/engine/internal/cli/credentials.go
- packages/engine/internal/metrics/metrics.go
- Add cobra.NoArgs to env list command to reject accidental extra args - Add package doc comments to health, db, metrics, and cli packages https://claude.ai/code/session_01Kt2cz2tqC28r9b6bM1c7LE
Collaborator
Author
|
@coderabbitai the remaining feedback from your last review has been addressed in commit e3b03fc:
Please re-review when you get a chance. Generated by Claude Code |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 877 🏁 Script executed: Length of output: 1354
All review feedback has been addressed. This PR looks great — approving! 🚀 [approve] 🧠 Learnings used |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--values) for supplying input and env overrides from YAML files, similar to Helm--valuesor Terraform.tfvars--env) backed by Postgres, enablingmantle run workflow --env productionwithout a file on diskmantle env create/list/show/deleteCLI subcommands for environment CRUDworkflow defaults < named env < values file < --input flagsenvvariable respects the same layering:config < values/env < MANTLE_ENV_*New Commands
Test plan
LoadValues,LoadValuesBytes(8 cases: valid, missing file, bad YAML, unknown keys, inputs-only, env-only, uppercase normalization, empty)MergeInputs(7 cases including 4-layer precedence)SetValuesEnv(3 cases: layering, OS precedence, nil clears)EnvironmentStoreCRUD against real Postgres via testcontainers (6 cases: create+get, not found, list, delete, duplicate name, invalid name)go vet ./...cleango build ./cmd/mantlecleanCloses #53
🤖 Generated with Claude Code
Note
Medium Risk
Adds a new Postgres-backed
environmentstable plus new CLI surfaces and changes howmantle runassembles inputs/env, which can affect execution behavior and requires migration correctness.Overview
Adds values-file overrides and persistent named environments for parameterizing workflow execution.
mantle runnow supports--values(load YAMLinputs/env) and--env(fetch stored overrides from Postgres), and merges inputs with precedence named env < values file < inline--input;planadds flags to validate a values file or environment exists.Introduces
mantle env create/list/show/deletefor CRUD of named environments, backed by a newenvironmentstable/migration andenvironment.Storeimplementation, and updates CELenvresolution to include an intermediate “values/env overrides” layer between config andMANTLE_ENV_*OS variables (with new tests covering precedence and parsing/validation).Reviewed by Cursor Bugbot for commit 767999a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests