Skip to content

fix(server): the batched 1Password read reaches op at all - #30

Merged
yordis merged 1 commit into
mainfrom
yordis/fix-op-inject-stdin
Aug 21, 2026
Merged

fix(server): the batched 1Password read reaches op at all#30
yordis merged 1 commit into
mainfrom
yordis/fix-op-inject-stdin

Conversation

@yordis

@yordis yordis commented Aug 21, 2026

Copy link
Copy Markdown
Member
  • The batch that was supposed to buy one vault unlock for the whole fleet never ran. op only accepts piped input from a named pipe, and a child spawned from Node is handed a socket pair, so the template was never seen and every boot silently fell back to one authorization per provider. The failure is structural, not timing, so it happened on every single boot.
  • Delivering the template through a file rather than a shell pipe keeps the spawn free of a shell and works on Windows, and the file holds only op:// references, which already sit in settings in plain text. The secrets come back on stdout and never touch disk.
  • The previous tests could not have caught this: a fake spawner accepts whatever input the code offers. The template is now read back the same way op reads it, so a change that stops writing the file fails the suite.

op only accepts piped input from a named pipe, and a child spawned from
Node is handed a socket pair, so the batch template was never seen and
every fleet fell back to one authorization per provider.

Signed-off-by: Yordis Prieto <[email protected]>
@cursor

cursor Bot commented Aug 21, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches 1Password secret resolution and temp-file delivery of credential references. The file holds only already-plaintext op:// URIs and is scoped-deleted, but this is still credential plumbing.

Overview
Fixes batched 1Password prime so op inject actually receives the template. Node hands children a socket pair, not a named pipe, so piping stdin always failed and every boot fell back to one vault unlock per provider.

readSecretsTogether now writes the template (only op:// references) to a scoped temp file and invokes op inject -i. Secrets still come back on stdout. Tests read that file from argv the same way op would, and assert the file is gone after the batch.

Reviewed by Cursor Bugbot for commit 01753c8. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Aug 21, 2026
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a509a9ac-a62c-445d-84e1-e463f22d96b7

📥 Commits

Reviewing files that changed from the base of the PR and between a7cf32d and 01753c8.

📒 Files selected for processing (2)
  • apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts
  • apps/server/src/provider/Layers/ProviderSecretResolverLive.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

Changes

Secret injection batch reads now write templates to scoped temporary files and pass them to op with -i. The provider requires FileSystem, and tests verify file delivery, stdin avoidance, argument handling, and cleanup.

Secret injection template files

Layer / File(s) Summary
Scoped template file execution
apps/server/src/provider/Layers/ProviderSecretResolverLive.ts
The provider writes templates to scoped temporary files, invokes op inject -i, requires FileSystem, and releases the file after the batch read.
Template delivery and cleanup tests
apps/server/src/provider/Layers/ProviderSecretResolverLive.test.ts
Tests provide NodeFileSystem, read templates from the -i path, verify no stdin stream is used, and confirm temporary files are removed.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 01753

The PR changes batched secret resolution to use a file-backed template while keeping secrets off disk and adds tests for that path; no actionable merge-blocking risk remains beyond normal checks.

Sequence Diagram(s)

sequenceDiagram
  participant ProviderSecretResolverLive
  participant FileSystem
  participant op
  ProviderSecretResolverLive->>FileSystem: Create and write template file
  ProviderSecretResolverLive->>op: Run inject with -i template path
  op-->>ProviderSecretResolverLive: Return injected secret results
  FileSystem-->>ProviderSecretResolverLive: Remove scoped template file
Loading

Poem

I’m a rabbit with a file in my paws,
op reads templates by clearer laws.
No stdin stream hops through the night,
-i points to the path just right.
When the batch is done, the file takes flight.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: ensuring batched 1Password reads reach the op process.
Description check ✅ Passed The description clearly explains the change and rationale; UI changes are not applicable, although the template headings and checklist are omitted.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch yordis/fix-op-inject-stdin

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@yordis
yordis merged commit 1f9ff0f into main Aug 21, 2026
9 of 13 checks passed
@yordis
yordis deleted the yordis/fix-op-inject-stdin branch August 21, 2026 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant