Open
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add explicit ChildProcess type to suppress implicit-any lint warning - Add no-op error handler to prevent unhandled EventEmitter crash on ENOENT - Extract getBrowserCommand() pure function for testability - Add unit tests covering all platforms and URL passthrough Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ub-api Feature/auth and quality hub api
Contributor
There was a problem hiding this comment.
Pull request overview
Improves the auth login flow’s browser-launch and callback-server shutdown behavior, and updates docs/versioning for the beta release.
Changes:
- Refactors browser opening to use a testable
getBrowserCommand()helper and a detachedspawn()“fire-and-forget” pattern. - Updates the localhost OAuth callback responses to use
Connection: closeand attempts to proactively close lingering connections. - Bumps package version and clarifies install instructions for MCP beta support in docs/README.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/provar/auth/login.test.ts | Adds unit tests for getBrowserCommand() platform selection. |
| src/services/auth/loginFlow.ts | Refactors browser open logic; adds command-selection helper; tweaks callback server connection handling. |
| package.json | Bumps version to 1.5.0-beta.5. |
| docs/mcp.md | Clarifies that @beta install is required for MCP support. |
| README.md | Mirrors MCP @beta install requirement note in quick-start instructions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
131
to
133
| server.close(); | ||
| server.closeAllConnections?.(); | ||
| reject(new Error('OAuth callback state mismatch — possible CSRF. Try again.')); |
There was a problem hiding this comment.
Same Node-version concern here: server.closeAllConnections?.() won’t run on Node 18.0/18.1 even though the package supports those versions, so a lingering keep-alive socket could still prevent the process from exiting. Consider adding a socket-tracking fallback when closeAllConnections is undefined so shutdown is reliable across the full supported Node range.
Comment on lines
+75
to
+83
| @@ -76,21 +76,35 @@ function isPortFree(port: number): Promise<boolean> { | |||
| * Open a URL in the system browser. The URL is passed as an argument — not | |||
| * interpolated into a shell string — to avoid command injection. | |||
| */ | |||
| export function openBrowser(url: string): void { | |||
| switch (process.platform) { | |||
| /** | |||
| * Return the platform-specific command and argument list for opening a URL | |||
| * in the system browser. Exported so tests can assert the correct command is | |||
| * chosen for each platform without actually spawning a process. | |||
| */ | |||
There was a problem hiding this comment.
There are two adjacent JSDoc blocks here; the first one (“Open a URL in the system browser…”) is now orphaned and no longer documents any symbol. This can confuse generated docs and makes it unclear which function the injection-safety note applies to. Consider merging the two blocks or moving the injection-safety text onto the openBrowser JSDoc and keeping only one JSDoc block per export.
Comment on lines
+147
to
+150
| server.close(); | ||
| // Destroy any sockets that are still open (e.g. a browser that ignores | ||
| // the Connection:close header). Requires Node 18.2+. | ||
| server.closeAllConnections?.(); |
There was a problem hiding this comment.
server.closeAllConnections?.() only exists on Node >=18.2, but package.json allows Node 18.0+. On Node 18.0/18.1, if a browser keeps the socket open despite Connection: close, server.close() can still keep the event loop alive. If you need this to work reliably on the full supported Node range, consider tracking active sockets via server.on('connection', ...) and destroying them on shutdown as a fallback when closeAllConnections is unavailable.
spawnSync with shell:false cannot execute Windows batch files (.cmd/.bat) or extensionless commands like 'sf' that map to sf.cmd on disk. Added needsWindowsShell() to detect these cases and pass shell:true accordingly. Includes unit tests for the helper and for shell option propagation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The stored key was returned as-is with no trim() or prefix check, unlike the env var path which had both. A key with trailing whitespace (e.g. from manual editing) or a pre-pv_k_ format key would be sent to the Quality Hub API unchanged, causing a 401 and triggering local_fallback even when a valid key exists. Tests added for both cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sources: PROVARDX_CLI_FEEDBACK.md from provar-manager-regression session. Source changes: - testCaseValidate.ts: DATA-001 warning for <dataTable> (silently ignored by CLI standalone runner); ASSERT-001 warning for AssertValues with namedValues id="values" format (silently passes as null=null for variable comparisons); improved TC_012/TC_031 suggestion strings naming crypto.randomUUID() and the UUID v4 variant byte rule. - automationTools.ts: filterTestRunOutput() strips com.networknt.schema and logger-lock SEVERE noise before returning testrun response; output_lines_suppressed field added when lines are dropped. - rcaTools.ts: incrementSiblingDirs() detects Results(N) siblings (Provar Increment mode creates siblings, not numeric children); 5 new Salesforce DML error RCA rules: SALESFORCE_VALIDATION, SALESFORCE_PICKLIST, SALESFORCE_REFERENCE, SALESFORCE_ACCESS, SALESFORCE_TRIGGER. Tests (22 new, 626 total): - testCaseValidate.test.ts: DATA-001 (fires/doesn't fire), ASSERT-001 (fires/doesn't fire for correct args/non-AssertValues), TC_012/TC_031 suggestion text. - automationTools.test.ts: filterTestRunOutput pure-function tests (7), testrun output_lines_suppressed handler tests (2). - rcaTools.test.ts: Results(N) sibling detection (2), Salesforce category classification (4). Docs: docs/mcp.md — DATA-001/ASSERT-001 warning rules, Salesforce RCA categories with infrastructure_issues note, output_lines_suppressed field. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Merge orphaned JSDoc block on getBrowserCommand (two adjacent JSDoc
blocks; first was left over from before the refactor)
- Add socket tracking in listenForCallback as a Node 18.0/18.1 fallback
for server.closeAllConnections() (added in Node 18.2). Tracks open
sockets via server.on('connection') and destroys them on shutdown when
closeAllConnections is unavailable, preventing lingering keep-alive
sockets from blocking the event loop on older Node 18 patch versions.
- Bump version 1.5.0-beta.5 → 1.5.0-beta.6
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use typeof check before calling .trim() so a missing or non-string api_key field in a manually edited credentials.json cannot throw a TypeError. Added tests for missing-field and numeric api_key cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- filterTestRunOutput: split on /\r?\n/ and strip trailing \r so Windows CRLF output does not leave stray \r chars or break noise pattern matching (Copilot suggestion) - rcaTools run_index schema: add .int().positive() validation on both provar.testrun.report.locate and provar.testrun.rca to reject run_index <= 0 before it reaches resolveResultsLocation (Copilot suggestion) - test: add CRLF-safety test for filterTestRunOutput Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- setSfPathCacheForTesting now accepts undefined to reset the probe cache - setSfPlatformForTesting added to allow cross-OS testing of Windows shell logic - resolveSfExecutable uses two-phase probe (shell:false first, shell:true retry on win32 ENOENT) - runSfCommand uses _sfPlatform override and calls assertShellSafePath for shell:true + user sf_path - Tests: platform override in Windows shell tests; probe phase tests; injection hardening tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-feedback feat(mcp): implement 7 test-loop improvements from real usage feedback
fix(auth): trim and validate stored api key in resolveApiKey
There was a parsing error with validations resulting in a 500 error code.
Combine needsWindowsShell/setSfPlatformForTesting (from PR branch) with filterTestRunOutput (from develop) into a single import line. Both exports exist in automationTools.ts and are exercised by existing test suites. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix(mcp): use shell:true for .cmd/.bat executables on Windows
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.