Correct the deploy-with-stack skill's env precedence and ports/hostname advice - #289
Merged
Conversation
The deploy-with-stack skill states env precedence backwards: it says config.env overrides the inline environment: block, when later sources win and inline beats config.env, as docs/stack-files.md describes and test_pod_file_env_overrides_deployment_config asserts. It then recommends the pattern that mistake implies -- put defaults inline, override with --config at init -- which is exactly the shadowing the warning added in e4cb078 exists to report. Following it silently discards the deployer's value. In the case that prompted this, a composefile default of PUBLIC_BASE_URL=http://localhost survived --config PUBLIC_BASE_URL=https://<public-host>, and the application used it to decide whether to mark its session cookie Secure, so a public HTTPS deployment issued session cookies without it. Replace the bullet with the real ordering, the forwarding form (- SOME_VAR=${SOME_VAR}) as the way to let a deployment-time value through, and a note that stack deploy now reports shadowing. Also extend the hostnames bullet with the consequence of 56fd7df: a service needs no ports: to be addressable, so publishing one merely to obtain a hostname is unnecessary, and on compose exposes it on the host under --map-ports-to-host. docs/stack-files.md was updated alongside those fixes; the skill was not, and is the copy an agent reads first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Follow-up to #281.
docs/stack-files.mdwas corrected alongside the fixes in that issueand in #280;
skills/deploy-with-stack/SKILL.mdwas not, and it is the copy an agentreads before touching a stack file.
Env precedence
The skill states the ordering backwards:
docs/stack-files.md("Environment Variable Precedence") says later sources win —config.env, thenenv_file:, then inline — andtest_pod_file_env_overrides_deployment_configasserts it.The following sentence then recommends what that inversion implies: "Put sane defaults
inline for non-secret settings; anything the deployer should choose (external URLs,
feature flags) is supplied via
--configat init." That is the shadowing case thewarning added in e4cb078 exists to report, offered as the house style.
It is not a theoretical problem. Following it, a composefile carrying
PUBLIC_BASE_URL=http://localhostsilently survived--config PUBLIC_BASE_URL=https://<public-host>; the application used that value todecide whether to set
Secureon its session cookie, so a public HTTPS deploymentissued session cookies without it, over a login form. Nothing in the deploy output
mentioned an override — which is what e4cb078 now fixes, but the skill still steers
authors into the case.
Replaced with the real ordering, the forwarding form as the way to let a deployment-time
value through, and a mention of the new warning.
Hostnames and ports
Extended with the consequence of 56fd7df: a service needs no
ports:to be addressable,so there is no reason to publish one merely to obtain a hostname — and on the compose
target doing so exposes it on the host under
--map-ports-to-host. That was the othermistake made from reading the skill alone:
ports: 5432added to a database purely tomake its name resolve.
Not changed
The example composefile needs no edit — its inline entries (
DATABASE_HOST=dbandfriends) are fixed internal values, not deployer-settable ones, so they model the correct
use. Checked the other three no-paas skills too; none repeats either claim.
🤖 Generated with Claude Code