PDX-469: chore(mcp) — harden MCP infra: typo guard, release token, diff scoping, test isolation#181
Merged
Conversation
…ff scoping, test isolation RCA: Four high-priority hardening items surfaced in the 1.5.1 PR #172 release review. (H1) parseActiveGroups in server.ts already warned on empty comma-only PROVAR_MCP_TOOLS values but did not validate group names against TOOL_GROUPS keys — a typo like PROVAR_MCP_TOOLS=validaton was silently ignored and the server started with only provardx_ping registered. (H2) The DeployManual.yml publish job's "Install dependencies and build" step did not export GITHUB_TOKEN, so prepack's fetch-nitrox-packages.cjs fell back to the bundled NitroX catalog/schemas on every release; PDX-463/464's "always fetch latest from main" guarantee did not actually deliver in CI. (H3) validationDiff storage was shared across tools and contexts under ~/.provardx/validation/<sub>, with no per-context scoping — a run_id from project A could be fed to a validate call against project B and would diff against unrelated data without any indication. (H4) testSuiteValidate.test.ts wrote to the real ~/.provardx/validation/testsuite path because the unit test did not stub os.homedir, polluting the developer/CI home and exposing tests to pre-existing run state. Fix: (H1) parseActiveGroups now intersects requested groups with Object.keys(TOOL_GROUPS), warns on unknown names, and falls back to null (all groups) when nothing matches — typos are loud and never produce an empty Provar tool surface. (H2) Added GH_TOKEN: secrets.GITHUB_TOKEN env to the build step of DeployManual.yml; the auto-provided token has read access to factPackages so fetch-nitrox-packages.cjs now pulls fresh schemas on every release. (H3) validationDiff now exposes computeContextHash(toolTag, context) and records a context_hash on each run; loadBaselineViolations rejects a baseline_run_id whose context_hash does not match the calling context, preventing cross-context diffs. Added resolveValidationDir(subdir) which honors a new PROVAR_MCP_VALIDATION_DIR env override (falling back to ~/.provardx/validation/<subdir>) for restricted CI/dev environments. testCaseValidate, testSuiteValidate, and projectValidateFromPath all pass their tool tag + context to saveRun/loadBaselineViolations. (H4) Moved the testSuiteValidate.test.ts before/afterEach hooks INSIDE the describe block — mocha top-level hooks attach to the root suite and run before every test in every file, which leaked the os.homedir stub into auth/rotate.test.ts and other downstream files. Discovered while running the full mocha suite — rotate.test.ts flapped only when testSuiteValidate ran first; scoping the hooks fixes it cleanly. Tests: New startupTuning.test.ts cases assert that PROVAR_MCP_TOOLS=validaton returns null (typo footgun) and that mixed lists keep known names while dropping unknown ones. New validationDiff.test.ts cases cover computeContextHash determinism + per-tool/per-context distinctness, loadBaselineViolations rejection on context_hash mismatch (including legacy records that predate the field), loadBaselineViolations back-compat when no expectedContextHash is provided, and resolveValidationDir defaulting vs PROVAR_MCP_VALIDATION_DIR override. testSuiteValidate.test.ts hooks moved inside the describe to scope the os.homedir stub. Validation: yarn compile clean, full mocha 1155 passing / 0 failing, yarn lint clean, 55/55 smoke pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Quality Orchestrator🟢 LOW · 🧪 Tests to Run · Running 5 of 51 tests
▶ Run commandnpx vitest run \
unit/mcp/server.test.ts \
unit/mcp/projectValidateFromPath.test.ts \
unit/mcp/testCaseValidate.test.ts \
unit/mcp/testSuiteValidate.test.ts \
unit/mcp/validationDiff.test.ts⚡ quality-orchestrator · |
Contributor
There was a problem hiding this comment.
Pull request overview
Four small, independent hardening fixes that surfaced during the 1.5.1 release review. Each one closes a real correctness or hygiene gap discovered after PR #172/#180, without changing user-facing behaviour for valid inputs.
Changes:
parseActiveGroups()now intersects requested groups against the knownTOOL_GROUPSkeys, warns on unknowns, and falls back to "all groups" when nothing matches — so a typo likePROVAR_MCP_TOOLS=validatonno longer silently disables every Provar tool.- New
computeContextHash(toolTag, context)plus an optionalcontext_hashfield on persisted runs scopes baseline diffs per tool/context; newresolveValidationDir(subdir)introduces aPROVAR_MCP_VALIDATION_DIRenv override; wired throughtestCaseValidate,testSuiteValidate, andprojectValidateFromPath. - Release workflow
DeployManual.ymlnow passesGH_TOKEN=${{ secrets.GITHUB_TOKEN }}into the install/build step soscripts/fetch-nitrox-packages.cjscan actually fetch the latest NitroX schemas fromfactPackagesinstead of silently shipping the bundled fallback. testSuiteValidate.test.tsmoves itsbeforeEach/afterEachinside thedescribeblock and stubsos.homedir()to a temp dir, preventing a stub leak to the mocha root suite (which had been flappingauth/rotate.test.ts) and stopping writes into the real~/.provardx/validation/testsuite.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/DeployManual.yml |
Adds GH_TOKEN to the install/build step so prepack's NitroX fetch authenticates against factPackages during release. |
src/mcp/server.ts |
parseActiveGroups validates requested groups against TOOL_GROUPS keys, warns on unknowns, returns null (all) when nothing matches. |
src/mcp/utils/validationDiff.ts |
Adds computeContextHash, resolveValidationDir, optional context_hash on RunRecord; saveRun/loadBaselineViolations extended with context-hash plumbing. |
src/mcp/tools/testCaseValidate.ts |
Switches storage dir to resolveValidationDir('testcase'), computes per-call contextHash and passes it to saveRun/loadBaselineViolations. |
src/mcp/tools/testSuiteValidate.ts |
Same wiring for the suite tool using tag 'suite' and suite_name as context. |
src/mcp/tools/projectValidateFromPath.ts |
Adds contextHash (tag 'project', project_path as context) to baseline load/save; storage dir continues to honour results_dir/project-local default. |
test/unit/mcp/startupTuning.test.ts |
Adds H1 cases for typo-only and mixed known/unknown PROVAR_MCP_TOOLS inputs. |
test/unit/mcp/validationDiff.test.ts |
Adds tests for computeContextHash determinism/distinctness, loadBaselineViolations context-hash gating (mismatch, legacy record, no expected hash), and resolveValidationDir env-override behaviour. |
test/unit/mcp/testSuiteValidate.test.ts |
Moves hooks into describe, stubs os.homedir to a temp dir and restores it in afterEach to prevent root-suite leak. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
Four hardening items surfaced during the 1.5.1 PR #172 release review. None of these block 1.5.1 (PR #180 covered the blockers), but each one closes a real loose end:
PROVAR_MCP_TOOLStypo footgun.parseActiveGroupswarned on empty/comma-only inputs but did not validate group names againstTOOL_GROUPSkeys. A typo likePROVAR_MCP_TOOLS=validatonwas silently ignored and the server started with onlyprovardx_ping. Now intersects against known group names, warns on unknowns, and falls back to all-groups when nothing matches..github/workflows/DeployManual.ymlranyarn prepack(which callsscripts/fetch-nitrox-packages.cjs) withoutGH_TOKENin env, so every release silently fell back to bundled NitroX schemas. PDX-463/PDX-464's "always fetch latest frommain" guarantee did not actually deliver in CI. The auto-providedGITHUB_TOKENhas read access to factPackages, so wiring it through is a one-line fix.~/.provardx/validation/<sub>storage was shared across all contexts within each tool with no per-context scoping. Arun_idfrom project A could be fed to a validate call against project B and would diff against unrelated data without any indication. AddedcomputeContextHash(toolTag, context)and a newcontext_hashfield on each persisted run;loadBaselineViolationsrejects abaseline_run_idwhosecontext_hashdoes not match. Also added aPROVAR_MCP_VALIDATION_DIRenv override for restricted CI/dev environments. Legacy records (pre-H3) withoutcontext_hashare gracefully invalidated within 1-2 new runs as the FIFO cap evicts them.testSuiteValidate.test.tswrote to the real~/.provardx/validation/testsuitebecauseos.homedir()was not stubbed. Added per-test stubbing scoped inside thedescribeblock. Discovered (and resolved) a related issue while testing: a previous version of this fix put the hooks at the file top level, which attached them to the mocha root suite and leaked the stub into other test files —auth/rotate.test.ts:89flapped because itsreadStoredCredentials()resolves the credentials path at runtime viaos.homedir().AC traceback
Test plan
startupTuning.test.tscases: `PROVAR_MCP_TOOLS=validaton` (typo) returns null; mixed lists keep known names and drop unknownvalidationDiff.test.tscases:computeContextHashdeterminism + per-tool/per-context distinctness;loadBaselineViolationsrejects mismatchedcontext_hash; legacy records withoutcontext_hashare correctly rejected when a hash is expected; back-compat when noexpectedContextHashis passed;resolveValidationDirdefaults vsPROVAR_MCP_VALIDATION_DIRenv overridetestSuiteValidate.test.tshooks scoped insidedescribeto prevent root-suite leakyarn compilecleanyarn lintcleannode scripts/mcp-smoke.cjs— 55/55 PASSNotes for reviewer
generateRunIdargument, andloadBaselineViolationsaccepts the newexpectedContextHashas an optional arg (call sites that don't pass it keep the old behavior). Legacy records persist correctly via the optionalcontext_hashfield onRunRecord.secrets.GITHUB_TOKENsince theProvarTesting/factPackagesrepo grants read access to it. If that ever changes, the env block can be re-pointed at a dedicated secret without other workflow changes.auth/rotate.test.ts:89— it capturescredsPath = getCredentialsPath()at file load time (real homedir) butreadStoredCredentials()resolvesos.homedir()again at runtime. The test only passes if no earlier test in the run leaks anos.homedirstub. My fix here closes the specific leak fromtestSuiteValidate.test.ts, but the rotate test itself is structurally brittle. Worth a follow-up hygiene ticket — out of scope here.Stacking
Built on top of PR #180. Now that #180 is merged to develop, this PR's diff cleanly shows only the H1-H4 commit (
d53f2ae).🤖 Generated with Claude Code