Repository navigation
[3278 by Claude Opus 5.5] Avoid duplicate PROPR_INSTANCE_ID in hosted API container - #3279
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…NSTANCE_ID` entry. The new regression test passes, and the existing launcher tests (135), CLI typecheck/lint and the tunnel command tests (35) are all green. Cause: every app container already gets the id from the shared `--env-file`. `tunnelApiEnvArgs()` then always added `-e PROPR_INSTANCE_ID=<lowercased id>` for the API, and Docker records both entries without merging them. Fix (`docker/launcher/orchestrator.mjs`): - New helper `dockerEnvFileBindings()` reads the env file the way the Docker CLI does: the value is taken exactly as written, with no quote, comment or `export` handling. The launcher's own parser does strip quotes, so it can't show what the container will actually see. - Config: `resolveConfig()` records those values as `envFileProprInstanceIdBindings`. - Skip rule: `tunnelApiEnvArgs()` leaves out the `-e` override only when the env file already provides exactly the lowercased, trimmed id. In every other case it still adds the override, so lowercasing, validation, tunnel URL derivation and local (no-tunnel) behaviour are unchanged. Those cases are: - a mixed-case value - a quoted value, which Docker would pass through with the quotes - an id set only in the process environment, or one that differs from the env file - an env file the launcher can't check The env file itself is not modified, so the other services still get the id from it. - Type: I added the optional field to `packages/cli/src/orchestrator/types.ts`. The CLI's copy of the orchestrator is a gitignored build copy; I refreshed it locally only. Regression test: `test/orchestratorInstanceIdBinding.test.mjs` builds each container's final environment from its `docker run` arguments (env-file entries plus `-e` entries, not deduplicated, as Docker stores them). It checks that: - the hosted `propr-api` and every other app service have exactly one `PROPR_INSTANCE_ID=abc123`. - the derived tunnel/public API URLs are unchanged. - each override case above still adds the lowercased `-e` binding. - a local stack gets no binding. The two exactly-one tests fail against the unmodified launcher, as expected. Things to know: - Mixed-case or quoted ids still give the API two entries. Lowercasing has to override the file's value, and the file can't be changed. Fleet would still reject those cases; canonical lowercase ids, the normal case, now produce one entry. - Connect host-config path unchanged: when the CLI resolves config from pre-parsed env values, the exact Docker values aren't available, so the override is still added there. As far as I can see that path only inspects tunnel status and doesn't start containers. As the issue says, `integry/propr-fleet` needs to refresh its pinned launcher fixture before the blocked qualification provision is retried. There's nothing visual in this change, so I made no previews.
|
/review codex-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex-astraOverall EvaluationThe PR fixes duplicate identity injection for the usual lowercase hosted configuration, but needs changes to satisfy the visible requirement of exactly one binding while preserving normalization. ✅ Common hosted case fixed — A single canonical env-file binding suppresses the redundant API ✅ Literal values distinguished — The new parser preserves quotes and value whitespace, avoiding confusion between Docker’s literal input and normalized configuration. The issue description is truncated; this assessment uses its visible requirements. Current checks show no failures, with 22 pending. Review was static; no commands were run. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Normalization still produces duplicate identity bindings
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Verify Docker’s recorded environmentAn optional Docker-backed regression checking ScoreThe common hosted case is addressed with focused changes, but supported normalization cases still violate the central uniqueness requirement. Pending checks are not failures, although their completion remains necessary for final validation. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix F1 |
|
✅ Applied the requested follow-up changes in commit 1866381
Summary of ChangesI fixed F1: the hosted Why the old approach couldn't work: the shared env file can't be changed, so when it held a non-canonical value, any The fix (
The shared env file is never modified, and the other services keep using it. Tunnel URL derivation and the local (no-tunnel) behaviour are unchanged. In Tests (
Results:
Separate from F1, not changed: in two cases the launcher still can't see what Docker will read, so it keeps the old behaviour (shared file plus
🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…ly one `PROPR_INSTANCE_ID`, the lowercased id. That holds for mixed-case and quoted values too, and when the process environment overrides the file. I fixed F1: the hosted `propr-api` container now gets exactly one `PROPR_INSTANCE_ID`, the lowercased id. That holds for mixed-case and quoted values too, and when the process environment overrides the file. **Why the old approach couldn't work:** the shared env file can't be changed, so when it held a non-canonical value, any `-e` override became a second entry. **The fix** (`docker/launcher/orchestrator.mjs`, new `apiIdentityEnv()`): the launcher reads the shared env file once, the same way Docker does. That one read both decides what to do and supplies the content for the copy below. - **File already has exactly the lowercased id:** the API uses the shared file as is, with no `-e`. - **File has no binding:** the API uses the shared file plus one `-e` with the lowercased id. This is unchanged. - **Anything else** (mixed case, quotes, a repeated line, trailing whitespace, a bare `PROPR_INSTANCE_ID` line, or a differing process-env override): the API gets a private copy of the shared file with its `PROPR_INSTANCE_ID` lines removed and every other line kept exactly. It then gets one `-e` with the lowercased id. - The copy holds the stack's secrets, so it goes in a private temp directory (owner-only access) and the file itself is owner read/write only. - `startService` and `startServiceAsync` delete it in a `finally` once `docker run` has read it, including when the run fails. The shared env file is never modified, and the other services keep using it. Tunnel URL derivation and the local (no-tunnel) behaviour are unchanged. In `packages/cli/src/orchestrator/types.ts`, the config field from the earlier commit is replaced by a flag, `envFileDockerLiteral`, saying whether the env file can be read as Docker reads it. **Tests** (`test/orchestratorInstanceIdBinding.test.mjs`): the mixed-case, quoted and differing-override tests now expect a single lowercased entry. I added cases for: - repeated identical lines - a trailing-space value - a bare line resolved from the docker CLI's environment - the private copy: other lines kept exactly, owner-only permissions, shared file untouched, other services still using the shared file - a `startService` run against a stand-in `docker` on `PATH`, showing what `docker run` read and that the copy is deleted afterwards **Results:** - All 11 tests in that file pass. Against the previous launcher, 7 of them fail, as expected. - The launcher tests (`test/orchestrator*.test.mjs`, 140) pass, and so do the hosted-tunnel config tests. - CLI typecheck and eslint are clean. The CLI tunnel, connect and `hostActions` tests pass. `hostActions` needs `--experimental-test-module-mocks` to run at all. **Separate from F1, not changed:** in two cases the launcher still can't see what Docker will read, so it keeps the old behaviour (shared file plus `-e`) and a duplicate is possible: - **Pre-parsed env values:** when the CLI passes these in (the connect host-config path), the launcher doesn't read the file the way Docker does. As far as I can tell that path doesn't start containers. - **Unreadable env file:** `docker run` would fail on that file anyway. PR: #3279 Comment by: @integry (ID: 6280417728) Model: Claude Opus 5.5
|
/review codex-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex-astraOverall EvaluationThe PR removes the redundant API identity argument when the shared env file already contains the canonical value, and uses a private filtered copy when normalization requires an override. Ready to merge within the reviewed scope, conditional on pending checks completing successfully. ✅ Canonical binding preserved — A single canonical env-file binding produces no additional ✅ Shared configuration protected — Noncanonical API bindings are filtered into a private file with mode ✅ Lifecycle cleanup covered — Both synchronous and asynchronous starts remove scratch files in This was a static review of the supplied code; no commands or tests were run. The issue specification is explicitly truncated, so conclusions cover its visible requirements. The supplied current checks show no failures, with 22 still pending. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S2: 🟢 Remove test fixture directories
S3: 🟢 Exercise failure cleanup pathsThe new cleanup test covers successful synchronous startup. Additional tests for failed container launch and asynchronous startup would strengthen confidence that scratch files containing stack secrets are removed on those paths. The implementation already places cleanup in both ScoreThe implementation addresses the visible hosted duplication problem with focused changes and useful regression coverage. Static inspection found no introduced merge blocker; final CI results remain pending. Score: 9/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3278
Branch:
3278/claude-opus-5-5-avoid-duplicate-propr_ins-20261020-0020-gwlCommits: ✅ Changes committed (e5648d1)
AI Processing Completed
Execution Details:
Summary:
I've fixed it: the hosted API container now gets exactly one
PROPR_INSTANCE_IDentry. The new regression test passes, and the existing launcher tests (135), CLI typecheck/lint and the tunnel command tests (35) are all green.Cause: every app container already gets the id from the shared
--env-file.tunnelApiEnvArgs()then always added-e PROPR_INSTANCE_ID=<lowercased id>for the API, and Docker records both entries without merging them.Fix (
docker/launcher/orchestrator.mjs):New helper
dockerEnvFileBindings()reads the env file the way the Docker CLI does: the value is taken exactly as written, with no quote, comment orexporthandling. The launcher's own parser does strip quotes, so it can't show what the container will actually see.Config:
resolveConfig()records those values asenvFileProprInstanceIdBindings.Skip rule:
tunnelApiEnvArgs()leaves out the-eoverride only when the env file already provides exactly the lowercased, trimmed id. In every other case it still adds the override, so lowercasing, validation, tunnel URL derivation and local (no-tunnel) behaviour are unchanged. Those cases are:The env file itself is not modified, so the other services still get the id from it.
Type: I added the optional field to
packages/cli/src/orchestrator/types.ts. The CLI's copy of the orchestrator is a gitignored build copy; I refreshed it locally only.Regression test:
test/orchestratorInstanceIdBinding.test.mjsbuilds each container's final environment from itsdocker runarguments (env-file entries plus-eentries, not deduplicated, as Docker stores them). It checks that:propr-apiand every other app service have exactly onePROPR_INSTANCE_ID=abc123.-ebinding.The two exactly-one tests fail against the unmodified launcher, as expected.
Things to know:
As the issue says,
integry/propr-fleetneeds to refresh its pinned launcher fixture before the blocked qualification provision is retried. There's nothing visual in this change, so I made no previews.Detailed Logs:
f8090297-a581-473b-b912-54dcc5e3f31bLog files stored at:
/usr/src/app/logs/executions/issue-3278-2026-10-20T00-25-49-385Z-conversation.json/usr/src/app/logs/executions/issue-3278-2026-10-20T00-25-49-385Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3278.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!