Develop#145
Merged
Merged
Conversation
RCA: The git log extraction picked up all feat/fix commits including internal process commits (PR review addressing, merge conflict resolution, CI tooling, and implementation details that are meaningless to end users). Fix: Add two extra grep filters — one to strip (ci)-scoped commits entirely, and one deny-list for patterns that indicate purely internal work: review comments, merge conflicts, feedback items, pre-landing fixes, session references, technical implementation tokens (testCasePath, planitem, uuid lookup, buildTestCase, server.tool, registerTool, awk regex). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…se notes RCA: git tag --sort=-creatordate | head -1 picks the most recently created tag regardless of branch ancestry. When a dispatch runs from a branch that diverged before the last tag was cut on develop/main, the range includes commits already shipped in the previous release. Fix: Replace with git describe --tags --abbrev=0 which walks the ancestry from HEAD and returns only a tag that is a true ancestor, guaranteeing the range only covers commits introduced since the last release on this line. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…etters RCA: On Windows, fs.realpathSync does not canonicalise drive-letter case, so assertPathAllowed failed when Copilot sent lowercase c:\ paths but allowed paths used uppercase C:\. Fix: Normalise both sides of the path comparison to lowercase on win32; Linux/macOS behaviour is unchanged. Four regression tests added to pathPolicy.test.ts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
RCA: In bash, using || "" attempts to execute an empty string as a command, which fails with "command not found" in strict environments and produces noisy stderr output in CI logs.
Fix: Replace || "" with || true so PREV is empty string when no ancestor tag exists; the ${PREV:+${PREV}..}HEAD range expansion already handles the empty case correctly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…to CI matrix RCA: Three issues in CI workflows — $RANGE unquoted in git log allows shell injection via crafted tag names; dot.notation in grep filter was an unescaped regex matching any character; unit tests never ran in CI so Windows path-policy regression tests had zero automated coverage. Fix: Quote "$RANGE" in DeployManual.yml git log call; escape dot\.notation in grep -Eiv pattern; add windows-latest to CI_Execution.yml default OS matrix and add a unit test step before smoke tests so all platform-specific code paths (including Windows case-insensitive path comparison) run on every PR. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
RCA: CI improvements and Windows path-policy fix constitute a release-worthy change set that requires a new beta version. Fix: Increment beta suffix from .15 to .16 in package.json and server.json; both top-level version and packages[0].version updated to stay in sync. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…urity model RCA: The pathPolicy change (case-insensitive comparison on win32) was undocumented in the security model sections of mcp.md and mcp-pilot-guide.md, leaving users and integrators with no explanation of why c:\ and C:\ paths are treated as equivalent. Fix: Add one paragraph to each doc's path-policy section explaining the Windows case-insensitive behavior and the fs.realpathSync limitation that motivates it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…alization RCA: Two pre-existing bugs were exposed by adding macOS to CI matrix and the unit test step. On macOS, os.tmpdir() returns /var/... which is a symlink to /private/var/...; the path policy fallback only tried one parent level and fell back to path.resolve() which does not follow symlinks, causing allowed-path comparisons to fail for non-existent output paths. Separately, testPlanTools built absolute paths with path.join() before normalizing backslashes, so Windows-style paths from test fixtures resolved incorrectly on macOS/Linux where backslash is a literal character. Fix: Replace single-level parent fallback with an ancestor walk-up loop in assertPathAllowed so realpathSync is called on the deepest existing ancestor, preserving symlink resolution. Hoist normalizedTestCasePath (using existing toForwardSlashes helper) to before path.join() in testPlanTools add-instance handler so backslashes are normalized before any path ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ol descriptions RCA: Three tools that generate or edit XML test steps (provar_testcase_generate, provar_testcase_step_edit, provar_testcase_validate) lacked the fallback instruction to consult the provar://docs/step-reference MCP resource when the corpus API returns count:0 with a warning field. Agents calling these tools directly (outside a loop prompt) had no guidance on where to look for step structure when corpus is unavailable. Fix: Add the standard corpus fallback sentence to all three tool descriptions, matching the pattern already used in loopPrompts.ts and migrationPrompts.ts. Update MockMcpServer/CapturingServer in the three test files to capture descriptions, and add one describe block per tool verifying the step-reference URI appears. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…compatibility RCA: node_modules/.bin/mocha is a bash shebang script on all platforms. Calling it as `node node_modules/.bin/mocha` works on Unix because npm adds a proper launcher, but on Windows Node.js tries to parse the bash script as JavaScript and throws SyntaxError. CI started failing on the windows-latest runner as soon as the unit tests step was added to the matrix. Fix: Replace `node node_modules/.bin/mocha` with `npx mocha`. npx resolves the platform-correct entry point automatically: .cmd wrapper on Windows, bash script on Unix. Uses the locally installed version (no registry download). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion RCA: Tools lacked compound XML generation and embedded variable detection Fix: Add buildCompoundValue() to testCaseGenerate and VAR-REF-002 rule to testCaseValidate with full test coverage (888 passing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…le strings RCA: docs/mcp.md lacked a VAR-REF-002 entry after the rule was added to the validator; CLAUDE.md requires new error codes to be documented. Fix: Add VAR-REF-002 bullet to the testcase validate error code section explaining compound XML usage and provar_testcase_generate auto-handling. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… in strings
Req: When {VarName} was mixed with surrounding literal text in a step argument, the generator emitted a plain valueClass="string" element; Provar runtime passes these tokens literally rather than resolving the variable reference. The validator also lacked a rule for this compound-variable pattern.
Fix: Add buildCompoundValue() in testCaseGenerate to emit class="compound"/<parts> when a string mixes literals and {VarName} tokens; split VAR-REF-001/002 in testCaseValidate to detect both pure and embedded variable misuse.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eployManual, and testPlanTools Req: PR 143 review identified three bugs: pathPolicy containment check produced double separators for root paths (e.g. C:\ or //), DeployManual used tail -1 on a descending-sorted tag list so it selected the oldest tag instead of newest, and testPlanTools did not call assertPathAllowed on test_case_path before file access allowing directory traversal. Fix: Strip trailing separator before prefix check in pathPolicy; change tail -1 to head -1 in DeployManual; add assertPathAllowed(absoluteTestCasePath) in testPlanTools; add path traversal unit test covering the escape-via-dotdot case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
v1.5.0-beta.16 fix(mcp): Windows path case-insensitivity + CI hardening
Req: PR #143 was merged to develop after this branch diverged, bringing in compound value generation, VAR-REF-002 validation, macOS symlink walk-up loop in pathPolicy, and backslash normalization before path.join in testPlanTools. This branch had duplicate feature code and conflicting path handling that broke the backslash normalization test on Linux CI. Fix: Merge develop to absorb duplicated feature commits; resolve testPlanTools conflict by combining develop's toForwardSlashes normalization with this branch's assertPathAllowed on absoluteTestCasePath; take develop's testCaseValidate.test.ts to eliminate duplicate VAR-REF-002 tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ath.join normalizes '..' away Req: path.join(projectRoot, test_case_path) normalizes '..' segments before assertPathAllowed inspects the path, so a traversal input like '../escape.testcase' bypassed the PATH_TRAVERSAL check and was only caught by containment — meaning it went undetected entirely when allowedPaths is empty (unrestricted mode). Fix: Explicitly reject '..' segments in normalizedTestCasePath before calling path.join(), ensuring PATH_TRAVERSAL is raised regardless of allowedPaths configuration; update the unit test to use an unrestricted server (allowedPaths: []) to prove the fix covers the unrestricted-mode gap. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e-emission Worktree fix+compound value emission
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the MCP tooling around test case generation/validation and filesystem safety, adding support for compound {VarName} interpolation patterns, improving path-policy behavior (including Windows), and expanding CI coverage.
Changes:
- Add
{VarName}-in-string handling: generator emitsclass="compound"XML; validator introducesVAR-REF-002and tests/documentation. - Improve path-policy behavior (non-existent paths, Windows case-insensitive comparisons) and expand unit tests/CI matrix (now includes Windows + unit test run).
- Improve tool descriptions with explicit “grounding” guidance (corpus tool +
provar://docs/step-reference) and bump versions to1.5.0-beta.16.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/mcp/testPlanTools.test.ts | Adds unit coverage for path-policy behavior in provar_testplan_add-instance. |
| test/unit/mcp/testCaseValidate.test.ts | Adds VAR-REF-002 tests and tool-description capture assertions. |
| test/unit/mcp/testCaseStepTools.test.ts | Captures tool descriptions and asserts guidance text is present. |
| test/unit/mcp/testCaseGenerate.test.ts | Adds generator tests for compound emission and tool-description assertions. |
| test/unit/mcp/pathPolicy.test.ts | Adds Windows-specific case-insensitive path comparison tests. |
| src/mcp/tools/testPlanTools.ts | Tightens test_case_path normalization/path-policy enforcement for add-instance. |
| src/mcp/tools/testCaseValidate.ts | Adds VAR-REF-002 detection + updates tool description with step-reference guidance. |
| src/mcp/tools/testCaseStepTools.ts | Updates tool description with corpus + step-reference fallback guidance. |
| src/mcp/tools/testCaseGenerate.ts | Emits class="compound" XML when {VarName} tokens are embedded in surrounding text. |
| src/mcp/security/pathPolicy.ts | Improves handling for non-existent paths and Windows case-insensitive comparisons. |
| server.json | Bumps server/package versions to 1.5.0-beta.16 and reformats runtime arguments. |
| package.json | Bumps package version to 1.5.0-beta.16. |
| docs/mcp.md | Documents Windows case-insensitive path policy + VAR-REF-002. |
| docs/mcp-pilot-guide.md | Documents Windows case-insensitive path policy. |
| .github/workflows/DeployManual.yml | Improves release-note range selection and filters out noisy commit subjects. |
| .github/workflows/CI_Execution.yml | Adds Windows to the matrix and runs unit tests in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+73
to
+86
| // Windows file paths are case-insensitive; fs.realpathSync does not always | ||
| // canonicalize drive-letter case (e.g. `c:\` vs `C:\`), so compare case-insensitively. | ||
| const isWindows = process.platform === 'win32'; | ||
| const normalizeForCompare = (p: string): string => (isWindows ? p.toLowerCase() : p); | ||
| const resolvedKey = normalizeForCompare(resolved); | ||
|
|
||
| if ( | ||
| resolvedAllowed.length > 0 && | ||
| !resolvedAllowed.some((base) => resolved === base || resolved.startsWith(base + path.sep)) | ||
| !resolvedAllowed.some((base) => { | ||
| const baseKey = normalizeForCompare(base); | ||
| // Strip a trailing separator so roots like '/' or 'C:\' don't produce '//' or 'C:\\' | ||
| const baseKeyNorm = baseKey.endsWith(path.sep) ? baseKey.slice(0, -1) : baseKey; | ||
| return resolvedKey === baseKey || resolvedKey.startsWith(baseKeyNorm + path.sep); | ||
| }) |
Collaborator
Author
There was a problem hiding this comment.
Fixed in eeda921. Added trailing-separator normalization inside the .some callback: strip trailing path.sep from the base key before comparison, but only when the base is not a filesystem root (/ on Unix, C:\ on Windows). Added two regression tests in pathPolicy.test.ts covering the trailing-sep-on-allowed-root case for both child-path and exact-match checks.
Comment on lines
+263
to
+269
| // Check for '..' before path.join() normalizes them away; otherwise traversal goes undetected | ||
| // when allowedPaths is empty (unrestricted mode). Then enforce containment on the resolved path. | ||
| const normalizedTestCasePath = toForwardSlashes(test_case_path); | ||
| if (normalizedTestCasePath.split('/').some((seg) => seg === '..')) { | ||
| throw new PathPolicyError('PATH_TRAVERSAL', `Path traversal detected in test_case_path: ${test_case_path}`); | ||
| } | ||
| const absoluteTestCasePath = path.join(projectRoot, normalizedTestCasePath); |
Collaborator
Author
There was a problem hiding this comment.
Fixed in eeda921. Added a path.isAbsolute(test_case_path) check before path.join in the add-instance handler, returning INVALID_PATH with a clear message when an absolute path is supplied. Added a regression test in testPlanTools.test.ts verifying absolute paths are rejected.
RCA: Copilot review on PR #145 identified two security gaps: (1) pathPolicy.ts did not strip trailing separators from allowed-root keys, causing double-sep prefix checks ("/tmp//") and equality mismatches that incorrectly reject valid child paths when users pass roots like "/tmp/"; (2) testPlanTools.ts accepted absolute test_case_path values, allowing path.join(projectRoot, absolutePath) to silently discard projectRoot on Windows (drive-letter) and Unix, enabling reads outside the project. Fix: Strip trailing path.sep from allowed-root keys in pathPolicy (except filesystem roots "/" and "C:\"). Reject absolute test_case_path values in add-instance handler before path.join. Add two new tests: trailing-sep cases in pathPolicy.test.ts and an absolute-path rejection in testPlanTools.test.ts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…st robustness RCA: PR #146 Copilot review found three issues in the previous security fix: (1) filesystem root allowed-paths (/ or C:\) were broken — appending path.sep to a root key produces // or C:\ so startsWith always fails for children, meaning only the root itself was allowed instead of everything under it; (2) no regression test existed for the filesystem-root case; (3) the absolute-path rejection test relied on projectDir being absolute rather than constructing the absolute path explicitly. Fix: Handle root base keys separately in assertPathAllowed — use startsWith(rawBaseKey) directly since the root already ends with its own separator. Add a filesystem-root regression test using path.parse(tmp).root. Rewrite the absolute-path test to derive the escape path from path.parse(projectDir).root so it is always absolute regardless of how projectDir is constructed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PDX-0: fix(mcp): address PR #145 review comments for security hardening
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.