Skip to content

fix(scripts): fail design-intent fixture mutations that match nothing - #2738

Open
Jamie Kim (jkim323) wants to merge 1 commit into
mainfrom
fix/2722-design-intent-fixture-crlf
Open

fix(scripts): fail design-intent fixture mutations that match nothing#2738
Jamie Kim (jkim323) wants to merge 1 commit into
mainfrom
fix/2722-design-intent-fixture-crlf

Conversation

@jkim323

Copy link
Copy Markdown
Collaborator

Pull Request

Description

Set-AuthoredText in scripts/tests/linting/Validate-DesignIntent.Tests.ps1 applied an
unchecked regex -replace to the authored fixture and wrote the result without verifying
that anything changed. Three call sites passed multi-line patterns containing bare \n,
but the fixture is CRLF on a Windows checkout. Those patterns never matched, the fixture
was never mutated, and the validator correctly reported no issues.

Two tests failed visibly. A third asserted a negative and therefore passed vacuously.

This PR does two things:

  1. Makes the helper fail loudly. Set-AuthoredText now compares the substitution result
    against the original and throws when nothing changed. This is the durable half: it converts
    this entire class of defect from silent-green into loud-red across all 19 call sites, and
    prevents recurrence regardless of line endings or platform.
  2. Makes the three patterns line-ending tolerant. The bare \n sequences became \r?\n.

CI was unaffected because Linux runners check out LF, which is why main was green.

Related Issue(s)

Fixes #2722

Type of Change

Code & Documentation:

  • Bug fix (non-breaking change fixing an issue)

Other:

  • Script/automation (.ps1, .sh, .py)

Testing

Check Result
npm run test:ps -- -TestPath "scripts/tests/linting/Validate-DesignIntent.Tests.ps1" 51 passed, 0 failed (previously 49 passed, 2 failed on a CRLF checkout)
Non-vacuity of the custom-deciding test Temporarily inverted its assertion to Should -Contain 'schema-violation'; the test failed. Reverted; green again.
npm run lint:ps All files pass PSScriptAnalyzer
scripts/linting/Validate-DesignIntent.ps1 unchanged Confirmed via git status --short

The passing count rose from 49 to 51 because the two previously-failing tests now do real
work, not because tests were added.

Checklist

Required Checks

  • Documentation is updated (if applicable) - no documentation change required
  • Files follow existing naming conventions
  • Changes are backwards compatible (if applicable)
  • Tests added for new functionality (if applicable) - repairs existing tests

Required Local Checks

  • Local validation aggregate: npm run validate:local - see Additional Notes
  • Documentation validation (if docs changed): not applicable, no docs changed
  • Spell checking: npm run spell-check
  • Link validation: npm run lint:md-links - see Additional Notes

Security Considerations

  • This PR does not contain any sensitive or NDA information
  • Any new dependencies have been reviewed for security issues - no new dependencies
  • Security-related scripts follow the principle of least privilege

Additional Notes

npm run validate:local halts at lint:md-links on pre-existing external link failures in
SUPPORT.md and several RAI and security skill reference files. None of those files are
touched by this PR, and the failures are unrelated external URL responses. Every other
stage of the aggregate passes, including the stages after the halt when run individually:
lint:frontmatter, lint:design-intent, lint:py, and validate:skills.

One deliberate non-change: the replacement strings still emit LF-only newlines via `n,
so a mutated CRLF fixture ends up with mixed line endings. The YAML parser does not care and
each test operates on a fresh fixture copy, so this has no effect. Normalizing it would
exceed the scope of the issue.

@jkim323
Jamie Kim (jkim323) requested a review from a team as a code owner August 20, 2026 15:02
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.88%. Comparing base (6f22d31) to head (84dc681).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2738      +/-   ##
==========================================
- Coverage   83.17%   82.88%   -0.29%     
==========================================
  Files         180      169      -11     
  Lines       32201    32411     +210     
  Branches       25        0      -25     
==========================================
+ Hits        26782    26864      +82     
- Misses       5416     5547     +131     
+ Partials        3        0       -3     
Flag Coverage Δ
docusaurus ?
pester 83.30% <ø> (-0.91%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

Eval Execution

⚠️ No eval summary was produced.

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.

fix(tests): design-intent fixture mutations silently no-op on CRLF checkouts

3 participants