My earlier concern is fully addressed. The specific failure path I flagged — [services.console] with an args-only config silently producing a task with an empty command — is now closed.
At appconfig/appconfig.go lines 437–442, the console guard is the first check inside the if svcConfig.Args != nil block. That ordering is exactly right: it fires before the empty-args and command+args checks, so any args value on a service named console (nil, empty, or populated) gets rejected with a clear error pointing the user to [tasks.console]. And TestServiceArgsValidationAndParsing / "rejects args on the deprecated console service" exercises that path directly, asserting both halves of the error message.
Everything else from my earlier review is still solid — the buildServicesConfig schema threading, the mutual-exclusion validation, and the broader test coverage are all in good shape. This is ready to merge.
Posted to the PR's comment threads when you submit.
if svcConfig.Args != nil block, so [services.console] args = ["rails", "c"] (no command) is rejected before it can reach migrateConsoleService. The test in TestServiceArgsValidationAndParsing confirms it. Happy to resolve this now.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1108,
"verdict": "ready",
"event": "approve",
"summary": "My earlier concern is fully addressed. The specific failure path I flagged — `[services.console]` with an `args`-only config silently producing a task with an empty command — is now closed.\n\nAt `appconfig/appconfig.go` lines 437–442, the console guard is the **first** check inside the `if svcConfig.Args != nil` block. That ordering is exactly right: it fires before the empty-args and command+args checks, so any `args` value on a service named `console` (nil, empty, or populated) gets rejected with a clear error pointing the user to `[tasks.console]`. And `TestServiceArgsValidationAndParsing` / \"rejects args on the deprecated console service\" exercises that path directly, asserting both halves of the error message.\n\nEverything else from my earlier review is still solid — the `buildServicesConfig` schema threading, the mutual-exclusion validation, and the broader test coverage are all in good shape. This is ready to merge.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6d1Qd8",
"body": "The fix landed exactly as I described. Lines 437–442 put the console guard *first* inside the `if svcConfig.Args != nil` block, so `[services.console] args = [\"rails\", \"c\"]` (no command) is rejected before it can reach `migrateConsoleService`. The test in `TestServiceArgsValidationAndParsing` confirms it. Happy to resolve this now.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true
}I re-read the current head carefully. The merge conflict is resolved, the PR is mergeable, and the general structure of the feature is solid — schema threading, buildServicesConfig claiming services on Args != nil, the mutual-exclusion validation, and the blackbox test coverage are all still in good shape.
My earlier concern is partially addressed, but the specific failure path I flagged is still open.
The validation added at appconfig/appconfig.go lines 436–449 rejects two cases: an empty args = [] and a command + args combination. Neither of those prevents the silent data loss I described. The remaining hole is:
# [services.console] with args-only (no command) — this passes validation
[services.console]
args = ["rails", "c"]
This input passes all current validation checks: Args is non-nil, non-empty, and Command is "" so the mutual-exclusion guard doesn't fire. Then migrateConsoleService (build.go:323–327) creates a TaskConfig{Command: svc.Command, ...} where Command is still "" — the args are never transferred, because TaskConfig has no Args field by design. The result is a console task with an empty command that silently fails at runtime.
The thread was marked resolved by CodeRabbit's note "Addressed in commits 138a811 to cd36107," but I've verified in the current head that the args-only-on-console path reaches migrateConsoleService unimpeded. The code doesn't protect against it.
This is a narrow combination — legacy [services.console] plus the new args field — but the failure is silent: the deploy succeeds, the task exists, and it only breaks when someone actually tries to run a console. The fix is a one-liner in appconfig.Validate: inside the for serviceName, svcConfig := range ac.Services loop, if serviceName == appconfig.ConsoleName && svcConfig.Args != nil, return a ValidationError pointing at [tasks.console]. That's exactly what I described in the original thread.
Everything else in this PR is solid and I'd encourage shipping it once that guard is in place.
args-only-on-console migration hole is still open. [services.console] args = ["rails", "c"] (no command) passes all three checks here — non-nil, non-empty, no mutual-exclusion — then migrateConsoleService in build.go creates TaskConfig{Command: ""}, silently discarding the args. Add a guard specifically for the console name:if serviceName == appconfig.ConsoleName && svcConfig.Args != nil {
return &ValidationError{
KeyPath: svcPrefix + ".args",
Message: "service console: args is not supported on the deprecated [services.console]; use [tasks.console] with a command string instead",
}
}
This should sit inside the if svcConfig.Args != nil block, so the error only fires when args is actually set.
Posted to the PR's comment threads when you submit.
args-only on the console service name. A config with [services.console]\nargs = ["rails", "c"] (no command) still passes all three guards, reaches migrateConsoleService, and has its args silently dropped when TaskConfig{Command: svc.Command} is built — Command is "" and TaskConfig has no Args field. The thread was marked resolved, but the specific hole I described is still there. I'm leaving the thread open because the code doesn't close it yet.Verdict: caveats
The PR has a human approval from @evanphx and the implementation is correct for the happy path. I'll note the mergeable: false status — there's a merge conflict with main that needs resolving, but that's a process step, not a code problem.
One real correctness issue surfaced since my first review, flagged by CodeRabbit and confirmed by reading the code: migrateConsoleService silently drops args when it converts a legacy [services.console] block to a [tasks.console] task. At build.go:323–327:
ac.Tasks[appconfig.ConsoleName] = &appconfig.TaskConfig{
Command: svc.Command, // empty when only args was set
Trigger: appconfig.TriggerManual,
EnvVars: svc.EnvVars,
// Args never transferred — TaskConfig has no Args field
}
If someone writes [services.console] args = ["rails", "c"], the migrated console task has an empty command and fails silently at runtime. TaskConfig intentionally has no Args field (tasks are always shell-form by design), so the right fix is to reject args on [services.console] in appconfig.Validate with a clear error pointing at the deprecation — something like "service console: args is not supported on the deprecated [services.console]; use [tasks.console] with a command instead".
This path is narrow (the deprecated [services.console] config plus the new args field together), but the failure mode is a silently broken console that looked like it deployed fine. Worth a one-liner guard before merge.
Everything else I checked in my previous review stands. The schema threading, the buildServicesConfig claim-on-args fix, the mutual exclusion validation, the sandbox spec encoding, and the integration test coverage are all solid.
migrateConsoleService copies svc.Command and svc.EnvVars but not svc.Args. Since TaskConfig has no Args field, a user who writes [services.console] args = ["rails", "c"] will get a silently broken console task with an empty command.The cleanest fix is to reject args on the console service in appconfig.Validate before the migration runs. Adding a guard like:
if serviceName == appconfig.ConsoleName && svcConfig.Args != nil {
return &ValidationError{
KeyPath: svcPrefix + ".args",
Message: "service console: args is not supported on the deprecated [services.console]; use [tasks.console] with a command string instead",
}
}
This catches it at config-parse time with a clear message rather than silently misconfiguring the task at build time.
Posted to the PR's comment threads when you submit.
migrateConsoleService (build.go:323-327) creates a TaskConfig with only Command and EnvVars — Args is never transferred, and TaskConfig has no Args field by design. So a [services.console] with args produces a migrated task with an empty command. The fix belongs in appconfig.Validate: reject args on a service named console with an error pointing to [tasks.console]. I've left a per-line comment with the exact guidance.web = false and task-only apps — it's not introduced by this PR, and it's the kind of docs improvement that deserves its own pass rather than holding up this feature. I don't think it should block merge.Verdict: caveats
This is a well-executed feature addition. I read through the full stack — schema changes, config parsing, spec building, the containerd layer, and the tests — and I'm satisfied the mechanics are correct.
What it does: Adds an args field to [services.*] in app.toml that maps to OCI's exec-form argv array, replacing the image's CMD while leaving its ENTRYPOINT intact. This is distinct from the existing command field, which runs inside /bin/sh -c and always replaces both.
Key decisions I looked at carefully:
appspec.Build (around line 320) picks args over command and, crucially, does not prepend cfgSpec.Entrypoint to the args path — because oci.WithImageConfigArgs preserves the OCI image's own ENTRYPOINT at the containerd layer, while the shell-form command path needed the CNB launcher prefix to be runnable. That distinction is correct and important.buildServicesConfig in servers/build/build.go (line 582) marks a service as "claimed" when v.Args != nil, so a same-named Procfile entry can't silently replace an args-only service. Good defensive call.command and args is enforced in appconfig.Validate with a clear error message.sandbox_frozen_test.go hash is updated with a proper audit comment explaining why the saga path doesn't need a parallel edit (it reaches buildSubContainerSpec through sandboxOps.BootContainers).Test coverage: Strong at every layer — appconfig_test.go for parsing and validation, appspec_test.go for the spec-build semantics (including that an explicit opts.Command override wins over service Args), and a blackbox integration test that specifically verifies the dollar-sign-no-expansion and multi-word argument-boundary behavior that would regress most obviously.
One observation, not a blocker: The validation guard at appconfig.go:436 reads:
if svcConfig.Args != nil {
if len(svcConfig.Args) == 0 { ... }
}
The outer != nil check is redundant in practice — TOML decodes args = [] as an empty non-nil slice, so the nil path is unreachable via config. The inner check therefore fires correctly in all real cases. It's not a correctness bug and the test for "rejects an empty list" covers it.
This is a draft, but the substance is ready to graduate to human review. The implementation is complete, the edge cases are tested, and the documentation changes are present.
Verdict: ready