fix(secrets): read one service's env with --service; refuse to run on an ambiguous name - #193
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/secrets.ts">
<violation number="1" location="src/commands/secrets.ts:54">
P2: The collision guard fails open on platforms that do not honor the new `on_collision=withhold` query param. Every general read now sends `on_collision=withhold`, so the safety of `insta run` (and the .env write) rests entirely on the platform honoring it. The PR's own rollout note says non-service reads still default to merge until the platform ships withhold; on such a platform the response is the merged value with no `collisions` field, and `?? []` reads that as "no collisions". `runWithSecrets` then spawns the child with the merged (leaking) value and no refusal — exactly the leak this change is meant to close — because "platform withheld, none collided" and "platform merged, no report" are indistinguishable. Consider failing closed (e.g. surfacing a warning that withhold couldn't be confirmed) until the platform feature is guaranteed, rather than treating an absent `collisions` field as proof the read was safe.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ): Promise<SecretBundle | null> { | ||
| const res = await api.rawRequest('GET', `/projects/${projectId}/secrets${bundleQuery(opts)}`) | ||
| if (handleApproval(res, opts.json)) return null | ||
| return { secrets: res.body.secrets as Record<string, string>, collisions: (res.body.collisions ?? []) as Collision[] } |
There was a problem hiding this comment.
P2: The collision guard fails open on platforms that do not honor the new on_collision=withhold query param. Every general read now sends on_collision=withhold, so the safety of insta run (and the .env write) rests entirely on the platform honoring it. The PR's own rollout note says non-service reads still default to merge until the platform ships withhold; on such a platform the response is the merged value with no collisions field, and ?? [] reads that as "no collisions". runWithSecrets then spawns the child with the merged (leaking) value and no refusal — exactly the leak this change is meant to close — because "platform withheld, none collided" and "platform merged, no report" are indistinguishable. Consider failing closed (e.g. surfacing a warning that withhold couldn't be confirmed) until the platform feature is guaranteed, rather than treating an absent collisions field as proof the read was safe.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/secrets.ts, line 54:
<comment>The collision guard fails open on platforms that do not honor the new `on_collision=withhold` query param. Every general read now sends `on_collision=withhold`, so the safety of `insta run` (and the .env write) rests entirely on the platform honoring it. The PR's own rollout note says non-service reads still default to merge until the platform ships withhold; on such a platform the response is the merged value with no `collisions` field, and `?? []` reads that as "no collisions". `runWithSecrets` then spawns the child with the merged (leaking) value and no refusal — exactly the leak this change is meant to close — because "platform withheld, none collided" and "platform merged, no report" are indistinguishable. Consider failing closed (e.g. surfacing a warning that withhold couldn't be confirmed) until the platform feature is guaranteed, rather than treating an absent `collisions` field as proof the read was safe.</comment>
<file context>
@@ -4,23 +4,99 @@ import { join } from 'node:path'
+): Promise<SecretBundle | null> {
+ const res = await api.rawRequest('GET', `/projects/${projectId}/secrets${bundleQuery(opts)}`)
+ if (handleApproval(res, opts.json)) return null
+ return { secrets: res.body.secrets as Record<string, string>, collisions: (res.body.collisions ?? []) as Collision[] }
+}
+
</file context>
… its branch Three review findings from #193, all of which reproduce. childEnv deleted colliding names by exact key. The spread that builds the child environment copies process.env into a PLAIN object, which loses the case-insensitivity Windows env names have — while CreateProcess keeps it. So a parent that exported `Admin_Password` survived a delete of `ADMIN_PASSWORD` and the child read it as ADMIN_PASSWORD: the inheritance hole this branch exists to close, still open on one platform. The delete is now case-insensitive on win32 and stays exact elsewhere, where `Admin_Password` is a different variable that is none of our business. The platform is a parameter, not `process.platform`, so both branches run on every CI host — the Windows job could not have caught this anyway, since the tests inject spawnImpl and no real child environment is ever read. `secrets unset --service` sent no branch when --branch was omitted. A service exists on a branch, so the platform rejects that pair — the flag could not delete a single service's copy at all without a second flag. It now defaults to the linked branch exactly as `secrets set --service` does, and both the human line and --json report the effective branch, so the user can see which scope was actually deleted. The remediation hints dropped an explicit --branch: after `insta secrets --branch feat-x`, the suggested `insta secrets --service compute/hermes` would have read the linked branch instead — different secrets, no sign of the swap. collisionLines and run's refusal now name the branch whenever it is not the linked one. Not addressed, deliberately: a platform that ignores on_collision returns a merged value with no `collisions` field, which `?? []` reads as "nothing collided". That fails open. It is resolved by shipping order — insta-platform#388 merges before this CLI is released — per the repo owner, and fetchSecretBundle now carries a comment saying so, so nobody later reads that fallback as unconditionally safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The code paths for collision handling look sound, but the PR is missing the repo-required command reference update for the new flags.
Requirements context
I based intent on the PR description at #193, the new README behavior notes in README.md:113-128, and the repo guidance in AGENTS.md:15-17 / .claude/skills/developing-insta-cli/SKILL.md:36-38. The linked platform PR InsForge/insta-platform#388 was not accessible from this environment: https://github.com/InsForge/insta-platform/pull/388 returned 404, so I assessed the platform dependency from the PR description alone.
Findings
Critical:
AGENTS.md:15-17,.claude/skills/developing-insta-cli/SKILL.md:36-38,src/index.ts:93-99,src/index.ts:176-188— The PR adds public flags oninsta run,insta secrets, andinsta secrets unset, but the requiredskills/insta/cli-reference.mdmirror is not included in this change. The repo guide calls command/flag reference updates non-negotiable and says new or renamed flags are only half-done until that document is updated in the same change set. The PR description also lists this as outstanding, so this blocks merge under the stated repo requirements.
Suggestion:
src/commands/secrets.ts:166-172— Consider applying the new empty---serviceguard to the existingsecrets set --servicepath too. This PR correctly guards the new read/unset/run service paths, butsecrets set NAME value --service ""still falls through as an unscoped write because the body only includesservicewhen the option is truthy.
Information:
src/commands/secrets.ts:36-61,src/commands/run.ts:87-105,test/secrets-collisions.test.ts:43-385— Software engineering/functionality coverage is strong for the changed behavior: query construction, stderr-only collision reporting, service-scoped fetch/delete, refusal without spawn,--ignore-collisions, and Windows case-insensitive env handling are all covered by focused tests.src/commands/secrets.ts:36-61,src/commands/run.ts:50-102— No security-relevant issue found in the changed code: user-controlled branch/service/name values are URL-encoded for HTTP paths/query params, no secret values are newly logged, and no new dependencies are introduced.src/commands/secrets.ts:72-90,src/commands/run.ts:50-69— No performance issue found; the new work is bounded formatting plus small environment-object/set operations on CLI command paths.- Local verification could not complete in this checkout:
npm run typecheckfailed withtsc: not found, and the affected Vitest run could not loadvitest/configbecause dependencies are not installed.git diff --check main...HEADpassed.
Verdict
Request changes. The code implementation is otherwise reviewable, but the required CLI reference mirror needs to land with the flag change or as a clearly linked same-change-set PR before this should merge.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/run.ts">
<violation number="1" location="src/commands/run.ts:64">
P1: When a Windows bundle contains names that differ only by case, this loop injects both variants instead of resolving the ambiguity. Detect and withhold case-insensitive duplicates before spawning so the child cannot receive an implementation-dependent secret.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| for (const key of Object.keys(env)) if (claimed.has(key.toLowerCase())) delete env[key] | ||
| // A withheld name stays gone in every casing — including one the bundle itself carried, which | ||
| // is a platform that answered `collisions` while still merging the values. | ||
| for (const [k, v] of Object.entries(bundle)) if (!withheld.has(k.toLowerCase())) env[k] = v |
There was a problem hiding this comment.
P1: When a Windows bundle contains names that differ only by case, this loop injects both variants instead of resolving the ambiguity. Detect and withhold case-insensitive duplicates before spawning so the child cannot receive an implementation-dependent secret.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/run.ts, line 64:
<comment>When a Windows bundle contains names that differ only by case, this loop injects both variants instead of resolving the ambiguity. Detect and withhold case-insensitive duplicates before spawning so the child cannot receive an implementation-dependent secret.</comment>
<file context>
@@ -38,24 +38,33 @@ export function bundleFetcher(
+ for (const key of Object.keys(env)) if (claimed.has(key.toLowerCase())) delete env[key]
+ // A withheld name stays gone in every casing — including one the bundle itself carried, which
+ // is a platform that answered `collisions` while still merging the values.
+ for (const [k, v] of Object.entries(bundle)) if (!withheld.has(k.toLowerCase())) env[k] = v
return env
}
</file context>
There was a problem hiding this comment.
Not taken, and the reasoning is recorded in the code rather than only here.
The mechanism is real: on win32 that loop would write both variants and CreateProcess would resolve one of them arbitrarily. The input is not reachable, though — two keys differing only by case requires a lowercase letter, and the platform's name rule (^[A-Z][A-Z0-9_]{0,63}$, enforced on secrets set, bind, GitHub connect and template manifest) forbids one. Minted names are the fixed uppercase constants in CANONICAL_KEYS.
Detecting and withholding an impossible input would add a branch and a set to a function whose whole job is to be auditable, so d1d2c34 documents the invariant where it would bite instead: if that name rule ever loosens, this loop is the place both variants get written and Windows picks one arbitrarily.
Contrast with the __proto__ finding on the same line, which I did take: there the fix is one call and costs nothing to carry, so the reachability argument does not have to hold for it to be worth having.
|
Both addressed in Critical — the Suggestion — One thing your review helped me findYour requirements note quoted the flag as "reading/injecting exactly one compute service's env" — which is what the help text said, and it is misleading. Two reviewers independently read the container meaning into that sentence and filed the resulting mismatch as a finding — one on the platform PR's API text, one on the docs mirror. So the help string was the shared source. Both On the verification note: dependencies are not installed in that checkout — |
…us name
`GET /secrets` answers with a flat `{ secrets }` map, but env is scoped per compute service, so
hermes, claude-code and codex can each hold their own ADMIN_PASSWORD. One name cannot carry three
values: the platform's read overwrote, newest row winning, and `insta secrets --print` showed
codex's password under ADMIN_PASSWORD while the hand-set hermes value was nowhere. It was reported
as "my hermes password disappeared".
The platform's fix (branch fix/secret-service-scoped-read) withholds a colliding name and reports it
as `collisions: [{ name, services }]`. This is the client half:
- every general read now sends `on_collision=withhold`, so it gets the correct answer as soon as the
platform ships and the old merge behaviour is never asked for again;
- `--service <type/name>` on `insta secrets`, `insta run` and `insta secrets unset` addresses one
service — the read returns exactly what that service receives, and the unset removes only its copy
(the DELETE has always honoured ?service=, the CLI simply never exposed it);
- every collision is reported on stderr, never stdout: `secrets --print` writes the env to stdout and
`insta run`'s stdout belongs entirely to the child. `secrets --json` carries the list too, which
moves the values one level down into `secrets` — the one breaking change here.
`insta run` REFUSES rather than warns, because a warning would not be safe. The child is spawned with
`env: { ...process.env, ...bundle }`, and a withheld name is simply MISSING from the bundle — so the
parent's environment supplies it, and a developer who once exported codex's password would run
hermes' command against it with nothing on screen saying so. Clearing the name instead of inheriting
it is also not enough: the child may hold a compiled-in default or load its own .env, so a credential
that silently went missing need not be observable from inside the command at all. The only honest
answer is to stop before anything runs and make the user choose — `--service` to scope the run, or
`--ignore-collisions`, which runs with every colliding name deleted from the child environment (so
the parent's value still cannot stand in for it) and says so on stderr.
The refusal exits 2, the code handleApproval already uses for a gate: nothing ran, a redirected
stdout must not read it as success, it is not a plain failure (die owns 1), and re-running as the
message says will work. util.refuse() carries that shape.
The platform is not deployed yet, so the tests drive injected responses through RunDeps and a stub
api: run refuses and spawns nothing; --ignore-collisions spawns with the colliding name absent even
though process.env holds a value for it (the regression above, pinned); --service fetches the scoped
URL; collisions land on stderr and never stdout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…branch The platform 400s a `service` query param that arrives empty, and the client built the general query whenever the flag was falsy — so `--service ""`, or the ordinary `--service "$SVC"` with SVC unset, quietly answered a different question: the branch-wide merge instead of one service's env. On `insta secrets` that writes the ambiguous bundle to .env with only a stderr note; the failure this whole change exists to remove. assertServiceRef fails locally on secrets, secrets unset and run, naming the shape it wants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit wrapped `insta secrets --json` in `{secrets, collisions}` so a machine reader
could see a withheld name. That broke a documented agent-facing surface for every consumer,
including calls that pass none of the new flags and would never see a collision — the bare
`{NAME: value}` map is what they parse.
The envelope only existed because the --json branch returned before warnCollisions, leaving that
mode with no collision signal at all: a return-order problem, not a reason to change the contract.
So --json now reports the collisions the same way every other mode does — on stderr, as one
`{"collisions":[…]}` line — and stdout stays exactly what it was. Nothing is written when there
are no collisions; a quiet stream is the signal, and an empty array would only make every caller
inspect a field that says nothing.
This is the rule the rest of the change already followed: stdout belongs to the payload, which is
why `secrets --print` gives it the env and `insta run` gives it entirely to the child. The --json
branch was the one place it was not applied.
Tests pin the contract in both directions: stdout carries no `secrets` or `collisions` key on top,
so the envelope cannot come back unnoticed, and the stderr line parses on its own.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… its branch Three review findings from #193, all of which reproduce. childEnv deleted colliding names by exact key. The spread that builds the child environment copies process.env into a PLAIN object, which loses the case-insensitivity Windows env names have — while CreateProcess keeps it. So a parent that exported `Admin_Password` survived a delete of `ADMIN_PASSWORD` and the child read it as ADMIN_PASSWORD: the inheritance hole this branch exists to close, still open on one platform. The delete is now case-insensitive on win32 and stays exact elsewhere, where `Admin_Password` is a different variable that is none of our business. The platform is a parameter, not `process.platform`, so both branches run on every CI host — the Windows job could not have caught this anyway, since the tests inject spawnImpl and no real child environment is ever read. `secrets unset --service` sent no branch when --branch was omitted. A service exists on a branch, so the platform rejects that pair — the flag could not delete a single service's copy at all without a second flag. It now defaults to the linked branch exactly as `secrets set --service` does, and both the human line and --json report the effective branch, so the user can see which scope was actually deleted. The remediation hints dropped an explicit --branch: after `insta secrets --branch feat-x`, the suggested `insta secrets --service compute/hermes` would have read the linked branch instead — different secrets, no sign of the swap. collisionLines and run's refusal now name the branch whenever it is not the linked one. Not addressed, deliberately: a platform that ignores on_collision returns a merged value with no `collisions` field, which `?? []` reads as "nothing collided". That fails open. It is resolved by shipping order — insta-platform#388 merges before this CLI is released — per the repo owner, and fetchSecretBundle now carries a comment saying so, so nobody later reads that fallback as unconditionally safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit made a WITHHELD name win over whatever casings the parent had. An injected one
still did not, and that is the more consequential half: injecting cloud credentials is what
`insta run` is for. On win32 `{ ...parent, ...bundle }` can hold `Database_Url` from the shell and
`DATABASE_URL` from the bundle as two keys for one variable, and the choice between them is
CreateProcess's, not ours — so a stale shell export could quietly beat the fetched credential, for
every variable rather than only a colliding one.
childEnv now takes both halves together on win32: every name this run decides — injected or
withheld — is removed from the parent in all its casings, then the bundle is written back in its
own casing. A withheld name is not written back at all, including one a merging platform carried in
the bundle, so it stays gone in every casing. Off win32 nothing changes; POSIX names really are
case-sensitive, and collapsing `Database_Url` into `DATABASE_URL` there would be a bug of our own.
Stated plainly: this is NOT verified against a real Windows host. The mechanism is the one from the
finding confirmed by inspection — a plain JS object does not carry Windows' case-insensitive name
semantics, while CreateProcess does — but no test here executes CreateProcess. The tests pin OUR
normalisation (one key per variable, in our casing, with our value) rather than Windows'
resolution, and they take the platform as a parameter so both branches run on every CI host.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ling the read a container env Two review findings. `secrets set --service ""` fell through to a PROJECT-WIDE write. The scoping test is a truthiness check, so a client interpolating an absent variable put the secret at a WIDER scope than it asked for — visible to every service on the branch. The read paths were already guarded; the write is the one where getting it wrong actually stores something in the wrong place. Guarded now, asserted through `secretsSet` rather than the helper so the wiring is pinned, and verified by mutation: removing the guard turns the case red. `insta run --service`'s help said "inject exactly what one compute service RECEIVES". Two reviewers independently read that as the container's env and filed the resulting mismatch as a finding — on this PR and on the docs mirror. The read is one service's slice of the branch bundle and deliberately WIDER than the deployed env: it also carries the branch's provider credentials, which a container gets only where bound. Both `--service` help strings now say so, so the misreading has no source left. typecheck clean; 36 collision tests; suite at the 6-failure setup-agent.test.ts baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bef0a35 to
9c1f6e3
Compare
…name survives
Automated review flagged two things about the win32 reconstruction. One is
taken; the other is unreachable and is now documented rather than coded around.
TAKEN — a bundle key of `__proto__` would set the prototype under `env[k] = v`
instead of creating an entry, and the secret would vanish with no error. The
platform's name rule (`^[A-Z][A-Z0-9_]{0,63}$`, enforced on all four write
paths) makes that unreachable today, and that is precisely why it is worth one
`Object.defineProperty` rather than a trusted invariant: this CLI points at
whatever `INSTA_API_URL` names, including a self-hosted insta-oss daemon whose
validation is not this repo's to guarantee. A security property should not rest
on someone else's regex when the alternative costs one call.
NOT TAKEN — two bundle keys differing only by case, which the win32 loop would
write both of. The mechanism is real; the input is not. Case-differing needs a
lowercase letter and the same name rule forbids one. Detecting and withholding
an impossible input would add a branch and a set to a function whose whole job
is to be auditable, so the invariant is written where it would bite instead:
if that rule ever loosens, this loop is the place both variants get written and
Windows picks one arbitrarily.
The test for this needed fixing before it tested anything: `{ __proto__: 'v' }`
as an object literal is the prototype-setting syntax, not a key — and with a
string value the spec ignores it outright — so the bundle was simply empty. Built
with `JSON.parse('{"__proto__":"v"}')` now, asserted on both platform branches
(the non-win32 spread creates the own key via CopyDataProperties), and verified
by mutation: restoring `env[k] = v` turns the win32 case red.
Also rebased onto main (0.0.64 released as insta-cli#194): typecheck clean and
850/850 tests, the six `setup-agent.test.ts` failures that were this branch's
baseline having been fixed on main in the meantime.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
The runtime changes look aligned with the PR intent, with no blocking correctness, security, or performance findings.
Requirements context
I used the PR title/description, the README updates in this repo, the existing command conventions in src/index.ts, and the checked-in tests. The linked platform PR #388 was not publicly accessible from this environment, so I assessed that part against the PR description. The intended behavior is: general bundle reads ask for on_collision=withhold; secrets/run can read one service via --service; secrets unset --service deletes only that service's copy; run refuses with exit 2 on collisions unless --ignore-collisions; collision reports stay off stdout.
Findings
Critical
(none)
Suggestion
README.md:115-120still saysinsta secrets/insta runfetch only user-defined secrets and that provider-minted credentials are not in the bundle. That conflicts with the new CLI help's local-seam distinction insrc/index.ts:97and with the linked docs PR's stated correction that localsecrets/runnow carry canonical provider credentials while compute containers receive provider credentials only through explicit bindings. Since this PR edits the same README section, consider updating that stale paragraph here too.
Information
src/commands/secrets.ts:45-61intentionally treats a missingcollisionsfield as no reported collisions. That preserves behavior against older platforms, but the safety guarantee depends on the rollout order described in the PR: platform #388 must be deployed before this CLI is released.- Software engineering: the new tests cover query construction, stderr/stdout separation,
runrefusal before spawn,--ignore-collisionsenv stripping, Windows case-insensitive env handling, and service-scoped unset/read behavior. - Security: no secret values are newly logged; collision reporting exposes names and service refs only, and service/branch values are URL-encoded before reaching the API.
- Performance: no concerning performance changes; the added work is one existing bundle API request plus small linear scans over environment/secrets/collisions.
- Verification: I did not run the test suite because this review was explicitly read-only and
node_modulesis absent in the checkout; I did rungit diff --check main...HEAD, which was clean.
Verdict
Approved per the requested verdict rule: there are no Critical findings. This is a bot comment verdict, not a human GitHub approval.
Ships the service-scoped secret reads from #193: --service on secrets, run and unset; run refusing on a same-name collision with --ignore-collisions as the escape hatch; collisions reported on stderr. The platform side (InsForge/instacloud-platform#388) is already merged and DEPLOYED to production, which this release depends on: the CLI sends on_collision=withhold on every general read, and a platform that did not know the param would ignore it, answer merged and return no collisions — which the CLI reads as 'nothing collided'. Verified live before cutting this: GET /projects/{id}/secrets on api.instacloud.com advertises service and on_collision, and its 200 carries collisions. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
The CLI half of InsForge/insta-platform#388. Reported in Slack as "admin username and password in my hermes's secrets disappeared" — nothing was deleted, but
insta secrets --printreturned codex's password underADMIN_PASSWORDbecause three compute services each held their own and the flat read kept whichever row was newest.Flags
insta secrets--service <type/name>— read one compute service's own env instead of the branch-wide bundleinsta secrets unset--service <type/name>— remove only that service's copy. Without it the delete is WIDE, not narrow: the platform's unscopedDELETEhas noservice_idpredicate, so it removes every non-minted row for that(project, branch, name)— every service's copy. Verified on staging: three compute services each holdingADMIN_PASSWORD, oneDELETE ?branch=main, all three gone. That is the long-standing backward-compatible behaviour, not something this PR changes;--serviceis what makes a narrow delete possible at all, and the CLI never exposed it beforeinsta run--service <type/name>— inject exactly what one compute service receivesinsta run--ignore-collisions— run anyway, with every colliding name removed from the child environmentEvery general (non-
--service) read now sendson_collision=withhold, so this CLI gets correct behaviour as soon as the platform ships. Until then the platform's default ismergeandcollisionsis[], so nothing changes.insta runrefuses rather than warnsThis is the important one.
run.tsspawns withenv: { ...process.env, ...bundle }, so a name missing from the bundle falls through to the parent process's value — a developer who once exported codex's password would run hermes' command against it while the CLI believed it had withheld the name.Clearing is not sufficient either: it stops the parent's value being inherited and nothing more, since the child may hold a compiled-in default or load its own
.envand proceed on some other value silently. Sorunprints the collisions and exits 2 having spawned nothing. Exit 2 is the repo's existing "gate" code —handleApprovalandonError'sAgentApprovalRequiredboth use it, for the same reasons (not success, so a redirected stdout cannot swallow it; not a plain error, sincedieowns 1).util.refuse()carries that shape.--ignore-collisionsproceeds butchildEnv()deletes every colliding name, so the parent's export cannot stand in. Therunbanner moved to anannouncecallback fired only once we are actually spawning, so it can never precede a refusal.--jsonstdout is deliberately unchangedinsta secrets --jsonstdout is byte-identical to before this branch — still the bare{NAME: value}map. Collisions ride stderr as one JSON line,{"collisions":[…]}, or nothing at all when there are none. stdout carries the payload, which is whysecrets --printgives it the env andrungives it entirely to the child; and the flat map is the honest representation, because its destination is a.envfile and a process environment, both flat. Underwithholdevery name in it has exactly one true answer, so nothing in it is a guess.An earlier revision wrapped it as
{secrets, collisions}. That broke every existing--jsonconsumer including calls passing none of the new flags, for a signal that already had a home on stderr, so it was reverted. A test now pins the absence of envelope keys at the top level so it cannot come back unnoticed.Testing
test/secrets-collisions.test.ts(new, 23 cases), written failing first. Highlights:runrefuses: the injectedspawnImplwas never called and stdout is empty.--ignore-collisionsstrips the name even withprocess.env.ADMIN_PASSWORDset — a real child that exits 7 only if the name arrived absent. This is the inheritance hole, proven closed.--jsonkeeps stdout the bare map with no envelope keys; reports collisions as a parseable stderr line; says nothing on stderr when there are none.secrets --service/unset --servicesend the right query params; collisions never reach stdout.npm run typecheckclean.npm test: 820 tests, 814 passed, 6 failed — the 6 are pre-existing intest/setup-agent.test.ts(setupAgent→issueAgentSessionmakes a livePOST /agent/sessionsagainst the prod API, which 404s); verified identical on untouchedorigin/main.The
cli-reference.mdmirror — done, in InsForge/instacloud-skills#80AGENTS.md non-negotiable #4 requires it, and it cannot be in this change set:
skills/insta/cli-reference.mdlives in a different repository (InsForge/insta-skills, aninsta-cloudsubmodule) whosemainis protected too. So it is a same-change-set pair, not a same-commit edit:cli-reference.md(thesecrets,secrets unsetand a newinsta runrow),SKILL.md, and four reference pagesThat PR also corrects a claim that had been wrong since platform
#323: four places said provider credentials never reachinsta secrets/insta run, which stopped being true when the general read began carrying the branch's canonical credentials.Merge order, per the platform PR's spec: frontend #400 (independent) → platform #388 → platform deployed → this PR → released → #80. The release must follow the platform deploy: a released CLI sends
on_collision=withhold, and a platform that does not yet know the param ignores it and returns nocollisions, which this CLI would read as "nothing collided".Still not mirrored, tracked for that same docs PR:
skills/insta/SKILL.md'sinsta runcheat-sheet line.🤖 Generated with Claude Code