feat(settings): point provider environment variables at a secret provider#27115

Please move secret reload followed by provider refresh into a domain service operation. This adds feature-specific conditional, multi-step dispatch to the RPC handler; the transport should call one service method that owns refresh ordering, rather than coordinate the instance mutator and provider registry itself.

^{Posted via Macroscope — Effect Service Conventions}

Fixed in b05724eae4: ProviderRegistry.reloadSecretsAndRefresh owns the reload then refresh, and the RPC handler calls only that method. Covered by ProviderRegistry.test.ts and ws.test.ts.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

Please define this new service with Context.Service and its interface inline, using SecretResolver["Service"] for method types. The standalone SecretResolverShape and default-backed Context.Reference do not follow the canonical service definition; acquire the resolver through its layer rather than silently substituting a no-op implementation. This requires updating the tag and its consumers together.

^{Posted via Macroscope — Effect Service Conventions}

Fixed in b05724eae4: SecretResolver is now a Context.Service with its interface inline, built with SecretResolver.of and provided through layer. Covered by SecretResolver.test.ts and ProviderInstanceRegistry.test.ts.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

Please replace raw CLI stderr, reference strings, and String(error) log annotations with bounded, normalized diagnostics (for example, stage, exit status, and reference count). The new logging here and in readTogether/bounded exposes command output and arbitrary failure text, which the service conventions prohibit even if the CLI normally avoids printing secrets. Update all three logging boundaries; preserve underlying errors only as causes where applicable.

^{Posted via Macroscope — Effect Service Conventions}

Fixed in b05724eae4: logs now carry only stage, exit code, reference count, or the error tag. Stderr, references, the account, and String(error) are no longer logged.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🟠 High secrets/SecretResolver.ts:59

nonEmpty strips leading and trailing whitespace from successfully read secrets, so a password such as " pass " reaches the driver as "pass" and authentication fails. Use trimming only to test whether the value is empty, and return the original value.

Fixed in b05724eae4: values are returned verbatim, trimming only decides emptiness, and the batch separator follows every reference so op's trailing newline stays out of the values. SecretResolver.test.ts asserts a value with surrounding whitespace survives.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

External image Major · Security & Privacy · Quick win

Reject template delimiters in SecretReferencePath.

A schema-valid reference can add another op inject directive to the batch template. For example:

The batch can resolve both references inside one separator-delimited segment. The output count still matches the input count, so the independent-read fallback does not run. The provider environment can then receive the additional secret.

Reject {, }, and line breaks in SecretReferencePath.

✅ Confirmed as addressed by @yordis

Fixed in b05724eae4: SecretReferencePath now only decodes the 1Password op:// reference syntax, so braces, line breaks, and a leading dash cannot reach the inject template. settings.test.ts rejects an injection attempt and a non op:// reference.

@yordis Verified the fix by source inspection. SecretReferencePath now restricts references to the op:// pattern and excludes template braces. settings.test.ts includes rejection cases for the injection payload, a leading dash, and a missing op:// prefix.

This addresses the reported template-injection issue. I did not run the tests.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Security & Privacy · Quick win

Do not log raw 1Password arguments or diagnostics.

result.stderr.trim() logs raw command output, and String(error) logs arbitrary defect text. The individual-read warning also logs the raw account and reference command arguments. Replacing only detail with its length is not sufficient. The DeviceBootError.diagnostics exception does not apply here.

✅ Confirmed as addressed by @yordis

Fixed in b05724eae4: the same bounded diagnostics apply to the batch path (stage, exit code, reference count).

@yordis Verified in the current code. The individual and batch warnings now log only stage, exit code, and reference count where applicable. Command failures log the stage and error tag, not raw error text. This addresses the finding.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Stability & Availability · Quick win

Skip per-reference fallback after a batch timeout.

bounded converts a timed-out op inject call to undefined. The current code then runs readOne for every reference, and each call can consume the full 45-second timeout. This can hold both resolver and registry semaphores for approximately (N + 1) × 45 seconds.

