Skip to content

Fix C-quoted filename handling in push_signed_commits.cjs#26277

Merged
pelikhan merged 3 commits intomainfrom
copilot/fix-copy-vs-rename-handling
Apr 14, 2026
Merged

Fix C-quoted filename handling in push_signed_commits.cjs#26277
pelikhan merged 3 commits intomainfrom
copilot/fix-copy-vs-rename-handling

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 14, 2026

git diff-tree --raw C-quotes filenames containing spaces, non-ASCII characters, or control characters (e.g. "h\303\251llo.txt"). The code was passing these raw quoted strings directly to fs.readFileSync and the GraphQL payload, causing failures for any commit touching such files.

Changes

  • push_signed_commits.cjs: Added unquoteCPath(s) helper that strips surrounding double-quotes and decodes C escape sequences (\\, \", \a\v) and octal sequences (\NNN) as UTF-8 bytes — matching git's encoding of multi-byte Unicode filenames. Applied to all three path extraction sites: filePath, renamedPath, and copiedPath. The helper is exported for direct testability.

  • push_signed_commits.test.cjs: Added integration tests covering:

    • Filenames with spaces
    • Filenames with Unicode characters (e.g. héllo_wörld.txt)
    • Rename: old path in deletions, new path in additions
    • Copy: source path absent from deletions, destination in additions

    Added a dedicated unit test suite for unquoteCPath covering: plain strings (returned unchanged), quote stripping, all standard C escape sequences, octal sequences decoded as UTF-8 bytes, unknown escapes (backslash preserved), and edge cases (empty string, single character, 1/2/3-digit octals).

// Before: raw C-quoted string used as path → readFileSync fails
const filePath = paths[0]; // e.g. "\"h\\303\\251llo.txt\""

// After: decoded to the actual filesystem path
const filePath = unquoteCPath(paths[0]); // → "héllo.txt"

Copilot AI changed the title [WIP] Fix copy vs rename handling and double-quoted filenames Fix C-quoted filename handling in push_signed_commits.cjs Apr 14, 2026
Copilot AI requested a review from pelikhan April 14, 2026 20:34
* @param {string} s - Raw path token from git output (may or may not be quoted)
* @returns {string} Unescaped path
*/
function unquoteCPath(s) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot add test suite for this function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a dedicated unquoteCPath test suite in commit f01052f. The 9 tests cover: plain strings returned unchanged, quote stripping, all standard C escape sequences (\\, \", \a/\b/\f/\n/\r/\t/\v), octal sequences decoding to UTF-8 bytes (e.g. \303\251é), filenames with spaces, unknown escapes (backslash preserved), edge cases (single char, empty string, 1/2/3-digit octals). The function is also now exported from the module to make it directly importable in tests.

Copilot AI requested a review from pelikhan April 14, 2026 20:49
@pelikhan pelikhan marked this pull request as ready for review April 14, 2026 20:50
Copilot AI review requested due to automatic review settings April 14, 2026 20:50
@github-actions github-actions bot mentioned this pull request Apr 14, 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 handling of C-quoted filenames emitted by git diff-tree --raw so push_signed_commits.cjs correctly reads file contents and emits proper GraphQL fileChanges paths for files with special characters (notably UTF-8/unicode names).

Changes:

  • Added unquoteCPath(s) to decode git C-quoted path tokens (C escapes + octal-encoded UTF-8 bytes).
  • Applied unquoteCPath to raw diff-tree path extraction for normal, rename, and copy entries.
  • Added unit tests for unquoteCPath plus new integration coverage for unicode/spaces, rename, and copy scenarios.
Show a summary per file
File Description
actions/setup/js/push_signed_commits.cjs Adds and uses unquoteCPath when parsing git diff-tree --raw paths; exports helper for testing.
actions/setup/js/push_signed_commits.test.cjs Adds unit tests for unquoteCPath and new integration tests for special-character paths and rename/copy behavior.

Copilot's findings

Tip

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

Comments suppressed due to low confidence (1)

actions/setup/js/push_signed_commits.cjs:190

  • Same issue as rename handling: paths[1] may be missing for an unexpected/partial copy entry, and unquoteCPath(paths[1]) will throw before the subsequent if (!copiedPath) guard can run. Add an existence check before unquoting (or adjust unquoteCPath to handle non-strings) to preserve the intended skip-with-warning behavior.
        } else if (status && status.startsWith("C")) {
          // Copy: source path is kept (no deletion), only the destination path is added
          const copiedPath = unquoteCPath(paths[1]);
          if (!copiedPath) {
            core.warning(`pushSignedCommits: copy entry missing destination path, skipping: ${line}`);
            continue;
  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment on lines 168 to 172
} else if (status && status.startsWith("R")) {
// Rename: source path is deleted, destination path is added
const renamedPath = paths[1];
const renamedPath = unquoteCPath(paths[1]);
if (!renamedPath) {
core.warning(`pushSignedCommits: rename entry missing destination path, skipping: ${line}`);
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

paths[1] can be undefined for malformed/edge-case git diff-tree --raw output (the code already anticipates this via the if (!renamedPath) guard). Calling unquoteCPath(paths[1]) before checking existence will throw a TypeError and skip the warning/continue behavior. Guard paths[1] before passing it to unquoteCPath (or make unquoteCPath tolerate non-string inputs) so missing destination paths are handled gracefully.

This issue also appears on line 185 of the same file.

Copilot uses AI. Check for mistakes.
Comment on lines +41 to +43
} else {
const esc = inner[i++];
switch (esc) {
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

If the quoted token is malformed (e.g. starts with " but is missing a closing quote) and the inner string ends with a single trailing backslash, const esc = inner[i++] evaluates to undefined and later esc.charCodeAt(0) will throw. Add an i >= inner.length check after consuming \\ to treat a trailing backslash as a literal instead of crashing.

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 13
✅ Design tests (behavioral contracts) 13 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 10 (77%)
Duplicate test clusters 0
Test inflation detected Yes — 206 test lines / 77 production lines = 2.67:1 (penalty applied, but contextually justified)
🚨 Coding-guideline violations None

Test Classification Details

📋 All 13 test classifications
Test File Classification Notes
should return unquoted strings unchanged push_signed_commits.test.cjs ✅ Design Identity + empty-string edge case
should strip surrounding double-quotes from plain filenames push_signed_commits.test.cjs ✅ Design Core quoting contract
should unescape standard C escape sequences push_signed_commits.test.cjs ✅ Design 9 assertions covering all escape types
should decode octal sequences as UTF-8 bytes (unicode filenames) push_signed_commits.test.cjs ✅ Design Multi-byte UTF-8 edge case
should decode filenames with spaces push_signed_commits.test.cjs ✅ Design Space-in-path edge case
should preserve unknown escape sequences with backslash intact push_signed_commits.test.cjs ✅ Design Fallback/error-path edge case
should handle a quoted string with only one character push_signed_commits.test.cjs ✅ Design Boundary value
should handle a quoted empty string push_signed_commits.test.cjs ✅ Design Boundary value
should handle 1-, 2-, and 3-digit octal sequences push_signed_commits.test.cjs ✅ Design Boundary values incl. NUL byte (\0)
should handle filenames with spaces (integration) push_signed_commits.test.cjs ✅ Design Real subprocess; end-to-end path contract
should handle filenames with unicode characters (integration) push_signed_commits.test.cjs ✅ Design Real subprocess; unicode filename contract
should add old path to deletions and new path to additions on rename push_signed_commits.test.cjs ✅ Design Real subprocess; rename operation contract
should not add source to deletions on copy (only destination in additions) push_signed_commits.test.cjs ✅ Design Real subprocess; negative assertion on deletions

Flagged Tests — Requires Review

No tests flagged. All 13 tests verify observable behavioral contracts.


Score Breakdown

Component Pts Earned Max Calculation
Behavioral Coverage 40 40 13/13 design tests = 100%
Error/Edge Case Coverage 23 30 10/13 tests with edge cases = 77%
Low Duplication 20 20 0 duplicate clusters
Proportional Growth 0 10 Inflation ratio 2.67:1 exceeds 2:1 threshold
Total 83 100

Note on inflation: The 2.67:1 ratio is contextually expected — the PR introduces a new pure-function helper (unquoteCPath) that requires extensive input/output coverage for a parser, plus 4 real-subprocess integration tests. The binary penalty is applied per rubric, but this should not be read as a quality concern.


Language Support

Tests analyzed:

  • 🟨 JavaScript (*.test.cjs): 13 tests (vitest) — 9 unit tests for unquoteCPath, 4 integration tests using real git subprocess

i️ No Go test files (*_test.go) were added or modified in this PR.


Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). All 13 new tests verify behavioral contracts, including boundary values, C escape sequence handling, octal UTF-8 decoding, and end-to-end rename/copy behavior through real git subprocess execution.


📖 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.

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

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 new tests are implementation tests (threshold: 30%). All 13 new tests verify behavioral contracts with strong edge-case coverage (77%), including boundary values, C escape sequences, multi-byte UTF-8 octal decoding, unknown-escape fallback behavior, and end-to-end rename/copy contracts via real git subprocess execution.

@pelikhan pelikhan merged commit 681b20e into main Apr 14, 2026
158 of 160 checks passed
@pelikhan pelikhan deleted the copilot/fix-copy-vs-rename-handling branch April 14, 2026 21:05
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.

[plan] Fix copy vs rename handling and double-quoted filenames in push_signed_commits.cjs

3 participants