Skip to content

Fix redact_secrets gateway-token tests after MCP config path refactor#26681

Merged
pelikhan merged 2 commits intomainfrom
copilot/fix-tests-yet-again
Apr 16, 2026
Merged

Fix redact_secrets gateway-token tests after MCP config path refactor#26681
pelikhan merged 2 commits intomainfrom
copilot/fix-tests-yet-again

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 16, 2026

CI was failing in actions/setup/js/redact_secrets.test.cjs because test-time string patching no longer matched the runtime construction of MCP_GATEWAY_CONFIG_PATHS. As a result, gateway config fixtures were not injected, and token extraction/redaction assertions regressed.

  • Root cause alignment

    • Updated test script rewrites to target the current path expression:
      • path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/gateway-output.json")
      • path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/mcp-servers.json")
    • Removed stale rewrites that assumed hardcoded "/tmp/gh-aw/..." literals.
  • Scope of impact

    • Restores failing extractMCPGatewayTokens cases (direct token extraction, ****** split, dedup behavior).
    • Restores integration coverage asserting gateway-token redaction in main().
// before (no longer matches implementation)
.replace('"/tmp/gh-aw/mcp-config/gateway-output.json"', `"${gatewayOutput}"`)

// after (matches current implementation)
.replace(
  'path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/gateway-output.json")',
  `"${gatewayOutput}"`
)

Copilot AI and others added 2 commits April 16, 2026 06:05
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d4d8acf5-7ed0-48dd-b144-a5bda3554cdd

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan pelikhan marked this pull request as ready for review April 16, 2026 16:29
Copilot AI review requested due to automatic review settings April 16, 2026 16:29
@pelikhan pelikhan merged commit a391d2c into main Apr 16, 2026
@pelikhan pelikhan deleted the copilot/fix-tests-yet-again branch April 16, 2026 16:29
@github-actions github-actions bot mentioned this pull request Apr 16, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes failing redact_secrets gateway-token tests by updating the test-time script rewriting to match the current runtime construction of MCP_GATEWAY_CONFIG_PATHS.

Changes:

  • Update test string replacements to target path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/...") expressions instead of stale hardcoded "/tmp/gh-aw/..." literals.
  • Restore correctness of MCP gateway token extraction/redaction assertions by ensuring fixtures are injected at the right config paths.
Show a summary per file
File Description
actions/setup/js/redact_secrets.test.cjs Adjusts script-patching in tests so mocked MCP gateway config paths align with the implementation after the config-path refactor.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines 571 to +573
const script = redactScript
.replace('"/tmp/gh-aw/mcp-config/gateway-output.json"', `"${gatewayOutput.replace(/\\/g, "\\\\")}"`)
.replace('"/tmp/gh-aw/mcp-config/mcp-servers.json"', `"${path.join(configDir, "mcp-servers.json").replace(/\\/g, "\\\\")}"`);
.replace('path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/gateway-output.json")', `"${gatewayOutput.replace(/\\/g, "\\\\")}"`)
.replace('path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/mcp-servers.json")', `"${path.join(configDir, "mcp-servers.json").replace(/\\/g, "\\\\")}"`);
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests rely on exact string matching of the implementation expression (path.join(process.env.RUNNER_TEMP || "/tmp", ...)) in multiple places. This is brittle (any formatting/refactor breaks the replacements) and duplicated across several test cases. Consider extracting these source substrings into shared constants/helpers in the test, or (since redact_secrets.cjs exports MCP_GATEWAY_CONFIG_PATHS and the array is mutable) requiring the module and overriding the array entries directly instead of patching the script text.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 83/100

Excellent test quality

Metric Value
New/modified tests analyzed 7 (modified only)
✅ Design tests (behavioral contracts) 7 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 3 (43%)
Duplicate test clusters 0
Test inflation detected No (+2 net lines)
🚨 Coding-guideline violations 0

Nature of This PR

All changes are mechanical fixes — the 7 modified test blocks update their script-patching .replace() strings from hardcoded paths ("/tmp/gh-aw/mcp-config/gateway-output.json") to the refactored path.join(process.env.RUNNER_TEMP || "/tmp", "gh-aw/mcp-config/...") construction. No assertions, no test logic, and no setup/teardown was altered. This is exactly the right way to adapt tests after a production-code refactor.


Test Classification Details

View All 7 Modified Test Blocks
Test File Classification Issues Detected
should extract gateway token from gateway-output.json redact_secrets.test.cjs:~569 ✅ Design Asserts observable return value via expect(tokens).toContain(...)
should extract token from mcp-servers.json redact_secrets.test.cjs:~594 ✅ Design Asserts observable return value via expect(tokens).toContain(...)
should handle Bearer token format redact_secrets.test.cjs:~619 ✅ Design Asserts observable return value via expect(tokens).toContain(...)
should deduplicate tokens shared across multiple servers redact_secrets.test.cjs:~643 ✅ Design Edge case: asserts deduplication — expect(tokens.filter(...)).toHaveLength(1)
should return empty array when config files do not exist redact_secrets.test.cjs:~652 ✅ Design Error/edge case: asserts expect(tokens).toEqual([]) for missing files
should silently ignore malformed JSON config files redact_secrets.test.cjs:~665 ✅ Design Error case: asserts .not.toThrow() + expect(tokens).toEqual([]) for bad JSON
should redact MCP gateway token from agent-stdio.log in main() redact_secrets.test.cjs:~694 ✅ Design Integration: asserts file content .not.toContain(gatewayToken) + .toContain("***REDACTED***")

Language Support

Tests analyzed:

  • 🟨 JavaScript (*.test.cjs): 7 modified tests (vitest)

Verdict

Check passed. 0% of modified tests are implementation tests (threshold: 30%). The PR correctly updates test infrastructure strings to match a production-code path refactor while preserving all behavioral assertions. Mocking is limited to global.core (GitHub Actions runtime) — a legitimate target. No coding-guideline violations detected.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §24521822809

🧪 Test quality analysis by Test Quality Sentinel · ● 792.5K ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 83/100. Test quality is excellent — 0% of modified tests are implementation tests (threshold: 30%). This PR correctly fixes 7 test blocks to match the refactored MCP config path construction, preserving all behavioral assertions intact.

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.

3 participants