Conversation
Short IDs landed in PR #696 and are allocated on every entity, but the CLI only used them in `sandbox list`. Everywhere else still showed full entity IDs — and `app status` was actively broken, slicing `deployment-CZdDHFf6Xyy99RrzRkxBg` with [:8] to produce "deployme" on every line. Added short_id fields to DeploymentInfo, DeploymentLockInfo, and VersionInfo RPC types. The deployment server now threads entity objects through listDeploymentsInternal so it can extract short IDs without extra RPCs, and batch-resolves app version short IDs for list views. The app server's List endpoint uses GetByIdWithEntity to populate version short IDs. On the CLI side, a new DisplayShortID helper provides graceful fallback — when the server hasn't been updated or short IDs aren't allocated, it falls back to CleanEntityID. Updated app status, app list, app history, deploy, env set/delete, set, activation poller, app rollback, and sandbox-pool list to use short IDs. Resolves MIR-947
📝 WalkthroughWalkthroughThis change introduces support for human-friendly short identifiers throughout the platform. Optional Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
servers/deployment/server.go (2)
509-514: Note: "Batch-resolve" is sequential under the hood.The
resolveShortIDshelper performs N sequentialEAC.Getcalls rather than a true batch operation. This is bounded by the limit (default 100) and acceptable for this use case, but be aware of potential latency impact for large lists.Consider adding a comment documenting this trade-off, or consider a future optimization if latency becomes noticeable:
// Batch-resolve app version short IDs + // Note: This performs sequential lookups; consider true batch API if latency is problematic. versionIDs := make([]string, 0, len(deployments))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/deployment/server.go` around lines 509 - 514, The current batch-resolve block that builds versionIDs and calls d.resolveShortIDs(ctx, versionIDs) relies on resolveShortIDs which internally issues N sequential EAC.Get calls (not a true batch), so add an inline comment above this block (near versionIDs / d.resolveShortIDs) documenting that resolveShortIDs performs sequential EAC.Get requests, is bounded by the configured limit (default 100), may add latency for large lists, and suggest future optimizations (true batch API, parallel requests with concurrency limit, or caching) if latency becomes a concern; reference resolveShortIDs, EAC.Get, versionIDs and deployments in the comment so readers can locate the behavior easily.
1349-1368:resolveShortIDssilently swallows errors without logging.When
d.EAC.Getfails, the function silently continues without logging the error. While this is acceptable for graceful degradation (missing short IDs are handled byDisplayShortIDfallback), consider adding debug-level logging to aid troubleshooting when short IDs unexpectedly don't appear.💡 Optional: Add debug logging for failed lookups
func (d *DeploymentServer) resolveShortIDs(ctx context.Context, ids []string) map[string]string { result := make(map[string]string, len(ids)) for _, id := range ids { if id == "" { continue } if _, ok := result[id]; ok { continue // already resolved } resp, err := d.EAC.Get(ctx, id) if err != nil { + d.Log.Debug("failed to resolve short ID", "entity_id", id, "error", err) continue } if sid := shortIDFromRPCEntity(resp.Entity()); sid != "" { result[id] = sid } } return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/deployment/server.go` around lines 1349 - 1368, The resolveShortIDs function currently swallows errors from d.EAC.Get; update it to log debug-level information when Get returns an error so failed lookups are visible during troubleshooting. Specifically, inside resolveShortIDs where you call d.EAC.Get(ctx, id) handle err by calling the server's logger (e.g., d.Logger.Debugf or equivalent) with a concise message including the id and err, but continue to preserve the graceful fallback behavior used by DisplayShortID; ensure the log call is only at debug level so normal behavior isn't noisy.
🤖 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/env.go`:
- Around line 403-404: The call to res.Deployment().AppVersionShortId() can
panic if Deployment() is nil—apply the same defensive pattern used in EnvSet:
first grab deployment := res.Deployment(), check if deployment == nil and fall
back to a safe value (e.g. use res.VersionId() or "unknown") for versionDisplay,
otherwise call deployment.AppVersionShortId(); then build versionDisplay and
call ctx.Printf as before so no nil dereference occurs when computing
versionDisplay.
- Around line 158-159: The call to res.Deployment().AppVersionShortId() can
panic if res.Deployment() is nil; add a defensive nil check before using
Deployment(): call dep := res.Deployment(); if dep == nil use a fallback (e.g.
empty string or "unknown") when computing versionDisplay via
ui.DisplayShortID(dep.AppVersionShortId(), res.VersionId()), and ensure
ctx.Printf still prints opts.App and the fallback versionDisplay; update the
code paths around ui.DisplayShortID, res.VersionId(), and ctx.Printf to use the
safe dep variable.
---
Nitpick comments:
In `@servers/deployment/server.go`:
- Around line 509-514: The current batch-resolve block that builds versionIDs
and calls d.resolveShortIDs(ctx, versionIDs) relies on resolveShortIDs which
internally issues N sequential EAC.Get calls (not a true batch), so add an
inline comment above this block (near versionIDs / d.resolveShortIDs)
documenting that resolveShortIDs performs sequential EAC.Get requests, is
bounded by the configured limit (default 100), may add latency for large lists,
and suggest future optimizations (true batch API, parallel requests with
concurrency limit, or caching) if latency becomes a concern; reference
resolveShortIDs, EAC.Get, versionIDs and deployments in the comment so readers
can locate the behavior easily.
- Around line 1349-1368: The resolveShortIDs function currently swallows errors
from d.EAC.Get; update it to log debug-level information when Get returns an
error so failed lookups are visible during troubleshooting. Specifically, inside
resolveShortIDs where you call d.EAC.Get(ctx, id) handle err by calling the
server's logger (e.g., d.Logger.Debugf or equivalent) with a concise message
including the id and err, but continue to preserve the graceful fallback
behavior used by DisplayShortID; ensure the log call is only at debug level so
normal behavior isn't noisy.
🪄 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: c7fbdfdc-c558-4235-abd4-32bcc7af8759
📒 Files selected for processing (16)
api/app/app_v1alpha/rpc.gen.goapi/app/rpc.ymlapi/deployment/deployment_v1alpha/rpc.gen.goapi/deployment/rpc.ymlcli/commands/activation_poller.gocli/commands/app_history.gocli/commands/app_list.gocli/commands/app_rollback.gocli/commands/app_status.gocli/commands/deploy.gocli/commands/env.gocli/commands/sandbox_pool_list.gocli/commands/set.gopkg/ui/entity.goservers/app/app.goservers/deployment/server.go
| versionDisplay := ui.DisplayShortID(res.Deployment().AppVersionShortId(), res.VersionId()) | ||
| ctx.Printf("✓ Set env vars on %s — new version: %s\n", opts.App, versionDisplay) |
There was a problem hiding this comment.
Potential nil dereference if res.Deployment() is nil.
The code calls res.Deployment().AppVersionShortId() without first checking if Deployment() returns nil. While the server should always return a deployment on success, defensive coding would add a check.
🛡️ Suggested defensive check
- versionDisplay := ui.DisplayShortID(res.Deployment().AppVersionShortId(), res.VersionId())
+ versionDisplay := res.VersionId()
+ if res.HasDeployment() && res.Deployment() != nil {
+ versionDisplay = ui.DisplayShortID(res.Deployment().AppVersionShortId(), res.VersionId())
+ }
ctx.Printf("✓ Set env vars on %s — new version: %s\n", opts.App, versionDisplay)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/commands/env.go` around lines 158 - 159, The call to
res.Deployment().AppVersionShortId() can panic if res.Deployment() is nil; add a
defensive nil check before using Deployment(): call dep := res.Deployment(); if
dep == nil use a fallback (e.g. empty string or "unknown") when computing
versionDisplay via ui.DisplayShortID(dep.AppVersionShortId(), res.VersionId()),
and ensure ctx.Printf still prints opts.App and the fallback versionDisplay;
update the code paths around ui.DisplayShortID, res.VersionId(), and ctx.Printf
to use the safe dep variable.
There was a problem hiding this comment.
res.Deployment() can't be nil here — the server always sets it on the success path (createEnvVarDeployment calls results.SetDeployment() at server.go:1160), and all error paths set results.Error which the CLI checks and returns early at lines 149-156.
| versionDisplay := ui.DisplayShortID(res.Deployment().AppVersionShortId(), res.VersionId()) | ||
| ctx.Printf("✓ Deleted env vars from %s — new version: %s\n", opts.App, versionDisplay) |
There was a problem hiding this comment.
Same nil dereference risk as EnvSet.
Apply the same defensive check for res.Deployment() here.
🛡️ Suggested defensive check
- versionDisplay := ui.DisplayShortID(res.Deployment().AppVersionShortId(), res.VersionId())
+ versionDisplay := res.VersionId()
+ if res.HasDeployment() && res.Deployment() != nil {
+ versionDisplay = ui.DisplayShortID(res.Deployment().AppVersionShortId(), res.VersionId())
+ }
ctx.Printf("✓ Deleted env vars from %s — new version: %s\n", opts.App, versionDisplay)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| versionDisplay := ui.DisplayShortID(res.Deployment().AppVersionShortId(), res.VersionId()) | |
| ctx.Printf("✓ Deleted env vars from %s — new version: %s\n", opts.App, versionDisplay) | |
| versionDisplay := res.VersionId() | |
| if res.HasDeployment() && res.Deployment() != nil { | |
| versionDisplay = ui.DisplayShortID(res.Deployment().AppVersionShortId(), res.VersionId()) | |
| } | |
| ctx.Printf("✓ Deleted env vars from %s — new version: %s\n", opts.App, versionDisplay) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/commands/env.go` around lines 403 - 404, The call to
res.Deployment().AppVersionShortId() can panic if Deployment() is nil—apply the
same defensive pattern used in EnvSet: first grab deployment :=
res.Deployment(), check if deployment == nil and fall back to a safe value (e.g.
use res.VersionId() or "unknown") for versionDisplay, otherwise call
deployment.AppVersionShortId(); then build versionDisplay and call ctx.Printf as
before so no nil dereference occurs when computing versionDisplay.
There was a problem hiding this comment.
Same as above — the error check at lines 394-401 guarantees we only reach this line when Deployment() is populated.
There was a problem hiding this comment.
@phinze, understood — the same server-side invariant applies: a successful response (past the HasError guard) always has Deployment() populated, so the nil-dereference concern doesn't hold. Thanks for the consistent explanation across both EnvSet and EnvDelete.
✏️ Learnings added
Learnt from: phinze
Repo: mirendev/runtime PR: 721
File: cli/commands/env.go:403-404
Timestamp: 2026-04-02T20:26:30.972Z
Learning: In `cli/commands/env.go` (mirendev/runtime), after the `if res.HasError() && res.Error() != ""` early-return guard in both `EnvSet` and `EnvDelete`, `res.Deployment()` is guaranteed to be non-nil by server contract. Do not flag `res.Deployment().AppVersionShortId()` calls in the success path as nil-dereference risks.
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: phinze
Repo: mirendev/runtime PR: 680
File: cli/commands/deploy.go:588-595
Timestamp: 2026-03-16T23:12:12.569Z
Learning: In `cli/commands/deploy.go` (mirendev/runtime), the `\r\033[K` ANSI escape pattern used for in-place progress updates in explain-mode (`--explain`) was pre-existing behavior before PR `#680`. It is a known issue that raw escape bytes can leak in non-TTY or `--explain-format=plain/rawjson` runs, but fixing it is deferred to a future cleanup PR.
Learnt from: phinze
Repo: mirendev/runtime PR: 680
File: cli/commands/deploy_ui.go:115-116
Timestamp: 2026-03-16T23:12:12.403Z
Learning: In `cli/commands/deploy_ui.go` (mirendev/runtime), `deployInfo.uploadPct` (float64) intentionally starts at `0.0` with no `-1` sentinel. The `-1` sentinel was removed during review because the manifest is always available in practice and the upload fraction arrives almost immediately, making `0%` as an initial state correct and acceptable UX. Do not flag this as a bug.
Learnt from: phinze
Repo: mirendev/runtime PR: 637
File: controllers/deployment/launcher.go:214-220
Timestamp: 2026-03-02T01:19:30.972Z
Learning: In controllers/deployment/launcher.go, the `cleanupOldVersionPools` function operates at the version level, not the service level. It scales down pools belonging to old versions regardless of which service they're for. If one service fails to create a pool during reconciliation, the other services' old pools still need cleanup once their new pools are ready—cleanup should not be skipped per-service.
Learnt from: phinze
Repo: mirendev/runtime PR: 203
File: cli/commands/set.go:25-31
Timestamp: 2025-09-02T23:17:15.167Z
Learning: Repo: mirendev/runtime — The `set` command in cli/commands/set.go is planned to be removed soon, so suggestions for improvements to this command should be avoided unless critical.
Learnt from: phinze
Repo: mirendev/runtime PR: 478
File: cli/commands/debug_netdb.go:155-156
Timestamp: 2025-12-15T17:54:35.854Z
Learning: In reviews of Go CLI commands under cli/commands (mirendev/runtime), prefer validating inputs on the server side and skip duplicating client-side validation when the CLI is the only client. This avoids code duplication and keeps validation centralized. If local validation is absolutely necessary for UX, keep it slim and clearly justified, and ensure it does not duplicate server logic. Document the rationale and ensure server API validation remains the source of truth.
Learnt from: phinze
Repo: mirendev/runtime PR: 637
File: controllers/deployment/launcher.go:848-853
Timestamp: 2026-03-02T01:19:45.274Z
Learning: In Go polling loops that call EAC.Get (EntityAccessClient.Get) in mirendev/runtime, avoid wrapping every call with a per-call context.WithTimeout. Instead rely on the parent context for cancellation and perform manual deadline checks between iterations (e.g., time.Now().After(deadline)) while also handling cancellation in the select on the parent context. This provides sufficient timeout enforcement without the overhead and complexity of creating a new child context per iteration. Apply this guideline to polling patterns in controllers/deployment/launcher.go and similar polling scenarios across the repository.
|
Re: the nitpicks on Sequential lookups comment (509-514): Skipping for now. The function name and existing comment are clear enough, and we'd rather document performance trade-offs when they become real concerns rather than speculatively. Debug logging in |
2 participants
Short IDs (PR #696, RFD-69) have been working great — every entity gets a compact, human-friendly identifier on creation, and
sandbox listwas already showing them. But that was the only place. Everywhere else in the CLI still showed full entity IDs likedeployment-CZdDHFf6Xyy99RrzRkxBg, andapp statushad a particularly embarrassing bug where it sliced the deployment ID with[:8]producing the literal string "deployme" on every line of recent activity.The core challenge was that most CLI commands get their IDs from RPC responses as plain strings, not as entity objects, so
ui.BriefId()couldn't be called directly. The fix takes a hybrid approach: enrich the key RPC response types with short ID fields (DeploymentInfo, DeploymentLockInfo, VersionInfo), populate them server-side from entity attributes we already have in hand, and useBriefId()directly where entity objects are available (like sandbox-pool list).The deployment server's
listDeploymentsInternalwas already fetching full entities but throwing them away after decoding — now it preserves them alongside the decoded structs sotoDeploymentInfocallers can extract short IDs for free. App version short IDs are batch-resolved to avoid N+1 queries in list views.On the CLI side, everything goes through a new
DisplayShortID(shortID, fullID)helper that gracefully falls back toCleanEntityIDwhen the server hasn't been updated or short IDs aren't available. This means the new CLI works fine against older servers — you just don't get short IDs until the server is deployed too.Sandbox list POOL/VERSION cross-references (which require batch entity resolution for referenced entities) are deferred for a follow-up.
Resolves MIR-947