Skip to content

fix: address post-merge review findings - #31

Merged
TheStreamCode merged 2 commits into
mainfrom
fix/post-merge-review
Nov 1, 2026
Merged

TheStreamCode merged 2 commits into
mainfrom
fix/post-merge-review

Conversation

@TheStreamCode

Copy link
Copy Markdown
Owner

Summary

Follow-up to #30, addressing the findings from the delayed review of that PR. The merge landed before the review finished; this branch fixes the valid points on top of main.

Change

  • Native registry lifecycle: the temporary directory is made owner-only before the file exists, a failed restriction aborts the session instead of leaving keys readable, cleanup covers SIGINT/SIGTERM/SIGHUP, cleanup failures are reported, and stale registries older than a week are swept at the next native startup for the uncatchable cases.
  • --native-byok --dry-run no longer writes provider keys to disk. With no active provider it returns the native shape (mode: native-byok, empty arrays) instead of falling back to the stock dry-run shape.
  • --legacy accepts a provider that only has apiKeyCommand (Copilot CLI 1.0.84+). --list-providers and doctor mark it as legacy-only, because the CLI's providers.json registry has no credential-command equivalent.
  • The reasoning-effort ladder steps down only when a 400 body mentions the reasoning effort. Ordinary context, schema or content-policy 400s are forwarded unchanged instead of consuming seven retries and polluting the rejection memory.
  • doctor sanitizes the environment of the Copilot version probe, reports discovery failures in --online instead of an empty success, and distinguishes keyed providers, authless providers and credential-command providers.
  • COPILOT_PROVIDER_HEADERS is redacted in dry-run output, and headers must be an object of strings like modelsHeaders.
  • The published schema descriptions no longer describe the old reasoning-effort behavior.
  • Tests: registry permission failure and mode checks, stale sweep, real native session with a fake Copilot, unrelated-400 passthrough, per-test effort state reset, doctor sanitization and discovery failure, command-only provider, live discovery in models, headers validation.

Verification

  • npm run check: lint clean; 235 tests, 233 pass, 2 skipped (POSIX-only), 0 fail.
  • npm pack --dry-run: 43 files, 79.3 kB.
  • npm audit --omit=dev --audit-level=high: 0 vulnerabilities.
  • markdownlint on the root documents: 0 issues.
  • No real provider account or live data residency tenant was available; those remain unverified and are labelled as such in the verification matrix.

Checklist

  • npm run check passes
  • npm pack --dry-run passes
  • npm audit --omit=dev --audit-level=high passes
  • Tests added or updated for behavior changes
  • Cross-platform behavior considered for Windows, macOS, and Linux
  • Verified with at least one real provider when provider behavior changed (unit and mock only; the native session path is exercised with a fake Copilot)
  • Required CI checks pass (waiting for the PR run)
  • Focused change, one problem per pull request
  • Public behavior documented in the README or the relevant docs/ file
  • CHANGELOG.md updated when user-visible behavior changed
  • No credentials, inline secret examples, or secret-bearing URLs are included
  • No dependency added without a stated reason

Michael Gasperini added 2 commits November 1, 2026 19:47
Native registry: lock the temporary directory down before the file exists, abort when the restriction fails, clean up on SIGINT/SIGTERM/SIGHUP, report cleanup failures, and sweep stale registries from hard kills. Dry runs no longer write keys to disk.

Legacy mode accepts a provider with only apiKeyCommand and reports it as legacy-only in --list-providers and doctor. The router steps down only when a 400 body mentions the reasoning effort. Doctor sanitizes the version probe environment, reports discovery failures and distinguishes keyed from authless providers. Redact COPILOT_PROVIDER_HEADERS in dry-run output and validate headers as a string map.
Describe the permission, cleanup and stale-sweep guarantees, the legacy-only apiKeyCommand, the effort-400 behavior, and the new verification evidence. Update CHANGELOG and AGENTS.
@TheStreamCode
TheStreamCode merged commit b5101cf into main Nov 1, 2026
11 checks passed
@TheStreamCode
TheStreamCode deleted the fix/post-merge-review branch November 1, 2026 18:49
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.

1 participant