Skip to content

fix: remove Windows secret helper runtime compilation - #185

Merged
mohanagy merged 7 commits into
developmentfrom
fix/115-windows-provider-readiness-v2
Jul 22, 2026
Merged

fix: remove Windows secret helper runtime compilation#185
mohanagy merged 7 commits into
developmentfrom
fix/115-windows-provider-readiness-v2

Conversation

@mohanagy

@mohanagy mohanagy commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Pull request

Summary

Fixes #115.

Windows secret providers compiled the full native Job Object helper with Add-Type on every invocation. Failed-run timing isolated that cold compilation as the unstable first-use boundary. This PR moves the unchanged helper to canonical C# source, ships a bounded gzip-compressed AnyCPU .NET Framework assembly, and loads it with [Reflection.Assembly]::Load.

It also fixes a reviewed handle-ownership bug: the helper no longer closes the parent standard-input handle after a provider exits.

Security impact

  • Preserves kill-on-close Job Object containment and assigns PowerShell to the job before any provider is created.
  • Preserves shell: false, exact argument arrays, CreateProcessW, and the explicit inherited handle list.
  • Preserves bounded request, stdin, stdout, stderr, cancellation, and timeout behavior; no timeout was increased.
  • Bounds the encoded assembly to 8,192 characters and decompressed assembly to 16 KiB before loading.
  • Clears request, stdin, helper source, and assembly environment values before the provider starts, so providers and descendants do not inherit helper payloads.
  • Adds Windows CI checks for normalized source freshness, independent source compilation, PE bounds, and the checked runtime artifact contract.
  • Adds no dependencies and changes no public API or configuration schema.

Validation

Exact head: 67e1b50928cd3d084f1e8e6dabbbc946c6781e37

  • A failing test was observed first for each behavior or configuration-contract change.
    • Runtime compilation contract failed before implementation.
    • Windows run 29944092843 on head 6777940 reproduced the stdin ownership regression as native-run-exits=0,1 before the fix.
  • npm run lint
  • npm run typecheck
  • npm test
  • npm run build
  • node dist/cli/main.js schema
  • npm run check:pack
  • npm run test:core
  • npm run test:coverage — 1,247 passed, 23 platform-skipped; 95.39% statements and 91.85% branches.
  • npm run test:package — 18 passed.
  • Fixtures, logs, screenshots, and examples contain no credentials or private data.
  • User-facing documentation and CHANGELOG.md are updated when applicable. No user-facing command, configuration, or documented behavior changed.
  • Dependency and packaged-file changes are intentional and reviewed. No dependency changed; the generated assembly is bundled into the existing CLI artifact.
  • Undisclosed vulnerabilities are reported privately instead of in this pull request.

Windows exact-head evidence so far:

  • Node 20: cold provider 575 ms; repeated native helper launch passed.
  • Node 22: cold provider 485 ms; repeated native helper launch passed; source and artifact verification passed.
  • Node 24: cold provider 579 ms; repeated native helper launch passed.

CodeRabbit hit its documented review limit. A local Claude Code Opus review found four actionable issues; all were fixed, and its focused re-review reports no remaining actionable findings.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The Windows secret-provider helper now loads a bounded, precompiled .NET Framework Job Object assembly instead of compiling C# at runtime. The canonical source, embedded payload, source fingerprint, runtime loading path, tests, and Windows CI verification were added or updated.

Changes

Windows secret job assembly

Layer / File(s) Summary
Canonical Job Object helper
src/secrets/windows-secret-job.cs
Adds Job Object initialization, child-process execution, standard-handle wiring, argument validation, exit-code handling, and resource cleanup.
Assembly packaging and source fingerprint
scripts/export-windows-secret-job-assembly.ps1, src/secrets/windows-secret-job-assembly.ts
Compiles the helper, gzip-compresses and base64-encodes the DLL, and exports the payload with a normalized-source SHA-256 fingerprint.
Runtime assembly loading
src/secrets/windows-secret-command.ts, tests/secret-providers.test.ts
Passes the encoded assembly through the environment, decompresses and loads it with reflection, and updates Windows provider tests to use the precompiled path.
Assembly verification and CI enforcement
scripts/verify-windows-secret-job-assembly.ps1, tests/windows-secret-command-contract.test.ts, .github/workflows/ci.yml
Checks source freshness, assembly structure and contract, verifies the no-runtime-compilation path, and runs verification on Windows Node.js 22 CI jobs.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SecretCommand
  participant PowerShellHelper
  participant EmbeddedAssembly
  participant ChildProcess
  SecretCommand->>PowerShellHelper: pass encoded assembly through environment
  PowerShellHelper->>EmbeddedAssembly: decompress and load assembly bytes
  PowerShellHelper->>ChildProcess: initialize Job Object and run executable
Loading

Possibly related PRs

Poem

I’m a rabbit with a DLL in my pack,
No runtime compiler slowing us back.
Hashes match, bytes load bright,
Job Objects guard each flight.
Windows hops through CI tonight! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes implement the linked issue's fix by precompiling the Windows helper, loading it without Add-Type, and verifying the contract in CI.
Out of Scope Changes check ✅ Passed No unrelated or out-of-scope changes are evident; the scripts, tests, CI updates, and assembly artifacts all support the Windows helper fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly names the main change: removing runtime compilation for the Windows secret helper.
Description check ✅ Passed The description matches the template with Summary, Security impact, and a detailed Validation section listing commands and results.
✨ 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 fix/115-windows-provider-readiness-v2

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

@mohanagy
mohanagy marked this pull request as ready for review July 22, 2026 17:37
@mohanagy

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mohanagy

Copy link
Copy Markdown
Owner Author

Review fallback evidence: CodeRabbit reported Review rate limited and produced no review threads. A local Claude Code Opus review was therefore run against origin/development...HEAD in safe read-only mode. It found four actionable issues: misleading source/artifact verification claims, no guard for the actual 8,192-character encoded artifact limit, a tautological CRLF assertion, and closing a non-owned parent stdin handle. All four were addressed. The stdin regression was observed red on immutable CI head 6777940 in run 29944092843 (native-run-exits=0,1 on Windows Node 20 and 24) before the C# fix and assembly regeneration. A focused Opus re-review of the follow-up diff reports no remaining actionable findings. Final exact-head CI is still in progress and will remain the merge gate.

@mohanagy
mohanagy merged commit bde4457 into development Jul 22, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: make Windows secret-provider cold start reliable

1 participant