Return undefined for every reference when the batch times out. Keep the per-reference fallback for completed batch failures. These results remain cached until the next settings refresh invalidates them.

✅ Confirmed as addressed by @yordis

Fixed in b05724eae4: a batch timeout now returns unreadable for every reference without the per-reference fallback; a completed batch failure still falls back. SecretResolver.test.ts drives the timeout with TestClock and asserts a single op invocation.

@yordis Thanks for fixing this in b05724eae4. The reported change addresses the timeout concern and preserves fallback for completed batch failures. The TestClock test covers the single-invocation requirement.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Functional Correctness · Quick win

Reject references that the selected provider cannot read.

If a user commits a nonblank path without op://, SecretReference accepts it, but SecretResolver.accountFor rejects it. The settings save succeeds while the provider becomes unavailable. Validate the selected provider’s path format before onSecretChange and show an error for an invalid commit.

✅ Confirmed as addressed by @yordis

Fixed in 7b2fe53b0d: the input decodes with the same contract SecretReference schema and shows an inline error for anything else. SecretSourceInput.browser.test.tsx covers the rejection.

@yordis Verified the fix in 7b2fe53b0d by source inspection. The shared SecretReference schema requires op://. Invalid commits show an inline error and do not call onSecretChange.

The browser test covers rejection of a reference without op://. I did not run the test.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Major · Data Integrity & Integration · Quick win

Wait for a new reference before changing providers.

If a referenced variable switches sources, this call publishes the old reference with the new provider ID. The server can then read that path from a different 1Password account and rebuild the instance with the wrong credential. Keep the new provider selection pending and publish the pair only after the user commits its reference.

✅ Confirmed as addressed by @yordis

Fixed in 7b2fe53b0d: picking another provider only stages it, and nothing is published until a reference for that provider is committed. SecretSourceInput.browser.test.tsx covers the switch.

@yordis Thanks for fixing this in 7b2fe53b0d. Staging the provider selection until its reference is committed addresses the concern.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Functional Correctness · Quick win

Remove an existing reference when its textbox is cleared.

If the user clears an existing reference and commits the blank value, this branch leaves onSecretChange uncalled. DraftInput then restores the old reference, and the provider setting keeps using it. Call onSecretChange(undefined) when the cleared reference belongs to the selected provider. Keep an uncommitted provider switch from clearing the original reference. (raw.githubusercontent.com)

✅ Confirmed as addressed by @yordis

Fixed in 593bcb0311: committing a blank reference for the saved provider now removes the reference, while a staged provider switch still leaves the saved one untouched. SecretSourceInput.browser.test.tsx covers the clear.

@yordis Thanks for fixing this in 593bcb0311 and adding browser test coverage. The reported change addresses clearing the saved reference while preserving it during a staged provider switch.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Major · Stability & Availability · Heavy lift

Check the decoded enabled state before reading secrets.

mayBeEnabled returns true when both saved flags are absent. Antigravity’s decoded default is disabled, so a valid Antigravity instance with a secret reference still triggers a vault read during startup and refresh. That read can delay startup until the 45-second timeout. A successful refresh can also rebuild the disabled instance repeatedly because its stored environment omits the reference. Use the same decoded enabled state as buildEntry before selecting references. (raw.githubusercontent.com)

✅ Confirmed as addressed by @yordis

Fixed in 510e74da91: the prefetch in reconcile and the reloadSecrets targets now use the same decoded enabled state as buildEntry, so an instance whose config decodes as disabled is never read, invalidated, or rebuilt on refresh. ProviderInstanceRegistry.test.ts covers it with a driver that defaults to disabled.

@yordis The supplied code confirms that prefetch and reloadSecrets now use isEntryEnabled, which decodes the config and applies the same enabled-state logic as buildEntry. This addresses the disabled-instance secret reads and refresh rebuilds.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}